Re: [PATCH for-11.2 v3 00/15] qdev: Clarify and enforce the device realization lifecycle

Akihiko Odaki <[email protected]> writes:

> qdev currently represents a device's realization state with a single
> boolean. This cannot distinguish a device that has never been realized
> from one whose realization has failed or that has been unrealized, nor
> can it represent realization in progress. Consequently, the same device
> can enter DeviceClass::realize() reentrantly or more than once.

.realized is initially false.  It is only ever modified in
device_set_realized(), which is the setter of QOM property "realized" of
"device" and its subtypes.  device_set_realized() is only called when
the property is set.  It does nothing when the new value is the same as
the old value.

Code changes property "realized" only in qdev_realize() and
qdev_unrealize().

Implementations of .realize may call qdev_realize() for their
components.  Having this loop back would be a bug.

If your claim "can enter reentrantly" is correct, we have bugs to fix.
I believe it is incorrect.

There are two kinds of devices, onboard and user-created.

User-created devices go through qdev_device_add_from_qdict().  If
qdev_realize() fails, the device is immediately destroyed.

Onboard devices get created and realized by board code.  It commonly
treats qdev_realize() failure as fatal error.  Trying again instead
would be a bug.

If your claim "can enter more than once" is correct, we have bugs to
fix.  I believe it is incorrect.

Exception, sort of: users can manipulate properties with QMP command
qom-set.  This is generally unsupported and a Very Bad Idea[*].  There
might be supported exceptions (I don't know), but "realized" is
definitely not among them.  Unsurprisingly, it's a fast path to grief:

    $ qemu-system-x86_64 -S -display none -monitor stdio -nodefaults
    QEMU 11.0.90 monitor - type 'help' for more information
    (qemu) qom-set /machine/i440fx realized false
    (qemu) qom-set /machine/i440fx realized true
    qemu-system-x86_64: ../system/memory.c:2585: memory_region_add_subregion_common: Assertion `!subregion->container' failed.

I figure your series rejects the second qom-set.  I'm pretty sure the
first qom-set already wounds the VM fatally[**].  Same as for a
multitude of other properties that aren't prepared to be qom-set at
arbitrary times.

Mind, I'm not objecting to adding additional guards against .realize()
getting called more than exactly once.  I'm only challenging your cover
letter, and probably your commit messages (which I haven't read).

[...]


[*] One could argue that having command qom-set is a Very Bad Idea.

[**] It just doesn't crash a stopped VM right away for me.