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

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Mon, Sep 15, 2014 at 11:44:03PM +0300, Chrysostomos Nanakos wrote:
> 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.

It is not guest-triggerable.

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

I agree, AioContext *aio_context_new(Error **errp) is simpler.

The callers don't need to know the errno because they don't act on its
value, just use error_setg_errno() so the error message captures it.

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

Please don't use errno unless the function already sets it.  QEMU almost
never uses errno.

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

Yes, please.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]