[PATCH 00/12] QOM: container_get() removal

[PATCH 00/12] QOM: container_get() removal

[Peter Xu]

This series is not for 9.2, but for 10.0.  It is intended to replace this
previous patchset:

  [PATCH 0/5] QOM: Enforce container_get() to operate on containers only
  https://lore.kernel.org/r/20241118221330.3480246-1-peterx@redhat.com

Since it's a different patchset, the versioning starts from v1.

The series is about container_get() and its removal.  Meanwhile after the
whole series applied, all the containers will be created explicitly.  No
lookup code will implicitly create container anymore.

In general, container_get() is a flaky interface in a few things.  Firstly,
some of the users use it without getting a container object.  Secondly, it
can implicitly create containers on the fly.

As Dan (mostly) pointed out, we don't have a super complicated container
layout.  It looks like this:

  /objects
  /chardevs
  /backend         (conditional, ui/ only)
  /dr-connector    (conditional, ppc/ only)
  /machine
    /peripheral
    /peripheral-anon
    /unattached

This series create these containers explicitly.

There's a side benefit of dropping container_get(), which is to avoid
complicated string operations, as container_get() is never used in more
than one depth.  It means switching to object_resolve_path_component()
should make existing code even tiny little faster.

To achieve this, some test needs to be fixed first.  For that, "tests: Fix
test-qdev-global-props on anonymous qdev realize()".

Comments welcomed, thanks.

Peter Xu (12):
  qom: Add TYPE_CONTAINER macro
  qom: New container_create()
  tests: Fix test-qdev-global-props on anonymous qdev realize()
  tests: Explicitly create containers in test_qom_partial_path()
  ui/console: Explicitly create "/backend" container
  hw/ppc: Explicitly create the drc container
  ppc/e500: Avoid abuse of container_get()
  qdev: Make qdev_get_machine() not use container_get()
  qdev: Add machine_get_container()
  qom: Create system containers explicitly
  qom: Add object_get_container()
  qom: Drop container_get()

 include/hw/qdev-core.h              | 10 ++++++++
 include/qom/object.h                | 25 +++++++++++++-----
 backends/cryptodev.c                |  4 +--
 chardev/char.c                      |  2 +-
 hw/arm/stellaris.c                  |  2 +-
 hw/core/gpio.c                      |  3 +--
 hw/core/machine.c                   | 19 +++++++++++---
 hw/core/qdev.c                      | 28 +++++++++++++++++---
 hw/core/sysbus.c                    |  4 +--
 hw/i386/pc.c                        |  4 +--
 hw/pci-host/ppce500.c               |  4 +--
 hw/ppc/spapr_drc.c                  | 40 +++++++++++++++++++++--------
 qom/container.c                     | 31 ++++++++--------------
 qom/object.c                        | 30 +++++++++++++++++++---
 scsi/pr-manager.c                   |  4 +--
 system/ioport.c                     |  2 +-
 system/memory.c                     |  2 +-
 system/qdev-monitor.c               |  6 ++---
 system/vl.c                         |  3 +--
 tests/unit/check-qom-proplist.c     |  2 +-
 tests/unit/test-qdev-global-props.c | 21 +++++++++++++++
 ui/console.c                        |  4 +--
 ui/dbus-chardev.c                   |  2 +-
 23 files changed, 180 insertions(+), 72 deletions(-)

-- 
2.45.0

[PATCH 01/12] qom: Add TYPE_CONTAINER macro

[Peter Xu]

Provide a macro for the container type across QEMU source tree, rather than
hard code it every time.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 1 +
 hw/arm/stellaris.c   | 2 +-
 qom/container.c      | 4 ++--
 qom/object.c         | 4 ++--
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 43c135984a..3ba370ce9b 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -26,6 +26,7 @@ typedef struct InterfaceClass InterfaceClass;
 typedef struct InterfaceInfo InterfaceInfo;
 
 #define TYPE_OBJECT "object"
+#define TYPE_CONTAINER "container"
 
 typedef struct ObjectProperty ObjectProperty;
 
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 376746251e..6d518b9cde 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1053,7 +1053,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     flash_size = (((board->dc0 & 0xffff) + 1) << 1) * 1024;
     sram_size = ((board->dc0 >> 18) + 1) * 1024;
 
-    soc_container = object_new("container");
+    soc_container = object_new(TYPE_CONTAINER);
     object_property_add_child(OBJECT(ms), "soc", soc_container);
 
     /* Flash programming is done via the SCU, so pretend it is ROM.  */
diff --git a/qom/container.c b/qom/container.c
index 455e8410c6..cfec92a944 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -15,7 +15,7 @@
 #include "qemu/module.h"
 
 static const TypeInfo container_info = {
-    .name          = "container",
+    .name          = TYPE_CONTAINER,
     .parent        = TYPE_OBJECT,
 };
 
