Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices

[Paolo Bonzini]

On 25/05/20 08:38, Markus Armbruster wrote:
> Paolo Bonzini <address@hidden> writes:
> 
>> On 20/05/20 17:02, Markus Armbruster wrote:
>>>>>
>>>>> qdev_realize_and_unref() remains restricted, because its reference
>>>>> counting would become rather confusing for bus-less devices.
>>>> I think it would be fine, you would just rely on the reference held by
>>>> the QOM parent (via the child property).
>>> I took one look at the contract I wrote for it, and balked :)
>>>
>>> qdev_realize()'s contract before this patch:
>>>
>>>     /*
>>>      * Realize @dev.
>>>      * @dev must not be plugged into a bus.
>>>      * Plug @dev into @bus.  This takes a reference to @dev.
>>>      * If @dev has no QOM parent, make one up, taking another reference.
>>>      * On success, return true.
>>>      * On failure, store an error through @errp and return false.
>>>      */
>>>     bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
>>>
>>> Simple enough.
>>>
>>> This patch merely adds "If @bus, " before "plug".  Still simple enough.
>>>
>>> qdev_realize_and_unref()'s contract:
>>>
>>>     /*
>>>      * Realize @dev and drop a reference.
>>>      * This is like qdev_realize(), except it steals a reference rather
>>>      * than take one to plug @dev into @bus.  On failure, it drops that
>>>      * reference instead.  @bus must not be null.  Intended use:
>>>      *     dev = qdev_new();
>>>      *     [...]
>>>      *     qdev_realize_and_unref(dev, bus, errp);
>>>      * Now @dev can go away without further ado.
>>>      */
>>>     bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error 
>>> **errp)
>>>
>>> If @bus is null, who gets to hold the stolen reference?
>>>
>>> You seem to suggest the QOM parent.  What if @dev already has a parent?
>>
>> The caller would still hold the stolen reference, and it would be
>> dropped.
> 
> I read this sentence three times, and still don't get it.  Is the
> reference held or is it dropped?

To call qdev_realize_and_unref, you need to have your own reference,
which you probably got from qdev_new.

The function might add one via object_property_add_child or it might
not; it might add one via qdev_set_parent_bus or it might not.  But in
any case, when it returns you won't have a reference anymore.

One possibility is to think of it in terms of stealing the reference and
passing it to the bus.  However, as in the lifetime phases that I posted
earlier, once you realize a device you are no longer in charge of its
lifetime.  Instead, the unparent callback will take care of unrealizing
the device and dropping all outstanding long-living references.

So...

>> Or alternatively, ignore all the stolen references stuff, and merely see
>> qdev_realize_and_unref as a shortcut for qdev_realize+object_unref,
>> because it's a common idiom.
> 
> Even common idioms need to make sense :)

... that's why the common idiom makes sense.

> The contract must specify exactly what happens to the reference count,
> case by case.

For both qdev_realize and qdev_realize_and_unref, on return the caller
need not care about keeping alive the device in the long-term.

For qdev_realize_and_unref, the caller must _also_ have a "private"
reference to the object, which will be dropped on return.

For qdev_realize, the caller _can_ have a private reference that it has
to later drop at a convenient time, but it could also ensure that the
device has a long-term reference via object->parent instead.

Perhaps this tells us that the /machine/unattached automation actually
shouldn't be moved to qdev_realize, but rather to
qdev_realize_and_unref, and qdev_realize could assert that there is a
parent already at the time of the call.  However it is probably too
early to make a decision on that.

Paolo