[PATCH v2 00/13] QOM: container_get() removal

[PATCH v2 00/13] QOM: container_get() removal

[Peter Xu]

This series is not for 9.2, but for 10.0.

v1: https://lore.kernel.org/r/20241120215703.3918445-1-peterx@redhat.com

v2 changelog:
- Rename container_create() to object_property_add_new_container() [Markus]
- Drop "ui/console: Explicitly create "/backend" container", instead create
  "/backend" container along with the rest root containers [Dan]
- Assert in qdev_get_machine() instead of allowing return NULL [Dan]
- machine_get_container(), remove assert, and tiny doc fix [Dan, Markus]
- Make machine_containers[] "const char *const" [Phil]
- Create ppc drc container in drc device class_init [Phil]
- Split the last patch into three [Phil, Dan]

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.

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

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

  /objects
  /chardevs
  /backend
  /dr-connector    (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, see "tests:
Fix test-qdev-global-props on anonymous qdev realize()".

Comments welcomed, thanks.

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

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

-- 
2.45.0

[PATCH v2 01/13] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 v2 02/13] qom: New object_property_add_new_container()

[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      | 14 +++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 3ba370ce9b..597e961b8c 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);
 
+/**
+ * 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 *object_property_add_new_container(Object *obj, const char *name);
+
 /**
  * object_property_help:
  * @name: the name of the property
diff --git a/qom/container.c b/qom/container.c
index cfec92a944..20ab74b0e8 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -24,6 +24,16 @@ static void container_register_types(void)
     type_register_static(&container_info);
 }
 
+Object *object_property_add_new_container(Object *obj, const char *name)
+{
+    Object *child = object_new(TYPE_CONTAINER);
+
+    object_property_add_child(obj, name, child);
+    object_unref(child);
+
+    return child;
+}
+
 Object *container_get(Object *root, const char *path)
 {
     Object *obj, *child;
@@ -37,9 +47,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 = object_property_add_new_container(obj, parts[i]);
         }
     }
 
-- 
2.45.0

Re: [PATCH v2 02/13] qom: New object_property_add_new_container()

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, 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      | 14 +++++++++++---
>   2 files changed, 23 insertions(+), 3 deletions(-)

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

Re: [PATCH v2 02/13] qom: New object_property_add_new_container()

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 03/13] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/unit/test-qdev-global-props.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tests/unit/test-qdev-global-props.c b/tests/unit/test-qdev-global-props.c
index c8862cac5f..85756d39ce 100644
--- a/tests/unit/test-qdev-global-props.c
+++ b/tests/unit/test-qdev-global-props.c
@@ -72,6 +72,26 @@ 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 = object_property_add_new_container(
+        object_get_root(), "machine");
+
+    /* This container must exist for anonymous qdevs to realize() */
+    object_property_add_new_container(machine, "unattached");
+}
+
 /* Test simple static property setting to default value */
 static void test_static_prop_subprocess(void)
 {
@@ -295,6 +315,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 v2 04/13] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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..b48e890577 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 = object_property_add_new_container(root, "cont1");
     Object *obj1  = object_new(TYPE_DUMMY);
     Object *obj2a = object_new(TYPE_DUMMY);
     Object *obj2b = object_new(TYPE_DUMMY);
-- 
2.45.0

[PATCH v2 05/13] 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
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 v2 06/13] 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 | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1484e3209d..7868048bb9 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,16 @@ static const VMStateDescription vmstate_spapr_drc = {
     }
 };
 
+static void drc_container_create(void)
+{
+    object_property_add_new_container(object_get_root(), DRC_CONTAINER_PATH);
+}
+
+static Object *drc_container_get(void)
+{
+    return object_resolve_path_component(object_get_root(), DRC_CONTAINER_PATH);
+}
+
 static void drc_realize(DeviceState *d, Error **errp)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
