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

[Peter Xu <peterx@redhat.com>]

On Fri, Dec 20, 2024 at 12:25:44PM +0100, Philippe Mathieu-Daudé wrote:
> 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 --.

Thanks for taking a look, Phil.  Yes this sounds clean.

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

I initially thought about this, which could also be clean but I then
noticed LINUX_USER is poisoned..

===8<===
diff --git a/qom/object.c b/qom/object.c
index 58897a79a7..da26e8d69b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1729,7 +1729,19 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
     return prop->type;
 }

+/*
+ * Create all QEMU default containers.
+ *
+ * For system emulations, "machine" and its sub-containers are only created
+ * when machine initializes (qemu_create_machine()).
+ *
+ * For user emulations, create "machine" before hand to make qdev realize()
+ * work by default.
+ */
 static const char *const root_containers[] = {
+#ifdef CONFIG_LINUX_USER
+    "machine",
+#endif
     "chardevs",
     "objects",
     "backend"
@@ -1740,10 +1752,6 @@ static Object *object_root_initialize(void)
     Object *root = object_new(TYPE_CONTAINER);
     int i;

-    /*
-     * Create all QEMU system containers.  "machine" and its sub-containers
-     * are only created when machine initializes (qemu_create_machine()).
-     */
     for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
         object_property_add_new_container(root, root_containers[i]);
     }
===8<===

Maybe we could still move it somewhere that LINUX_USER is not poisoned
(plus "unattached" be created too, more below)?

OTOH, this works for me:

===8<===
diff --git a/linux-user/main.c b/linux-user/main.c
index b09af8d436..009b7695f2 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp)
     set_preferred_target_page_bits(ctz32(host_page_size));
     finalize_target_page_bits();
 
+    Object *fake_obj = object_property_add_new_container(object_get_root(),
+                                                         "machine");
+    object_property_add_new_container(fake_obj, "unattached");
+
     cpu = cpu_create(cpu_type);
     env = cpu_env(cpu);
     cpu_reset(cpu);
===8<===

So we need both "/machine" and "/machine/unattached" so far to make
linux-user work.  Not sure if bsd-user/main.c needs similar care, but none
of these look as clean.

Thanks,

-- 
Peter Xu