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

[Chrysostomos Nanakos <cnanakos@grnet.gr>]

On Mon, Sep 15, 2014 at 09:59:00PM +0200, Benoît Canet wrote:
> 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 <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.
> 
> 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.
> 

I agree, it seems to be much cleaner and straightforward. Thanks again.

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