[Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Chrysostomos Nanakos]

v3->v4
------
* Remove escaped single quotes from error messages.
* Rephrase commit log.

v2->v3
------
* Remove errno usage and print the detailed message based on errno when
  event_notifier_init() fails.
* Propagate error and return from iothread_complete() if aio_context_new() fails.
* Return if !iothread->ctx from iothread_instance_finalize(), used by QOM
  when object_unref(obj) is called after user_creatable_complete() fails.
* Remove cosmetic fixes accidentally introduced by editor and fix code style
  issues.

v1->v2
------
* aio_context_new() returns NULL if the initialization of event notifier fails.
* Add descriptive error messages if aio_context_new() and event_notifier_init()
  fail.
* Fix gpollfds leak.


Chrysostomos Nanakos (1):
  async: aio_context_new(): Handle event_notifier_init failure

 async.c                  |   16 +++++++++++-----
 include/block/aio.h      |    2 +-
 include/qemu/main-loop.h |    2 +-
 iothread.c               |   11 ++++++++++-
 main-loop.c              |    9 +++++++--
 qemu-img.c               |    8 +++++++-
 qemu-io.c                |    7 ++++++-
 qemu-nbd.c               |    6 +++++-
 tests/test-aio.c         |   10 +++++++++-
 tests/test-thread-pool.c |   10 +++++++++-
 tests/test-throttle.c    |   10 +++++++++-
 vl.c                     |    5 +++--
 12 files changed, 78 insertions(+), 18 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Chrysostomos Nanakos]

On a system with a low limit of open files the initialization
of the event notifier could fail and QEMU exits without printing any
error information to the user.

The problem can be easily reproduced by enforcing a low limit of open
files and start QEMU with enough I/O threads to hit this limit.

The same problem raises, without the creation of I/O threads, while
QEMU initializes the main event loop by enforcing an even lower limit of
open files.

This commit adds an error message on failure:

 # qemu [...] -object iothread,id=iothread0 -object iothread,id=iothread1
 qemu: Failed to initialize event notifier: Too many open files in system

Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr>
---
 async.c                  |   16 +++++++++++-----
 include/block/aio.h      |    2 +-
 include/qemu/main-loop.h |    2 +-
 iothread.c               |   11 ++++++++++-
 main-loop.c              |    9 +++++++--
 qemu-img.c               |    8 +++++++-
 qemu-io.c                |    7 ++++++-
 qemu-nbd.c               |    6 +++++-
 tests/test-aio.c         |   10 +++++++++-
 tests/test-thread-pool.c |   10 +++++++++-
 tests/test-throttle.c    |   10 +++++++++-
 vl.c                     |    5 +++--
 12 files changed, 78 insertions(+), 18 deletions(-)

diff --git a/async.c b/async.c
index a99e7f6..6e1b282 100644
--- a/async.c
+++ b/async.c
@@ -289,18 +289,24 @@ static void aio_rfifolock_cb(void *opaque)
     aio_notify(opaque);
 }
 
-AioContext *aio_context_new(void)
+AioContext *aio_context_new(Error **errp)
 {
+    int ret;
     AioContext *ctx;
     ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext));
+    ret = event_notifier_init(&ctx->notifier, false);
+    if (ret < 0) {
+        g_source_destroy(&ctx->source);
+        error_setg_errno(errp, -ret, "Failed to initialize event notifier");
+        return NULL;
+    }
+    aio_set_event_notifier(ctx, &ctx->notifier,
+                           (EventNotifierHandler *)
+                           event_notifier_test_and_clear);
     ctx->pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
     ctx->thread_pool = NULL;
     qemu_mutex_init(&ctx->bh_lock);
     rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx);
-    event_notifier_init(&ctx->notifier, false);
-    aio_set_event_notifier(ctx, &ctx->notifier, 
-                           (EventNotifierHandler *)
-                           event_notifier_test_and_clear);
     timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx);
 
     return ctx;
diff --git a/include/block/aio.h b/include/block/aio.h
index 4603c0f..f3d5517 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -99,7 +99,7 @@ void aio_set_dispatching(AioContext *ctx, bool dispatching);
  * They also provide bottom halves, a service to execute a piece of code
  * as soon as possible.
  */
