From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35225) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHbbT-0007RH-H4 for qemu-devel@nongnu.org; Fri, 08 Jan 2016 13:17:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHbbO-0007wO-Gx for qemu-devel@nongnu.org; Fri, 08 Jan 2016 13:17:35 -0500 Received: from mail-wm0-x232.google.com ([2a00:1450:400c:c09::232]:34535) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHbbO-0007wH-9z for qemu-devel@nongnu.org; Fri, 08 Jan 2016 13:17:30 -0500 Received: by mail-wm0-x232.google.com with SMTP id u188so146900533wmu.1 for ; Fri, 08 Jan 2016 10:17:30 -0800 (PST) Sender: Paolo Bonzini References: <1445253099-16894-1-git-send-email-pbonzini@redhat.com> <87ziytehr2.fsf@blackfin.pond.sub.org> <563B4629.10903@suse.de> From: Paolo Bonzini Message-ID: <568FFD32.5040904@redhat.com> Date: Fri, 8 Jan 2016 19:17:22 +0100 MIME-Version: 1.0 In-Reply-To: <563B4629.10903@suse.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit 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: =?UTF-8?Q?Andreas_F=c3=a4rber?= , Markus Armbruster Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On 05/11/2015 13:06, Andreas Färber wrote: > Am 04.11.2015 um 19:34 schrieb Markus Armbruster: >> Paolo Bonzini writes: >> >>> 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 >> >> Let's see whether I understand this. >> >>> --- >>> 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; >>> >>> DeviceState *dev = DEVICE(obj); >>> - qemu_opts_del(dev->opts); >>> >>> 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, &error_abort); >> >> DEVICE_DELETED sent here. >> >>> g_free(path); >>> } >>> + >>> + qemu_opts_del(dev->opts); >>> + dev->opts = NULL; >>> } >>> >>> static void device_class_init(ObjectClass *class, void *data) >> >> object_finalize_child_property() runs during unplug: >> >> static void object_finalize_child_property(Object *obj, const char *name, >> void *opaque) >> { >> Object *child = opaque; >> >> if (child->class->unparent) { >> (child->class->unparent)(child); <--- calls device_unparent() >> } >> child->parent = NULL; >> object_unref(child); <--- calls device_finalize() >> } >> >> 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. >> >> Two remarks: >> >> 1. Wouldn't it be cleaner to delete dev-opts *before* sending >> DEVICE_DELETED? Like this: >> >> +++ b/hw/core/qdev.c >> @@ -1244,6 +1244,9 @@ static void device_unparent(Object *obj) >> dev->parent_bus = NULL; >> } >> >> + qemu_opts_del(dev->opts); >> + dev->opts = NULL; >> + >> /* Only send event if the device had been completely realized */ >> if (dev->pending_deleted_event) { >> gchar *path = 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? >> >> See also https://bugzilla.redhat.com/show_bug.cgi?id=1256044 > > 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. Ping? Paolo