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

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

[Chrysostomos Nanakos]

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

[Chrysostomos Nanakos]

If event_notifier_init fails QEMU exits without printing
any error information to the user. This commit adds an error
message on failure:

 # qemu [...]
 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..e2b8968 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..a066dfc 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..01b765e 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 v3] async: aio_context_new(): Handle event_notifier_init failure

[Eric Blake]

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

On 09/16/2014 12:04 PM, Chrysostomos Nanakos wrote:
> If event_notifier_init fails QEMU exits without printing
> any error information to the user. This commit adds an error
> message on failure:
> 
>  # qemu [...]

Showing the actual command line you used would be helpful.

>  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(-)
> 

> -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);

Does g_source_destroy() guarantee that errno is unmolested? If not,

> +        error_setg_errno(errp, -ret, "Failed to initialize event notifier");

then this logs the wrong error. Swap the lines to be safe.

> +        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);

Is hoisting the event notifier init to occur before the mutex init going
to cause any grief?

> +++ b/main-loop.c

> @@ -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;
> +    }

Can the earlier call to qemu_signal_init() consume any resources that
are now leaked?

> @@ -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\'",

Use of \' inside "" is unusual. (Multiple times in your 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 v3] async: aio_context_new(): Handle event_notifier_init failure

[Benoît Canet]

The Tuesday 16 Sep 2014 à 13:40:24 (-0600), Eric Blake wrote :
> On 09/16/2014 12:04 PM, Chrysostomos Nanakos wrote:
> > If event_notifier_init fails QEMU exits without printing
> > any error information to the user. This commit adds an error
> > message on failure:
> > 
> >  # qemu [...]
> 
> Showing the actual command line you used would be helpful.
> 
> >  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(-)
> > 
> 
> > -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);
> 
> Does g_source_destroy() guarantee that errno is unmolested? If not,
> 
> > +        error_setg_errno(errp, -ret, "Failed to initialize event notifier");

Actually -ret is passed to error_setg_errno.

See.

void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,          
                     const char *fmt, ...) 
and
    if (os_errno != 0) {                                                        
            err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));  

> Use of \' inside "" is unusual. (Multiple times in your patch)

My fault.

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

[Chrysostomos Nanakos]

On 09/16/2014 10:40 PM, Eric Blake wrote:
> On 09/16/2014 12:04 PM, Chrysostomos Nanakos wrote:
>> If event_notifier_init fails QEMU exits without printing
>> any error information to the user. This commit adds an error
>> message on failure:
>>
>>   # qemu [...]
> Showing the actual command line you used would be helpful.

The problem raised after having a system with a low limit of open files 
and QEMU with 8 iothread objects. Do you believe that we should add such 
a command line in the commit description? The problem can be easily 
reproduced with any combination of low limit of open files and iothread 
objects or even a low limit of open files without the creation of iothreads.

>
>>   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(-)
>>
>> -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);
> Does g_source_destroy() guarantee that errno is unmolested? If not,
>
>> +        error_setg_errno(errp, -ret, "Failed to initialize event notifier");
> then this logs the wrong error. Swap the lines to be safe.

Good catch! I believe that Benoit has covered this point.

>
>> +        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);
> Is hoisting the event notifier init to occur before the mutex init going
> to cause any grief?

No I can't find any problem, none of the functions use the mutex on 
initialization. The mutex is being used for adding/removing BH's to/from 
the BH list. So it's safe at the moment to init mutex after the 
initialization of the event notifier.

>
>> +++ b/main-loop.c
>> @@ -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;
>> +    }
> Can the earlier call to qemu_signal_init() consume any resources that
> are now leaked?
  I can only see signal set manipulation and the setup of a signal 
handler. Nothing is leaked here.

>
>> @@ -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\'",
> Use of \' inside "" is unusual. (Multiple times in your patch)

I will fix that and if everyone agrees I will resend the patch.

Regards,
Chrysostomos.

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

[Eric Blake]

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

On 09/17/2014 03:16 AM, Chrysostomos Nanakos wrote:
> On 09/16/2014 10:40 PM, Eric Blake wrote:
>> On 09/16/2014 12:04 PM, Chrysostomos Nanakos wrote:
>>> If event_notifier_init fails QEMU exits without printing
>>> any error information to the user. This commit adds an error
>>> message on failure:
>>>
>>>   # qemu [...]
>> Showing the actual command line you used would be helpful.
> 
> The problem raised after having a system with a low limit of open files
> and QEMU with 8 iothread objects. Do you believe that we should add such
> a command line in the commit description? The problem can be easily
> reproduced with any combination of low limit of open files and iothread
> objects or even a low limit of open files without the creation of
> iothreads.

It is ALWAYS valuable to make your commit message give a good recipe for
how to reproduce the problem your commit is fixing.  It's not a
prerequisite (a patch can be accepted on inspection alone), but giving
full details of how to reproduce it makes life easier when revisiting
the patch a year from now or when writing a test case to ensure no
regressions.

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

[Chrysostomos Nanakos]

On 09/17/2014 04:01 PM, Eric Blake wrote:
> On 09/17/2014 03:16 AM, Chrysostomos Nanakos wrote:
>> On 09/16/2014 10:40 PM, Eric Blake wrote:
>>> On 09/16/2014 12:04 PM, Chrysostomos Nanakos wrote:
>>>> If event_notifier_init fails QEMU exits without printing
>>>> any error information to the user. This commit adds an error
>>>> message on failure:
>>>>
>>>>    # qemu [...]
>>> Showing the actual command line you used would be helpful.
>> The problem raised after having a system with a low limit of open files
>> and QEMU with 8 iothread objects. Do you believe that we should add such
>> a command line in the commit description? The problem can be easily
>> reproduced with any combination of low limit of open files and iothread
>> objects or even a low limit of open files without the creation of
>> iothreads.
> It is ALWAYS valuable to make your commit message give a good recipe for
> how to reproduce the problem your commit is fixing.  It's not a
> prerequisite (a patch can be accepted on inspection alone), but giving
> full details of how to reproduce it makes life easier when revisiting
> the patch a year from now or when writing a test case to ensure no
> regressions.
>

No problem Eric, thanks for the advice. I will make the appropriates 
changes to the commit log
and resend the patch.

Regards,
Chrysostomos.