[Qemu-devel] [PATCH 0/6] Use &error_fatal in some initialization code

[Qemu-devel] [PATCH 0/6] Use &error_fatal in some initialization code

[Eduardo Habkost]

This changes some initialization code in vl.c, qemu-img, qemu-io,
and qemu-nbd to use &error_fatal instead of manual error
checking/reporting.

The series is based on my machine branch, available at:
  git://github.com/ehabkost/qemu.git machine

Eduardo Habkost (6):
  vl: Use &error_fatal when parsing VNC options
  vl: Use &error_fatal when parsing monitor options
  main-loop: Use Error** to report qemu_signal_init() errors
  main-loop: Use &error_fatal when calling qemu_init_main_loop()
  main-loop: Make qemu_init_main_loop() and qemu_signal_init() void
  Use &error_fatal when initializing crypto on qemu-{img,io,nbd}

 include/qemu/main-loop.h |  2 +-
 main-loop.c              | 24 ++++++++++--------------
 qemu-img.c               | 11 ++---------
 qemu-io.c                | 11 ++---------
 qemu-nbd.c               | 10 ++--------
 vl.c                     | 28 ++++------------------------
 6 files changed, 21 insertions(+), 65 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/6] vl: Use &error_fatal when parsing VNC options

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/vl.c b/vl.c
index 82f18b5..bd05609 100644
--- a/vl.c
+++ b/vl.c
@@ -2149,11 +2149,7 @@ static DisplayType select_display(const char *p)
 #endif
     } else if (strstart(p, "vnc", &opts)) {
         if (*opts == '=') {
-            Error *err = NULL;
-            if (vnc_parse(opts + 1, &err) == NULL) {
-                error_report_err(err);
-                exit(1);
-            }
+            vnc_parse(opts + 1, &error_fatal);
         } else {
             error_report("VNC requires a display argument vnc=<display>");
             exit(1);
@@ -3707,15 +3703,8 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_vnc:
-            {
-                Error *local_err = NULL;
-
-                if (vnc_parse(optarg, &local_err) == NULL) {
-                    error_report_err(local_err);
-                    exit(1);
-                }
+                vnc_parse(optarg, &error_fatal);
                 break;
-            }
             case QEMU_OPTION_no_acpi:
                 acpi_enabled = 0;
                 break;
-- 
2.5.5

[Qemu-devel] [PATCH 2/6] vl: Use &error_fatal when parsing monitor options

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index bd05609..eea8a04 100644
--- a/vl.c
+++ b/vl.c
@@ -2402,7 +2402,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
 static void monitor_parse(const char *optarg, const char *mode, bool pretty)
 {
     static int monitor_device_index = 0;
-    Error *local_err = NULL;
     QemuOpts *opts;
     const char *p;
     char label[32];
@@ -2423,11 +2422,7 @@ static void monitor_parse(const char *optarg, const char *mode, bool pretty)
         }
     }
 
-    opts = qemu_opts_create(qemu_find_opts("mon"), label, 1, &local_err);
-    if (!opts) {
-        error_report_err(local_err);
-        exit(1);
-    }
+    opts = qemu_opts_create(qemu_find_opts("mon"), label, 1, &error_fatal);
     qemu_opt_set(opts, "mode", mode, &error_abort);
     qemu_opt_set(opts, "chardev", label, &error_abort);
     qemu_opt_set_bool(opts, "pretty", pretty, &error_abort);
-- 
2.5.5

[Qemu-devel] [PATCH 3/6] main-loop: Use Error** to report qemu_signal_init() errors

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 main-loop.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index 89a6994..8e8eafc 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
     }
 }
 
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
 {
     int sigfd;
     sigset_t set;
@@ -96,7 +96,7 @@ static int qemu_signal_init(void)
     sigdelset(&set, SIG_IPI);
     sigfd = qemu_signalfd(&set);
     if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
+        error_setg_errno(errp, errno, "failed to create signalfd");
         return -errno;
     }
 
@@ -109,7 +109,7 @@ static int qemu_signal_init(void)
 
 #else /* _WIN32 */
 
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
 {
     return 0;
 }
@@ -148,8 +148,9 @@ int qemu_init_main_loop(Error **errp)
 
     init_clocks();
 
