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

[Chrysostomos Nanakos <cnanakos@grnet.gr>]

Hi Benoit,
thanks for reviewing and replying.

On Mon, Sep 15, 2014 at 09:03:18PM +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 <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.

Mean either but I am pretty sure that aio_context_new is not guest triggered.
> 
> 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

Yes you are right, I will try to find something more descriptive.

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

That was my second approach but I thought returning errno might be used by the
caller. Returning NULL maybe is the way to go then.

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

I am leaking it for sure, I'll move it after the test. Thanks!

If anyone agrees I'll resend the patch with the proposed changes.

Regards,
Chrysostomos.

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