@@ -37,7 +37,7 @@ Object *container_get(Object *root, const char *path)
     for (i = 1; parts[i] != NULL; i++, obj = child) {
         child = object_resolve_path_component(obj, parts[i]);
         if (!child) {
-            child = object_new("container");
+            child = object_new(TYPE_CONTAINER);
             object_property_add_child(obj, parts[i], child);
             object_unref(child);
         }
diff --git a/qom/object.c b/qom/object.c
index 9edc06d391..214d6eb4c1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1739,7 +1739,7 @@ Object *object_get_root(void)
     static Object *root;
 
     if (!root) {
-        root = object_new("container");
+        root = object_new(TYPE_CONTAINER);
     }
 
     return root;
@@ -1755,7 +1755,7 @@ Object *object_get_internal_root(void)
     static Object *internal_root;
 
     if (!internal_root) {
-        internal_root = object_new("container");
+        internal_root = object_new(TYPE_CONTAINER);
     }
 
     return internal_root;
-- 
2.45.0

[PATCH 02/12] qom: New container_create()

[Peter Xu]

To move towards explicit creations of containers, starting that by
providing a helper for creating container objects.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 12 ++++++++++++
 qom/container.c      | 18 +++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 3ba370ce9b..41ef53241e 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -2033,6 +2033,18 @@ int object_child_foreach_recursive(Object *obj,
  */
 Object *container_get(Object *root, const char *path);
 
+
+/**
+ * container_create:
+ * @root: root of the object to create the new container
+ * @name: name of the new container
+ *
+ * Create a container object under @root with @name.
+ *
+ * Returns: the newly created container object.
+ */
+Object *container_create(Object *root, const char *name);
+
 /**
  * object_property_help:
  * @name: the name of the property
diff --git a/qom/container.c b/qom/container.c
index cfec92a944..da657754a4 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -24,6 +24,20 @@ static void container_register_types(void)
     type_register_static(&container_info);
 }
 
+Object *container_create(Object *obj, const char *name)
+{
+    Object *child = object_new(TYPE_CONTAINER);
+
+    object_property_add_child(obj, name, child);
+    /*
+     * Simplify the caller by always drop the refcount directly here, as
+     * containers are normally never destroyed after created anyway.
+     */
+    object_unref(child);
+
+    return child;
+}
+
 Object *container_get(Object *root, const char *path)
 {
     Object *obj, *child;
@@ -37,9 +51,7 @@ Object *container_get(Object *root, const char *path)
     for (i = 1; parts[i] != NULL; i++, obj = child) {
         child = object_resolve_path_component(obj, parts[i]);
         if (!child) {
-            child = object_new(TYPE_CONTAINER);
-            object_property_add_child(obj, parts[i], child);
-            object_unref(child);
+            child = container_create(obj, parts[i]);
         }
     }
 
-- 
2.45.0

[PATCH 03/12] tests: Fix test-qdev-global-props on anonymous qdev realize()

[Peter Xu]

test-qdev-global-props creates a few subprocesses and test things based on
qdev realize().  One thing was overlooked since the start, that anonymous
creations of qdev (then realize() the device) requires the machine object's
presence, as all these devices need to be attached to QOM tree, by default
to path "/machine/unattached".

The test didn't crash simply because container_get() has an implicit
semantic to silently create any missing container, hence what happened here
is container_get() (when running these tests) will try to create containers
at QOM path "/machine" on the fly.  That's probably unexpected by the test,
but worked like charm before.

We're going to fix device_set_realized() soon, but before that make the
test case prepared, by creating the machine object on its own.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/unit/test-qdev-global-props.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/unit/test-qdev-global-props.c b/tests/unit/test-qdev-global-props.c
index c8862cac5f..b2a1598f8e 100644
--- a/tests/unit/test-qdev-global-props.c
+++ b/tests/unit/test-qdev-global-props.c
@@ -72,6 +72,25 @@ static const TypeInfo subclass_type = {
     .parent = TYPE_STATIC_PROPS,
 };
 
+/*
+ * Initialize a fake machine, being prepared for future tests.
+ *
+ * All the tests later (even if to be run in subprocesses.. which will
+ * inherit the global states of the parent process) will try to create qdev
+ * and realize the device.
+ *
+ * Realization of such anonymous qdev (with no parent object) requires both
+ * the machine object and its "unattached" container to be at least present.
+ */
+static void test_init_machine(void)
+{
+    /* This is a fake machine - it doesn't need to be a machine object */
+    Object *machine = container_create(object_get_root(), "machine");
+
+    /* This container must exist for anonymous qdevs to realize() */
+    container_create(machine, "unattached");
+}
+
 /* Test simple static property setting to default value */
 static void test_static_prop_subprocess(void)
 {
@@ -295,6 +314,8 @@ int main(int argc, char **argv)
     type_register_static(&nohotplug_type);
     type_register_static(&nondevice_type);
 
+    test_init_machine();
+
     g_test_add_func("/qdev/properties/static/default/subprocess",
                     test_static_prop_subprocess);
     g_test_add_func("/qdev/properties/static/default",
-- 
2.45.0

[PATCH 04/12] tests: Explicitly create containers in test_qom_partial_path()

[Peter Xu]

Drop one use of container_get(), instead switch to the explicit function to
create a container.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/unit/check-qom-proplist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/unit/check-qom-proplist.c b/tests/unit/check-qom-proplist.c
index 79d4a8b89d..21b1e167ca 100644
--- a/tests/unit/check-qom-proplist.c
+++ b/tests/unit/check-qom-proplist.c
@@ -610,7 +610,7 @@ static void test_dummy_delchild(void)
 static void test_qom_partial_path(void)
 {
     Object *root  = object_get_objects_root();
-    Object *cont1 = container_get(root, "/cont1");
+    Object *cont1 = container_create(root, "cont1");
     Object *obj1  = object_new(TYPE_DUMMY);
     Object *obj2a = object_new(TYPE_DUMMY);
     Object *obj2b = object_new(TYPE_DUMMY);
-- 
2.45.0

[PATCH 05/12] ui/console: Explicitly create "/backend" container

[Peter Xu]

Follow the trend to explicitly create containers, do that for console.c on
"/backend" container.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 ui/console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 5165f17125..36f8c6debb 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1154,14 +1154,14 @@ DisplayState *init_displaystate(void)
 {
     gchar *name;
     QemuConsole *con;
+    Object *backend = container_create(object_get_root(), "backend");
 
     QTAILQ_FOREACH(con, &consoles, next) {
         /* Hook up into the qom tree here (not in object_new()), once
          * all QemuConsoles are created and the order / numbering
          * doesn't change any more */
         name = g_strdup_printf("console[%d]", con->index);
-        object_property_add_child(container_get(object_get_root(), "/backend"),
-                                  name, OBJECT(con));
+        object_property_add_child(backend, name, OBJECT(con));
         g_free(name);
     }
 
-- 
2.45.0

[PATCH 06/12] hw/ppc: Explicitly create the drc container

[Peter Xu]

QEMU will start to not rely on implicit creations of containers soon.  Make
PPC drc devices follow by explicitly create the container whenever a drc
device is realized, dropping container_get() calls.

No functional change intended.

Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1484e3209d..3d6ef26b38 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -27,7 +27,7 @@
 #include "sysemu/reset.h"
 #include "trace.h"
 
-#define DRC_CONTAINER_PATH "/dr-connector"
+#define DRC_CONTAINER_PATH "dr-connector"
 #define DRC_INDEX_TYPE_SHIFT 28
 #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
 
@@ -514,6 +514,26 @@ static const VMStateDescription vmstate_spapr_drc = {
     }
 };
 
+static GOnce drc_container_created = G_ONCE_INIT;
+
+static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
+{
+    container_create(object_get_root(), DRC_CONTAINER_PATH);
+    return NULL;
+}
+
+static Object *drc_container_get(void)
+{
+    return object_resolve_path_component(
+        object_get_root(), DRC_CONTAINER_PATH);
+}
+
+/* TODO: create the container in an ppc init function */
+static void drc_container_create_once(void)
+{
+    g_once(&drc_container_created, drc_container_create, NULL);
+}
+
 static void drc_realize(DeviceState *d, Error **errp)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
@@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
     Object *root_container;
     const char *child_name;
 
+    /* Whenever a DRC device is realized, create the container */
+    drc_container_create_once();
+
     trace_spapr_drc_realize(spapr_drc_index(drc));
     /* NOTE: we do this as part of realize/unrealize due to the fact
      * that the guest will communicate with the DRC via RTAS calls
@@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
      * inaccessible by the guest, since lookups rely on this path
      * existing in the composition tree
      */
-    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
+    root_container = drc_container_get();
     child_name = object_get_canonical_path_component(OBJECT(drc));
     trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
     object_property_add_alias(root_container, link_name,
@@ -543,12 +566,10 @@ static void drc_unrealize(DeviceState *d)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
     g_autofree gchar *name = g_strdup_printf("%x", spapr_drc_index(drc));
-    Object *root_container;
 
     trace_spapr_drc_unrealize(spapr_drc_index(drc));
     vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
-    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
-    object_property_del(root_container, name);
+    object_property_del(drc_container_get(), name);
 }
 
 SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
@@ -796,9 +817,8 @@ static const TypeInfo spapr_drc_pmem_info = {
 SpaprDrc *spapr_drc_by_index(uint32_t index)
 {
     Object *obj;
-    g_autofree gchar *name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH,
-                                             index);
-    obj = object_resolve_path(name, NULL);
+    g_autofree gchar *name = g_strdup_printf("%x", index);
+    obj = object_resolve_path_component(drc_container_get(), name);
 
     return !obj ? NULL : SPAPR_DR_CONNECTOR(obj);
 }
@@ -860,7 +880,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
     /* aliases for all DRConnector objects will be rooted in QOM
      * composition tree at DRC_CONTAINER_PATH
      */
-    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
+    root_container = drc_container_get();
 
     object_property_iter_init(&iter, root_container);
     while ((prop = object_property_iter_next(&iter))) {
@@ -953,7 +973,7 @@ void spapr_drc_reset_all(SpaprMachineState *spapr)
     ObjectProperty *prop;
     ObjectPropertyIterator iter;
 
-    drc_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
+    drc_container = drc_container_get();
 restart:
     object_property_iter_init(&iter, drc_container);
     while ((prop = object_property_iter_next(&iter))) {
-- 
2.45.0

[PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Peter Xu]

container_get() is going to become strict on not allowing to return a
non-container.

Switch the e500 user to use object_resolve_path_component() explicitly.

Cc: Bharat Bhushan <r65777@freescale.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/pci-host/ppce500.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index b70631045a..65233b9e3f 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -418,8 +418,8 @@ static const VMStateDescription vmstate_ppce500_pci = {
 static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
 {
     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
-    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
-                                  "/e500-ccsr"));
+    PPCE500CCSRState *ccsr = CCSR(
+        object_resolve_path_component(qdev_get_machine(), "e500-ccsr"));
 
     memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
                              0, int128_get64(ccsr->ccsr_space.size));
-- 
2.45.0

[PATCH 08/12] qdev: Make qdev_get_machine() not use container_get()

[Peter Xu]

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.
+         */
+        dev = object_resolve_path_component(object_get_root(), "machine");
     }
 
     return dev;
-- 
2.45.0

[PATCH 09/12] qdev: Add machine_get_container()

[Peter Xu]

Add a helper to fetch machine containers.  Add some sanity check around.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/hw/qdev-core.h | 10 ++++++++++
 hw/core/qdev.c         | 17 +++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5be9844412..38edfb1b54 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -996,6 +996,16 @@ const char *qdev_fw_name(DeviceState *dev);
 void qdev_assert_realized_properly(void);
 Object *qdev_get_machine(void);
 
+/**
+ * machine_get_container:
+ * @name: The name of container to lookup
+ *
+ * Get a container of the machine (QOM path "/machine/XXX").
+ *
+ * Returns: the machine container object.
+ */
+Object *machine_get_container(const char *name);
+
 /**
  * qdev_get_human_name() - Return a human-readable name for a device
  * @dev: The device. Must be a valid and non-NULL pointer.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c869c47a27..6cb4fe4691 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -829,6 +829,23 @@ Object *qdev_get_machine(void)
     return dev;
 }
 
+Object *machine_get_container(const char *name)
+{
+    Object *container, *machine;
+
+    /*
+     * NOTE: nobody should call this _before_ machine is created.  If it
+     * happens, it's a programming error.
+     */
+    machine = qdev_get_machine();
+    assert(machine);
+
+    container = object_resolve_path_component(machine, name);
+    assert(object_dynamic_cast(container, TYPE_CONTAINER));
+
+    return container;
+}
+
 char *qdev_get_human_name(DeviceState *dev)
 {
     g_assert(dev != NULL);
-- 
2.45.0

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

[Peter Xu]

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/hw/core/machine.c b/hw/core/machine.c
index a35c4a8fae..a184dbf8f0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1193,14 +1193,27 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
     }
 }
 
+const char *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++) {
+        container_create(machine, machine_containers[i]);
+    }
+}
+
 static void machine_initfn(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
 
-    container_get(obj, "/peripheral");
-    container_get(obj, "/peripheral-anon");
-
+    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/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");
+
+    return root;
+}
+
 Object *object_get_root(void)
 {
     static Object *root;
 
     if (!root) {
-        root = object_new(TYPE_CONTAINER);
+        root = object_root_initialize();
     }
 
     return root;
-- 
2.45.0

[PATCH 11/12] qom: Add object_get_container()

[Peter Xu]

Add a helper to fetch a root container (under object_get_root()).  Sanity
check on the type of the object.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 10 ++++++++++
 qom/object.c         | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 41ef53241e..87b13f9681 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1524,6 +1524,16 @@ const char *object_property_get_type(Object *obj, const char *name,
  */
 Object *object_get_root(void);
 
+/**
+ * object_get_container:
+ * @name: the name of container to lookup
+ *
+ * Lookup a root level container.
+ *
+ * Returns: the container with @name.
+ */
+Object *object_get_container(const char *name);
+
 
 /**
  * object_get_objects_root:
diff --git a/qom/object.c b/qom/object.c
index 810e6f2bd9..cf66803a6a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1748,6 +1748,16 @@ static Object *object_root_initialize(void)
     return root;
 }
 
+Object *object_get_container(const char *name)
+{
+    Object *container;
+
+    container = object_resolve_path_component(object_get_root(), name);
+    assert(object_dynamic_cast(container, TYPE_CONTAINER));
+
+    return container;
+}
+
 Object *object_get_root(void)
 {
     static Object *root;
-- 
2.45.0

[PATCH 12/12] qom: Drop container_get()

[Peter Xu]

Now we should be ready to always create containers upfront, meanwhile we
have explicit helpers to fetch either:

  - Root containers (object_get_container()), or
  - Machine containers (machine_get_container()).

Change all rest container_get() users to use the explicit & fast version of
container lookup helpers, finally remove container_get().

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h  | 12 ------------
 backends/cryptodev.c  |  4 ++--
 chardev/char.c        |  2 +-
 hw/core/gpio.c        |  3 +--
 hw/core/qdev.c        |  3 +--
 hw/core/sysbus.c      |  4 ++--
 hw/i386/pc.c          |  4 ++--
 qom/container.c       | 23 -----------------------
 qom/object.c          |  2 +-
 scsi/pr-manager.c     |  4 ++--
 system/ioport.c       |  2 +-
 system/memory.c       |  2 +-
 system/qdev-monitor.c |  6 +++---
 system/vl.c           |  3 +--
 ui/dbus-chardev.c     |  2 +-
 15 files changed, 19 insertions(+), 57 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 87b13f9681..c6e7a7fc08 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -2031,18 +2031,6 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
 int object_child_foreach_recursive(Object *obj,
                                    int (*fn)(Object *child, void *opaque),
                                    void *opaque);
-/**
- * container_get:
- * @root: root of the #path, e.g., object_get_root()
- * @path: path to the container
- *
- * Return a container object whose path is @path.  Create more containers
- * along the path if necessary.
- *
- * Returns: the container object.
- */
-Object *container_get(Object *root, const char *path);
-
 
 /**
  * container_create:
diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index d8bd2a1ae6..263de4913b 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -97,7 +97,7 @@ static int qmp_query_cryptodev_foreach(Object *obj, void *data)
 QCryptodevInfoList *qmp_query_cryptodev(Error **errp)
 {
     QCryptodevInfoList *list = NULL;
-    Object *objs = container_get(object_get_root(), "/objects");
+    Object *objs = object_get_container("objects");
 
     object_child_foreach(objs, qmp_query_cryptodev_foreach, &list);
 
@@ -557,7 +557,7 @@ static void cryptodev_backend_stats_cb(StatsResultList **result,
     switch (target) {
     case STATS_TARGET_CRYPTODEV:
     {
-        Object *objs = container_get(object_get_root(), "/objects");
+        Object *objs = object_get_container("objects");
         StatsArgs stats_args;
         stats_args.result.stats = result;
         stats_args.names = names;
diff --git a/chardev/char.c b/chardev/char.c
index a1722aa076..22fc5f7f76 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -48,7 +48,7 @@
 
 Object *get_chardevs_root(void)
 {
-    return container_get(object_get_root(), "/chardevs");
+    return object_get_container("chardevs");
 }
 
 static void chr_be_event(Chardev *s, QEMUChrEvent event)
diff --git a/hw/core/gpio.c b/hw/core/gpio.c
index 80d07a6ec9..6e32a8eec6 100644
--- a/hw/core/gpio.c
+++ b/hw/core/gpio.c
@@ -121,8 +121,7 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
                                      name ? name : "unnamed-gpio-out", n);
     if (input_pin && !OBJECT(input_pin)->parent) {
         /* We need a name for object_property_set_link to work */
-        object_property_add_child(container_get(qdev_get_machine(),
-                                                "/unattached"),
+        object_property_add_child(machine_get_container("unattached"),
                                   "non-qdev-gpio[*]", OBJECT(input_pin));
     }
     object_property_set_link(OBJECT(dev), propname,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6cb4fe4691..8051961fe5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -476,8 +476,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (!obj->parent) {
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
-            object_property_add_child(container_get(qdev_get_machine(),
-                                                    "/unattached"),
+            object_property_add_child(machine_get_container("unattached"),
                                       name, obj);
             unattached_parent = true;
             g_free(name);
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index e64d99c8ed..9355849ff0 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -65,9 +65,9 @@ void foreach_dynamic_sysbus_device(FindSysbusDeviceFunc *func, void *opaque)
     };
 
     /* Loop through all sysbus devices that were spawned outside the machine */
-    container = container_get(qdev_get_machine(), "/peripheral");
+    container = machine_get_container("peripheral");
     find_sysbus_device(container, &find);
-    container = container_get(qdev_get_machine(), "/peripheral-anon");
+    container = machine_get_container("peripheral-anon");
     find_sysbus_device(container, &find);
 }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 317aaca25a..b8ec2506e1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -460,7 +460,7 @@ static int check_fdc(Object *obj, void *opaque)
 }
 
 static const char * const fdc_container_path[] = {
-    "/unattached", "/peripheral", "/peripheral-anon"
+    "unattached", "peripheral", "peripheral-anon"
 };
 
 /*
@@ -474,7 +474,7 @@ static ISADevice *pc_find_fdc0(void)
     CheckFdcState state = { 0 };
 
     for (i = 0; i < ARRAY_SIZE(fdc_container_path); i++) {
-        container = container_get(qdev_get_machine(), fdc_container_path[i]);
+        container = machine_get_container(fdc_container_path[i]);
         object_child_foreach(container, check_fdc, &state);
     }
 
diff --git a/qom/container.c b/qom/container.c
index da657754a4..5e36fb8773 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -38,27 +38,4 @@ Object *container_create(Object *obj, const char *name)
     return child;
 }
 
-Object *container_get(Object *root, const char *path)
-{
-    Object *obj, *child;
-    char **parts;
-    int i;
-
-    parts = g_strsplit(path, "/", 0);
-    assert(parts != NULL && parts[0] != NULL && !parts[0][0]);
-    obj = root;
-
-    for (i = 1; parts[i] != NULL; i++, obj = child) {
-        child = object_resolve_path_component(obj, parts[i]);
-        if (!child) {
-            child = container_create(obj, parts[i]);
-        }
-    }
-
-    g_strfreev(parts);
-
-    return obj;
-}
-
-
 type_init(container_register_types)
diff --git a/qom/object.c b/qom/object.c
index cf66803a6a..aeb04c483c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1771,7 +1771,7 @@ Object *object_get_root(void)
 
 Object *object_get_objects_root(void)
 {
-    return container_get(object_get_root(), "/objects");
+    return object_get_container("objects");
 }
 
 Object *object_get_internal_root(void)
diff --git a/scsi/pr-manager.c b/scsi/pr-manager.c
index fb5fc29730..1977d99ce0 100644
--- a/scsi/pr-manager.c
+++ b/scsi/pr-manager.c
@@ -21,7 +21,7 @@
 #include "qemu/module.h"
 #include "qapi/qapi-commands-block.h"
 
-#define PR_MANAGER_PATH     "/objects"
+#define PR_MANAGER_PATH     "objects"
 
 typedef struct PRManagerData {
     PRManager *pr_mgr;
@@ -135,7 +135,7 @@ PRManagerInfoList *qmp_query_pr_managers(Error **errp)
 {
     PRManagerInfoList *head = NULL;
     PRManagerInfoList **prev = &head;
-    Object *container = container_get(object_get_root(), PR_MANAGER_PATH);
+    Object *container = object_get_container(PR_MANAGER_PATH);
 
     object_child_foreach(container, query_one_pr_manager, &prev);
     return head;
diff --git a/system/ioport.c b/system/ioport.c
index fd551d0375..55c2a75239 100644
--- a/system/ioport.c
+++ b/system/ioport.c
@@ -258,7 +258,7 @@ static void portio_list_add_1(PortioList *piolist,
     object_ref(&mrpio->mr);
     object_unparent(OBJECT(&mrpio->mr));
     if (!piolist->owner) {
-        owner = container_get(qdev_get_machine(), "/unattached");
+        owner = machine_get_container("unattached");
     } else {
         owner = piolist->owner;
     }
diff --git a/system/memory.c b/system/memory.c
index 85f6834cb3..fba351b030 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1238,7 +1238,7 @@ static void memory_region_do_init(MemoryRegion *mr,
         char *name_array = g_strdup_printf("%s[*]", escaped_name);
 
         if (!owner) {
-            owner = container_get(qdev_get_machine(), "/unattached");
+            owner = machine_get_container("unattached");
         }
 
         object_property_add_child(owner, name_array, OBJECT(mr));
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 4c09b38ffb..4d46af2c8d 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -348,7 +348,7 @@ static Object *qdev_get_peripheral(void)
     static Object *dev;
 
     if (dev == NULL) {
-        dev = container_get(qdev_get_machine(), "/peripheral");
+        dev = machine_get_container("peripheral");
     }
 
     return dev;
@@ -359,7 +359,7 @@ static Object *qdev_get_peripheral_anon(void)
     static Object *dev;
 
     if (dev == NULL) {
-        dev = container_get(qdev_get_machine(), "/peripheral-anon");
+        dev = machine_get_container("peripheral-anon");
     }
 
     return dev;
@@ -1085,7 +1085,7 @@ static GSList *qdev_build_hotpluggable_device_list(Object *peripheral)
 static void peripheral_device_del_completion(ReadLineState *rs,
                                              const char *str)
 {
-    Object *peripheral = container_get(qdev_get_machine(), "/peripheral");
+    Object *peripheral = machine_get_container("peripheral");
     GSList *list, *item;
 
     list = qdev_build_hotpluggable_device_list(peripheral);
diff --git a/system/vl.c b/system/vl.c
index 3bb8f2db9a..822f7ff656 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2120,8 +2120,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));
-    object_property_add_child(container_get(OBJECT(current_machine),
-                                            "/unattached"),
+    object_property_add_child(machine_get_container("unattached"),
                               "sysbus", OBJECT(sysbus_get_default()));
 
     if (machine_class->minimum_page_bits) {
diff --git a/ui/dbus-chardev.c b/ui/dbus-chardev.c
index 1d3a7122a1..bf061cbc93 100644
--- a/ui/dbus-chardev.c
+++ b/ui/dbus-chardev.c
@@ -106,7 +106,7 @@ dbus_chardev_init(DBusDisplay *dpy)
     dpy->notifier.notify = dbus_display_on_notify;
     dbus_display_notifier_add(&dpy->notifier);
 
-    object_child_foreach(container_get(object_get_root(), "/chardevs"),
+    object_child_foreach(object_get_container("chardevs"),
                          dbus_display_chardev_foreach, dpy);
 }
 
-- 
2.45.0

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

[Philippe Mathieu-Daudé]

On 20/11/24 22:57, 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/hw/core/machine.c b/hw/core/machine.c
> index a35c4a8fae..a184dbf8f0 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1193,14 +1193,27 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
>       }
>   }
>   
> +const char *machine_containers[] = {

    const char *const machine_containers[] = {

> +    "unattached",
> +    "peripheral",
> +    "peripheral-anon"
> +};

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 00/12] QOM: container_get() removal

[Philippe Mathieu-Daudé]

On 20/11/24 22:56, Peter Xu wrote:

> Peter Xu (12):
>    qom: Add TYPE_CONTAINER macro
>    qom: New container_create()
>    tests: Fix test-qdev-global-props on anonymous qdev realize()
>    tests: Explicitly create containers in test_qom_partial_path()
>    ui/console: Explicitly create "/backend" container
>    hw/ppc: Explicitly create the drc container
>    ppc/e500: Avoid abuse of container_get()
>    qdev: Make qdev_get_machine() not use container_get()
>    qdev: Add machine_get_container()
>    qom: Create system containers explicitly
>    qom: Add object_get_container()
>    qom: Drop container_get()

Maybe split the last patch in 3, reordering as:

- qom: Create system containers explicitly
- qdev: Make qdev_get_machine() not use container_get()
- qdev: Add machine_get_container()
- qdev: Use machine_get_container()
   (part 1 of 'qom: Drop container_get()')
- qom: Add object_get_container()
- qom: Use object_get_container()
   (part 2 of 'qom: Drop container_get()')
- qom: Drop container_get()
   (part 3 of 'qom: Drop container_get()')

Re: [PATCH 05/12] ui/console: Explicitly create "/backend" container

[Philippe Mathieu-Daudé]

On 20/11/24 22:56, Peter Xu wrote:
> Follow the trend to explicitly create containers, do that for console.c on
> "/backend" container.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   ui/console.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 04/12] tests: Explicitly create containers in test_qom_partial_path()

[Philippe Mathieu-Daudé]

On 20/11/24 22:56, Peter Xu wrote:
> Drop one use of container_get(), instead switch to the explicit function to
> create a container.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   tests/unit/check-qom-proplist.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 03/12] tests: Fix test-qdev-global-props on anonymous qdev realize()

[Philippe Mathieu-Daudé]

On 20/11/24 22:56, Peter Xu wrote:
> test-qdev-global-props creates a few subprocesses and test things based on
> qdev realize().  One thing was overlooked since the start, that anonymous
> creations of qdev (then realize() the device) requires the machine object's
> presence, as all these devices need to be attached to QOM tree, by default
> to path "/machine/unattached".
> 
> The test didn't crash simply because container_get() has an implicit
> semantic to silently create any missing container, hence what happened here
> is container_get() (when running these tests) will try to create containers
> at QOM path "/machine" on the fly.  That's probably unexpected by the test,
> but worked like charm before.
> 
> We're going to fix device_set_realized() soon, but before that make the
> test case prepared, by creating the machine object on its own.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   tests/unit/test-qdev-global-props.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/12] qom: Add TYPE_CONTAINER macro

[Philippe Mathieu-Daudé]

On 20/11/24 22:56, Peter Xu wrote:
> Provide a macro for the container type across QEMU source tree, rather than
> hard code it every time.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qom/object.h | 1 +
>   hw/arm/stellaris.c   | 2 +-
>   qom/container.c      | 4 ++--
>   qom/object.c         | 4 ++--
>   4 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 11/12] qom: Add object_get_container()

[Philippe Mathieu-Daudé]

On 20/11/24 22:57, Peter Xu wrote:
> Add a helper to fetch a root container (under object_get_root()).  Sanity
> check on the type of the object.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qom/object.h | 10 ++++++++++
>   qom/object.c         | 10 ++++++++++
>   2 files changed, 20 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 09/12] qdev: Add machine_get_container()

[Philippe Mathieu-Daudé]

On 20/11/24 22:57, Peter Xu wrote:
> Add a helper to fetch machine containers.  Add some sanity check around.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/hw/qdev-core.h | 10 ++++++++++
>   hw/core/qdev.c         | 17 +++++++++++++++++
>   2 files changed, 27 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 06/12] hw/ppc: Explicitly create the drc container

[Philippe Mathieu-Daudé]

Hi Peter,

On 20/11/24 22:56, Peter Xu wrote:
> QEMU will start to not rely on implicit creations of containers soon.  Make
> PPC drc devices follow by explicitly create the container whenever a drc
> device is realized, dropping container_get() calls.
> 
> No functional change intended.
> 
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
>   1 file changed, 30 insertions(+), 10 deletions(-)


> +static GOnce drc_container_created = G_ONCE_INIT;
> +
> +static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
> +{
> +    container_create(object_get_root(), DRC_CONTAINER_PATH);
> +    return NULL;
> +}
> +
> +static Object *drc_container_get(void)
> +{
> +    return object_resolve_path_component(
> +        object_get_root(), DRC_CONTAINER_PATH);
> +}
> +
> +/* TODO: create the container in an ppc init function */
> +static void drc_container_create_once(void)
> +{
> +    g_once(&drc_container_created, drc_container_create, NULL);
> +}
> +
>   static void drc_realize(DeviceState *d, Error **errp)
>   {
>       SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
> @@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
>       Object *root_container;
>       const char *child_name;
>   
> +    /* Whenever a DRC device is realized, create the container */
> +    drc_container_create_once();

Can't we just create it once in spapr_dr_connector_class_init(),
removing the G_ONCE_INIT need?

>       trace_spapr_drc_realize(spapr_drc_index(drc));
>       /* NOTE: we do this as part of realize/unrealize due to the fact
>        * that the guest will communicate with the DRC via RTAS calls
> @@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
>        * inaccessible by the guest, since lookups rely on this path
>        * existing in the composition tree
>        */
> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> +    root_container = drc_container_get();
>       child_name = object_get_canonical_path_component(OBJECT(drc));
>       trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
>       object_property_add_alias(root_container, link_name,

Re: [PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Cédric Le Goater]

On 11/20/24 22:56, Peter Xu wrote:
> container_get() is going to become strict on not allowing to return a
> non-container.
> 
> Switch the e500 user to use object_resolve_path_component() explicitly.
> 
> Cc: Bharat Bhushan <r65777@freescale.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/pci-host/ppce500.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index b70631045a..65233b9e3f 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -418,8 +418,8 @@ static const VMStateDescription vmstate_ppce500_pci = {
>   static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
>   {
>       PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
> -                                  "/e500-ccsr"));
> +    PPCE500CCSRState *ccsr = CCSR(
> +        object_resolve_path_component(qdev_get_machine(), "e500-ccsr"));


why not simply use :

       CCSR(object_resolve_path("/machine/e500-ccsr", NULL));

Thanks,

C.

>   
>       memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
>                                0, int128_get64(ccsr->ccsr_space.size));

Re: [PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Cédric Le Goater]

On 11/21/24 10:38, Cédric Le Goater wrote:
> On 11/20/24 22:56, Peter Xu wrote:
>> container_get() is going to become strict on not allowing to return a
>> non-container.
>>
>> Switch the e500 user to use object_resolve_path_component() explicitly.
>>
>> Cc: Bharat Bhushan <r65777@freescale.com>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>   hw/pci-host/ppce500.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
>> index b70631045a..65233b9e3f 100644
>> --- a/hw/pci-host/ppce500.c
>> +++ b/hw/pci-host/ppce500.c
>> @@ -418,8 +418,8 @@ static const VMStateDescription vmstate_ppce500_pci = {
>>   static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
>>   {
>>       PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
>> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
>> -                                  "/e500-ccsr"));
>> +    PPCE500CCSRState *ccsr = CCSR(
>> +        object_resolve_path_component(qdev_get_machine(), "e500-ccsr"));
> 
> 
> why not simply use :
> 
>        CCSR(object_resolve_path("/machine/e500-ccsr", NULL));


I guess we want to avoid the absolute paths. If so,

Reviewed-by: Cédric Le Goater <clg@redhat.com>


We might want to convert these lookups to object_resolve_path_component
too, not in this patchset.

hw/i386/acpi-build.c:    host = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
hw/i386/acpi-build.c:        host = PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));
target/i386/kvm/kvm.c:        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
target/i386/tcg/sysemu/tcg-cpu.c:        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);

Thanks,

C.

Re: [PATCH 01/12] qom: Add TYPE_CONTAINER macro

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:56:52PM -0500, Peter Xu wrote:
> Provide a macro for the container type across QEMU source tree, rather than
> hard code it every time.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 1 +
>  hw/arm/stellaris.c   | 2 +-
>  qom/container.c      | 4 ++--
>  qom/object.c         | 4 ++--
>  4 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 02/12] qom: New container_create()

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:56:53PM -0500, Peter Xu wrote:
> To move towards explicit creations of containers, starting that by
> providing a helper for creating container objects.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 12 ++++++++++++
>  qom/container.c      | 18 +++++++++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 03/12] tests: Fix test-qdev-global-props on anonymous qdev realize()

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:56:54PM -0500, Peter Xu wrote:
> test-qdev-global-props creates a few subprocesses and test things based on
> qdev realize().  One thing was overlooked since the start, that anonymous
> creations of qdev (then realize() the device) requires the machine object's
> presence, as all these devices need to be attached to QOM tree, by default
> to path "/machine/unattached".
> 
> The test didn't crash simply because container_get() has an implicit
> semantic to silently create any missing container, hence what happened here
> is container_get() (when running these tests) will try to create containers
> at QOM path "/machine" on the fly.  That's probably unexpected by the test,
> but worked like charm before.
> 
> We're going to fix device_set_realized() soon, but before that make the
> test case prepared, by creating the machine object on its own.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/unit/test-qdev-global-props.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 04/12] tests: Explicitly create containers in test_qom_partial_path()

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:56:55PM -0500, Peter Xu wrote:
> Drop one use of container_get(), instead switch to the explicit function to
> create a container.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/unit/check-qom-proplist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

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.

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

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 09/12] qdev: Add machine_get_container()

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:57:00PM -0500, Peter Xu wrote:
> Add a helper to fetch machine containers.  Add some sanity check around.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/hw/qdev-core.h | 10 ++++++++++
>  hw/core/qdev.c         | 17 +++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 5be9844412..38edfb1b54 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -996,6 +996,16 @@ const char *qdev_fw_name(DeviceState *dev);
>  void qdev_assert_realized_properly(void);
>  Object *qdev_get_machine(void);
>  
> +/**
> + * machine_get_container:
> + * @name: The name of container to lookup
> + *
> + * Get a container of the machine (QOM path "/machine/XXX").
> + *
> + * Returns: the machine container object.
> + */
> +Object *machine_get_container(const char *name);
> +
>  /**
>   * qdev_get_human_name() - Return a human-readable name for a device
>   * @dev: The device. Must be a valid and non-NULL pointer.
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c869c47a27..6cb4fe4691 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -829,6 +829,23 @@ Object *qdev_get_machine(void)
>      return dev;
>  }
>  
> +Object *machine_get_container(const char *name)
> +{
> +    Object *container, *machine;
> +
> +    /*
> +     * NOTE: nobody should call this _before_ machine is created.  If it
> +     * happens, it's a programming error.
> +     */
> +    machine = qdev_get_machine();
> +    assert(machine);

