From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53588) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTbZf-0001EB-Bn for qemu-devel@nongnu.org; Mon, 15 Sep 2014 15:04:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTbZa-0001SP-7P for qemu-devel@nongnu.org; Mon, 15 Sep 2014 15:04:31 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:53892 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTbZZ-0001RB-Tp for qemu-devel@nongnu.org; Mon, 15 Sep 2014 15:04:26 -0400 Date: Mon, 15 Sep 2014 21:03:18 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140915190318.GC19397@irqsave.net> References: <1410690193-21813-1-git-send-email-cnanakos@grnet.gr> <1410690193-21813-2-git-send-email-cnanakos@grnet.gr> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1410690193-21813-2-git-send-email-cnanakos@grnet.gr> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v1] async: aio_context_new(): Handle event_notifier_init failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chrysostomos Nanakos Cc: kwolf@redhat.com, pingfank@linux.vnet.ibm.com, famz@redhat.com, kroosec@gmail.com, jan.kiszka@siemens.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, stefanha@redhat.com, sw@weilnetz.de, pbonzini@redhat.com, afaerber@suse.de, aliguori@amazon.com The Sunday 14 Sep 2014 =E0 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: >=20 > # qemu [...] > qemu: event_notifier_init failed: Too many open files in system > qemu: qemu_init_main_loop failed >=20 > Signed-off-by: Chrysostomos Nanakos > --- > 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(-) >=20 > 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); > } > =20 > -AioContext *aio_context_new(void) > +int aio_context_new(AioContext **context, Error **errp) > { > + int ret; > AioContext *ctx; > ctx =3D (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioC= ontext)); > + ret =3D 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 actuall= y 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 mor= e like a trace than an actual QEMU error message. Please grep the code for example error messages: they are usually more pl= aintext 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. > + return ret; > + } > + aio_set_event_notifier(ctx, &ctx->notifier, > + (EventNotifierHandler *) > + event_notifier_test_and_clear); > iothread->stopping =3D false; > - iothread->ctx =3D aio_context_new(); > + ret =3D aio_context_new(&iothread->ctx, &local_error); > + if (ret < 0) { > + errno =3D -ret; You don't seem to reuse errno further down in the code. > gpollfds =3D g_array_new(FALSE, FALSE, sizeof(GPollFD)); > - qemu_aio_context =3D aio_context_new(); > + ret =3D aio_context_new(&qemu_aio_context, &local_error); > + if (ret < 0) { > + errno =3D -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 =3D g_array_new after the end of the test. > + error_propagate(errp, local_error); > + return ret; > + } > + > =20 > - qemu_init_main_loop(); > + ret =3D qemu_init_main_loop(&local_error); > + if (ret < 0) { > + errno =3D -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 =3D 0; > int flags =3D BDRV_O_UNMAP; > + Error *local_error =3D NULL; > =20 > #ifdef CONFIG_POSIX > signal(SIGPIPE, SIG_IGN); > @@ -454,7 +456,13 @@ int main(int argc, char **argv) > exit(1); > } > =20 > - qemu_init_main_loop(); > + ret =3D 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(); > =20 > /* 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",=20 > + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode= : %s", > error_get_pretty(local_err)); > } > if (detect_zeroes =3D=3D BLOCKDEV_DETECT_ZEROES_OPTIONS_UN= MAP && > !(flags & BDRV_O_UNMAP)) { > errx(EXIT_FAILURE, "setting detect-zeroes to unmap is = not allowed " > - "without setting discard operation = to unmap");=20 > + "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)); > } > =20 > - qemu_init_main_loop(); > + ret =3D qemu_init_main_loop(&local_err); > + if (ret < 0) { > + errno =3D -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); > =20 > diff --git a/tests/test-aio.c b/tests/test-aio.c > index c6a8713..a9c9073 100644