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

[Chrysostomos Nanakos]

On 09/16/2014 10:40 PM, Eric Blake wrote:
> On 09/16/2014 12:04 PM, 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 [...]
> Showing the actual command line you used would be helpful.

The problem raised after having a system with a low limit of open files 
and QEMU with 8 iothread objects. Do you believe that we should add such 
a command line in the commit description? The problem can be easily 
reproduced with any combination of low limit of open files and iothread 
objects or even a low limit of open files without the creation of iothreads.

> >   qemu: Failed to initialize event notifier: Too many open files in system
> >
> > Signed-off-by: Chrysostomos Nanakos <address@hidden>
> > ---
> >   async.c                  |   16 +++++++++++-----
> >   include/block/aio.h      |    2 +-
> >   include/qemu/main-loop.h |    2 +-
> >   iothread.c               |   11 ++++++++++-
> >   main-loop.c              |    9 +++++++--
> >   qemu-img.c               |    8 +++++++-
> >   qemu-io.c                |    7 ++++++-
> >   qemu-nbd.c               |    6 +++++-
> >   tests/test-aio.c         |   10 +++++++++-
> >   tests/test-thread-pool.c |   10 +++++++++-
> >   tests/test-throttle.c    |   10 +++++++++-
> >   vl.c                     |    5 +++--
> >   12 files changed, 78 insertions(+), 18 deletions(-)
> >
> > -AioContext *aio_context_new(void)
> > +AioContext *aio_context_new(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);
> Does g_source_destroy() guarantee that errno is unmolested? If not,
>
> > +        error_setg_errno(errp, -ret, "Failed to initialize event notifier");
> then this logs the wrong error. Swap the lines to be safe.

Good catch! I believe that Benoit has covered this point.

> > +        return NULL;
> > +    }
> > +    aio_set_event_notifier(ctx, &ctx->notifier,
> > +                           (EventNotifierHandler *)
> > +                           event_notifier_test_and_clear);
> >       ctx->pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
> >       ctx->thread_pool = NULL;
> >       qemu_mutex_init(&ctx->bh_lock);
> >       rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx);
> > -    event_notifier_init(&ctx->notifier, false);
> Is hoisting the event notifier init to occur before the mutex init going
> to cause any grief?

No I can't find any problem, none of the functions use the mutex on 
initialization. The mutex is being used for adding/removing BH's to/from 
the BH list. So it's safe at the moment to init mutex after the 
initialization of the event notifier.

> > +++ b/main-loop.c
> > @@ -138,8 +139,12 @@ int qemu_init_main_loop(void)
> >           return ret;
> >       }
> >   
> > +    qemu_aio_context = aio_context_new(&local_error);
> > +    if (!qemu_aio_context) {
> > +        error_propagate(errp, local_error);
> > +        return -1;
> > +    }
> Can the earlier call to qemu_signal_init() consume any resources that
> are now leaked?
 I can only see signal set manipulation and the setup of a signal 
handler. Nothing is leaked here.

> > @@ -205,10 +206,17 @@ static void test_cancel(void)
> >   int main(int argc, char **argv)
> >   {
> >       int ret;
> > +    Error *local_error;
> >   
> >       init_clocks();
> >   
> > -    ctx = aio_context_new();
> > +    ctx = aio_context_new(&local_error);
> > +    if (!ctx) {
> > +        error_report("Failed to create AIO Context: \'%s\'",
> Use of \' inside "" is unusual. (Multiple times in your patch)

I will fix that and if everyone agrees I will resend the patch.

Regards,
Chrysostomos.