IMHO this assert should be in  qdev_get_machine() itself any
premature call of it is a programming error.

> +
> +    container = object_resolve_path_component(machine, name);
> +    assert(object_dynamic_cast(container, TYPE_CONTAINER));
> +
> +    return container;
> +}
> +
>  char *qdev_get_human_name(DeviceState *dev)
>  {
>      g_assert(dev != NULL);
> -- 
> 2.45.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 05/12] ui/console: Explicitly create "/backend" container

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:56:56PM -0500, Peter Xu wrote:
> Follow the trend to explicitly create containers, do that for console.c on
> "/backend" container.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  ui/console.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/console.c b/ui/console.c
> index 5165f17125..36f8c6debb 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1154,14 +1154,14 @@ DisplayState *init_displaystate(void)
>  {
>      gchar *name;
>      QemuConsole *con;
> +    Object *backend = container_create(object_get_root(), "backend");

What's the rationale for keeping this in the console code ?

I'd consider this to be a similar situation to '/chardevs' and
'/objects', and be a common container rather than UI specific.
IOW, be created by your later patch.

>  
>      QTAILQ_FOREACH(con, &consoles, next) {
>          /* Hook up into the qom tree here (not in object_new()), once
>           * all QemuConsoles are created and the order / numbering
>           * doesn't change any more */
>          name = g_strdup_printf("console[%d]", con->index);
> -        object_property_add_child(container_get(object_get_root(), "/backend"),
> -                                  name, OBJECT(con));
> +        object_property_add_child(backend, name, OBJECT(con));
>          g_free(name);
>      }



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:56:58PM -0500, Peter Xu wrote:
> container_get() is going to become strict on not allowing to return a
> non-container.
> 
> Switch the e500 user to use object_resolve_path_component() explicitly.
> 
> Cc: Bharat Bhushan <r65777@freescale.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/pci-host/ppce500.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

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

> +
> +    return root;
> +}


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 11/12] qom: Add object_get_container()