@@ -529,7 +539,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 +553,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,
@@ -585,6 +593,8 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
 {
     DeviceClass *dk = DEVICE_CLASS(k);
 
+    drc_container_create();
+
     dk->realize = drc_realize;
     dk->unrealize = drc_unrealize;
     /*
@@ -796,9 +806,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 +869,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 +962,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

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

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, 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 | 29 +++++++++++++++++++----------
>   1 file changed, 19 insertions(+), 10 deletions(-)

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

[PATCH v2 07/13] 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.  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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c |  3 ---
 qom/object.c      | 24 +++++++++++++++++++++++-
 system/vl.c       | 16 ++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a35c4a8fae..a72c001c3d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1198,9 +1198,6 @@ 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");
-
     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..2fb0b8418e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1734,12 +1734,34 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
     return prop->type;
 }
 
+static const char *const root_containers[] = {
+    "chardevs",
+    "objects",
+    "backend"
+};
+
+static Object *object_root_initialize(void)
+{
+    Object *root = object_new(TYPE_CONTAINER);
+    int i;
+
+    /*
+     * Create all QEMU system containers.  "machine" and its sub-containers
+     * are only created when machine initializes (qemu_create_machine()).
+     */
+    for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
+        object_property_add_new_container(root, root_containers[i]);
+    }
+
+    return root;
+}
+
 Object *object_get_root(void)
 {
     static Object *root;
 
     if (!root) {
-        root = object_new(TYPE_CONTAINER);
+        root = object_root_initialize();
     }
 
     return root;
diff --git a/system/vl.c b/system/vl.c
index 3bb8f2db9a..15e35162c6 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(container_get(OBJECT(current_machine),
                                             "/unattached"),
                               "sysbus", OBJECT(sysbus_get_default()));
-- 
2.45.0

Re: [PATCH v2 07/13] qom: Create system containers explicitly

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, 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.  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
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/machine.c |  3 ---
>   qom/object.c      | 24 +++++++++++++++++++++++-
>   system/vl.c       | 16 ++++++++++++++++
>   3 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a35c4a8fae..a72c001c3d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1198,9 +1198,6 @@ 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");
> -
>       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..2fb0b8418e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1734,12 +1734,34 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
>       return prop->type;
>   }
>   
> +static const char *const root_containers[] = {
> +    "chardevs",
> +    "objects",
> +    "backend"
> +};
> +
> +static Object *object_root_initialize(void)
> +{

(root_containers[] can be declared here)

> +    Object *root = object_new(TYPE_CONTAINER);
> +    int i;
> +
> +    /*
> +     * Create all QEMU system containers.  "machine" and its sub-containers
> +     * are only created when machine initializes (qemu_create_machine()).

I'm not sure this comment is helpful. Regardless,

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

> +     */
> +    for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
> +        object_property_add_new_container(root, root_containers[i]);
> +    }
> +
> +    return root;
> +}

