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

[Benoît Canet]

The Monday 15 Sep 2014 à 21:03:18 (+0200), Benoît Canet wrote :
> 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 <address@hidden>
> > ---
> >  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.

I though a bit more about this.

You could just do

    if (ret < 0) {
         g_source_destroy(&ctx->source);
         error_setg_errno(errp, -ret, "event_notifier_init failed");
         errno = -ret;
         return NULL;
    }

when the test fail.

This way you have both a straightforward return path _and_
a regular errno usage to propagate it to the callers if you need so.

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