On Thu, Aug 23, 2012 at 12:36 AM, Anthony Liguori <anth...@codemonkey.ws> wrote: > Paolo Bonzini <pbonz...@redhat.com> writes: > >> Il 22/08/2012 05:02, Liu Ping Fan ha scritto: >>> From: Liu Ping Fan <pingf...@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 <pingf...@linux.vnet.ibm.com> >> >> Acked-by: Paolo Bonzini <pbonz...@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
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); >>> } >>>