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

["Benoît Canet" <benoit.canet@irqsave.net>]

The Sunday 14 Sep 2014 à 13:23:13 (+0300), 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 [...]
>  qemu: event_notifier_init failed: Too many open files in system
>  qemu: qemu_init_main_loop failed
> 
> Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr>
> ---
>  async.c                  |   19 +++++++++++++------
>  include/block/aio.h      |    2 +-
>  include/qemu/main-loop.h |    2 +-
>  iothread.c               |   12 +++++++++++-
>  main-loop.c              |   11 +++++++++--
>  qemu-img.c               |   11 ++++++++++-
>  qemu-io.c                |   10 +++++++++-
>  qemu-nbd.c               |   12 +++++++++---
>  tests/test-aio.c         |   12 +++++++++++-
>  tests/test-thread-pool.c |   10 +++++++++-
>  tests/test-throttle.c    |   12 +++++++++++-
>  vl.c                     |    7 +++++--
>  12 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/async.c b/async.c
> index a99e7f6..d4b6687 100644
> --- a/async.c
> +++ b/async.c
> @@ -289,21 +289,28 @@ static void aio_rfifolock_cb(void *opaque)
>      aio_notify(opaque);
>  }
>  
> -AioContext *aio_context_new(void)
> +int aio_context_new(AioContext **context, 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, "event_notifier_init failed");

aio_context_new does not seems to be guest triggered so it may be actually correct
to bake an error message in it. I don't have enough AIO knowledge to juge this.

However your current error message: "event_notifier_init failed" look more like
a trace than an actual QEMU error message.

Please grep the code for example error messages: they are usually more plaintext
than this one

Also switching to returning a -errno make the caller's code convoluted.
Maybe returning NULL would be enough.

I see further in the patch that the return code is not really used on the
top most level maybe it's a hint that return NULL here would be better.

> +        return ret;
> +    }
> +    aio_set_event_notifier(ctx, &ctx->notifier,
> +                           (EventNotifierHandler *)
> +                           event_notifier_test_and_clear);
>      iothread->stopping = false;
> -    iothread->ctx = aio_context_new();
> +    ret = aio_context_new(&iothread->ctx, &local_error);
> +    if (ret < 0) {

> +        errno = -ret;
You don't seem to reuse errno further down in the code.

>      gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
> -    qemu_aio_context = aio_context_new();
> +    ret = aio_context_new(&qemu_aio_context, &local_error);
> +    if (ret < 0) {

> +        errno = -ret;

Same here errno does not seems used by error propagate.

Also you seems to leak gpollfds but probably you count
on the fact that the kernel will collect it for you
because QEMU will die.

I think you could move down the gpollfds = g_array_new
after the end of the test.

> +        error_propagate(errp, local_error);
> +        return ret;
> +    }
> +
>  
> -    qemu_init_main_loop();
> +    ret = qemu_init_main_loop(&local_error);
> +    if (ret < 0) {
> +        errno = -ret;
> +        error_report("%s", error_get_pretty(local_error));
> +        error_free(local_error);
> +        error_exit("qemu_init_main_loop failed");
> +    }
> +
>      bdrv_init();
>      if (argc < 2) {
>          error_exit("Not enough arguments");
> diff --git a/qemu-io.c b/qemu-io.c
> index 33c96c4..f10dab5 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -387,8 +387,10 @@ int main(int argc, char **argv)
>          { NULL, 0, NULL, 0 }
>      };
>      int c;
> +    int ret;
>      int opt_index = 0;
>      int flags = BDRV_O_UNMAP;
> +    Error *local_error = NULL;
>  
>  #ifdef CONFIG_POSIX
>      signal(SIGPIPE, SIG_IGN);
> @@ -454,7 +456,13 @@ int main(int argc, char **argv)
>          exit(1);
>      }
>  
> -    qemu_init_main_loop();
> +    ret = qemu_init_main_loop(&local_error);
> +    if (ret < 0) {
> +        error_report("%s", error_get_pretty(local_error));
> +        error_report("qemu_init_main_loop failed");
> +        error_free(local_error);
> +        return ret;
> +    }
>      bdrv_init();
>  
>      /* initialize commands */
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9bc152e..4ba9635 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -498,13 +498,13 @@ int main(int argc, char **argv)
>                                  BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
>                                  &local_err);
>              if (local_err) {
> -                errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", 
> +                errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
>                       error_get_pretty(local_err));
>              }
>              if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>                  !(flags & BDRV_O_UNMAP)) {
>                  errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
> -                                   "without setting discard operation to unmap"); 
> +                                   "without setting discard operation to unmap");
>              }
>              break;
>          case 'b':
> @@ -674,7 +674,13 @@ int main(int argc, char **argv)
>          snprintf(sockpath, 128, SOCKET_PATH, basename(device));
>      }
>  
> -    qemu_init_main_loop();
> +    ret = qemu_init_main_loop(&local_err);
> +    if (ret < 0) {
> +        errno = -ret;
> +        error_report("%s", error_get_pretty(local_err));
> +        error_free(local_err);
> +        err(EXIT_FAILURE, "qemu_init_main_loop failed");
> +    }
>      bdrv_init();
>      atexit(bdrv_close_all);
>  
> diff --git a/tests/test-aio.c b/tests/test-aio.c
> index c6a8713..a9c9073 100644