So far, qdev_realize() supports only devices that plug into a bus:
argument @bus cannot be null.  Extend it to support bus-less devices,
too.

qdev_realize_and_unref() remains restricted, because its reference
counting would become rather confusing for bus-less devices.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 323b6328c8..0662bbc812 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -408,7 +408,7 @@ void qdev_init_nofail(DeviceState *dev)
 /*
  * Realize @dev.
  * @dev must not be plugged into a bus.
- * Plug @dev into @bus.  This takes a reference to @dev.
+ * If @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.
@@ -418,9 +418,12 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
     Error *err = NULL;
 
     assert(!dev->realized && !dev->parent_bus);
-    assert(bus);
 
-    qdev_set_parent_bus(dev, bus);
+    if (bus) {
+        qdev_set_parent_bus(dev, bus);
+    } else {
+        assert(!DEVICE_GET_CLASS(dev)->bus_type);
+    }
 
     object_ref(OBJECT(dev));
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
@@ -442,7 +445,7 @@ void qdev_unrealize(DeviceState *dev)
  * 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.  Intended use:
+ * reference instead.  @bus must not be null.  Intended use:
  *     dev = qdev_new();
  *     [...]
  *     qdev_realize_and_unref(dev, bus, errp);
@@ -452,6 +455,8 @@ bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp)
 {
     bool ret;
 
+    assert(bus);
+
     ret = qdev_realize(dev, bus, errp);
     object_unref(OBJECT(dev));
     return ret;
-- 
2.21.1

On 19/05/20 16:55, Markus Armbruster wrote:
> So far, qdev_realize() supports only devices that plug into a bus:
> argument @bus cannot be null.  Extend it to support bus-less devices,
> too.
> 
> 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).

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 19/05/20 16:55, Markus Armbruster wrote:
>> So far, qdev_realize() supports only devices that plug into a bus:
>> argument @bus cannot be null.  Extend it to support bus-less devices,
>> too.
>> 
>> 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?

Oh, sod it, let's go shopping.

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.  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.

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.

Paolo

Paolo Bonzini <pbonzini@redhat.com> 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.

On 25/05/20 08:38, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> 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

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/05/20 08:38, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> 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.

I need a contract.  The difficulty of writing a clear contract, caused
by a case that doesn't actually occur, is what made me limit null bus to
qdev_realize().  I admittedly didn't try hard.  Next try:

    /*
     * Realize @dev and drop a reference.
     * This is like qdev_realize(), except the caller must hold a
     * (private) reference, which is dropped on return regardless of
     * success or failure.  Intended use:
     *     dev = qdev_new();
     *     [...]
     *     qdev_realize_and_unref(dev, bus, errp);
     * Now @dev can go away without further ado.
     */

> 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.

The common pairings are qdev_new() with qdev_realize_and_unref(), and
object_initialize_child() with qdev_realize().  Your idea obviously
works for these.  Whether there are other uses where it might not work,
I can't say offhand.

On 26/05/20 07:14, Markus Armbruster wrote:
>>> 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.
> 
> I need a contract.  The difficulty of writing a clear contract, caused
> by a case that doesn't actually occur, is what made me limit null bus to
> qdev_realize().  I admittedly didn't try hard.  Next try:
> 
>     /*
>      * Realize @dev and drop a reference.
>      * This is like qdev_realize(), except the caller must hold a
>      * (private) reference, which is dropped on return regardless of
>      * success or failure.  Intended use:
>      *     dev = qdev_new();
>      *     [...]
>      *     qdev_realize_and_unref(dev, bus, errp);
>      * Now @dev can go away without further ado.
>      */

Works for me!

>> 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.
> 
> The common pairings are qdev_new() with qdev_realize_and_unref(), and
> object_initialize_child() with qdev_realize().  Your idea obviously
> works for these.  Whether there are other uses where it might not work,
> I can't say offhand.

Yes, let's look at it after this is committed.  But I think it is at
least sensible, in the long term, for the *_new variants or their
callers to all take care of adding the child, and then
qdev_realize_and_unref() can go away.

Paolo

Paolo