Re: [PATCH 3/5] qdev: Make device_set_realized() always safe in tests

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Nov 18, 2024 at 05:13:28PM -0500, Peter Xu wrote:
> Currently, a device can be realized even before machine is created, but
> only in one of QEMU's qtest, test-global-qdev-props.c.
> 
> Right now, the test_static_prop_subprocess() test (which creates one simple
> object without machine created) will internally make "/machine" to be a
> container, which may not be expected when developing the test.
> 
> Now explicitly support that case when there's no real "/machine" object
> around, then unattached devices will be put under root ("/") rather than
> "/machine".  Mostly only for this single test case, or for any future test
> cases when some device needs to be realized before the machine is present.
> 
> This shouldn't affect anything else when QEMU runs as an emulator, as that
> always relies on a real machine being created before realizing any devices.
> It's because if "/machine" is wrongly created as a container, it'll fail
> QEMU very soon later on qemu_create_machine() trying to create the real
> machine, conflicting with the "/machine" container.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/qdev.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5f13111b77..eff297e584 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -475,9 +475,17 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>          if (!obj->parent) {
>              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> +            Object *root = qdev_get_machine();
>  
> -            object_property_add_child(container_get(qdev_get_machine(),
> -                                                    "/unattached"),
> +            /*
> +             * We could have qdev test cases trying to realize() a device
> +             * without machine created.  In that case we use the root.
> +             */
> +            if (!root) {
> +                root = object_get_root();
> +            }

IMHO modifying the qdev.c code to workaround limitations of the test suite
is not a nice approach. Even if it is more work, I'd say it is better to
properly stub a /machine object in the test case, so that it complies with
expectations of qdev.c

> +
> +            object_property_add_child(container_get(root, "/unattached"),
>                                        name, obj);
>              unattached_parent = true;
>              g_free(name);

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|