-    ret = qemu_signal_init();
+    ret = qemu_signal_init(&local_error);
     if (ret) {
+        error_propagate(errp, local_error);
         return ret;
     }
 
-- 
2.5.5

[Qemu-devel] [PATCH 4/6] main-loop: Use &error_fatal when calling qemu_init_main_loop()

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-img.c | 5 +----
 qemu-io.c  | 5 +----
 qemu-nbd.c | 5 +----
 vl.c       | 6 +-----
 4 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 46f2a6d..d3cfec3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3465,10 +3465,7 @@ int main(int argc, char **argv)
     error_set_progname(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
-    if (qemu_init_main_loop(&local_error)) {
-        error_report_err(local_error);
-        exit(EXIT_FAILURE);
-    }
+    qemu_init_main_loop(&error_fatal);
 
     if (qcrypto_init(&local_error) < 0) {
         error_reportf_err(local_error, "cannot initialize crypto: ");
diff --git a/qemu-io.c b/qemu-io.c
index 0598251..1507559 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -527,10 +527,7 @@ int main(int argc, char **argv)
         exit(1);
     }
 
-    if (qemu_init_main_loop(&local_error)) {
-        error_report_err(local_error);
-        exit(1);
-    }
+    qemu_init_main_loop(&error_fatal);
 
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
diff --git a/qemu-nbd.c b/qemu-nbd.c
index c55b40f..f2685e67 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -820,10 +820,7 @@ int main(int argc, char **argv)
 
     saddr = nbd_build_socket_address(sockpath, bindto, port);
 
-    if (qemu_init_main_loop(&local_err)) {
-        error_report_err(local_err);
-        exit(EXIT_FAILURE);
-    }
+    qemu_init_main_loop(&error_fatal);
     bdrv_init();
     atexit(bdrv_close_all);
 
diff --git a/vl.c b/vl.c
index eea8a04..e435926 100644
--- a/vl.c
+++ b/vl.c
@@ -2964,7 +2964,6 @@ int main(int argc, char **argv, char **envp)
     ram_addr_t maxram_size;
     uint64_t ram_slots = 0;
     FILE *vmstate_dump_file = NULL;
-    Error *main_loop_err = NULL;
     Error *err = NULL;
 
     qemu_init_cpu_loop();
@@ -4001,10 +4000,7 @@ int main(int argc, char **argv, char **envp)
 
     os_daemonize();
 
-    if (qemu_init_main_loop(&main_loop_err)) {
-        error_report_err(main_loop_err);
-        exit(1);
-    }
+    qemu_init_main_loop(&error_fatal);
 
     if (qemu_opts_foreach(qemu_find_opts("sandbox"),
                           parse_sandbox, NULL, NULL)) {
-- 
2.5.5

[Qemu-devel] [PATCH 5/6] main-loop: Make qemu_init_main_loop() and qemu_signal_init() void

[Eduardo Habkost]

Their return values are not used by their callers anymore, so the
functions can be void.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/qemu/main-loop.h |  2 +-
 main-loop.c              | 21 ++++++++-------------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 19b5de3..572cf8e 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -42,7 +42,7 @@
  *
  * In the case of QEMU tools, this will also start/initialize timers.
  */
-int qemu_init_main_loop(Error **errp);
+void qemu_init_main_loop(Error **errp);
 
 /**
  * main_loop_wait: Run one iteration of the main loop.
diff --git a/main-loop.c b/main-loop.c
index 8e8eafc..90685ee 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
     }
 }
 
-static int qemu_signal_init(Error **errp)
+static void qemu_signal_init(Error **errp)
 {
     int sigfd;
     sigset_t set;
@@ -97,21 +97,18 @@ static int qemu_signal_init(Error **errp)
     sigfd = qemu_signalfd(&set);
     if (sigfd == -1) {
         error_setg_errno(errp, errno, "failed to create signalfd");
-        return -errno;
+        return;
     }
 
     fcntl_setfl(sigfd, O_NONBLOCK);
 
     qemu_set_fd_handler(sigfd, sigfd_handler, NULL, (void *)(intptr_t)sigfd);
-
-    return 0;
 }
 
 #else /* _WIN32 */
 
-static int qemu_signal_init(Error **errp)
+static void qemu_signal_init(Error **errp)
 {
-    return 0;
 }
 #endif
 
@@ -140,25 +137,24 @@ void qemu_notify_event(void)
 
 static GArray *gpollfds;
 
-int qemu_init_main_loop(Error **errp)
+void qemu_init_main_loop(Error **errp)
 {
-    int ret;
     GSource *src;
     Error *local_error = NULL;
 
     init_clocks();
 
-    ret = qemu_signal_init(&local_error);
-    if (ret) {
+    qemu_signal_init(&local_error);
+    if (local_error) {
         error_propagate(errp, local_error);
-        return ret;
+        return;
     }
 
     qemu_aio_context = aio_context_new(&local_error);
     qemu_notify_bh = qemu_bh_new(notify_event_cb, NULL);
     if (!qemu_aio_context) {
         error_propagate(errp, local_error);
-        return -EMFILE;
+        return;
     }
     gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
     src = aio_get_g_source(qemu_aio_context);
@@ -167,7 +163,6 @@ int qemu_init_main_loop(Error **errp)
     src = iohandler_get_g_source();
     g_source_attach(src, NULL);
     g_source_unref(src);
-    return 0;
 }
 
 static int max_priority;
-- 
2.5.5

[Qemu-devel] [PATCH 6/6] Use &error_fatal when initializing crypto on qemu-{img, io, nbd}

[Eduardo Habkost]

In addition to making the code simpler, this will replace the
long error messages:
  cannot initialize crypto: Unable to initialize GNUTLS library: [...]
  cannot initialize crypto: Unable to initialize gcrypt
with shorter messages:
  Unable to initialize GNUTLS library: [...]
  Unable to initialize gcrypt

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-img.c | 6 +-----
 qemu-io.c  | 6 +-----
 qemu-nbd.c | 5 +----
 3 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d3cfec3..5e09a6a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3450,7 +3450,6 @@ int main(int argc, char **argv)
 {
     const img_cmd_t *cmd;
     const char *cmdname;
-    Error *local_error = NULL;
     int c;
     static const struct option long_options[] = {
         {"help", no_argument, 0, 'h'},
@@ -3467,10 +3466,7 @@ int main(int argc, char **argv)
 
     qemu_init_main_loop(&error_fatal);
 
-    if (qcrypto_init(&local_error) < 0) {
-        error_reportf_err(local_error, "cannot initialize crypto: ");
-        exit(1);
-    }
+    qcrypto_init(&error_fatal);
 
     module_call_init(MODULE_INIT_QOM);
     bdrv_init();
diff --git a/qemu-io.c b/qemu-io.c
index 1507559..1a6478c 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -433,7 +433,6 @@ int main(int argc, char **argv)
     int opt_index = 0;
     int flags = BDRV_O_UNMAP;
     bool writethrough = true;
-    Error *local_error = NULL;
     QDict *opts = NULL;
     const char *format = NULL;
 
@@ -444,10 +443,7 @@ int main(int argc, char **argv)
     progname = basename(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
-    if (qcrypto_init(&local_error) < 0) {
-        error_reportf_err(local_error, "cannot initialize crypto: ");
-        exit(1);
-    }
+    qcrypto_init(&error_fatal);
 
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_object_opts);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index f2685e67..2a30e46 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -521,10 +521,7 @@ int main(int argc, char **argv)
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
 
-    if (qcrypto_init(&local_err) < 0) {
-        error_reportf_err(local_err, "cannot initialize crypto: ");
-        exit(1);
-    }
+    qcrypto_init(&error_fatal);
 
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_object_opts);
-- 
2.5.5

Re: [Qemu-devel] [PATCH 0/6] Use &error_fatal in some initialization code

[Paolo Bonzini]

> This changes some initialization code in vl.c, qemu-img, qemu-io,
> and qemu-nbd to use &error_fatal instead of manual error
> checking/reporting.
> 
> The series is based on my machine branch, available at:
>   git://github.com/ehabkost/qemu.git machine

Patches 1-2-6 are obvious.

I'm not sure about 3-4-5, these are internal functions where the error
is somewhere below "should never happen" (&error_abort)---it actually
should never happen, but exiting due to SIGABRT sounds wrong.  It's not
the kind of function that I'd expect to have an Error** argument...

Paolo

> Eduardo Habkost (6):
>   vl: Use &error_fatal when parsing VNC options
>   vl: Use &error_fatal when parsing monitor options
>   main-loop: Use Error** to report qemu_signal_init() errors
>   main-loop: Use &error_fatal when calling qemu_init_main_loop()
>   main-loop: Make qemu_init_main_loop() and qemu_signal_init() void
>   Use &error_fatal when initializing crypto on qemu-{img,io,nbd}
> 
>  include/qemu/main-loop.h |  2 +-
>  main-loop.c              | 24 ++++++++++--------------
>  qemu-img.c               | 11 ++---------
>  qemu-io.c                | 11 ++---------
>  qemu-nbd.c               | 10 ++--------
>  vl.c                     | 28 ++++------------------------
>  6 files changed, 21 insertions(+), 65 deletions(-)
> 
> --
> 2.5.5
> 
> 

Re: [Qemu-devel] [PATCH 0/6] Use &error_fatal in some initialization code

[Eduardo Habkost]

On Thu, May 12, 2016 at 12:48:41PM -0400, Paolo Bonzini wrote:
> 
> > This changes some initialization code in vl.c, qemu-img, qemu-io,
> > and qemu-nbd to use &error_fatal instead of manual error
> > checking/reporting.
> > 
> > The series is based on my machine branch, available at:
> >   git://github.com/ehabkost/qemu.git machine
> 
> Patches 1-2-6 are obvious.

Thanks. I will apply 1-2 to my machine branch. Should I apply
patch 6, too?

> 
> I'm not sure about 3-4-5, these are internal functions where the error
> is somewhere below "should never happen" (&error_abort)---it actually
> should never happen, but exiting due to SIGABRT sounds wrong.  It's not
> the kind of function that I'd expect to have an Error** argument...

Assuming you still want to keep the Error** argument in
qemu_init_main_loop(), I can send a simpler patch that doesn't
touch qemu_signal_init().

In that case, qemu_init_main_loop() at least needs to be
consistent, and always set *errp if returning non-zero (it
doesn't, and QEMU will crash inside error_report_err() if
qemu_signal_init() fail).

Or, if you prefer, we can remove the Error** argument from
qemu_init_main_loop() completely, and make it report errors to
stderr.

> 
> Paolo
> 
> > Eduardo Habkost (6):
> >   vl: Use &error_fatal when parsing VNC options
> >   vl: Use &error_fatal when parsing monitor options
> >   main-loop: Use Error** to report qemu_signal_init() errors
> >   main-loop: Use &error_fatal when calling qemu_init_main_loop()
> >   main-loop: Make qemu_init_main_loop() and qemu_signal_init() void
> >   Use &error_fatal when initializing crypto on qemu-{img,io,nbd}
> > 
> >  include/qemu/main-loop.h |  2 +-
> >  main-loop.c              | 24 ++++++++++--------------
> >  qemu-img.c               | 11 ++---------
> >  qemu-io.c                | 11 ++---------
> >  qemu-nbd.c               | 10 ++--------
> >  vl.c                     | 28 ++++------------------------
> >  6 files changed, 21 insertions(+), 65 deletions(-)
> > 
> > --
> > 2.5.5
> > 
> > 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/6] Use &error_fatal in some initialization code

[Paolo Bonzini]

On 12/05/2016 19:04, Eduardo Habkost wrote:
>>> This changes some initialization code in vl.c, qemu-img, qemu-io,
>>> > > and qemu-nbd to use &error_fatal instead of manual error
>>> > > checking/reporting.
>>> > > 
>>> > > The series is based on my machine branch, available at:
>>> > >   git://github.com/ehabkost/qemu.git machine
>> > 
>> > Patches 1-2-6 are obvious.
> Thanks. I will apply 1-2 to my machine branch. Should I apply
> patch 6, too?

Yes, please.

>> > I'm not sure about 3-4-5, these are internal functions where the error
>> > is somewhere below "should never happen" (&error_abort)---it actually
>> > should never happen, but exiting due to SIGABRT sounds wrong.  It's not
>> > the kind of function that I'd expect to have an Error** argument...
> Assuming you still want to keep the Error** argument in
> qemu_init_main_loop(), I can send a simpler patch that doesn't
> touch qemu_signal_init().

Sure, that's good!

Paolo