Re: Infinite loop in bus_unparent(), qdev bug or qdev misuse?

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 05/05/20 18:03, Markus Armbruster wrote:
>>> That's a good one, and especially a safe one, since it matches
>>> qdev_device_add.  It has the disadvantage of having to touch all
>>> qdev_create() calls.
>> 
>> Also, it moves onboard devices from /machine/unattached/ to
>> /machine/peripheral-anon/.
>
> Uh indeed.  No that's too ugly.
>
>>> Even better however would be to move the bus argument (and thus
>>> qdev_set_parent_bus) to qdev_init, and likewise in qdev_device_add move
>>> qdev_set_id after qemu_opt_foreach.  I looked at the property setters
>>> and couldn't find anything suspicious (somewhat to my surprise), but I
>>> haven't honestly tried.
>> 
>> Thus, we satisfy bus_unparent()'s precondition "bus children have a QOM
>> parent"[*] by moving "add to parent bus" next to the place where we
>> ensure "has QOM parent" by putting orphans under /machine/unattached/.
>> Makes sense.
>> 
>> If we add to the bus first, the precondition ceases to hold until we
>> realize.  Ugly, but harmless unless we manage to actually call the
>> function then.
>
> Shouldn't be a big deal, since users should call either qdev_set_id or
> object_property_add_child before device_set_realized.

The issue isn't neglecting to set a QOM parent, it's destroying a device
before its bus children get their QOM parent.

Mostly harmless: by delaying "add to bus" until right before realize, we
narrow the window where the trap is armed, and keep it completely within
qdev_init_nofail(), qdev_device_add(), and possibly code that duplicates
their work.  Ensuring qdev_init_nofail() and qdev_device_add() don't
fail in this window should be easy enough (except for writing the
comment explaining it).  The duplicates, though... I guess we need to
double-check users of qdev_set_parent_bus().

Ugly: yes, compared to the pretty invariant "bus children all have QOM
parents".

>> I suspect we can't realize first, because the realize method may want to
>> use the parent bus.
>
> Right.
>
> Moving the bus to qdev_init would be quite large but hopefully scriptable.

Feels feasible.


[*] We might want to look into deduplicating: the string "realized"
occurs more than 450 times, and I figure almost always as property name.