From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41338) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTcRU-0003c0-5s for qemu-devel@nongnu.org; Mon, 15 Sep 2014 16:00:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTcRP-00013Z-3o for qemu-devel@nongnu.org; Mon, 15 Sep 2014 16:00:08 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:54992 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTcRO-0000vC-QP for qemu-devel@nongnu.org; Mon, 15 Sep 2014 16:00:03 -0400 Date: Mon, 15 Sep 2014 21:59:00 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140915195900.GA3365@irqsave.net> References: <1410690193-21813-1-git-send-email-cnanakos@grnet.gr> <1410690193-21813-2-git-send-email-cnanakos@grnet.gr> <20140915190318.GC19397@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140915190318.GC19397@irqsave.net> 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: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: Chrysostomos Nanakos , kwolf@redhat.com, pingfank@linux.vnet.ibm.com, famz@redhat.com, stefanha@redhat.com, jan.kiszka@siemens.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, kroosec@gmail.com, sw@weilnetz.de, pbonzini@redhat.com, afaerber@suse.de, aliguori@amazon.com The Monday 15 Sep 2014 =E0 21:03:18 (+0200), Beno=EEt Canet wrote : > 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(Ai= oContext)); > > + 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"); >=20 > aio_context_new does not seems to be guest triggered so it may be actua= lly correct > to bake an error message in it. I don't have enough AIO knowledge to ju= ge this. >=20 > However your current error message: "event_notifier_init failed" look m= ore like > a trace than an actual QEMU error message. >=20 > Please grep the code for example error messages: they are usually more = plaintext > than this one >=20 > Also switching to returning a -errno make the caller's code convoluted. > Maybe returning NULL would be enough. >=20 > I see further in the patch that the return code is not really used on t= he > top most level maybe it's a hint that return NULL here would be better. I though a bit more about this. You could just do if (ret < 0) { g_source_destroy(&ctx->source); error_setg_errno(errp, -ret, "event_notifier_init failed"); errno =3D -ret; return NULL; } when the test fail. This way you have both a straightforward return path _and_ a regular errno usage to propagate it to the callers if you need so. >=20 > > + 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) { >=20 > > + errno =3D -ret; > You don't seem to reuse errno further down in the code. >=20 > > 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) { >=20 > > + errno =3D -ret; >=20 > Same here errno does not seems used by error propagate. >=20 > 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. >=20 > I think you could move down the gpollfds =3D g_array_new > after the end of the test. >=20 > > + 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 mo= de: %s",=20 > > + errx(EXIT_FAILURE, "Failed to parse detect_zeroes mo= de: %s", > > error_get_pretty(local_err)); > > } > > if (detect_zeroes =3D=3D BLOCKDEV_DETECT_ZEROES_OPTIONS_= UNMAP && > > !(flags & BDRV_O_UNMAP)) { > > errx(EXIT_FAILURE, "setting detect-zeroes to unmap i= s not allowed " > > - "without setting discard operatio= n to unmap");=20 > > + "without setting discard operatio= n 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 >=20