On 05/11/2015 13:06, Andreas Färber wrote: > Am 04.11.2015 um 19:34 schrieb Markus Armbruster: >> Paolo Bonzini <pbonz...@redhat.com> 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 <m...@redhat.com> >>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> >> >> 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