Re: [Qemu-devel] [PATCH] qdev: free qemu-opts when the QOM path goes away

[Andreas Färber]

Am 04.11.2015 um 19:34 schrieb Markus Armbruster:
> Paolo Bonzini <address@hidden> 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 <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
> 
> 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)