> +
>   Object *object_get_root(void)
>   {
>       static Object *root;
>   
>       if (!root) {
> -        root = object_new(TYPE_CONTAINER);
> +        root = object_root_initialize();
>       }
>   
>       return root;
> diff --git a/system/vl.c b/system/vl.c
> index 3bb8f2db9a..15e35162c6 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)
> +{

(ditto machine_containers[])

> +    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(container_get(OBJECT(current_machine),
>                                               "/unattached"),
>                                 "sysbus", OBJECT(sysbus_get_default()));

[PATCH v2 08/13] 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5f13111b77..b622be15ee 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
     static Object *dev;
 
     if (dev == NULL) {
-        dev = container_get(object_get_root(), "/machine");
+        dev = object_resolve_path_component(object_get_root(), "machine");
+        /*
+         * Any call to this function before machine is created is treated
+         * as a programming error as of now.
+         */
+        assert(dev);
     }
 
     return dev;
-- 
2.45.0

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

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, Peter Xu wrote:
> Currently, qdev_get_machine() has a slight misuse on container_get(), as
> the helper says "get a container" but in reality the goal is to get the
> machine object.  It is still a "container" but not strictly.
> 
> Note that it _may_ get a container (at "/machine") in our current unit test
> of test-qdev-global-props.c before all these changes, but it's probably
> unexpected and worked by accident.
> 
> Switch to an explicit object_resolve_path_component(), with a side benefit
> that qdev_get_machine() can happen a lot, and we don't need to split the
> string ("/machine") every time.  This also paves way for making the helper
> container_get() never try to return a non-container at all.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/qdev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

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

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

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, Peter Xu wrote:
> Currently, qdev_get_machine() has a slight misuse on container_get(), as
> the helper says "get a container" but in reality the goal is to get the
> machine object.  It is still a "container" but not strictly.
> 
> Note that it _may_ get a container (at "/machine") in our current unit test
> of test-qdev-global-props.c before all these changes, but it's probably
> unexpected and worked by accident.
> 
> Switch to an explicit object_resolve_path_component(), with a side benefit
> that qdev_get_machine() can happen a lot, and we don't need to split the
> string ("/machine") every time.  This also paves way for making the helper
> container_get() never try to return a non-container at all.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/qdev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5f13111b77..b622be15ee 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>       static Object *dev;
>   
>       if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        dev = object_resolve_path_component(object_get_root(), "machine");
> +        /*
> +         * Any call to this function before machine is created is treated
> +         * as a programming error as of now.
> +         */
> +        assert(dev);

This fails for user-emulation:

./qemu-x86_64 /bin/echo foo
qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion `dev' 
failed.
Aborted (core dumped)

We need to skip this test for user emulation, but this file is in
hwcore_ss[] so the CONFIG_USER_ONLY definitions is not available.

Any simple enough idea to not block this?

>       }
>   
>       return dev;

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

[Philippe Mathieu-Daudé]

On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
> On 21/11/24 20:21, Peter Xu wrote:
>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>> the helper says "get a container" but in reality the goal is to get the
>> machine object.  It is still a "container" but not strictly.
>>
>> Note that it _may_ get a container (at "/machine") in our current unit 
>> test
>> of test-qdev-global-props.c before all these changes, but it's probably
>> unexpected and worked by accident.
>>
>> Switch to an explicit object_resolve_path_component(), with a side 
>> benefit
>> that qdev_get_machine() can happen a lot, and we don't need to split the
>> string ("/machine") every time.  This also paves way for making the 
>> helper
>> container_get() never try to return a non-container at all.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>   hw/core/qdev.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 5f13111b77..b622be15ee 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>>       static Object *dev;
>>       if (dev == NULL) {
>> -        dev = container_get(object_get_root(), "/machine");
>> +        dev = object_resolve_path_component(object_get_root(), 
>> "machine");
>> +        /*
>> +         * Any call to this function before machine is created is 
>> treated
>> +         * as a programming error as of now.
>> +         */
>> +        assert(dev);
> 
> This fails for user-emulation:
> 
> ./qemu-x86_64 /bin/echo foo
> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion `dev' 
> failed.
> Aborted (core dumped)

(gdb) bt
#5  0x00007ffff747171b in __assert_fail_base (fmt=0x7ffff7626130 
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x555555725150 
"dev",
     file=0x55555571aff9 "../../hw/core/qdev.c", line=824, 
function=<optimized out>) at ./assert/assert.c:92
#8  0x000055555565e400 in qdev_get_machine () at ../../hw/core/qdev.c:824
#9  machine_get_container (name=0x55555571b052 "unattached") at 
../../hw/core/qdev.c:834
#10 0x000055555565ea2d in device_set_realized (obj=0x5555558b6760, 
value=<optimized out>, errp=0x7fffffffdb50) at ../../hw/core/qdev.c:479
#11 0x000055555566181a in property_set_bool (obj=0x5555558b6760, 
v=<optimized out>, name=<optimized out>, opaque=0x555555813350, 
errp=0x7fffffffdb50)
     at ../../qom/object.c:2375
#12 0x00005555556649f8 in object_property_set 
(obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e 
"realized", v=v@entry=0x5555558c0680,
     errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1450
#13 0x0000555555668754 in object_property_set_qobject 
(obj=obj@entry=0x5555558b6760, name=name@entry=0x55555571b03e 
"realized", value=value@entry=0x5555558be490,
     errp=errp@entry=0x7fffffffdb50) at ../../qom/qom-qobject.c:28
#14 0x00005555556650c9 in object_property_set_bool (obj=0x5555558b6760, 
name=name@entry=0x55555571b03e "realized", value=value@entry=true,
     errp=errp@entry=0x7fffffffdb50) at ../../qom/object.c:1520
