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 <cnana...@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
pgp2StJDboiUs.pgp
Description: PGP signature
