[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices |
Date: |
Tue, 26 May 2020 07:14:42 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Paolo Bonzini <address@hidden> writes:
> 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.
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.
- [PATCH 37/55] sysbus: Drop useless OBJECT() in sysbus_init_child_obj() calls, (continued)
- [PATCH 37/55] sysbus: Drop useless OBJECT() in sysbus_init_child_obj() calls, Markus Armbruster, 2020/05/19
- [PATCH 25/55] usb: New usb_new(), usb_realize_and_unref(), Markus Armbruster, 2020/05/19
- [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Markus Armbruster, 2020/05/19
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Paolo Bonzini, 2020/05/20
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Markus Armbruster, 2020/05/20
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Paolo Bonzini, 2020/05/20
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Markus Armbruster, 2020/05/25
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Paolo Bonzini, 2020/05/25
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices,
Markus Armbruster <=
- Re: [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices, Paolo Bonzini, 2020/05/26
[PATCH 47/55] sysbus: Convert qdev_set_parent_bus() use with Coccinelle, part 3, Markus Armbruster, 2020/05/19
[PATCH 26/55] usb: Convert uses of usb_create(), Markus Armbruster, 2020/05/19
[PATCH 35/55] macio: Convert use of qdev_set_parent_bus(), Markus Armbruster, 2020/05/19
[PATCH 36/55] macio: Eliminate macio_init_child_obj(), Markus Armbruster, 2020/05/19
[PATCH 42/55] sysbus: New sysbus_realize(), sysbus_realize_and_unref(), Markus Armbruster, 2020/05/19
[PATCH 22/55] ssi: Convert uses of ssi_create_slave_no_init() with Coccinelle, Markus Armbruster, 2020/05/19
[PATCH 27/55] usb: usb_create() is now unused, drop, Markus Armbruster, 2020/05/19