#15 0x000055555565dd52 in qdev_realize (dev=<optimized out>, 
bus=bus@entry=0x0, errp=errp@entry=0x7fffffffdb50) at 
../../hw/core/qdev.c:276
#16 0x0000555555593dc9 in cpu_create (typename=<optimized out>) at 
../../hw/core/cpu-common.c:61
#17 0x00005555555925de in main (argc=3, argv=0x7fffffffe308, 
envp=<optimized out>) at ../../linux-user/main.c:823


> 
> We need to skip this test for user emulation, but this file is in
> hwcore_ss[] so the CONFIG_USER_ONLY definitions is not available.
> 
> Any simple enough idea to not block this?
> 
>>       }
>>       return dev;
> 

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

[Philippe Mathieu-Daudé]

On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
> On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
>> On 21/11/24 20:21, Peter Xu wrote:
>>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>>> the helper says "get a container" but in reality the goal is to get the
>>> machine object.  It is still a "container" but not strictly.
>>>
>>> Note that it _may_ get a container (at "/machine") in our current 
>>> unit test
>>> of test-qdev-global-props.c before all these changes, but it's probably
>>> unexpected and worked by accident.
>>>
>>> Switch to an explicit object_resolve_path_component(), with a side 
>>> benefit
>>> that qdev_get_machine() can happen a lot, and we don't need to split the
>>> string ("/machine") every time.  This also paves way for making the 
>>> helper
>>> container_get() never try to return a non-container at all.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>   hw/core/qdev.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 5f13111b77..b622be15ee 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>>>       static Object *dev;
>>>       if (dev == NULL) {
>>> -        dev = container_get(object_get_root(), "/machine");
>>> +        dev = object_resolve_path_component(object_get_root(), 
>>> "machine");
>>> +        /*
>>> +         * Any call to this function before machine is created is 
>>> treated
>>> +         * as a programming error as of now.
>>> +         */
>>> +        assert(dev);
>>
>> This fails for user-emulation:
>>
>> ./qemu-x86_64 /bin/echo foo
>> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion 
>> `dev' failed.

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

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

Regards,

Phil.

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

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

[Peter Xu]

On Fri, Dec 20, 2024 at 12:25:44PM +0100, Philippe Mathieu-Daudé wrote:
> On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
> > On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
> > > On 21/11/24 20:21, Peter Xu wrote:
> > > > Currently, qdev_get_machine() has a slight misuse on container_get(), as
> > > > the helper says "get a container" but in reality the goal is to get the
> > > > machine object.  It is still a "container" but not strictly.
> > > > 
> > > > Note that it _may_ get a container (at "/machine") in our
> > > > current unit test
> > > > of test-qdev-global-props.c before all these changes, but it's probably
> > > > unexpected and worked by accident.
> > > > 
> > > > Switch to an explicit object_resolve_path_component(), with a
> > > > side benefit
> > > > that qdev_get_machine() can happen a lot, and we don't need to split the
> > > > string ("/machine") every time.  This also paves way for making
> > > > the helper
> > > > container_get() never try to return a non-container at all.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >   hw/core/qdev.c | 7 ++++++-
> > > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > index 5f13111b77..b622be15ee 100644
> > > > --- a/hw/core/qdev.c
> > > > +++ b/hw/core/qdev.c
> > > > @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
> > > >       static Object *dev;
> > > >       if (dev == NULL) {
> > > > -        dev = container_get(object_get_root(), "/machine");
> > > > +        dev = object_resolve_path_component(object_get_root(),
> > > > "machine");
> > > > +        /*
> > > > +         * Any call to this function before machine is created
> > > > is treated
> > > > +         * as a programming error as of now.
> > > > +         */
> > > > +        assert(dev);
> > > 
> > > This fails for user-emulation:
> > > 
> > > ./qemu-x86_64 /bin/echo foo
> > > qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion
> > > `dev' failed.
> 
> OK so I guess I might have found a "fix" which is to simply not
> call qdev_get_machine() for user emulation, but this involves some
> invasive refactoring -- so will take time --.

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

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

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

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

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

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

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

OTOH, this works for me:

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

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

Thanks,

-- 
Peter Xu

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

[Philippe Mathieu-Daudé]

On 20/12/24 18:24, Peter Xu wrote:
> On Fri, Dec 20, 2024 at 12:25:44PM +0100, Philippe Mathieu-Daudé wrote:
>> On 19/12/24 19:27, Philippe Mathieu-Daudé wrote:
>>> On 19/12/24 19:20, Philippe Mathieu-Daudé wrote:
>>>> On 21/11/24 20:21, Peter Xu wrote:
>>>>> Currently, qdev_get_machine() has a slight misuse on container_get(), as
>>>>> the helper says "get a container" but in reality the goal is to get the
>>>>> machine object.  It is still a "container" but not strictly.
>>>>>
>>>>> Note that it _may_ get a container (at "/machine") in our
>>>>> current unit test
>>>>> of test-qdev-global-props.c before all these changes, but it's probably
>>>>> unexpected and worked by accident.
>>>>>
>>>>> Switch to an explicit object_resolve_path_component(), with a
>>>>> side benefit
>>>>> that qdev_get_machine() can happen a lot, and we don't need to split the
>>>>> string ("/machine") every time.  This also paves way for making
>>>>> the helper
>>>>> container_get() never try to return a non-container at all.
>>>>>
>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>> ---
>>>>>    hw/core/qdev.c | 7 ++++++-
>>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index 5f13111b77..b622be15ee 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -817,7 +817,12 @@ Object *qdev_get_machine(void)
>>>>>        static Object *dev;
>>>>>        if (dev == NULL) {
>>>>> -        dev = container_get(object_get_root(), "/machine");
>>>>> +        dev = object_resolve_path_component(object_get_root(),
>>>>> "machine");
>>>>> +        /*
>>>>> +         * Any call to this function before machine is created
>>>>> is treated
>>>>> +         * as a programming error as of now.
>>>>> +         */
>>>>> +        assert(dev);
>>>>
>>>> This fails for user-emulation:
>>>>
>>>> ./qemu-x86_64 /bin/echo foo
>>>> qemu-x86_64: ../../hw/core/qdev.c:825: qdev_get_machine: Assertion
>>>> `dev' failed.
>>
>> OK so I guess I might have found a "fix" which is to simply not
>> call qdev_get_machine() for user emulation, but this involves some
>> invasive refactoring -- so will take time --.
> 
> Thanks for taking a look, Phil.  Yes this sounds clean.
> 
>>
>> I'm dropping this series for now, planning to merge it again on top
>> of my refactor once it is ready. Any clever / simpler fix is
>> obviously welcomed first.
> 
> I initially thought about this, which could also be clean but I then
> noticed LINUX_USER is poisoned..
> 
> ===8<===
> diff --git a/qom/object.c b/qom/object.c
> index 58897a79a7..da26e8d69b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1729,7 +1729,19 @@ const char *object_property_get_type(Object *obj, const char *name, Error **errp
>       return prop->type;
>   }
> 
> +/*
> + * Create all QEMU default containers.
> + *
> + * For system emulations, "machine" and its sub-containers are only created
> + * when machine initializes (qemu_create_machine()).
> + *
> + * For user emulations, create "machine" before hand to make qdev realize()
> + * work by default.
> + */
>   static const char *const root_containers[] = {
> +#ifdef CONFIG_LINUX_USER
> +    "machine",
> +#endif
>       "chardevs",
>       "objects",
>       "backend"
> @@ -1740,10 +1752,6 @@ static Object *object_root_initialize(void)
>       Object *root = object_new(TYPE_CONTAINER);
>       int i;
> 
> -    /*
> -     * Create all QEMU system containers.  "machine" and its sub-containers
> -     * are only created when machine initializes (qemu_create_machine()).
> -     */
>       for (i = 0; i < ARRAY_SIZE(root_containers); i++) {
>           object_property_add_new_container(root, root_containers[i]);
>       }
> ===8<===
> 
> Maybe we could still move it somewhere that LINUX_USER is not poisoned
> (plus "unattached" be created too, more below)?
> 
> OTOH, this works for me:
> 
> ===8<===
> diff --git a/linux-user/main.c b/linux-user/main.c
> index b09af8d436..009b7695f2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp)
>       set_preferred_target_page_bits(ctz32(host_page_size));
>       finalize_target_page_bits();
>   
> +    Object *fake_obj = object_property_add_new_container(object_get_root(),
> +                                                         "machine");
> +    object_property_add_new_container(fake_obj, "unattached");
> +
>       cpu = cpu_create(cpu_type);
>       env = cpu_env(cpu);
>       cpu_reset(cpu);
> ===8<===

I like it, simple enough, allowing to remove container_get() now.

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

Maybe add a common method in hw/core/qdev-user.c?
qemu_create_machine() or qdev_create_fake_machine()?

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

[Peter Xu]

On Fri, Dec 20, 2024 at 10:38:40PM +0100, Philippe Mathieu-Daudé wrote:
> > OTOH, this works for me:
> > 
> > ===8<===
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index b09af8d436..009b7695f2 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -819,6 +819,11 @@ int main(int argc, char **argv, char **envp)
> >       set_preferred_target_page_bits(ctz32(host_page_size));
> >       finalize_target_page_bits();
> > +    Object *fake_obj = object_property_add_new_container(object_get_root(),
> > +                                                         "machine");
> > +    object_property_add_new_container(fake_obj, "unattached");
> > +
> >       cpu = cpu_create(cpu_type);
> >       env = cpu_env(cpu);
> >       cpu_reset(cpu);
> > ===8<===
> 
> I like it, simple enough, allowing to remove container_get() now.
> 
> > 
> > So we need both "/machine" and "/machine/unattached" so far to make
> > linux-user work.  Not sure if bsd-user/main.c needs similar care, but none
> > of these look as clean.
> 
> Maybe add a common method in hw/core/qdev-user.c?
> qemu_create_machine() or qdev_create_fake_machine()?

Sounds good at least to me. I'd vote for the latter to reflect it's unreal.

Thanks,

-- 
Peter Xu

[PATCH v2 09/13] qdev: Add machine_get_container()

[Peter Xu]

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

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/hw/qdev-core.h | 10 ++++++++++
 hw/core/qdev.c         | 11 +++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5be9844412..dc6cd951fa 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/NAME").
+ *
+ * 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 b622be15ee..fa3bc85d9a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -828,6 +828,17 @@ Object *qdev_get_machine(void)
     return dev;
 }
 
+Object *machine_get_container(const char *name)
+{
+    Object *container, *machine;
+
+    machine = qdev_get_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 v2 10/13] qom: Use machine_get_container()

[Peter Xu]

Use machine_get_container() whenever applicable across the tree.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/gpio.c        | 3 +--
 hw/core/qdev.c        | 3 +--
 hw/core/sysbus.c      | 4 ++--
 hw/i386/pc.c          | 4 ++--
 system/ioport.c       | 2 +-
 system/memory.c       | 2 +-
 system/qdev-monitor.c | 6 +++---
 system/vl.c           | 3 +--
 8 files changed, 12 insertions(+), 15 deletions(-)

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 fa3bc85d9a..d52b669355 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/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 15e35162c6..cdc0b6e10c 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2136,8 +2136,7 @@ static void qemu_create_machine(QDict *qdict)
     object_property_add_child(object_get_root(), "machine",
                               OBJECT(current_machine));
     qemu_create_machine_containers(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) {
-- 
2.45.0

Re: [PATCH v2 10/13] qom: Use machine_get_container()

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, Peter Xu wrote:
> Use machine_get_container() whenever applicable across the tree.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/gpio.c        | 3 +--
>   hw/core/qdev.c        | 3 +--
>   hw/core/sysbus.c      | 4 ++--
>   hw/i386/pc.c          | 4 ++--
>   system/ioport.c       | 2 +-
>   system/memory.c       | 2 +-
>   system/qdev-monitor.c | 6 +++---
>   system/vl.c           | 3 +--
>   8 files changed, 12 insertions(+), 15 deletions(-)

Preferably s/qom/qdev/ in subject (I can do it if queuing),

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

Re: [PATCH v2 10/13] qom: Use machine_get_container()

[Philippe Mathieu-Daudé]

(Cc'ing qemu-block@ for floppy disc device)

On 21/11/24 20:21, Peter Xu wrote:
> Use machine_get_container() whenever applicable across the tree.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/gpio.c        | 3 +--
>   hw/core/qdev.c        | 3 +--
>   hw/core/sysbus.c      | 4 ++--
>   hw/i386/pc.c          | 4 ++--
>   system/ioport.c       | 2 +-
>   system/memory.c       | 2 +-
>   system/qdev-monitor.c | 6 +++---
>   system/vl.c           | 3 +--
>   8 files changed, 12 insertions(+), 15 deletions(-)


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

Orthogonal to this series, but noticing while giving another look at
it. Is this method really using the correct API? It seems to poke at
a lower level.

>       }
>   

[PATCH v2 11/13] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 597e961b8c..4eeee7f7c4 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 2fb0b8418e..25316b9536 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1756,6 +1756,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 v2 12/13] qom: Use object_get_container()

[Peter Xu]

Use object_get_container() whenever applicable across the tree.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 backends/cryptodev.c | 4 ++--
 chardev/char.c       | 2 +-
 qom/object.c         | 2 +-
 scsi/pr-manager.c    | 4 ++--
 ui/console.c         | 2 +-
 ui/dbus-chardev.c    | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

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/qom/object.c b/qom/object.c
index 25316b9536..c9e53ed92c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1779,7 +1779,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/ui/console.c b/ui/console.c
index 5165f17125..914ed2cc76 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1160,7 +1160,7 @@ DisplayState *init_displaystate(void)
          * 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"),
+        object_property_add_child(object_get_container("backend"),
                                   name, OBJECT(con));
         g_free(name);
     }
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 v2 12/13] qom: Use object_get_container()

[Philippe Mathieu-Daudé]

On 21/11/24 20:22, Peter Xu wrote:
> Use object_get_container() whenever applicable across the tree.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   backends/cryptodev.c | 4 ++--
>   chardev/char.c       | 2 +-
>   qom/object.c         | 2 +-
>   scsi/pr-manager.c    | 4 ++--
>   ui/console.c         | 2 +-
>   ui/dbus-chardev.c    | 2 +-
>   6 files changed, 8 insertions(+), 8 deletions(-)


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

We can probably inline this string, since the definition isn't
really useful (and match the other uses). Anyway,

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

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

[PATCH v2 13/13] qom: Remove container_get()

[Peter Xu]

Now there's no user of container_get(), remove it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 11 -----------
 qom/container.c      | 23 -----------------------
 2 files changed, 34 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 4eeee7f7c4..f3a887e34f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -2031,17 +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);
 
 /**
  * object_property_add_new_container:
diff --git a/qom/container.c b/qom/container.c
index 20ab74b0e8..38a27ec1ed 100644
--- a/qom/container.c
+++ b/qom/container.c
@@ -34,27 +34,4 @@ Object *object_property_add_new_container(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 = object_property_add_new_container(obj, parts[i]);
-        }
-    }
-
-    g_strfreev(parts);
-
-    return obj;
-}
-
-
 type_init(container_register_types)
-- 
2.45.0

Re: [PATCH v2 13/13] qom: Remove container_get()

[Philippe Mathieu-Daudé]

On 21/11/24 20:22, Peter Xu wrote:
> Now there's no user of container_get(), remove it.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qom/object.h | 11 -----------
>   qom/container.c      | 23 -----------------------
>   2 files changed, 34 deletions(-)

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

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

[Philippe Mathieu-Daudé]

On 21/11/24 20:21, Peter Xu wrote:

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

Series queued, thanks.