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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

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

Look, a rabbit hole :)

I got rough patches, with several more to do.

There's one thing worth mentioning.  Unrealize is recursive: unrealizing
a qdev recurses into its qbuses, and unrealizing a qbus recurses into
its qdevs.  Realize, however, is TODO recursive :)  See your commit
5942a19040 "qdev: recursively unrealize devices when unrealizing bus",
2014-06-19.

Moving "put on qbus" from qdev_create() (and its wrappers) to
qdev_init_nofail() means we put on bus by realizing.  No use to
recursive realization then,

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

I wield a sharp hatchet.