Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target

[Anthony Liguori <anthony@codemonkey.ws>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> 
>> Currently, link property's target is only managed by
>> object_set_link_property(). This will raise such issue that when
>> the property is finalized, its target has no opportunity to release.
>> 
>> Fix this issue by introduce object_finalize_link_property()
>> 
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

This patch is correct but it uncovers a bigger problem.  Short of
reworking most of the hotplug code, I can't find an easy fix.

The first problem is that with this patch, all links are unreferenced at
property removal.  Right now, bus children are added as links but when
they are added, the link is already set (there is no explicit set).
This means that those links never get the extra reference.

We can fix this by adding an extra reference in add_link but this
creates yet another problem with hotplug.  Specificially, qdev_free()
asserts that ref > 0 because there is now a reference being held by the
bus.

This is the same problem we have with object_unparent.

The key problem here is how unplug is implemented.  Unplug wants to be
both synchronous and asynchronous.

I think we need to do the following:

1) Move object_unparent to qdev_device_del (the parent is added by
   qdev_device_add so this is quite logical).

2) Make DeviceState::unplug *never* call qdev_free().

3) Add an "unplugged" NotifierList to DeviceState.

4) Change the various hotplug consumers to call qdev_set_parent_bus() to
   NULL to unplug the device from the bus.  Change qdev_set_parent_bus()
   to allow this and remove the bus link and invoke the unplugged notifier.

5) Change qdev_device_del() to add a notifier to the object that calls
   object_unparent() and object_unref.

6) Rename DeviceState::unplug to DeviceState::request_unplug

7) Take Ping Fan's patch + another patch to add a reference count in
   object_property_add_link

Regards,

Anthony Liguori

>
> Paolo
>
>> ---
>>  qom/object.c |   12 +++++++++++-
>>  1 files changed, 11 insertions(+), 1 deletions(-)
>> 
>> diff --git a/qom/object.c b/qom/object.c
>> index a552be2..76b3d34 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -957,6 +957,16 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>>      }
>>  }
>>  
>> +static void object_finalize_link_property(Object *obj, const char *name,
>> +                                           void *opaque)
>> +{
>> +    Object **child = opaque;
>> +
>> +    if (*child != NULL) {
>> +        object_unref(*child);
>> +    }
>> +}
>> +
>>  void object_property_add_link(Object *obj, const char *name,
>>                                const char *type, Object **child,
>>                                Error **errp)
>> @@ -968,7 +978,7 @@ void object_property_add_link(Object *obj, const char *name,
>>      object_property_add(obj, name, full_type,
>>                          object_get_link_property,
>>                          object_set_link_property,
>> -                        NULL, child, errp);
>> +                        object_finalize_link_property, child, errp);
>>  
>>      g_free(full_type);
>>  }
>>