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. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)