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 <[email protected]>
> ---
> 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 :|