[Daniel P. Berrangé]

On Wed, Nov 20, 2024 at 04:57:02PM -0500, Peter Xu wrote:
> Add a helper to fetch a root container (under object_get_root()).  Sanity
> check on the type of the object.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 10 ++++++++++
>  qom/object.c         | 10 ++++++++++
>  2 files changed, 20 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 12/12] qom: Drop container_get()

[Daniel P. Berrangé]

Subject says drop container_get, actual commit does alot more.

Suggest splitting the conversion to (object/machine)_get_container,
from the dropping of container_get.

On Wed, Nov 20, 2024 at 04:57:03PM -0500, Peter Xu wrote:
> Now we should be ready to always create containers upfront, meanwhile we
> have explicit helpers to fetch either:
> 
>   - Root containers (object_get_container()), or
>   - Machine containers (machine_get_container()).
> 
> Change all rest container_get() users to use the explicit & fast version of
> container lookup helpers, finally remove container_get().
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h  | 12 ------------
>  backends/cryptodev.c  |  4 ++--
>  chardev/char.c        |  2 +-
>  hw/core/gpio.c        |  3 +--
>  hw/core/qdev.c        |  3 +--
>  hw/core/sysbus.c      |  4 ++--
>  hw/i386/pc.c          |  4 ++--
>  qom/container.c       | 23 -----------------------
>  qom/object.c          |  2 +-
>  scsi/pr-manager.c     |  4 ++--
>  system/ioport.c       |  2 +-
>  system/memory.c       |  2 +-
>  system/qdev-monitor.c |  6 +++---
>  system/vl.c           |  3 +--
>  ui/dbus-chardev.c     |  2 +-
>  15 files changed, 19 insertions(+), 57 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 87b13f9681..c6e7a7fc08 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -2031,18 +2031,6 @@ int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
>  int object_child_foreach_recursive(Object *obj,
>                                     int (*fn)(Object *child, void *opaque),
>                                     void *opaque);
> -/**
> - * container_get:
> - * @root: root of the #path, e.g., object_get_root()
> - * @path: path to the container
> - *
> - * Return a container object whose path is @path.  Create more containers
> - * along the path if necessary.
> - *
> - * Returns: the container object.
> - */
> -Object *container_get(Object *root, const char *path);
> -
>  
>  /**
>   * container_create:
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index d8bd2a1ae6..263de4913b 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -97,7 +97,7 @@ static int qmp_query_cryptodev_foreach(Object *obj, void *data)
>  QCryptodevInfoList *qmp_query_cryptodev(Error **errp)
>  {
>      QCryptodevInfoList *list = NULL;
> -    Object *objs = container_get(object_get_root(), "/objects");
> +    Object *objs = object_get_container("objects");
>  
>      object_child_foreach(objs, qmp_query_cryptodev_foreach, &list);
>  
> @@ -557,7 +557,7 @@ static void cryptodev_backend_stats_cb(StatsResultList **result,
>      switch (target) {
>      case STATS_TARGET_CRYPTODEV:
>      {
> -        Object *objs = container_get(object_get_root(), "/objects");
> +        Object *objs = object_get_container("objects");
>          StatsArgs stats_args;
>          stats_args.result.stats = result;
>          stats_args.names = names;
> diff --git a/chardev/char.c b/chardev/char.c
> index a1722aa076..22fc5f7f76 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -48,7 +48,7 @@
>  
>  Object *get_chardevs_root(void)
>  {
> -    return container_get(object_get_root(), "/chardevs");
> +    return object_get_container("chardevs");
>  }
>  
>  static void chr_be_event(Chardev *s, QEMUChrEvent event)
> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
> index 80d07a6ec9..6e32a8eec6 100644
> --- a/hw/core/gpio.c
> +++ b/hw/core/gpio.c
> @@ -121,8 +121,7 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>                                       name ? name : "unnamed-gpio-out", n);
>      if (input_pin && !OBJECT(input_pin)->parent) {
>          /* We need a name for object_property_set_link to work */
> -        object_property_add_child(container_get(qdev_get_machine(),
> -                                                "/unattached"),
> +        object_property_add_child(machine_get_container("unattached"),
>                                    "non-qdev-gpio[*]", OBJECT(input_pin));
>      }
>      object_property_set_link(OBJECT(dev), propname,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6cb4fe4691..8051961fe5 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -476,8 +476,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          if (!obj->parent) {
>              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>  
> -            object_property_add_child(container_get(qdev_get_machine(),
> -                                                    "/unattached"),
> +            object_property_add_child(machine_get_container("unattached"),
>                                        name, obj);
>              unattached_parent = true;
>              g_free(name);
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index e64d99c8ed..9355849ff0 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -65,9 +65,9 @@ void foreach_dynamic_sysbus_device(FindSysbusDeviceFunc *func, void *opaque)
>      };
>  
>      /* Loop through all sysbus devices that were spawned outside the machine */
> -    container = container_get(qdev_get_machine(), "/peripheral");
> +    container = machine_get_container("peripheral");
>      find_sysbus_device(container, &find);
> -    container = container_get(qdev_get_machine(), "/peripheral-anon");
> +    container = machine_get_container("peripheral-anon");
>      find_sysbus_device(container, &find);
>  }
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 317aaca25a..b8ec2506e1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -460,7 +460,7 @@ static int check_fdc(Object *obj, void *opaque)
>  }
>  
>  static const char * const fdc_container_path[] = {
> -    "/unattached", "/peripheral", "/peripheral-anon"
> +    "unattached", "peripheral", "peripheral-anon"
>  };
>  
>  /*
> @@ -474,7 +474,7 @@ static ISADevice *pc_find_fdc0(void)
>      CheckFdcState state = { 0 };
>  
>      for (i = 0; i < ARRAY_SIZE(fdc_container_path); i++) {
> -        container = container_get(qdev_get_machine(), fdc_container_path[i]);
> +        container = machine_get_container(fdc_container_path[i]);
>          object_child_foreach(container, check_fdc, &state);
>      }
>  
> diff --git a/qom/container.c b/qom/container.c
> index da657754a4..5e36fb8773 100644
> --- a/qom/container.c
> +++ b/qom/container.c
> @@ -38,27 +38,4 @@ Object *container_create(Object *obj, const char *name)
>      return child;
>  }
>  
> -Object *container_get(Object *root, const char *path)
> -{
> -    Object *obj, *child;
> -    char **parts;
> -    int i;
> -
> -    parts = g_strsplit(path, "/", 0);
> -    assert(parts != NULL && parts[0] != NULL && !parts[0][0]);
> -    obj = root;
> -
> -    for (i = 1; parts[i] != NULL; i++, obj = child) {
> -        child = object_resolve_path_component(obj, parts[i]);
> -        if (!child) {
> -            child = container_create(obj, parts[i]);
> -        }
> -    }
> -
> -    g_strfreev(parts);
> -
> -    return obj;
> -}
> -
> -
>  type_init(container_register_types)
> diff --git a/qom/object.c b/qom/object.c
> index cf66803a6a..aeb04c483c 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1771,7 +1771,7 @@ Object *object_get_root(void)
>  
>  Object *object_get_objects_root(void)
>  {
> -    return container_get(object_get_root(), "/objects");
> +    return object_get_container("objects");
>  }
>  
>  Object *object_get_internal_root(void)
> diff --git a/scsi/pr-manager.c b/scsi/pr-manager.c
> index fb5fc29730..1977d99ce0 100644
> --- a/scsi/pr-manager.c
> +++ b/scsi/pr-manager.c
> @@ -21,7 +21,7 @@
>  #include "qemu/module.h"
>  #include "qapi/qapi-commands-block.h"
>  
> -#define PR_MANAGER_PATH     "/objects"
> +#define PR_MANAGER_PATH     "objects"
>  
>  typedef struct PRManagerData {
>      PRManager *pr_mgr;
> @@ -135,7 +135,7 @@ PRManagerInfoList *qmp_query_pr_managers(Error **errp)
>  {
>      PRManagerInfoList *head = NULL;
>      PRManagerInfoList **prev = &head;
> -    Object *container = container_get(object_get_root(), PR_MANAGER_PATH);
> +    Object *container = object_get_container(PR_MANAGER_PATH);
>  
>      object_child_foreach(container, query_one_pr_manager, &prev);
>      return head;
> diff --git a/system/ioport.c b/system/ioport.c
> index fd551d0375..55c2a75239 100644
> --- a/system/ioport.c
> +++ b/system/ioport.c
> @@ -258,7 +258,7 @@ static void portio_list_add_1(PortioList *piolist,
>      object_ref(&mrpio->mr);
>      object_unparent(OBJECT(&mrpio->mr));
>      if (!piolist->owner) {
> -        owner = container_get(qdev_get_machine(), "/unattached");
> +        owner = machine_get_container("unattached");
>      } else {
>          owner = piolist->owner;
>      }
> diff --git a/system/memory.c b/system/memory.c
> index 85f6834cb3..fba351b030 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1238,7 +1238,7 @@ static void memory_region_do_init(MemoryRegion *mr,
>          char *name_array = g_strdup_printf("%s[*]", escaped_name);
>  
>          if (!owner) {
> -            owner = container_get(qdev_get_machine(), "/unattached");
> +            owner = machine_get_container("unattached");
>          }
>  
>          object_property_add_child(owner, name_array, OBJECT(mr));
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 4c09b38ffb..4d46af2c8d 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -348,7 +348,7 @@ static Object *qdev_get_peripheral(void)
>      static Object *dev;
>  
>      if (dev == NULL) {
> -        dev = container_get(qdev_get_machine(), "/peripheral");
> +        dev = machine_get_container("peripheral");
>      }
>  
>      return dev;
> @@ -359,7 +359,7 @@ static Object *qdev_get_peripheral_anon(void)
>      static Object *dev;
>  
>      if (dev == NULL) {
> -        dev = container_get(qdev_get_machine(), "/peripheral-anon");
> +        dev = machine_get_container("peripheral-anon");
>      }
>  
>      return dev;
> @@ -1085,7 +1085,7 @@ static GSList *qdev_build_hotpluggable_device_list(Object *peripheral)
>  static void peripheral_device_del_completion(ReadLineState *rs,
>                                               const char *str)
>  {
> -    Object *peripheral = container_get(qdev_get_machine(), "/peripheral");
> +    Object *peripheral = machine_get_container("peripheral");
>      GSList *list, *item;
>  
>      list = qdev_build_hotpluggable_device_list(peripheral);
> diff --git a/system/vl.c b/system/vl.c
> index 3bb8f2db9a..822f7ff656 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2120,8 +2120,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));
> -    object_property_add_child(container_get(OBJECT(current_machine),
> -                                            "/unattached"),
> +    object_property_add_child(machine_get_container("unattached"),
>                                "sysbus", OBJECT(sysbus_get_default()));
>  
>      if (machine_class->minimum_page_bits) {
> diff --git a/ui/dbus-chardev.c b/ui/dbus-chardev.c
> index 1d3a7122a1..bf061cbc93 100644
> --- a/ui/dbus-chardev.c
> +++ b/ui/dbus-chardev.c
> @@ -106,7 +106,7 @@ dbus_chardev_init(DBusDisplay *dpy)
>      dpy->notifier.notify = dbus_display_on_notify;
>      dbus_display_notifier_add(&dpy->notifier);
>  
> -    object_child_foreach(container_get(object_get_root(), "/chardevs"),
> +    object_child_foreach(object_get_container("chardevs"),
>                           dbus_display_chardev_foreach, dpy);
>  }
>  
> -- 
> 2.45.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Philippe Mathieu-Daudé]

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

What about creating "backend" container in qemu_create_machine()?

Re: [PATCH 02/12] qom: New container_create()

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> To move towards explicit creations of containers, starting that by
> providing a helper for creating container objects.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 12 ++++++++++++
>  qom/container.c      | 18 +++++++++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 3ba370ce9b..41ef53241e 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -2033,6 +2033,18 @@ int object_child_foreach_recursive(Object *obj,
>   */
>  Object *container_get(Object *root, const char *path);
>  
> +
> +/**
> + * container_create:
> + * @root: root of the object to create the new container
> + * @name: name of the new container

Is this the name of the property of @root to hold the new container?
Peeking ahead to the implementation... yes.

> + *
> + * Create a container object under @root with @name.
> + *
> + * Returns: the newly created container object.
> + */
> +Object *container_create(Object *root, const char *name);

