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