Re: [PATCH 08/12] qdev: Make qdev_get_machine() not use container_get()

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

On Wed, Nov 20, 2024 at 04:56:59PM -0500, Peter Xu wrote:
> Currently, qdev_get_machine() has a slight misuse on container_get(), as
> the helper says "get a container" but in reality the goal is to get the
> machine object.  It is still a "container" but not strictly.
> 
> Note that it _may_ get a container (at "/machine") in our current unit test
> of test-qdev-global-props.c before all these changes, but it's probably
> unexpected and worked by accident.
> 
> Switch to an explicit object_resolve_path_component(), with a side benefit
> that qdev_get_machine() can happen a lot, and we don't need to split the
> string ("/machine") every time.  This also paves way for making the helper
> container_get() never try to return a non-container at all.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/qdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5f13111b77..c869c47a27 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -817,7 +817,13 @@ Object *qdev_get_machine(void)
>      static Object *dev;
>  
>      if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        /*
> +         * NOTE: when the machine is not yet created, this helper will
> +         * also keep the cached object untouched and return NULL.  The next
> +         * invoke of the helper will try to look for the machine again.
> +         * It'll only cache the pointer when it's found the first time.
> +         */

This smells like a recipe for subtle bugs. I think I'd consider it a code
flaw if something called qdev_get_machine() in a scenario where the machine
does not yet exist.

> +        dev = object_resolve_path_component(object_get_root(), "machine");
>      }

IOW, I think we should assert that dev != NULL to ensure we immediately
diagnose the flawed sequence of calls.

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