-AioContext *aio_context_new(void);
+AioContext *aio_context_new(Error **errp);
 
 /**
  * aio_context_ref:
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 6f0200a..62c68c0 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(void);
+int qemu_init_main_loop(Error **errp);
 
 /**
  * main_loop_wait: Run one iteration of the main loop.
diff --git a/iothread.c b/iothread.c
index d9403cf..6e394a0 100644
--- a/iothread.c
+++ b/iothread.c
@@ -17,6 +17,7 @@
 #include "block/aio.h"
 #include "sysemu/iothread.h"
 #include "qmp-commands.h"
+#include "qemu/error-report.h"
 
 #define IOTHREADS_PATH "/objects"
 
@@ -53,6 +54,9 @@ static void iothread_instance_finalize(Object *obj)
 {
     IOThread *iothread = IOTHREAD(obj);
 
+    if (!iothread->ctx) {
+        return;
+    }
     iothread->stopping = true;
     aio_notify(iothread->ctx);
     qemu_thread_join(&iothread->thread);
@@ -63,10 +67,15 @@ static void iothread_instance_finalize(Object *obj)
 
 static void iothread_complete(UserCreatable *obj, Error **errp)
 {
+    Error *local_error = NULL;
     IOThread *iothread = IOTHREAD(obj);
 
     iothread->stopping = false;
-    iothread->ctx = aio_context_new();
+    iothread->ctx = aio_context_new(&local_error);
+    if (!iothread->ctx) {
+        error_propagate(errp, local_error);
+        return;
+    }
     iothread->thread_id = -1;
 
     qemu_mutex_init(&iothread->init_done_lock);
diff --git a/main-loop.c b/main-loop.c
index 3cc79f8..2f83d80 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -126,10 +126,11 @@ void qemu_notify_event(void)
 
 static GArray *gpollfds;
 
-int qemu_init_main_loop(void)
+int qemu_init_main_loop(Error **errp)
 {
     int ret;
     GSource *src;
+    Error *local_error = NULL;
 
     init_clocks();
 
@@ -138,8 +139,12 @@ int qemu_init_main_loop(void)
         return ret;
     }
 
+    qemu_aio_context = aio_context_new(&local_error);
+    if (!qemu_aio_context) {
+        error_propagate(errp, local_error);
+        return -1;
+    }
     gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
-    qemu_aio_context = aio_context_new();
     src = aio_get_g_source(qemu_aio_context);
     g_source_attach(src, NULL);
     g_source_unref(src);
diff --git a/qemu-img.c b/qemu-img.c
index 91d1ac3..dbf0904 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2879,6 +2879,7 @@ 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'},
@@ -2893,7 +2894,12 @@ int main(int argc, char **argv)
     error_set_progname(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
-    qemu_init_main_loop();
+    if (qemu_init_main_loop(&local_error)) {
+        error_report("%s", error_get_pretty(local_error));
+        error_free(local_error);
+        exit(EXIT_FAILURE);
+    }
+
     bdrv_init();
     if (argc < 2) {
         error_exit("Not enough arguments");
diff --git a/qemu-io.c b/qemu-io.c
index d2ab694..66cf3ef 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -379,6 +379,7 @@ int main(int argc, char **argv)
     int c;
     int opt_index = 0;
     int flags = BDRV_O_UNMAP;
+    Error *local_error = NULL;
 
 #ifdef CONFIG_POSIX
     signal(SIGPIPE, SIG_IGN);
@@ -444,7 +445,11 @@ int main(int argc, char **argv)
         exit(1);
     }
 
-    qemu_init_main_loop();
+    if (qemu_init_main_loop(&local_error)) {
+        error_report("%s", error_get_pretty(local_error));
+        error_free(local_error);
+        exit(1);
+    }
     bdrv_init();
 
     /* initialize commands */
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9bc152e..de9963f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -674,7 +674,11 @@ int main(int argc, char **argv)
         snprintf(sockpath, 128, SOCKET_PATH, basename(device));
     }
 
-    qemu_init_main_loop();
+    if (qemu_init_main_loop(&local_err)) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        exit(EXIT_FAILURE);
+    }
     bdrv_init();
     atexit(bdrv_close_all);
 
diff --git a/tests/test-aio.c b/tests/test-aio.c
index c6a8713..029716f 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -14,6 +14,7 @@
 #include "block/aio.h"
 #include "qemu/timer.h"
 #include "qemu/sockets.h"
+#include "qemu/error-report.h"
 
 static AioContext *ctx;
 
