From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56725) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJRIL-0002RB-Fl for qemu-devel@nongnu.org; Tue, 10 Sep 2013 13:00:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJRIE-0005YS-6P for qemu-devel@nongnu.org; Tue, 10 Sep 2013 13:00:05 -0400 Received: from cantor2.suse.de ([195.135.220.15]:53927 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJRID-0005YL-Tb for qemu-devel@nongnu.org; Tue, 10 Sep 2013 12:59:58 -0400 Message-ID: <522F5008.1060202@suse.de> Date: Tue, 10 Sep 2013 18:59:52 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1378830072-28926-1-git-send-email-stefanha@redhat.com> <1378830072-28926-3-git-send-email-stefanha@redhat.com> <522F4D9B.9030801@redhat.com> In-Reply-To: <522F4D9B.9030801@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Paolo Bonzini , Michael Tokarev , qemu-devel@nongnu.org, Anthony Liguori , Markus Armbruster Am 10.09.2013 18:49, schrieb Paolo Bonzini: > Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto: >> qdev_device_add() leaks the created qdev upon failure. I suspect this >> problem crept in because qdev_free() unparents the qdev but does not >> drop a reference - confusing name. >=20 > Right, the name a leftover from pre-refcounting days. >=20 > BTW, not dropping a reference is the right thing to do because the > reference is dropped much earlier, typically as soon as qdev_device_add > returns. The QOM object tree then will still provide means to access > devices, until they are unparented. >=20 > In this case, however, qdev_device_add's caller does not have a > reference to free; doing that is the responsibility of qdev_device_add, > since it returns NULL. >=20 >> Also drop trailing whitespace after curly bracket. >> >> Signed-off-by: Stefan Hajnoczi >> --- >> qdev-monitor.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 410cdcb..5657cdc 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> } >> if (qemu_opt_foreach(opts, set_property, qdev, 1) !=3D 0) { >> qdev_free(qdev); >> + object_unref(OBJECT(qdev)); >> return NULL; >> } >> if (qdev->id) { Given that qdev_free() doesn't do what one might expect, I would suggest to s/qdev_free/object_unparent/g above. >> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> object_property_add_child(qdev_get_peripheral_anon(), name, >> OBJECT(qdev), NULL); >> g_free(name); >> - } =20 >> + } >> if (qdev_init(qdev) < 0) { >> + object_unref(OBJECT(qdev)); >> qerror_report(QERR_DEVICE_INIT_FAILED, driver); >> return NULL; >> } >> >=20 > Reviewed-by: Paolo Bonzini I would like to take this through qom-next tree since I have pending variable cleanups there ("qdev" being touched here). Not sure how to handle that wrt block changes in this series? Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg