Re: [PATCH 10/12] qom: Create system containers explicitly

[Peter Xu <peterx@redhat.com>]

On Thu, Nov 21, 2024 at 06:29:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 21/11/24 18:17, Peter Xu wrote:
> > On Thu, Nov 21, 2024 at 02:01:45PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 21/11/24 11:30, Daniel P. Berrangé wrote:
> > > > On Wed, Nov 20, 2024 at 04:57:01PM -0500, Peter Xu wrote:
> > > > > Always explicitly create QEMU system containers upfront.
> > > > > 
> > > > > Root containers will be created when trying to fetch the root object the
> > > > > 1st time.  Machine sub-containers will be created only until machine is
> > > > > being initialized.
> > > > > 
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >    hw/core/machine.c | 19 ++++++++++++++++---
> > > > >    qom/object.c      | 16 +++++++++++++++-
> > > > >    2 files changed, 31 insertions(+), 4 deletions(-)
> > > > 
> > > > 
> > > > > diff --git a/qom/object.c b/qom/object.c
> > > > > index 214d6eb4c1..810e6f2bd9 100644
> > > > > --- a/qom/object.c
> > > > > +++ b/qom/object.c
> > > > > @@ -1734,12 +1734,26 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
> > > > >        return prop->type;
> > > > >    }
> > > > > +static Object *object_root_initialize(void)
> > > > > +{
> > > > > +    Object *root = object_new(TYPE_CONTAINER);
> > > > > +
> > > > > +    /*
> > > > > +     * Create all QEMU system containers.  "machine" and its sub-containers
> > > > > +     * are only created when machine initializes (qemu_create_machine()).
> > > > > +     */
> > > > > +    container_create(root, "chardevs");
> > > > > +    container_create(root, "objects");
> > > > 
> > > > This is where I would expect 'backend' to have been created
> > > > rather than ui/console.c, though you could potentially make
> > > > a case to create it from the machine function, snice console
> > > > stuff can't be used outside of the machine context, while
> > > > chardevs/objects can be used in qemu-img/qemu-nbd, etc
> > 
> > Would it hurt if we do it altogether here even if it won't be used in
> > qemu-img/qemu-nbd?
> > 
> > IMHO we should either make it simple (assuming empty containers won't hurt
> > there..), or we should just leave "backend" to ui/ code, so we don't assume
> > which binary is using the ui code: whoever uses it will create the container.
> > 
> > > 
> > > What about creating "backend" container in qemu_create_machine()?
> > 
> > I remember I started with that but it didn't work.  IIRC that's because
> > machine_initfn() (or somewhere around the init code) requires the
> > containers to present, hence it's too late even if we create the containers
> > right after this line:
> > 
> >      current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
> 
> So qemu_create_machine_containers() really belongs to qemu_create_machine()
> =)

Frankly, I don't immediately get this line..

But when I was trying again just to check my memory, I can't see anything
crash anymore, moving things over.

So while I'll test some more, I can switch to that if I cannot reproduce
any issue with it.  That's:

===8<===

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ed613ec4cb..a72c001c3d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1193,27 +1193,11 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
     }
 }
 
-static const char *const machine_containers[] = {
-    "unattached",
-    "peripheral",
-    "peripheral-anon"
-};
-
-static void qemu_create_machine_containers(Object *machine)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(machine_containers); i++) {
-        object_property_add_new_container(machine, machine_containers[i]);
-    }
-}
-
 static void machine_initfn(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
 
-    qemu_create_machine_containers(obj);
     ms->dump_guest_core = true;
     ms->mem_merge = (QEMU_MADV_MERGEABLE != QEMU_MADV_INVALID);
     ms->enable_graphics = true;
diff --git a/system/vl.c b/system/vl.c
index 822f7ff656..cdc0b6e10c 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2112,6 +2112,21 @@ static void parse_memory_options(void)
     loc_pop(&loc);
 }
 
+static const char *const machine_containers[] = {
+    "unattached",
+    "peripheral",
+    "peripheral-anon"
+};
+
+static void qemu_create_machine_containers(Object *machine)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(machine_containers); i++) {
+        object_property_add_new_container(machine, machine_containers[i]);
+    }
+}
+
 static void qemu_create_machine(QDict *qdict)
 {
     MachineClass *machine_class = select_machine(qdict, &error_fatal);
@@ -2120,6 +2135,7 @@ static void qemu_create_machine(QDict *qdict)
     current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
     object_property_add_child(object_get_root(), "machine",
                               OBJECT(current_machine));
+    qemu_create_machine_containers(OBJECT(current_machine));
     object_property_add_child(machine_get_container("unattached"),
                               "sysbus", OBJECT(sysbus_get_default()));
 

-- 
Peter Xu