[Qemu-devel] [PATCH 0/2] vl.c: unify exit on error handling

[Qemu-devel] [PATCH 0/2] vl.c: unify exit on error handling

[Igor Mammedov]

reduces amount of code needed for handling exit on error
for new users, turning it from 5LOC to 1 for each case.

Igor Mammedov (2):
  vl.c: use single local_err throughout main()
  vl.c: reduce exit on error code duplication

 vl.c | 62 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

-- 
1.9.3 (Apple Git-50)

[Qemu-devel] [PATCH 1/2] vl.c: use single local_err throughout main()

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 vl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 964d634..f563da1 100644
--- a/vl.c
+++ b/vl.c
@@ -2887,6 +2887,7 @@ int main(int argc, char **argv, char **envp)
     };
     const char *trace_events = NULL;
     const char *trace_file = NULL;
+    Error *local_err = NULL;
     const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
                                         1024 * 1024;
     ram_addr_t maxram_size = default_ram_size;
@@ -4188,7 +4189,6 @@ int main(int argc, char **argv, char **envp)
     configure_accelerator(current_machine);
 
     if (qtest_chrdev) {
-        Error *local_err = NULL;
         qtest_init(qtest_chrdev, qtest_log, &local_err);
         if (local_err) {
             error_report("%s", error_get_pretty(local_err));
@@ -4432,7 +4432,6 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_VNC
     /* init remote displays */
     if (vnc_display) {
-        Error *local_err = NULL;
         vnc_display_init(ds);
         vnc_display_open(ds, vnc_display, &local_err);
         if (local_err != NULL) {
@@ -4487,7 +4486,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (incoming) {
-        Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
         if (local_err) {
             error_report("-incoming %s: %s", incoming,
-- 
1.9.3 (Apple Git-50)

Re: [Qemu-devel] [PATCH 1/2] vl.c: use single local_err throughout main()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

On 10/15/2014 05:03 AM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  vl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH 2/2] vl.c: reduce exit on error code duplication

[Igor Mammedov]

use exit_if_error() helper instead of a bunch of
    if (local_err) {
        error_report(foo);
        error_free(local_err);
        exit(1);
    }
code blocks

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 vl.c | 58 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/vl.c b/vl.c
index f563da1..151ee2d 100644
--- a/vl.c
+++ b/vl.c
@@ -582,6 +582,30 @@ static void res_free(void)
     }
 }
 
+static void GCC_FMT_ATTR(2, 3) exit_if_error(Error *err, const char *fmt, ...)
+{
+    va_list ap;
+
+    if (!err) {
+        return;
+    }
+
+    if (fmt) {
+        char *optional_msg = NULL;
+
+        va_start(ap, fmt);
+        optional_msg = g_strdup_vprintf(fmt, ap);
+        va_end(ap);
+        error_report("%s: %s", optional_msg, error_get_pretty(err));
+        g_free(optional_msg);
+    } else {
+        error_report("%s", error_get_pretty(err));
+    }
+
+    error_free(err);
+    exit(EXIT_FAILURE);
+}
+
 static int default_driver_check(QemuOpts *opts, void *opaque)
 {
     const char *driver = qemu_opt_get(opts, "driver");
@@ -2380,11 +2404,7 @@ static int chardev_init_func(QemuOpts *opts, void *opaque)
     Error *local_err = NULL;
 
     qemu_chr_new_from_opts(opts, NULL, &local_err);
-    if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
-        return -1;
-    }
+    exit_if_error(local_err, NULL);
     return 0;
 }
 
@@ -2790,12 +2810,7 @@ static int machine_set_property(const char *name, const char *value,
     string_input_visitor_cleanup(siv);
     g_free(qom_name);
 
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
+    exit_if_error(local_err, NULL);
     return 0;
 }
 
@@ -4190,11 +4205,7 @@ int main(int argc, char **argv, char **envp)
 
     if (qtest_chrdev) {
         qtest_init(qtest_chrdev, qtest_log, &local_err);
-        if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
-            exit(1);
-        }
+        exit_if_error(local_err, NULL);
     }
 
     machine_opts = qemu_get_machine_opts();
@@ -4434,12 +4445,8 @@ int main(int argc, char **argv, char **envp)
     if (vnc_display) {
         vnc_display_init(ds);
         vnc_display_open(ds, vnc_display, &local_err);
-        if (local_err != NULL) {
-            error_report("Failed to start VNC server on `%s': %s",
-                         vnc_display, error_get_pretty(local_err));
-            error_free(local_err);
-            exit(1);
-        }
+        exit_if_error(local_err, "Failed to start VNC server on '%s'",
+                      vnc_display);
 
         if (show_vnc_port) {
             printf("VNC server running on `%s'\n", vnc_display_local_addr(ds));
@@ -4487,12 +4494,7 @@ int main(int argc, char **argv, char **envp)
 
     if (incoming) {
         qemu_start_incoming_migration(incoming, &local_err);
-        if (local_err) {
-            error_report("-incoming %s: %s", incoming,
-                         error_get_pretty(local_err));
-            error_free(local_err);
-            exit(1);
-        }
+        exit_if_error(local_err, "-incoming %s", incoming);
     } else if (autostart) {
         vm_start();
     }
-- 
1.9.3 (Apple Git-50)

Re: [Qemu-devel] [PATCH 2/2] vl.c: reduce exit on error code duplication

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

On 10/15/2014 05:03 AM, Igor Mammedov wrote:
> use exit_if_error() helper instead of a bunch of
>     if (local_err) {
>         error_report(foo);
>         error_free(local_err);
>         exit(1);
>     }
> code blocks
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  vl.c | 58 ++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 30 insertions(+), 28 deletions(-)

Not much net change, but I like the refactoring.


>  static int default_driver_check(QemuOpts *opts, void *opaque)
>  {
>      const char *driver = qemu_opt_get(opts, "driver");
> @@ -2380,11 +2404,7 @@ static int chardev_init_func(QemuOpts *opts, void *opaque)
>      Error *local_err = NULL;
>  
>      qemu_chr_new_from_opts(opts, NULL, &local_err);
> -    if (local_err) {
> -        error_report("%s", error_get_pretty(local_err));
> -        error_free(local_err);
> -        return -1;
> -    }
> +    exit_if_error(local_err, NULL);
>      return 0;
>  }

Idea for followup patch: this function now always returns 0 (if it
returns at all); therefore, change its signature to void and simplify
further.

>  
> @@ -2790,12 +2810,7 @@ static int machine_set_property(const char *name, const char *value,
>      string_input_visitor_cleanup(siv);
>      g_free(qom_name);
>  
> -    if (local_err) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return -1;
> -    }
> -
> +    exit_if_error(local_err, NULL);
>      return 0;
>  }

Same idea for simplification.

But as that should be a separate patch, this one is:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vl.c: reduce exit on error code duplication

[Igor Mammedov]

On Wed, 15 Oct 2014 08:35:53 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 10/15/2014 05:03 AM, Igor Mammedov wrote:
> > use exit_if_error() helper instead of a bunch of
> >     if (local_err) {
> >         error_report(foo);
> >         error_free(local_err);
> >         exit(1);
> >     }
> > code blocks
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  vl.c | 58 ++++++++++++++++++++++++++++++----------------------------
> >  1 file changed, 30 insertions(+), 28 deletions(-)
> 
> Not much net change, but I like the refactoring.
> 
> 
> >  static int default_driver_check(QemuOpts *opts, void *opaque)
> >  {
> >      const char *driver = qemu_opt_get(opts, "driver");
> > @@ -2380,11 +2404,7 @@ static int chardev_init_func(QemuOpts *opts, void *opaque)
> >      Error *local_err = NULL;
> >  
> >      qemu_chr_new_from_opts(opts, NULL, &local_err);
> > -    if (local_err) {
> > -        error_report("%s", error_get_pretty(local_err));
> > -        error_free(local_err);
> > -        return -1;
> > -    }
> > +    exit_if_error(local_err, NULL);
> >      return 0;
> >  }
> 
> Idea for followup patch: this function now always returns 0 (if it
> returns at all); therefore, change its signature to void and simplify
> further.
it won't work in this case since it's called by qemu_opts_foreach()
which requires return value.

> 
> >  
> > @@ -2790,12 +2810,7 @@ static int machine_set_property(const char *name, const char *value,
> >      string_input_visitor_cleanup(siv);
> >      g_free(qom_name);
> >  
> > -    if (local_err) {
> > -        qerror_report_err(local_err);
> > -        error_free(local_err);
> > -        return -1;
> > -    }
> > -
> > +    exit_if_error(local_err, NULL);
> >      return 0;
> >  }
> 
> Same idea for simplification.
ditto

> 
> But as that should be a separate patch, this one is:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Re: [Qemu-devel] [PATCH 2/2] vl.c: reduce exit on error code duplication

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> use exit_if_error() helper instead of a bunch of
>     if (local_err) {
>         error_report(foo);
>         error_free(local_err);
>         exit(1);
>     }
> code blocks
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

A quick "git-grep -B 2 -w exit" shows the pattern exists outside vl.c.
The instances in hw/ are of course suspect.  Anyway, the helper seems
more generally useful.  Let's put it in util/ now, rather than move it
later.

Suggest to name it something like error_report_fatal(), to make both the
fact that it reports and that it exits obvious from the name.