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

[Anthony Liguori]

Hi Paolo,

Paolo Bonzini <address@hidden> 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