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)
