Hi Paolo, Paolo Bonzini <pbonz...@redhat.com> writes:
> 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. Let's ignore parents in this discussion. I didn't mean to open up this debate. >> 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, I meant qmp_device_del and qmp_device_add. Sorry, typo on my parent. > but there is qdev_unplug. We could rename > qdev_unplug to qdev_request_unplug, and qdev_simple_unplug_cb to > qdev_unplug. I'm trying to separate the following things: 1) Requesting a device to be unplugged 2) Detaching a device from the bus 3) Deleting the device (1) happens based on issuing the qmp_device_del monitor command. (2) is typically only done based on a guest action but sometimes as a direct result of (1). (3) Should happen when all pending references are dropped. Normally, the bus holds a reference to a device, the container holds a reference, and an additional reference floats and is obstensively owned by the monitor. (1) should drop the floating reference and the reference held by the container. That's what I meant by calling object_unparent in qmp_device_del. (2) should simply remove the device from the bus (further releasing a reference). (3) would happen automatically from (1) and (2) if they were called in that order. If the guest instantiates a remove on it's own, the device would be disconnected from the bus (functionally unplugged) but still in the container so it would *not* go away. I think this is desirable behavior. >> 2) Make DeviceState::unplug *never* call qdev_free(). > > Right. It should always go through qdev_simple_unplug_cb. qdev_simple_unplug_cb() should just qdev_set_parent_bus(NULL). Or just drop that function entirely and call the above. That will drop a reference and *may* free the object. (Yes, I'm aware that object_unref doesn't free--that needs to be fixed too). >> 3) Add an "unplugged" NotifierList to DeviceState. > > Is this really needed? Probably can just be a QMP event. I'd really like to find a bridge between notifier lists and qmp events so that the same event can be consumed by through the management API and programmatically though... >> 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. Yeah, whether it's done via a wrapper like qdev_simple_unplug_cb() doesn't matter that much. >> 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. Cool, thanks! Regards, Anthony Liguori > > Paolo