From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54497) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTttJ-0006PI-Rs for qemu-devel@nongnu.org; Tue, 16 Sep 2014 10:38:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTttD-0000xf-7P for qemu-devel@nongnu.org; Tue, 16 Sep 2014 10:38:01 -0400 Received: from mail-we0-x235.google.com ([2a00:1450:400c:c03::235]:60547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTttC-0000wy-UE for qemu-devel@nongnu.org; Tue, 16 Sep 2014 10:37:55 -0400 Received: by mail-we0-f181.google.com with SMTP id w62so5875515wes.40 for ; Tue, 16 Sep 2014 07:37:51 -0700 (PDT) Date: Tue, 16 Sep 2014 15:37:48 +0100 From: Stefan Hajnoczi Message-ID: <20140916143748.GC10303@stefanha-thinkpad.redhat.com> References: <1410690193-21813-1-git-send-email-cnanakos@grnet.gr> <1410690193-21813-2-git-send-email-cnanakos@grnet.gr> <20140915190318.GC19397@irqsave.net> <20140915204403.GA490@apollo.local> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ghzN8eJ9Qlbqn3iT" Content-Disposition: inline In-Reply-To: <20140915204403.GA490@apollo.local> 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: =?iso-8859-1?Q?Beno=EEt?= Canet , 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 --ghzN8eJ9Qlbqn3iT Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 15, 2014 at 11:44:03PM +0300, Chrysostomos Nanakos wrote: > Hi Benoit, > thanks for reviewing and replying. >=20 > On Mon, Sep 15, 2014 at 09:03:18PM +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 > Mean either but I am pretty sure that aio_context_new is not guest trigge= red. It is not guest-triggerable. > >=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. >=20 > That was my second approach but I thought returning errno might be used b= y 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. > >=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. Please don't use errno unless the function already sets it. QEMU almost never uses errno. > > > 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 > I am leaking it for sure, I'll move it after the test. Thanks! >=20 > If anyone agrees I'll resend the patch with the proposed changes. Yes, please. Stefan --ghzN8eJ9Qlbqn3iT Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUGEs8AAoJEJykq7OBq3PI8M0IAID1ZunblM93fzA08iKxzP0A YSulkcV4etJbI1iQuHgqzmvS5N4wK8GP/bLz0mmngHJR2TG7Rxe5tREBfG5jIzJM Kkda7/ZZtIs0JyHpB8aN7f9tcWsD/gvxcxnVeIytF2nn+8/wzTNMUd3XrGukpHmb Kf1a5tk6xppw/ncgUZaup+xgGqD4tl2ZHUierOa4d93E/Y94DE3fj1ujGLjpkGnP XQu+rGj/7OhhSTAD6b4SKilSOetjk2yZpt3O4tF5hlRcF/GGMTfZNUjdwJQo5RhK SoEkTIGq7d7l9tIsBFey/Thd+gYOW6Xntdm7V1hi07Ianz7I4USxSVQ7g+LtIAc= =ZZKK -----END PGP SIGNATURE----- --ghzN8eJ9Qlbqn3iT--