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

[Markus Armbruster]

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?

>           You cannot have a device that goes away at the end of
> qdev_realize_and_unref, as long as dev has a QOM parent that clings onto
> dev.  Since dev will have /machine/unattached as the parent if it didn't
> already have one before, the function is safe even without a bus.

Write me a nice function contract, and I'll update the implementation to
match it.

> 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 :)

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

I chose to outlaw a case I see no use for, to keep the contract simpler.