Il 22/08/2012 18:36, Anthony Liguori ha scritto: > 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.
I think that's correct. Unplugging needs to remove the circular reference before object_delete is called. > This is the same problem we have with object_unparent. No, it's not. Parent-to-child is a one-way relationship, it cannot have circular references. It is also a "cosmetic" relationship, in that the void * is usually not accessed except when using QOM paths. If child properties are known to the parent (they are not, for example, when the parent is a TYPE_CONTAINER), the parent knows how to reach the child using a C expression. So unparenting at object_delete time (not before) makes sense. > 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). There is no qdev_device_del, but there is qdev_unplug. We could rename qdev_unplug to qdev_request_unplug, and qdev_simple_unplug_cb to qdev_unplug. > 2) Make DeviceState::unplug *never* call qdev_free(). Right. It should always go through qdev_simple_unplug_cb. > 3) Add an "unplugged" NotifierList to DeviceState. Is this really needed? > 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. This too, is it needed?... qdev_simple_unplug_cb could simply be the place where you call qdev_set_parent_bus(qdev, NULL), before qdev_free. That would break the circular link and keep object_delete() happy. > 5) Change qdev_device_del() to add a notifier to the object that calls > object_unparent() and object_unref. No need. > 6) Rename DeviceState::unplug to DeviceState::request_unplug Cosmetic, but I agree. :) > 7) Take Ping Fan's patch + another patch to add a reference count in > object_property_add_link Yes. BTW, the patch to fix usb_del is on its way. Paolo
