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

[Philippe Mathieu-Daudé]

On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
> On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
> > On 21/11/24 20:21, 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 | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 5f13111b77..b622be15ee 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
> > >       static Object *dev;
> > >       if (dev == NULL) {
> > > -        dev = container_get(object_get_root(), "/machine");
> > > +        dev = object_resolve_path_component(object_get_root(), 
> > > "machine");
> > > +        /*
> > > +         * Any call to this function before machine is created is 
> > > treated
> > > +         * as a programming error as of now.
> > > +         */
> > > +        assert(dev);
> >
> > This fails for user-emulation:
> >
> > ./qemu-x86_64 /bin/echo foo
> > qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion 
> > `dev' failed.

OK so I guess I might have found a "fix" which is to simply not
call qdev_get_machine() for user emulation, but this involves some
invasive refactoring -- so will take time --.

I'm dropping this series for now, planning to merge it again on top
of my refactor once it is ready. Any clever / simpler fix is
obviously welcomed first.

Regards,

Phil.

> > Aborted (core dumped)
>
> (gdb) bt
> #5  0x00007ffff747171b in __assert_fail_base (fmt=0x7ffff7626130 
> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x555555725150 
> "dev",
>      file=0x55555571aff9 "../../hw/core/qdev.c", line=824, 
> function=<optimized out>) at ./assert/assert.c:92
> #8  0x000055555565e400 in qdev_get_machine () at ../../hw/core/qdev.c:824
> #9  machine_get_container (name=0x55555571b052 "unattached") at ../../ 
> hw/core/qdev.c:834
> #10 0x000055555565ea2d in device_set_realized (obj=0x5555558b6760, 
> value=<optimized out>, errp=0x7fffffffdb50) at ../../hw/core/qdev.c:479
> #11 0x000055555566181a in property_set_bool (obj=0x5555558b6760, 
> v=<optimized out>, name=<optimized out>, opaque=0x555555813350, 
> errp=0x7fffffffdb50)
>      at ../../qom/object.c:2375
> #12 0x00005555556649f8 in object_property_set 
> (obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e 
> "realized", v=v@entry=0x5555558c0680,
>      errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1450
> #13 0x0000555555668754 in object_property_set_qobject 
> (obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e 
> "realized", value=value@entry=0x5555558be490,
>      errp=errp@entry=0x7fffffffdb50) at ../../qom/qom-qobject.c:28
> #14 0x00005555556650c9 in object_property_set_bool (obj=0x5555558b6760, 
> name=name@entry=0x55555571b03e "realized", value=value@entry=true,
>      errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1520
> #15 0x000055555565dd52 in qdev_realize (dev=<optimized out>, 
> bus=bus@entry=0x0, errp=errp@entry=0x7fffffffdb50) at ../../hw/core/ 
> qdev.c:276
> #16 0x0000555555593dc9 in cpu_create (typename=<optimized out>) 
> at ../../hw/core/cpu-common.c:61
> #17 0x00005555555925de in main (argc=3, argv=0x7fffffffe308, 
> envp=<optimized out>) at ../../linux-user/main.c:823
>
>
> > We need to skip this test for user emulation, but this file is in
> > hwcore_ss[] so the CONFIG_USER_ONLY definitions is not available.
> >
> > Any simple enough idea to not block this?
> >
> > >       }
> > >       return dev;