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

[liu ping fan]

On Thu, Aug 23, 2012 at 12:36 AM, Anthony Liguori <address@hidden> wrote:
> Paolo Bonzini <address@hidden> writes:
>
>> Il 22/08/2012 05:02, Liu Ping Fan ha scritto:
>>> From: Liu Ping Fan <address@hidden>
>>>
>>> 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 <address@hidden>
>>
>> Acked-by: Paolo Bonzini <address@hidden>
>
> 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

Yeah, I was wondering about why it does not like 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
>
I like this good name!

> 7) Take Ping Fan's patch + another patch to add a reference count in
>    object_property_add_link
>
In these days, I had thought about the way to do the hot unplug like
the following:
Suppose we start from devA
qdev_tree_delete(devA)
{
  qdev_walk_children(devA,  qdev_device_del,  qdev_bus_del, NULL)
  qdev_device_del(devA)
}

For qdev_device_del() do the following things:
--. delete link from parent bus
--. detached from parent bus->children
--. dev->parent_bus = NULL
--. object_unref(dev);

For qdev_bus_del(),
-- delete child property of parent
-- detach from parent->child_bus
-- bus->parent = NULL
-- object_unref(bus)

So leave anything handled by object_unref(), no call to qdev_free and so on

Thanks and regards,
pingfan
> 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);
>>>  }
>>>