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

[Paolo Bonzini]

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