@@ -810,11 +811,18 @@ static void test_source_timer_schedule(void)
 
 int main(int argc, char **argv)
 {
+    Error *local_error;
     GSource *src;
 
     init_clocks();
 
-    ctx = aio_context_new();
+    ctx = aio_context_new(&local_error);
+    if (!ctx) {
+        error_report("Failed to create AIO Context: '%s'",
+                     error_get_pretty(local_error));
+        error_free(local_error);
+        exit(1);
+    }
     src = aio_get_g_source(ctx);
     g_source_attach(src, NULL);
     g_source_unref(src);
diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
index f40b7fc..57c0836 100644
--- a/tests/test-thread-pool.c
+++ b/tests/test-thread-pool.c
@@ -4,6 +4,7 @@
 #include "block/thread-pool.h"
 #include "block/block.h"
 #include "qemu/timer.h"
+#include "qemu/error-report.h"
 
 static AioContext *ctx;
 static ThreadPool *pool;
@@ -205,10 +206,17 @@ static void test_cancel(void)
 int main(int argc, char **argv)
 {
     int ret;
+    Error *local_error;
 
     init_clocks();
 
-    ctx = aio_context_new();
+    ctx = aio_context_new(&local_error);
+    if (!ctx) {
+        error_report("Failed to create AIO Context: '%s'",
+                     error_get_pretty(local_error));
+        error_free(local_error);
+        exit(1);
+    }
     pool = aio_get_thread_pool(ctx);
 
     g_test_init(&argc, &argv, NULL);
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 000ae31..42141a0 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -14,6 +14,7 @@
 #include <math.h>
 #include "block/aio.h"
 #include "qemu/throttle.h"
+#include "qemu/error-report.h"
 
 static AioContext     *ctx;
 static LeakyBucket    bkt;
@@ -492,10 +493,17 @@ static void test_accounting(void)
 int main(int argc, char **argv)
 {
     GSource *src;
+    Error *local_error;
 
     init_clocks();
 
-    ctx = aio_context_new();
+    ctx = aio_context_new(&local_error);
+    if (!ctx) {
+        error_report("Failed to create AIO Context: '%s'",
+                     error_get_pretty(local_error));
+        error_free(local_error);
+        exit(1);
+    }
     src = aio_get_g_source(ctx);
     g_source_attach(src, NULL);
     g_source_unref(src);
diff --git a/vl.c b/vl.c
index 5db0d08..4711d00 100644
--- a/vl.c
+++ b/vl.c
@@ -2968,6 +2968,7 @@ int main(int argc, char **argv, char **envp)
     ram_addr_t maxram_size = default_ram_size;
     uint64_t ram_slots = 0;
     FILE *vmstate_dump_file = NULL;
+    Error *main_loop_err = NULL;
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
@@ -3998,8 +3999,8 @@ int main(int argc, char **argv, char **envp)
 
     os_daemonize();
 
-    if (qemu_init_main_loop()) {
-        fprintf(stderr, "qemu_init_main_loop failed\n");
+    if (qemu_init_main_loop(&main_loop_err)) {
+        error_report("%s", error_get_pretty(main_loop_err));
         exit(1);
     }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Andreas Färber]

Hi,

As a general comment, when sending a cover letter for a single patch,
please use --numbered, so that the cover letter becomes [PATCH 0/1] and
the patch [PATCH 1/1] so they don't end up with identical subjects.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Eric Blake]

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

On 09/17/2014 07:51 AM, Andreas Färber wrote:
> Hi,
> 
> As a general comment, when sending a cover letter for a single patch,
> please use --numbered, so that the cover letter becomes [PATCH 0/1] and
> the patch [PATCH 1/1] so they don't end up with identical subjects.

Or, when sending a single patch, omit the cover letter entirely, and
stick what would have been the cover letter after the --- of the lone patch.

-- 
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 v4] async: aio_context_new(): Handle event_notifier_init failure

[Nanakos Chrysostomos]

> On 17 Σεπ 2014, at 4:51 μ.μ., Andreas Färber <afaerber@suse.de> wrote:
> 
> Hi,
> 
> As a general comment, when sending a cover letter for a single patch,
> please use --numbered, so that the cover letter becomes [PATCH 0/1] and
> the patch [PATCH 1/1] so they don't end up with identical subjects.
> 


My fault. 

Re: [Qemu-devel] [PATCH v4] async: aio_context_new(): Handle event_notifier_init failure

[Stefan Hajnoczi]

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

On Wed, Sep 17, 2014 at 04:48:34PM +0300, Chrysostomos Nanakos wrote:
> diff --git a/iothread.c b/iothread.c
> index d9403cf..6e394a0 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -17,6 +17,7 @@
>  #include "block/aio.h"
>  #include "sysemu/iothread.h"
>  #include "qmp-commands.h"
> +#include "qemu/error-report.h"
>  
>  #define IOTHREADS_PATH "/objects"
>  
> @@ -53,6 +54,9 @@ static void iothread_instance_finalize(Object *obj)
>  {
>      IOThread *iothread = IOTHREAD(obj);
>  
> +    if (!iothread->ctx) {
> +        return;
> +    }
>      iothread->stopping = true;
>      aio_notify(iothread->ctx);
>      qemu_thread_join(&iothread->thread);
> @@ -63,10 +67,15 @@ static void iothread_instance_finalize(Object *obj)
>  
>  static void iothread_complete(UserCreatable *obj, Error **errp)
>  {
> +    Error *local_error = NULL;
>      IOThread *iothread = IOTHREAD(obj);
>  
>      iothread->stopping = false;
> -    iothread->ctx = aio_context_new();
> +    iothread->ctx = aio_context_new(&local_error);
> +    if (!iothread->ctx) {
> +        error_propagate(errp, local_error);
> +        return;
> +    }
>      iothread->thread_id = -1;

Please move this under ->stopping = false so that ->thread_id is
initialized.  That way qmp_query_iothreads() will display thread_id -1
for failed IOThread objects instead of an uninitialized value.

Normally QEMU should exit or the IOThread object should be deleted
before anyone has a chance to call qmp_query_iothreads() but you never
know so let's get the lifecycle right...

> diff --git a/main-loop.c b/main-loop.c
> index 3cc79f8..2f83d80 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -126,10 +126,11 @@ void qemu_notify_event(void)
>  
>  static GArray *gpollfds;
>  
> -int qemu_init_main_loop(void)
> +int qemu_init_main_loop(Error **errp)
>  {
>      int ret;
>      GSource *src;
> +    Error *local_error = NULL;
>  
>      init_clocks();
>  
> @@ -138,8 +139,12 @@ int qemu_init_main_loop(void)
>          return ret;
>      }
>  
> +    qemu_aio_context = aio_context_new(&local_error);
> +    if (!qemu_aio_context) {
> +        error_propagate(errp, local_error);
> +        return -1;

This function returns -errno so -1 would be -EPERM.  Please use an
actual errno constant like -EMFILE.

> diff --git a/tests/test-aio.c b/tests/test-aio.c
> index c6a8713..029716f 100644
> --- a/tests/test-aio.c
> +++ b/tests/test-aio.c
> @@ -14,6 +14,7 @@
>  #include "block/aio.h"
>  #include "qemu/timer.h"
>  #include "qemu/sockets.h"
> +#include "qemu/error-report.h"
>  
>  static AioContext *ctx;
>  
> @@ -810,11 +811,18 @@ static void test_source_timer_schedule(void)
>  
>  int main(int argc, char **argv)
>  {
> +    Error *local_error;

local_error = NULL

It must be NULL otherwise error_setg() will read uninitialized memory
(it checks that the error hasn't been set yet because Error objects can
only be set once).

> diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
> index f40b7fc..57c0836 100644
> --- a/tests/test-thread-pool.c
> +++ b/tests/test-thread-pool.c
> @@ -4,6 +4,7 @@
>  #include "block/thread-pool.h"
>  #include "block/block.h"
>  #include "qemu/timer.h"
> +#include "qemu/error-report.h"
>  
>  static AioContext *ctx;
>  static ThreadPool *pool;
> @@ -205,10 +206,17 @@ static void test_cancel(void)
>  int main(int argc, char **argv)
>  {
>      int ret;
> +    Error *local_error;

Same.

> diff --git a/tests/test-throttle.c b/tests/test-throttle.c
> index 000ae31..42141a0 100644
> --- a/tests/test-throttle.c
> +++ b/tests/test-throttle.c
> @@ -14,6 +14,7 @@
>  #include <math.h>
>  #include "block/aio.h"
>  #include "qemu/throttle.h"
> +#include "qemu/error-report.h"
>  
>  static AioContext     *ctx;
>  static LeakyBucket    bkt;
> @@ -492,10 +493,17 @@ static void test_accounting(void)
>  int main(int argc, char **argv)
>  {
>      GSource *src;
> +    Error *local_error;

Same.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]