On Thu, Nov 21, 2024 at 10:21:48AM +0000, Daniel P. Berrangé wrote:
> 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.
Returning NULL to catch such bug isn't that bad either if it appears, IMHO.
It'll crash on the null reference instead.
I did this not only because I used to allow this return NULL (in my other
patchset to enable singleton for vIOMMUs, then whoever wants to do that for
singleton doesn't need yet another qdev patch..), but also I think qemu is
complex enough, so for such frequently used global helper it won't surprise
me that users will start to detect NULL soon anyway.
For example, I learned only recently that migration_is_active() can be
invoked before migration object is initialized.. and it needs to keep
working like that..
>
> > + 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.
I'd confess indeed the current qemu can assert that. OK, let me switch.
--
Peter Xu