No function in this file is named like FOO_create().  Hmm.

Compare:

   /**
    * object_property_try_add_child:
    * @obj: the object to add a property to
    * @name: the name of the property
    * @child: the child object
    * @errp: pointer to error object
    *
    * Child properties form the composition tree.  All objects need to be a child
    * of another object.  Objects can only be a child of one object.
    *
    * There is no way for a child to determine what its parent is.  It is not
    * a bidirectional relationship.  This is by design.

Aside: this is nonsense.  While you're not supposed to simply use
obj->parent (it's documented as private), you can still get the child's
canonical path with object_get_canonical_path(), split off its last
component to get the parent's canonical path, then use
object_resolve_path() to get the parent.

    *
    * The value of a child property as a C string will be the child object's
    * canonical path. It can be retrieved using object_property_get_str().
    * The child object itself can be retrieved using object_property_get_link().
    *
    * Returns: The newly added property on success, or %NULL on failure.
    */

What about

   /**
    * object_property_add_new_container:
    * @obj: the parent object
    * @name: the name of the parent object's property to add
    *
    * Add a newly created container object to a parent object.
    *
    * Returns: the newly created container object.  Its reference count
    * is 1, and the reference is owned by the parent object.
    */

> +
>  /**
>   * object_property_help:
>   * @name: the name of the property
> diff --git a/qom/container.c b/qom/container.c
> index cfec92a944..da657754a4 100644
> --- a/qom/container.c
> +++ b/qom/container.c
> @@ -24,6 +24,20 @@ static void container_register_types(void)
>      type_register_static(&container_info);
>  }
>  
> +Object *container_create(Object *obj, const char *name)
> +{
> +    Object *child = object_new(TYPE_CONTAINER);
> +
> +    object_property_add_child(obj, name, child);
> +    /*
> +     * Simplify the caller by always drop the refcount directly here, as
> +     * containers are normally never destroyed after created anyway.
> +     */
> +    object_unref(child);

