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

[Philippe Mathieu-Daudé]

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.
> 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;