From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41884) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuJIv-0007KA-Lw for qemu-devel@nongnu.org; Thu, 05 Nov 2015 07:06:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuJIp-00088C-H8 for qemu-devel@nongnu.org; Thu, 05 Nov 2015 07:06:09 -0500 Received: from mx2.suse.de ([195.135.220.15]:37326) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuJIp-00087a-5n for qemu-devel@nongnu.org; Thu, 05 Nov 2015 07:06:03 -0500 References: <1445253099-16894-1-git-send-email-pbonzini@redhat.com> <87ziytehr2.fsf@blackfin.pond.sub.org> From: =?UTF-8?Q?Andreas_F=c3=a4rber?= Message-ID: <563B4629.10903@suse.de> Date: Thu, 5 Nov 2015 13:06:01 +0100 MIME-Version: 1.0 In-Reply-To: <87ziytehr2.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Paolo Bonzini Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" Am 04.11.2015 um 19:34 schrieb Markus Armbruster: > Paolo Bonzini writes: >=20 >> Otherwise there is a race where the DEVICE_DELETED event has been sent= but >> attempts to reuse the ID will fail. >> >> Reported-by: Michael S. Tsirkin >> Signed-off-by: Paolo Bonzini >=20 > Let's see whether I understand this. >=20 >> --- >> hw/core/qdev.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 4ab04aa..92bd8bb 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -1203,7 +1203,6 @@ static void device_finalize(Object *obj) >> NamedGPIOList *ngl, *next; >> =20 >> DeviceState *dev =3D DEVICE(obj); >> - qemu_opts_del(dev->opts); >> =20 >> QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { >> QLIST_REMOVE(ngl, node); >> @@ -1251,6 +1250,9 @@ static void device_unparent(Object *obj) >> qapi_event_send_device_deleted(!!dev->id, dev->id, path, &err= or_abort); >=20 > DEVICE_DELETED sent here. >=20 >> g_free(path); >> } >> + >> + qemu_opts_del(dev->opts); >> + dev->opts =3D NULL; >> } >> =20 >> static void device_class_init(ObjectClass *class, void *data) >=20 > object_finalize_child_property() runs during unplug: >=20 > static void object_finalize_child_property(Object *obj, const char = *name, > void *opaque) > { > Object *child =3D opaque; >=20 > if (child->class->unparent) { > (child->class->unparent)(child); <--- calls device_unparen= t() > } > child->parent =3D NULL; > object_unref(child); <--- calls device_finaliz= e() > } >=20 > device_unparent() sends DEVICE_DELETED, but dev->opts gets only deleted > later, in device_finalize. If the client tries to reuse the ID in the > meantime, it fails. >=20 > Two remarks: >=20 > 1. Wouldn't it be cleaner to delete dev-opts *before* sending > DEVICE_DELETED? Like this: >=20 > +++ b/hw/core/qdev.c > @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj) > dev->parent_bus =3D NULL; > } >=20 > + qemu_opts_del(dev->opts); > + dev->opts =3D NULL; > + > /* Only send event if the device had been completely realized = */ > if (dev->pending_deleted_event) { > gchar *path =3D object_get_canonical_path(OBJECT(dev)); To me this proposal sounds sane, but I did not get to tracing the code flow here. Paolo, which approach do you prefer and why? > 2. If the device is a block device, then unplugging it also deletes its > backend (ugly wart we keep for backward compatibility; *not* for > blockdev-add, though). This backend also has a QemuOpts. It gets > deleted in drive_info_del(). Just like device_finalize(), it runs > within object_unref(), i.e. after DEVICE_DELETED is sent. Same race= , > different ID, or am I missing something? >=20 > See also https://bugzilla.redhat.com/show_bug.cgi?id=3D1256044 If we can leave this patch decoupled from block layer and decide soonish on the desired approach, I'd be happy to include it in my upcoming qom-devices pull. Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Graham Norton; HRB 21284 (AG N=FC= rnberg)