Do we still need the comment if we document the reference count in the
function comment?

> +
> +    return child;
> +}
> +
>  Object *container_get(Object *root, const char *path)
>  {
>      Object *obj, *child;
> @@ -37,9 +51,7 @@ Object *container_get(Object *root, const char *path)
>      for (i = 1; parts[i] != NULL; i++, obj = child) {
>          child = object_resolve_path_component(obj, parts[i]);
>          if (!child) {
> -            child = object_new(TYPE_CONTAINER);
> -            object_property_add_child(obj, parts[i], child);
> -            object_unref(child);
> +            child = container_create(obj, parts[i]);
>          }
>      }

Re: [PATCH 09/12] qdev: Add machine_get_container()

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Add a helper to fetch machine containers.  Add some sanity check around.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/hw/qdev-core.h | 10 ++++++++++
>  hw/core/qdev.c         | 17 +++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 5be9844412..38edfb1b54 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -996,6 +996,16 @@ const char *qdev_fw_name(DeviceState *dev);
>  void qdev_assert_realized_properly(void);
>  Object *qdev_get_machine(void);
>  
> +/**
> + * machine_get_container:
> + * @name: The name of container to lookup
> + *
> + * Get a container of the machine (QOM path "/machine/XXX").

Suggest "/machine/NAME" here.

> + *
> + * Returns: the machine container object.
> + */
> +Object *machine_get_container(const char *name);
> +
>  /**
>   * qdev_get_human_name() - Return a human-readable name for a device
>   * @dev: The device. Must be a valid and non-NULL pointer.

[...]

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Always explicitly create QEMU system containers upfront.
>
> Root containers will be created when trying to fetch the root object the
> 1st time.

Which ones are affected?

Not a fan of creating stuff on first use, unless there may not be any
use.  But no worse than before.

>            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/hw/core/machine.c b/hw/core/machine.c
> index a35c4a8fae..a184dbf8f0 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1193,14 +1193,27 @@ static void machine_class_base_init(ObjectClass *oc, void *data)
>      }
>  }
>  
> +const char *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++) {
> +        container_create(machine, machine_containers[i]);
> +    }
> +}
> +
>  static void machine_initfn(Object *obj)
>  {
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
>  
> -    container_get(obj, "/peripheral");
> -    container_get(obj, "/peripheral-anon");
> -
> +    qemu_create_machine_containers(obj);

We now additionally create "/machine/unattached" here.  

>      ms->dump_guest_core = true;
>      ms->mem_merge = (QEMU_MADV_MERGEABLE != QEMU_MADV_INVALID);
>      ms->enable_graphics = true;
> 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");
> +
> +    return root;
> +}
> +
>  Object *object_get_root(void)
>  {
>      static Object *root;
>  
>      if (!root) {
> -        root = object_new(TYPE_CONTAINER);
> +        root = object_root_initialize();

We now additonally create "/chardevs" and "/objects" here, not just "/".

>      }
>  
>      return root;

Re: [PATCH 02/12] qom: New container_create()

[Peter Xu]

On Thu, Nov 21, 2024 at 02:20:44PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > To move towards explicit creations of containers, starting that by
> > providing a helper for creating container objects.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qom/object.h | 12 ++++++++++++
> >  qom/container.c      | 18 +++++++++++++++---
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 3ba370ce9b..41ef53241e 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -2033,6 +2033,18 @@ int object_child_foreach_recursive(Object *obj,
> >   */
> >  Object *container_get(Object *root, const char *path);
> >  
> > +
> > +/**
> > + * container_create:
> > + * @root: root of the object to create the new container
> > + * @name: name of the new container
> 
> Is this the name of the property of @root to hold the new container?
> Peeking ahead to the implementation... yes.
> 
> > + *
> > + * Create a container object under @root with @name.
> > + *
> > + * Returns: the newly created container object.
> > + */
> > +Object *container_create(Object *root, const char *name);
> 
> No function in this file is named like FOO_create().  Hmm.
> 
> Compare:
> 
>    /**
>     * object_property_try_add_child:
>     * @obj: the object to add a property to
>     * @name: the name of the property
>     * @child: the child object
>     * @errp: pointer to error object
>     *
>     * Child properties form the composition tree.  All objects need to be a child
>     * of another object.  Objects can only be a child of one object.
>     *
>     * There is no way for a child to determine what its parent is.  It is not
>     * a bidirectional relationship.  This is by design.
> 
> Aside: this is nonsense.  While you're not supposed to simply use
> obj->parent (it's documented as private), you can still get the child's
> canonical path with object_get_canonical_path(), split off its last
> component to get the parent's canonical path, then use
> object_resolve_path() to get the parent.
> 
>     *
>     * The value of a child property as a C string will be the child object's
>     * canonical path. It can be retrieved using object_property_get_str().
>     * The child object itself can be retrieved using object_property_get_link().
>     *
>     * Returns: The newly added property on success, or %NULL on failure.
>     */
> 
> What about
> 
>    /**
>     * object_property_add_new_container:
>     * @obj: the parent object
>     * @name: the name of the parent object's property to add
>     *
>     * Add a newly created container object to a parent object.
>     *
>     * Returns: the newly created container object.  Its reference count
>     * is 1, and the reference is owned by the parent object.
>     */

Sure, this may indeed align better with the rest function names.

> 
> > +
> >  /**
> >   * object_property_help:
> >   * @name: the name of the property
> > diff --git a/qom/container.c b/qom/container.c
> > index cfec92a944..da657754a4 100644
> > --- a/qom/container.c
> > +++ b/qom/container.c
> > @@ -24,6 +24,20 @@ static void container_register_types(void)
> >      type_register_static(&container_info);
> >  }
> >  
> > +Object *container_create(Object *obj, const char *name)
> > +{
> > +    Object *child = object_new(TYPE_CONTAINER);
> > +
> > +    object_property_add_child(obj, name, child);
> > +    /*
> > +     * Simplify the caller by always drop the refcount directly here, as
> > +     * containers are normally never destroyed after created anyway.
> > +     */
> > +    object_unref(child);
> 
> Do we still need the comment if we document the reference count in the
> function comment?

Probably not.  I'll drop this comment while taking above suggestion.

Thanks,

-- 
Peter Xu

Re: [PATCH 05/12] ui/console: Explicitly create "/backend" container

[Peter Xu]

On Thu, Nov 21, 2024 at 10:26:34AM +0000, Daniel P. Berrangé wrote:
> On Wed, Nov 20, 2024 at 04:56:56PM -0500, Peter Xu wrote:
> > Follow the trend to explicitly create containers, do that for console.c on
> > "/backend" container.
> > 
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  ui/console.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ui/console.c b/ui/console.c
> > index 5165f17125..36f8c6debb 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -1154,14 +1154,14 @@ DisplayState *init_displaystate(void)
> >  {
> >      gchar *name;
> >      QemuConsole *con;
> > +    Object *backend = container_create(object_get_root(), "backend");
> 
> What's the rationale for keeping this in the console code ?
> 
> I'd consider this to be a similar situation to '/chardevs' and
> '/objects', and be a common container rather than UI specific.
> IOW, be created by your later patch.

I was trying to be careful on always create then on demand like
before. E.g. I was thinking maybe this container shouldn't exist when
there's no display at all.

But looks like init_displaystate() is indeed always invoked for system
code.. so yeah, perhaps I can move it over too, and drop this patch (below
will be part of last patch to use objects_get_container() instead, or part
of its splits).

> 
> >  
> >      QTAILQ_FOREACH(con, &consoles, next) {
> >          /* Hook up into the qom tree here (not in object_new()), once
> >           * all QemuConsoles are created and the order / numbering
> >           * doesn't change any more */
> >          name = g_strdup_printf("console[%d]", con->index);
> > -        object_property_add_child(container_get(object_get_root(), "/backend"),
> > -                                  name, OBJECT(con));
> > +        object_property_add_child(backend, name, OBJECT(con));
> >          g_free(name);
> >      }
> 
> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

-- 
Peter Xu

Re: [PATCH 06/12] hw/ppc: Explicitly create the drc container

[Peter Xu]

On Thu, Nov 21, 2024 at 10:35:39AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Peter,
> 
> On 20/11/24 22:56, Peter Xu wrote:
> > QEMU will start to not rely on implicit creations of containers soon.  Make
> > PPC drc devices follow by explicitly create the container whenever a drc
> > device is realized, dropping container_get() calls.
> > 
> > No functional change intended.
> > 
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Cc: qemu-ppc@nongnu.org
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
> >   1 file changed, 30 insertions(+), 10 deletions(-)
> 
> 
> > +static GOnce drc_container_created = G_ONCE_INIT;
> > +
> > +static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
> > +{
> > +    container_create(object_get_root(), DRC_CONTAINER_PATH);
> > +    return NULL;
> > +}
> > +
> > +static Object *drc_container_get(void)
> > +{
> > +    return object_resolve_path_component(
> > +        object_get_root(), DRC_CONTAINER_PATH);
> > +}
> > +
> > +/* TODO: create the container in an ppc init function */
> > +static void drc_container_create_once(void)
> > +{
> > +    g_once(&drc_container_created, drc_container_create, NULL);
> > +}
> > +
> >   static void drc_realize(DeviceState *d, Error **errp)
> >   {
> >       SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
> > @@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
> >       Object *root_container;
> >       const char *child_name;
> > +    /* Whenever a DRC device is realized, create the container */
> > +    drc_container_create_once();
> 
> Can't we just create it once in spapr_dr_connector_class_init(),
> removing the G_ONCE_INIT need?

IIUC it's a matter of whether there's case where we have this file compiled
in, but never create any DRC device.  When that happens, the patch can
change the QEMU qom-tree slightly, in that there'll be an empty drc
container while we used to not have it.

I'm trying to be on the safe side in case something would detect the
container's existance to know whether drc devices are present.  lazy create
it in realize() is the safest way to behave 100% identical like before,
considering my ppc knowledge is merely zero (even if I have drc tests
covered in ppc64's qtest..).

However I also doubt whether it matters much.  It'll be great if I can get
an ACK from anyone familiar with this device (Phil, are you?), then I can
move it over.

> 
> >       trace_spapr_drc_realize(spapr_drc_index(drc));
> >       /* NOTE: we do this as part of realize/unrealize due to the fact
> >        * that the guest will communicate with the DRC via RTAS calls
> > @@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
> >        * inaccessible by the guest, since lookups rely on this path
> >        * existing in the composition tree
> >        */
> > -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> > +    root_container = drc_container_get();
> >       child_name = object_get_canonical_path_component(OBJECT(drc));
> >       trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
> >       object_property_add_alias(root_container, link_name,
> 

-- 
Peter Xu

Re: [PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Peter Xu]

On Thu, Nov 21, 2024 at 10:48:43AM +0100, Cédric Le Goater wrote:
> On 11/21/24 10:38, Cédric Le Goater wrote:
> > On 11/20/24 22:56, Peter Xu wrote:
> > > container_get() is going to become strict on not allowing to return a
> > > non-container.
> > > 
> > > Switch the e500 user to use object_resolve_path_component() explicitly.
> > > 
> > > Cc: Bharat Bhushan <r65777@freescale.com>
> > > Cc: qemu-ppc@nongnu.org
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >   hw/pci-host/ppce500.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> > > index b70631045a..65233b9e3f 100644
> > > --- a/hw/pci-host/ppce500.c
> > > +++ b/hw/pci-host/ppce500.c
> > > @@ -418,8 +418,8 @@ static const VMStateDescription vmstate_ppce500_pci = {
> > >   static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
> > >   {
> > >       PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
> > > -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
> > > -                                  "/e500-ccsr"));
> > > +    PPCE500CCSRState *ccsr = CCSR(
> > > +        object_resolve_path_component(qdev_get_machine(), "e500-ccsr"));
> > 
> > 
> > why not simply use :
> > 
> >        CCSR(object_resolve_path("/machine/e500-ccsr", NULL));
> 
> 
> I guess we want to avoid the absolute paths. If so,

It wasn't my intention, but what you said actually makes sense to me to
avoid hard-coded "/machine" if possible.

OTOH, object_resolve_path_component() was actually tiny little faster when
we know the depth of the path.

> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> 
> We might want to convert these lookups to object_resolve_path_component
> too, not in this patchset.
> 
> hw/i386/acpi-build.c:    host = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
> hw/i386/acpi-build.c:        host = PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));
> target/i386/kvm/kvm.c:        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
> target/i386/tcg/sysemu/tcg-cpu.c:        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);

Sounds reasonable to me to use the same style.  I'll stick with this patch
as of now in the current series.

Thanks,

-- 
Peter Xu

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

[Peter Xu]

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

Re: [PATCH 06/12] hw/ppc: Explicitly create the drc container

[Philippe Mathieu-Daudé]

On 21/11/24 17:36, Peter Xu wrote:
> On Thu, Nov 21, 2024 at 10:35:39AM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Peter,
>>
>> On 20/11/24 22:56, Peter Xu wrote:
>>> QEMU will start to not rely on implicit creations of containers soon.  Make
>>> PPC drc devices follow by explicitly create the container whenever a drc
>>> device is realized, dropping container_get() calls.
>>>
>>> No functional change intended.
>>>
>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>> Cc: qemu-ppc@nongnu.org
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>    hw/ppc/spapr_drc.c | 40 ++++++++++++++++++++++++++++++----------
>>>    1 file changed, 30 insertions(+), 10 deletions(-)
>>
>>
>>> +static GOnce drc_container_created = G_ONCE_INIT;
>>> +
>>> +static gpointer drc_container_create(gpointer unused G_GNUC_UNUSED)
>>> +{
>>> +    container_create(object_get_root(), DRC_CONTAINER_PATH);
>>> +    return NULL;
>>> +}
>>> +
>>> +static Object *drc_container_get(void)
>>> +{
>>> +    return object_resolve_path_component(
>>> +        object_get_root(), DRC_CONTAINER_PATH);
>>> +}
>>> +
>>> +/* TODO: create the container in an ppc init function */
>>> +static void drc_container_create_once(void)
>>> +{
>>> +    g_once(&drc_container_created, drc_container_create, NULL);
>>> +}
>>> +
>>>    static void drc_realize(DeviceState *d, Error **errp)
>>>    {
>>>        SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
>>> @@ -521,6 +541,9 @@ static void drc_realize(DeviceState *d, Error **errp)
>>>        Object *root_container;
>>>        const char *child_name;
>>> +    /* Whenever a DRC device is realized, create the container */
>>> +    drc_container_create_once();
>>
>> Can't we just create it once in spapr_dr_connector_class_init(),
>> removing the G_ONCE_INIT need?
> 
> IIUC it's a matter of whether there's case where we have this file compiled
> in, but never create any DRC device.  When that happens, the patch can
> change the QEMU qom-tree slightly, in that there'll be an empty drc
> container while we used to not have it.
> 
> I'm trying to be on the safe side in case something would detect the
> container's existance to know whether drc devices are present.  lazy create
> it in realize() is the safest way to behave 100% identical like before,

Class singleton must be taken care in class_init, not once in the
first instance realize(). I'd rather fix this bad QOM pattern.

> considering my ppc knowledge is merely zero (even if I have drc tests
> covered in ppc64's qtest..).
> 
> However I also doubt whether it matters much.  It'll be great if I can get
> an ACK from anyone familiar with this device (Phil, are you?), then I can
> move it over.

I'm not, but having to care about G_ONCE_INIT seems cumbersome.

> 
>>
>>>        trace_spapr_drc_realize(spapr_drc_index(drc));
>>>        /* NOTE: we do this as part of realize/unrealize due to the fact
>>>         * that the guest will communicate with the DRC via RTAS calls
>>> @@ -529,7 +552,7 @@ static void drc_realize(DeviceState *d, Error **errp)
>>>         * inaccessible by the guest, since lookups rely on this path
>>>         * existing in the composition tree
>>>         */
>>> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
>>> +    root_container = drc_container_get();
>>>        child_name = object_get_canonical_path_component(OBJECT(drc));
>>>        trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
>>>        object_property_add_alias(root_container, link_name,
>>
> 

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

[Peter Xu]

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

-- 
Peter Xu

Re: [PATCH 07/12] ppc/e500: Avoid abuse of container_get()

[Philippe Mathieu-Daudé]

On 21/11/24 17:41, Peter Xu wrote:
> On Thu, Nov 21, 2024 at 10:48:43AM +0100, Cédric Le Goater wrote:
>> On 11/21/24 10:38, Cédric Le Goater wrote:
>>> On 11/20/24 22:56, Peter Xu wrote:
>>>> container_get() is going to become strict on not allowing to return a
>>>> non-container.
>>>>
>>>> Switch the e500 user to use object_resolve_path_component() explicitly.
>>>>
>>>> Cc: Bharat Bhushan <r65777@freescale.com>
>>>> Cc: qemu-ppc@nongnu.org
>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>> ---
>>>>    hw/pci-host/ppce500.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
>>>> index b70631045a..65233b9e3f 100644
>>>> --- a/hw/pci-host/ppce500.c
>>>> +++ b/hw/pci-host/ppce500.c
>>>> @@ -418,8 +418,8 @@ static const VMStateDescription vmstate_ppce500_pci = {
>>>>    static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
>>>>    {
>>>>        PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
>>>> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
>>>> -                                  "/e500-ccsr"));
>>>> +    PPCE500CCSRState *ccsr = CCSR(
>>>> +        object_resolve_path_component(qdev_get_machine(), "e500-ccsr"));
>>>
>>>
>>> why not simply use :
>>>
>>>         CCSR(object_resolve_path("/machine/e500-ccsr", NULL));
>>
>>
>> I guess we want to avoid the absolute paths. If so,
> 
> It wasn't my intention, but what you said actually makes sense to me to
> avoid hard-coded "/machine" if possible.

Long term (heterogeneous emulation & dynamic machines in mind) I'm
unsure about qdev_get_machine() future. Not to take care now, but
I'm expecting it to evolve. So not using hardcoded path is better
IMHO.

> 
> OTOH, object_resolve_path_component() was actually tiny little faster when
> we know the depth of the path.
> 
>>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>
>>
>> We might want to convert these lookups to object_resolve_path_component
>> too, not in this patchset.
>>
>> hw/i386/acpi-build.c:    host = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
>> hw/i386/acpi-build.c:        host = PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));
>> target/i386/kvm/kvm.c:        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
>> target/i386/tcg/sysemu/tcg-cpu.c:        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
> 
> Sounds reasonable to me to use the same style.  I'll stick with this patch
> as of now in the current series.
> 
> Thanks,
> 

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

[Peter Xu]

On Thu, Nov 21, 2024 at 02:31:23PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Always explicitly create QEMU system containers upfront.
> >
> > Root containers will be created when trying to fetch the root object the
> > 1st time.
> 
> Which ones are affected?

I updated the commit message to this:

    qom: Create system containers explicitly
    
    Always explicitly create QEMU system containers upfront.
    
    Root containers will be created when trying to fetch the root object the
    1st time.  They are:
    
      /objects
      /chardevs
      /backend
    
    Machine sub-containers will be created only until machine is being
    initialized.  They are:
    
      /machine/unattached
      /machine/peripheral
      /machine/peripheral-anon

Note that I also added "backend" per request from Dan, as of now.

> 
> Not a fan of creating stuff on first use, unless there may not be any
> use.  But no worse than before.

True.. since that can be a separate question to answer, I avoided going too
far to dig into which binaries may use containers, which may not.

Thanks,

-- 
Peter Xu

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

[Philippe Mathieu-Daudé]

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() =)

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

[Peter Xu]

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

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

[Philippe Mathieu-Daudé]

On 21/11/24 19:03, Peter Xu wrote:
> 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..

"machine_initfn requires the containers to be present" -> machine_initfn
isn't the place to create them, it has to be before or after. Since it
can't be before, the "after" place is qemu_create_machine_containers().
Sorry for not being very clear :/

> 
> 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()));
>   
> 

Yes, this is exactly what I was thinking of / expecting :)