From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJRMp-0004A8-FI for qemu-devel@nongnu.org; Tue, 10 Sep 2013 13:04:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJRMj-00074q-G5 for qemu-devel@nongnu.org; Tue, 10 Sep 2013 13:04:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJRMj-00074i-6v for qemu-devel@nongnu.org; Tue, 10 Sep 2013 13:04:37 -0400 Message-ID: <522F5128.7070005@redhat.com> Date: Tue, 10 Sep 2013 19:04:40 +0200 From: Paolo Bonzini 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> <522F5008.1060202@suse.de> In-Reply-To: <522F5008.1060202@suse.de> 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: =?ISO-8859-15?Q?Andreas_F=E4rber?= Cc: Anthony Liguori , Michael Tokarev , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster Il 10/09/2013 18:59, Andreas F=E4rber ha scritto: > 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 thi= s >>> problem crept in because qdev_free() unparents the qdev but does not >>> drop a reference - confusing name. >> >> Right, the name a leftover from pre-refcounting days. >> >> BTW, not dropping a reference is the right thing to do because the >> reference is dropped much earlier, typically as soon as qdev_device_ad= d >> returns. The QOM object tree then will still provide means to access >> devices, until they are unparented. >> >> 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. >> >>> 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) { >=20 > Given that qdev_free() doesn't do what one might expect, I would sugges= t > to s/qdev_free/object_unparent/g above. Then do it everywhere... Paolo >>> @@ -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; >>> } >>> >> >> Reviewed-by: Paolo Bonzini >=20 > 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? >=20 > Andreas >=20