[PATCH V2 0/5] fast qom tree get

[PATCH V2 0/5] fast qom tree get

[Steve Sistare]

Using qom-list and qom-get to get all the nodes and property values in a
QOM tree can take multiple seconds because it requires 1000's of individual
QOM requests.  Some managers fetch the entire tree or a large subset
of it when starting a new VM, and this cost is a substantial fraction of
start up time.

To reduce this cost, consider QAPI calls that fetch more information in
each call:
  * qom-list-get: given a path, return a list of properties and values.
  * qom-list-getv: given a list of paths, return a list of properties and
    values for each path.
  * qom-tree-get: given a path, return all descendant nodes rooted at that
    path, with properties and values for each.

In all cases, a returned property is represented by ObjectPropertyValue,
with fields name, type, and value.  If an error occurs when reading a value
the value field is omitted.  Thus an error for one property will not cause a
bulk fetch operation to fail.

To evaluate each method, I modified scripts/qmp/qom-tree to use the method,
verified all methods produce the same output, and timed each using:

  qemu-system-x86_64 -display none \
    -chardev socket,id=monitor0,path=/tmp/vm1.sock,server=on,wait=off \
    -mon monitor0,mode=control &

  time qom-tree -s /tmp/vm1.sock > /dev/null

I only measured once per method, but the variation is low after a warm up run.
The 'real - user - sys' column is a proxy for QEMU CPU time.

method               real(s)   user(s)   sys(s)  (real - user - sys)(s)
qom-list / qom-get   2.048     0.932     0.057   1.059
qom-list-get         0.402     0.230     0.029   0.143
qom-list-getv        0.200     0.132     0.015   0.053
qom-tree-get         0.143     0.123     0.012   0.008

qom-tree-get is the clear winner, reducing elapsed time by a factor of 14X,
and reducing QEMU CPU time by 132X.

qom-list-getv is slower when fetching the entire tree, but can beat
qom-tree-get when only a subset of the tree needs to be fetched (not shown).
qom-list-get is shown for comparison only, and is not included in this series.

Changes in V2:
  * removed "qom: qom_resolve_path", which was pulled separately
  * dropped the error member
  * fixed missing _list_tree in qom.py
  * updated 10.0 to 10.1

Steve Sistare (5):
  qom: qom-tree-get
  python: use qom-tree-get
  tests/qtest/qom-test: unit test for qom-tree-get
  qom: qom-list-getv
  tests/qtest/qom-test: unit test for qom-list-getv

 python/qemu/utils/qom.py        |  36 ++++++-------
 python/qemu/utils/qom_common.py |  48 +++++++++++++++++
 qapi/qom.json                   |  90 ++++++++++++++++++++++++++++++++
 qom/qom-qmp-cmds.c              | 112 +++++++++++++++++++++++++++++++++++++++
 tests/qtest/qom-test.c          | 113 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 381 insertions(+), 18 deletions(-)

base-commit: 7be29f2f1a3f5b037d27eedbd5df9f441e8c8c16

-- 
1.8.3.1

[PATCH V2 1/5] qom: qom-tree-get

[Steve Sistare]

Define the qom-tree-get QAPI command, which fetches an entire tree of
properties and values with a single QAPI call.  This is much faster
than using qom-list plus qom-get for every node and property of the
tree.  See qom.json for details.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
 qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 28ce24c..94662ad 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -46,6 +46,38 @@
             '*default-value': 'any' } }
 
 ##
+# @ObjectPropertyValue:
+#
+# @name: the name of the property
+#
+# @type: the type of the property, as described in @ObjectPropertyInfo
+#
+# @value: the value of the property.  Omitted if cannot be read.
+#
+# Since 10.1
+##
+{ 'struct': 'ObjectPropertyValue',
+  'data': { 'name': 'str',
+            'type': 'str',
+            '*value': 'any' } }
+
+##
+# @ObjectNode:
+#
+# @name: the name of the node
+#
+# @children: child nodes
+#
+# @properties: properties of the node
+#
+# Since 10.1
+##
+{ 'struct': 'ObjectNode',
+  'data': { 'name': 'str',
+            'children': [ 'ObjectNode' ],
+            'properties': [ 'ObjectPropertyValue' ] }}
+
+##
 # @qom-list:
 #
 # This command will list any properties of a object given a path in
@@ -126,6 +158,30 @@
   'allow-preconfig': true }
 
 ##
+# @qom-tree-get:
+#
+# This command returns a tree of objects and their properties,
+# rooted at the specified path.
+#
+# @path: The absolute or partial path within the object model, as
+#     described in @qom-get
+#
+# Errors:
+#     - If path is not valid or is ambiguous, returns an error.
+#     - If a property cannot be read, the value field is omitted in
+#       the corresponding @ObjectPropertyValue.
+#
+# Returns: A tree of @ObjectNode.  Each node contains its name, list
+#     of properties, and list of child nodes.
+#
+# Since 10.1
+##
+{ 'command': 'qom-tree-get',
+  'data': { 'path': 'str' },
+  'returns': 'ObjectNode',
+  'allow-preconfig': true }
+
+##
 # @qom-set:
 #
 # This command will set a property from a object model path.
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 293755f..b876681 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -69,6 +69,78 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
     return props;
 }
 
+static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
+                                        ObjectPropertyValueList **props)
+{
+    ObjectPropertyValue *item = g_new0(ObjectPropertyValue, 1);
+    Error *err = NULL;
+
+    QAPI_LIST_PREPEND(*props, item);
+
+    item->name = g_strdup(prop->name);
+    item->type = g_strdup(prop->type);
+    item->value = object_property_get_qobject(obj, prop->name, &err);
+
+    if (!item->value) {
+        /*
+         * For bulk get, the error message is dropped, but the value field
+         * is omitted so the caller knows this property could not be read.
+         */
+        error_free(err);
+    }
+}
+
+static ObjectNode *qom_tree_get(const char *path, Error **errp)
+{
+    Object *obj;
+    ObjectProperty *prop;
+    ObjectNode *result, *child;
+    ObjectPropertyIterator iter;
+
+    obj = qom_resolve_path(path, errp);
+    if (obj == NULL) {
+        return NULL;
+    }
+
+    result = g_new0(ObjectNode, 1);
+
+    object_property_iter_init(&iter, obj);
+    while ((prop = object_property_iter_next(&iter))) {
+        if (strstart(prop->type, "child<", NULL)) {
+            g_autofree char *child_path = g_strdup_printf("%s/%s",
+                                                          path, prop->name);
+            child = qom_tree_get(child_path, errp);
+            if (!child) {
+                qapi_free_ObjectNode(result);
+                return NULL;
+            }
+            child->name = g_strdup(prop->name);
+            QAPI_LIST_PREPEND(result->children, child);
+        } else {
+            qom_list_add_property_value(obj, prop, &result->properties);
+        }
+    }
+
+    return result;
+}
+
+ObjectNode *qmp_qom_tree_get(const char *path, Error **errp)
+{
+    ObjectNode *result = qom_tree_get(path, errp);
+
+    if (result) {
+        /* Strip the path prefix if any */
+        const char *basename = strrchr(path, '/');
+
+        if (!basename || !basename[1]) {
+            result->name = g_strdup(path);
+        } else {
+            result->name = g_strdup(basename + 1);
+        }
+    }
+    return result;
+}
+
 void qmp_qom_set(const char *path, const char *property, QObject *value,
                  Error **errp)
 {
-- 
1.8.3.1

[PATCH V2 2/5] python: use qom-tree-get

[Steve Sistare]

Use qom-tree-get to speed up the qom-tree command.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 python/qemu/utils/qom.py        | 36 +++++++++++++++----------------
 python/qemu/utils/qom_common.py | 48 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/python/qemu/utils/qom.py b/python/qemu/utils/qom.py
index 426a0f2..33d0a60 100644
--- a/python/qemu/utils/qom.py
+++ b/python/qemu/utils/qom.py
@@ -35,6 +35,7 @@
 from qemu.qmp import ExecuteError
 
 from .qom_common import QOMCommand
+from .qom_common import ObjectNode
 
 
 try:
@@ -224,28 +225,27 @@ def __init__(self, args: argparse.Namespace):
         super().__init__(args)
         self.path = args.path
 
-    def _list_node(self, path: str) -> None:
+    def _list_tree(self, node: ObjectNode, path: str) -> None:
         print(path)
-        items = self.qom_list(path)
-        for item in items:
-            if item.child:
-                continue
-            try:
-                rsp = self.qmp.cmd('qom-get', path=path,
-                                   property=item.name)
-                print(f"  {item.name}: {rsp} ({item.type})")
-            except ExecuteError as err:
-                print(f"  {item.name}: <EXCEPTION: {err!s}> ({item.type})")
+
+        for item in node.properties:
+            value = item.value
+            if value == None:
+                value = f"<EXCEPTION: property could not be read>"
+            print(f"  {item.name}: {value} ({item.type})")
+
         print('')
-        for item in items:
-            if not item.child:
-                continue
-            if path == '/':
-                path = ''
-            self._list_node(f"{path}/{item.name}")
+        if path == '/':
+            path = ''
+
+        for child in node.children:
+            self._list_tree(child, f"{path}/{child.name}")
+
 
     def run(self) -> int:
-        self._list_node(self.path)
+        root = self.qom_tree_get(self.path)
+        self._list_tree(root, self.path)
+
         return 0
 
 
diff --git a/python/qemu/utils/qom_common.py b/python/qemu/utils/qom_common.py
index dd2c8b1..8c242cc 100644
--- a/python/qemu/utils/qom_common.py
+++ b/python/qemu/utils/qom_common.py
@@ -65,6 +65,47 @@ def link(self) -> bool:
         return self.type.startswith('link<')
 
 
+class ObjectPropertyValue:
+    """
+    Represents a property return from e.g. qom-tree-get
+    """
+    def __init__(self, name: str, type_: str, value: object):
+        self.name = name
+        self.type = type_
+        self.value = value
+
+    @classmethod
+    def make(cls, value: Dict[str, Any]) -> 'ObjectPropertyValue':
+        """
+        Build an ObjectPropertyValue from a Dict with an unknown shape.
+        """
+        assert value.keys() >= {'name', 'type'}
+        assert value.keys() <= {'name', 'type', 'value'}
+        return cls(value['name'], value['type'], value.get('value'))
+
+
+class ObjectNode:
+    """
+    Represents the return type from e.g. qom-tree-get
+    """
+    def __init__(self, name: str, children: List['ObjectNode'],
+                 properties: List[ObjectPropertyValue]):
+        self.name = name
+        self.children = children
+        self.properties = properties
+
+    @classmethod
+    def make(cls, value: Dict[str, Any]) -> 'ObjectNode':
+        """
+        Build an ObjectNode from a Dict with an unknown shape.
+        """
+        assert value.keys() == {'name', 'children', 'properties'}
+
+        props = [ObjectPropertyValue.make(x) for x in value['properties']]
+        children = [ObjectNode.make(x) for x in value['children']]
+        return cls(value['name'], children, props)
+
+
 CommandT = TypeVar('CommandT', bound='QOMCommand')
 
 
@@ -145,6 +186,13 @@ def qom_list(self, path: str) -> List[ObjectPropertyInfo]:
         assert isinstance(rsp, list)
         return [ObjectPropertyInfo.make(x) for x in rsp]
 
+    def qom_tree_get(self, path: str) -> ObjectNode:
+        """
+        :return: a strongly typed root node from the 'qom-tree-get' command.
+        """
+        rsp = self.qmp.cmd('qom-tree-get', path=path)
+        return ObjectNode.make(rsp)
+
     @classmethod
     def command_runner(
             cls: Type[CommandT],
-- 
1.8.3.1

[PATCH V2 3/5] tests/qtest/qom-test: unit test for qom-tree-get

[Steve Sistare]

Add a unit test for qom-tree-get

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 tests/qtest/qom-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
index 27d70bc..d8792bd 100644
--- a/tests/qtest/qom-test.c
+++ b/tests/qtest/qom-test.c
@@ -16,6 +16,54 @@
 
 static int verbosity_level;
 
+static void test_tree_node(QDict *node)
+{
+    QDict *prop, *child;
+    QList *props, *children;
+    QListEntry *entry;
+
+    g_assert(qdict_haskey(node, "name"));
+    g_assert(qdict_haskey(node, "properties"));
+
+    if (verbosity_level >= 3) {
+        g_test_message("%s", qdict_get_str(node, "name"));
+    }
+
+    props = qobject_to(QList, qdict_get(node, "properties"));
+    QLIST_FOREACH_ENTRY(props, entry) {
+        prop = qobject_to(QDict, qlist_entry_obj(entry));
+        g_assert(qdict_haskey(prop, "name"));
+        g_assert(qdict_haskey(prop, "type"));
+    }
+
+    if (!qdict_haskey(node, "children")) {
+        return;
+    }
+
+    children = qobject_to(QList, qdict_get(node, "children"));
+    QLIST_FOREACH_ENTRY(children, entry) {
+        child = qobject_to(QDict, qlist_entry_obj(entry));
+        test_tree_node(child);
+    }
+}
+
+static void test_tree(QTestState *qts, const char *path)
+{
+    g_autoptr(QDict) response = NULL;
+    QDict *node;
+
+    if (verbosity_level >= 2) {
+        g_test_message("Obtaining tree at %s", path);
+    }
+    response = qtest_qmp(qts, "{ 'execute': 'qom-tree-get',"
+                              "  'arguments': { 'path': %s } }", path);
+    g_assert(response);
+
+    g_assert(qdict_haskey(response, "return"));
+    node = qobject_to(QDict, qdict_get(response, "return"));
+    test_tree_node(node);
+}
+
 static void test_properties(QTestState *qts, const char *path, bool recurse)
 {
     char *child_path;
@@ -100,6 +148,7 @@ static void test_machine(gconstpointer data)
     }
 
     test_properties(qts, "/machine", true);
+    test_tree(qts, "/machine");
 
     response = qtest_qmp(qts, "{ 'execute': 'quit' }");
     g_assert(qdict_haskey(response, "return"));
-- 
1.8.3.1

[PATCH V2 4/5] qom: qom-list-getv

[Steve Sistare]

Define the qom-list-getv command, which fetches all the properties and
values for a list of paths.  This is faster than qom-tree-get when
fetching a subset of the QOM tree.  See qom.json for details.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 qapi/qom.json      | 34 ++++++++++++++++++++++++++++++++++
 qom/qom-qmp-cmds.c | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 94662ad..dc710d6 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -62,6 +62,16 @@
             '*value': 'any' } }
 
 ##
+# @ObjectPropertiesValues:
+#
+# @properties: a list of properties.
+#
+# Since 10.1
+##
+{ 'struct': 'ObjectPropertiesValues',
+  'data': { 'properties': [ 'ObjectPropertyValue' ] }}
+
+##
 # @ObjectNode:
 #
 # @name: the name of the node
@@ -158,6 +168,30 @@
   'allow-preconfig': true }
 
 ##
+# @qom-list-getv:
+#
+# This command returns a list of properties and their values for
+# each object path in the input list.
+#
+# @paths: The absolute or partial path for each object, as described
+#     in @qom-get
+#
+# Errors:
+#     - If any path is not valid or is ambiguous, returns an error.
+#     - If a property cannot be read, the value field is omitted in
+#       the corresponding @ObjectPropertyValue.
+#
+# Returns: A list of @ObjectPropertiesValues.  Each element contains
+#     the properties of the corresponding element in @paths.
+#
+# Since 10.1
+##
+{ 'command': 'qom-list-getv',
+  'data': { 'paths': [ 'str' ] },
+  'returns': [ 'ObjectPropertiesValues' ],
+  'allow-preconfig': true }
+
+##
 # @qom-tree-get:
 #
 # This command returns a tree of objects and their properties,
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index b876681..1f05956 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -90,6 +90,46 @@ static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
     }
 }
 
+static ObjectPropertyValueList *qom_get_property_value_list(const char *path,
+                                                            Error **errp)
+{
+    Object *obj;
+    ObjectProperty *prop;
+    ObjectPropertyIterator iter;
+    ObjectPropertyValueList *props = NULL;
+
+    obj = qom_resolve_path(path, errp);
+    if (obj == NULL) {
+        return NULL;
+    }
+
+    object_property_iter_init(&iter, obj);
+    while ((prop = object_property_iter_next(&iter))) {
+        qom_list_add_property_value(obj, prop, &props);
+    }
+
+    return props;
+}
+
+ObjectPropertiesValuesList *qmp_qom_list_getv(strList *paths, Error **errp)
+{
+    ObjectPropertiesValuesList *head = NULL, **tail = &head;
+
+    for ( ; paths ; paths = paths->next) {
+        ObjectPropertiesValues *item = g_new0(ObjectPropertiesValues, 1);
+
+        QAPI_LIST_APPEND(tail, item);
+
+        item->properties = qom_get_property_value_list(paths->value, errp);
+        if (!item->properties) {
+            qapi_free_ObjectPropertiesValuesList(head);
+            return NULL;
+        }
+    }
+
+    return head;
+}
+
 static ObjectNode *qom_tree_get(const char *path, Error **errp)
 {
     Object *obj;
-- 
1.8.3.1

[PATCH V2 5/5] tests/qtest/qom-test: unit test for qom-list-getv

[Steve Sistare]

Add a unit test for qom-list-getv.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 tests/qtest/qom-test.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
index d8792bd..671b757 100644
--- a/tests/qtest/qom-test.c
+++ b/tests/qtest/qom-test.c
@@ -11,6 +11,7 @@
 
 #include "qobject/qdict.h"
 #include "qobject/qlist.h"
+#include "qobject/qstring.h"
 #include "qemu/cutils.h"
 #include "libqtest.h"
 
@@ -64,6 +65,66 @@ static void test_tree(QTestState *qts, const char *path)
     test_tree_node(node);
 }
 
+static void test_getv(QTestState *qts, QList *paths)
+{
+    QListEntry *entry, *prop_entry, *path_entry;
+    g_autoptr(QDict) response = NULL;
+    QDict *args = qdict_new();
+    QDict *prop;
+    QList *return_list;
+
+    if (verbosity_level >= 2) {
+        g_test_message("Obtaining properties for paths:");
+        QLIST_FOREACH_ENTRY(paths, path_entry) {
+            QString *qstr = qobject_to(QString, qlist_entry_obj(path_entry));
+            g_test_message("  %s", qstring_get_str(qstr));
+        }
+    }
+
+    qdict_put_obj(args, "paths", QOBJECT(qlist_copy(paths)));
+    response = qtest_qmp(qts, "{ 'execute': 'qom-list-getv',"
+                              "  'arguments': %p }", args);
+    g_assert(response);
+    g_assert(qdict_haskey(response, "return"));
+    return_list = qobject_to(QList, qdict_get(response, "return"));
+
+    path_entry = QTAILQ_FIRST(&paths->head);
+    QLIST_FOREACH_ENTRY(return_list, entry) {
+        QDict *obj = qobject_to(QDict, qlist_entry_obj(entry));
+        g_assert(qdict_haskey(obj, "properties"));
+        QList *properties = qobject_to(QList, qdict_get(obj, "properties"));
+        bool has_child = false;
+
+        QLIST_FOREACH_ENTRY(properties, prop_entry) {
+            prop = qobject_to(QDict, qlist_entry_obj(prop_entry));
+            g_assert(qdict_haskey(prop, "name"));
+            g_assert(qdict_haskey(prop, "type"));
+            has_child |= strstart(qdict_get_str(prop, "type"), "child<", NULL);
+        }
+
+        if (has_child) {
+            /* build a list of child paths */
+            QString *qstr = qobject_to(QString, qlist_entry_obj(path_entry));
+            const char *path = qstring_get_str(qstr);
+            g_autoptr(QList) child_paths = qlist_new();
+
+            QLIST_FOREACH_ENTRY(properties, prop_entry) {
+                prop = qobject_to(QDict, qlist_entry_obj(prop_entry));
+                if (strstart(qdict_get_str(prop, "type"), "child<", NULL)) {
+                    g_autofree char *child_path = g_strdup_printf(
+                        "%s/%s", path, qdict_get_str(prop, "name"));
+                    qlist_append_str(child_paths, child_path);
+                }
+            }
+
+            /* fetch props for all children with one qom-list-getv call */
+            test_getv(qts, child_paths);
+        }
+
+        path_entry = QTAILQ_NEXT(path_entry, next);
+    }
+}
+
 static void test_properties(QTestState *qts, const char *path, bool recurse)
 {
     char *child_path;
@@ -133,6 +194,7 @@ static void test_machine(gconstpointer data)
     const char *machine = data;
     QDict *response;
     QTestState *qts;
+    g_autoptr(QList) paths = qlist_new();
 
     qts = qtest_initf("-machine %s", machine);
 
@@ -149,6 +211,8 @@ static void test_machine(gconstpointer data)
 
     test_properties(qts, "/machine", true);
     test_tree(qts, "/machine");
+    qlist_append_str(paths, "/machine");
+    test_getv(qts, paths);
 
     response = qtest_qmp(qts, "{ 'execute': 'quit' }");
     g_assert(qdict_haskey(response, "return"));
-- 
1.8.3.1

Re: [PATCH V2 0/5] fast qom tree get

[Fabiano Rosas]

Steve Sistare <steven.sistare@oracle.com> writes:

> Using qom-list and qom-get to get all the nodes and property values in a
> QOM tree can take multiple seconds because it requires 1000's of individual
> QOM requests.  Some managers fetch the entire tree or a large subset
> of it when starting a new VM, and this cost is a substantial fraction of
> start up time.
>
> To reduce this cost, consider QAPI calls that fetch more information in
> each call:
>   * qom-list-get: given a path, return a list of properties and values.
>   * qom-list-getv: given a list of paths, return a list of properties and
>     values for each path.
>   * qom-tree-get: given a path, return all descendant nodes rooted at that
>     path, with properties and values for each.
>
> In all cases, a returned property is represented by ObjectPropertyValue,
> with fields name, type, and value.  If an error occurs when reading a value
> the value field is omitted.  Thus an error for one property will not cause a
> bulk fetch operation to fail.
>
> To evaluate each method, I modified scripts/qmp/qom-tree to use the method,
> verified all methods produce the same output, and timed each using:
>
>   qemu-system-x86_64 -display none \
>     -chardev socket,id=monitor0,path=/tmp/vm1.sock,server=on,wait=off \
>     -mon monitor0,mode=control &
>
>   time qom-tree -s /tmp/vm1.sock > /dev/null
>
> I only measured once per method, but the variation is low after a warm up run.
> The 'real - user - sys' column is a proxy for QEMU CPU time.
>
> method               real(s)   user(s)   sys(s)  (real - user - sys)(s)
> qom-list / qom-get   2.048     0.932     0.057   1.059
> qom-list-get         0.402     0.230     0.029   0.143
> qom-list-getv        0.200     0.132     0.015   0.053
> qom-tree-get         0.143     0.123     0.012   0.008
>
> qom-tree-get is the clear winner, reducing elapsed time by a factor of 14X,
> and reducing QEMU CPU time by 132X.
>
> qom-list-getv is slower when fetching the entire tree, but can beat
> qom-tree-get when only a subset of the tree needs to be fetched (not shown).
> qom-list-get is shown for comparison only, and is not included in this series.
>
> Changes in V2:
>   * removed "qom: qom_resolve_path", which was pulled separately
>   * dropped the error member
>   * fixed missing _list_tree in qom.py
>   * updated 10.0 to 10.1
>
> Steve Sistare (5):
>   qom: qom-tree-get
>   python: use qom-tree-get
>   tests/qtest/qom-test: unit test for qom-tree-get
>   qom: qom-list-getv
>   tests/qtest/qom-test: unit test for qom-list-getv
>
>  python/qemu/utils/qom.py        |  36 ++++++-------
>  python/qemu/utils/qom_common.py |  48 +++++++++++++++++
>  qapi/qom.json                   |  90 ++++++++++++++++++++++++++++++++
>  qom/qom-qmp-cmds.c              | 112 +++++++++++++++++++++++++++++++++++++++
>  tests/qtest/qom-test.c          | 113 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 381 insertions(+), 18 deletions(-)
>
> base-commit: 7be29f2f1a3f5b037d27eedbd5df9f441e8c8c16

Acked-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH V2 1/5] qom: qom-tree-get

[Markus Armbruster]

Steve Sistare <steven.sistare@oracle.com> writes:

> Define the qom-tree-get QAPI command, which fetches an entire tree of
> properties and values with a single QAPI call.  This is much faster
> than using qom-list plus qom-get for every node and property of the
> tree.  See qom.json for details.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>  qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 128 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24c..94662ad 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -46,6 +46,38 @@
>              '*default-value': 'any' } }
>  
>  ##
> +# @ObjectPropertyValue:
> +#
> +# @name: the name of the property
> +#
> +# @type: the type of the property, as described in @ObjectPropertyInfo

That description is crap.  In part because what it tries to describe is
crap.  Neither is this patch's problem.

> +#
> +# @value: the value of the property.  Omitted if cannot be read.

Suggest "Absent when the property cannot be read."

> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectPropertyValue',
> +  'data': { 'name': 'str',
> +            'type': 'str',
> +            '*value': 'any' } }

ObjectPropertyValue suggests this describes a property's value.  It does
not.  It includes the name, i.e. it describes the *property*.

So does ObjectPropertyInfo.

The two overlap: both habe name and type.  Only ObjectPropertyValue has
the current value.  Only ObjectPropertyInfo has the default value and
description (I suspect the latter is useless in practice).

ObjectPropertyInfo is used with qom-list and qom-list-properties.

qom-list takes a QOM path, like your qom-tree-get and qom-list-getv.
I'd expect your commands to supersede qom-list in practice.

qom-list-properties is unlike your qom-tree-get and qom-list-getv: it
takes a type name.  It's unreliable for non-abstract types: it can miss
dynamically created properties.

Let's ignore all this for now.

> +
> +##
> +# @ObjectNode:
> +#
> +# @name: the name of the node
> +#
> +# @children: child nodes
> +#
> +# @properties: properties of the node
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectNode',
> +  'data': { 'name': 'str',
> +            'children': [ 'ObjectNode' ],
> +            'properties': [ 'ObjectPropertyValue' ] }}
> +
> +##
>  # @qom-list:
>  #
>  # This command will list any properties of a object given a path in
> @@ -126,6 +158,30 @@
>    'allow-preconfig': true }
>  
>  ##
> +# @qom-tree-get:
> +#
> +# This command returns a tree of objects and their properties,
> +# rooted at the specified path.
> +#
> +# @path: The absolute or partial path within the object model, as
> +#     described in @qom-get
> +#
> +# Errors:
> +#     - If path is not valid or is ambiguous, returns an error.

By convention, we use "If <condition>, <error>, where <error> is a
member of QapiErrorClass.

What are the possible error classes?  As far as I can tell:

         - If path is ambiguous, GenericError
         - If path cannot be resolved, DeviceNotFound

However, use of error classes other than GenericError is strongly
discouraged (see error_set() in qapi/error.h).

Is the ability to distinguish between these two errors useful?

Existing related commands such as qom-get also use DeviceNotFound.
Entirely undocumented, exact error conditions unclear.  Awesome.

Libvirt seems to rely on this undocumented behavior: I can see code
checking for DeviceNotFound.  Hyrum's law strikes.

qom-get fails with DeviceNotFound in both of the above cases.  It fails
with GenericError when @property doesn't exist or cannot be read.  Your
qom-tree-get fails differently.  Awesome again.

Choices:

1. Leave errors undocumented and inconsistent.

2. Document errors for all related commands.  Make the new ones as
consistent as we can.

> +#     - If a property cannot be read, the value field is omitted in
> +#       the corresponding @ObjectPropertyValue.

This is not an error, and therefore doesn't belong here.
ObjectPropertyValue's documentation also mentions it.  Good enough?

> +#
> +# Returns: A tree of @ObjectNode.  Each node contains its name, list
> +#     of properties, and list of child nodes.

Hmm.

A struct Object has no name.  Only properties have a name.

An ObjectNode has a name, and an ObjectPropertyValue has a name.

I may get back to this in a later message.

> +#
> +# Since 10.1
> +##
> +{ 'command': 'qom-tree-get',
> +  'data': { 'path': 'str' },
> +  'returns': 'ObjectNode',
> +  'allow-preconfig': true }
> +
> +##
>  # @qom-set:
>  #
>  # This command will set a property from a object model path.
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 293755f..b876681 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -69,6 +69,78 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>      return props;
>  }
>  
> +static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
> +                                        ObjectPropertyValueList **props)
> +{
> +    ObjectPropertyValue *item = g_new0(ObjectPropertyValue, 1);
> +    Error *err = NULL;
> +
> +    QAPI_LIST_PREPEND(*props, item);

List elements are in reverse iteration order.  Not wrong.  I would've
reached for QAPI_LIST_APPEND(), though.

Wait!  Existing command code uses QAPI_LIST_PREPEND().  Nevermind, carry
on!

> +
> +    item->name = g_strdup(prop->name);
> +    item->type = g_strdup(prop->type);
> +    item->value = object_property_get_qobject(obj, prop->name, &err);
> +
> +    if (!item->value) {
> +        /*
> +         * For bulk get, the error message is dropped, but the value field
> +         * is omitted so the caller knows this property could not be read.
> +         */
> +        error_free(err);

Simpler: pass NULL to object_property_get_qobject().

> +    }
> +}
> +
> +static ObjectNode *qom_tree_get(const char *path, Error **errp)
> +{
> +    Object *obj;
> +    ObjectProperty *prop;
> +    ObjectNode *result, *child;
> +    ObjectPropertyIterator iter;
> +
> +    obj = qom_resolve_path(path, errp);
> +    if (obj == NULL) {
> +        return NULL;
> +    }
> +
> +    result = g_new0(ObjectNode, 1);
> +
> +    object_property_iter_init(&iter, obj);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (strstart(prop->type, "child<", NULL)) {
> +            g_autofree char *child_path = g_strdup_printf("%s/%s",
> +                                                          path, prop->name);
> +            child = qom_tree_get(child_path, errp);
> +            if (!child) {
> +                qapi_free_ObjectNode(result);
> +                return NULL;
> +            }
> +            child->name = g_strdup(prop->name);

WAT?

> +            QAPI_LIST_PREPEND(result->children, child);
> +        } else {
> +            qom_list_add_property_value(obj, prop, &result->properties);
> +        }
> +    }
> +

Oh, result->name remains unset, and the caller is expected to fill it
in.  Two callers, "WAT" above, and ...

> +    return result;
> +}
> +
> +ObjectNode *qmp_qom_tree_get(const char *path, Error **errp)
> +{
> +    ObjectNode *result = qom_tree_get(path, errp);
> +
> +    if (result) {
> +        /* Strip the path prefix if any */
> +        const char *basename = strrchr(path, '/');
> +
> +        if (!basename || !basename[1]) {
> +            result->name = g_strdup(path);
> +        } else {
> +            result->name = g_strdup(basename + 1);
> +        }
> +    }

... this one.

Not a fan.  But it works.

> +    return result;
> +}
> +
>  void qmp_qom_set(const char *path, const char *property, QObject *value,
>                   Error **errp)
>  {

Re: [PATCH V2 4/5] qom: qom-list-getv

[Markus Armbruster]

Steve Sistare <steven.sistare@oracle.com> writes:

> Define the qom-list-getv command, which fetches all the properties and
> values for a list of paths.  This is faster than qom-tree-get when
> fetching a subset of the QOM tree.  See qom.json for details.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  qapi/qom.json      | 34 ++++++++++++++++++++++++++++++++++
>  qom/qom-qmp-cmds.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 94662ad..dc710d6 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -62,6 +62,16 @@
>              '*value': 'any' } }
>  
>  ##
> +# @ObjectPropertiesValues:
> +#
> +# @properties: a list of properties.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectPropertiesValues',
> +  'data': { 'properties': [ 'ObjectPropertyValue' ] }}
> +
> +##
>  # @ObjectNode:
>  #
>  # @name: the name of the node
> @@ -158,6 +168,30 @@
>    'allow-preconfig': true }
>  
>  ##
> +# @qom-list-getv:
> +#
> +# This command returns a list of properties and their values for
> +# each object path in the input list.

Imperative mood, please: "Return a list of ..."

> +#
> +# @paths: The absolute or partial path for each object, as described
> +#     in @qom-get
> +#
> +# Errors:
> +#     - If any path is not valid or is ambiguous, returns an error.
> +#     - If a property cannot be read, the value field is omitted in
> +#       the corresponding @ObjectPropertyValue.

My comment on qom-tree-get's Errors: section applies.

> +#
> +# Returns: A list of @ObjectPropertiesValues.  Each element contains
> +#     the properties of the corresponding element in @paths.

Again, ObjectPropertiesValues is an unfortunate name.

> +#
> +# Since 10.1
> +##
> +{ 'command': 'qom-list-getv',
> +  'data': { 'paths': [ 'str' ] },
> +  'returns': [ 'ObjectPropertiesValues' ],
> +  'allow-preconfig': true }
> +
> +##
>  # @qom-tree-get:
>  #
>  # This command returns a tree of objects and their properties,

I find this command *much* simpler than qom-tree-get.

qom-list-getv treats all properties the same.  References, whether they
are children and links, are the same: a QOM path.

qom-tree-get separates properties into children and non-children.
Children become nested ObjectNodes, links remain QOM paths.

> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index b876681..1f05956 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -90,6 +90,46 @@ static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
>      }
>  }
>  
> +static ObjectPropertyValueList *qom_get_property_value_list(const char *path,
> +                                                            Error **errp)
> +{
> +    Object *obj;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +    ObjectPropertyValueList *props = NULL;
> +
> +    obj = qom_resolve_path(path, errp);
> +    if (obj == NULL) {
> +        return NULL;
> +    }
> +
> +    object_property_iter_init(&iter, obj);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        qom_list_add_property_value(obj, prop, &props);
> +    }
> +
> +    return props;
> +}
> +
> +ObjectPropertiesValuesList *qmp_qom_list_getv(strList *paths, Error **errp)
> +{
> +    ObjectPropertiesValuesList *head = NULL, **tail = &head;
> +
> +    for ( ; paths ; paths = paths->next) {

I'd prefer a separate variable:

       for (tail = paths; tail; tail = tail->next) {

> +        ObjectPropertiesValues *item = g_new0(ObjectPropertiesValues, 1);
> +
> +        QAPI_LIST_APPEND(tail, item);
> +
> +        item->properties = qom_get_property_value_list(paths->value, errp);
> +        if (!item->properties) {
> +            qapi_free_ObjectPropertiesValuesList(head);
> +            return NULL;
> +        }
> +    }
> +
> +    return head;
> +}
> +
>  static ObjectNode *qom_tree_get(const char *path, Error **errp)
>  {
>      Object *obj;

The implementation is simpler than qom-tree's, too.

Re: [PATCH V2 0/5] fast qom tree get

[Markus Armbruster]

Steve Sistare <steven.sistare@oracle.com> writes:

> Using qom-list and qom-get to get all the nodes and property values in a
> QOM tree can take multiple seconds because it requires 1000's of individual
> QOM requests.  Some managers fetch the entire tree or a large subset
> of it when starting a new VM, and this cost is a substantial fraction of
> start up time.
>
> To reduce this cost, consider QAPI calls that fetch more information in
> each call:
>   * qom-list-get: given a path, return a list of properties and values.
>   * qom-list-getv: given a list of paths, return a list of properties and
>     values for each path.
>   * qom-tree-get: given a path, return all descendant nodes rooted at that
>     path, with properties and values for each.
>
> In all cases, a returned property is represented by ObjectPropertyValue,
> with fields name, type, and value.  If an error occurs when reading a value
> the value field is omitted.  Thus an error for one property will not cause a
> bulk fetch operation to fail.
>
> To evaluate each method, I modified scripts/qmp/qom-tree to use the method,
> verified all methods produce the same output, and timed each using:
>
>   qemu-system-x86_64 -display none \
>     -chardev socket,id=monitor0,path=/tmp/vm1.sock,server=on,wait=off \
>     -mon monitor0,mode=control &
>
>   time qom-tree -s /tmp/vm1.sock > /dev/null
>
> I only measured once per method, but the variation is low after a warm up run.
> The 'real - user - sys' column is a proxy for QEMU CPU time.
>
> method               real(s)   user(s)   sys(s)  (real - user - sys)(s)
> qom-list / qom-get   2.048     0.932     0.057   1.059
> qom-list-get         0.402     0.230     0.029   0.143
> qom-list-getv        0.200     0.132     0.015   0.053
> qom-tree-get         0.143     0.123     0.012   0.008
>
> qom-tree-get is the clear winner, reducing elapsed time by a factor of 14X,
> and reducing QEMU CPU time by 132X.
>
> qom-list-getv is slower when fetching the entire tree, but can beat
> qom-tree-get when only a subset of the tree needs to be fetched (not shown).
> qom-list-get is shown for comparison only, and is not included in this series.

How badly do you need the additional performance qom-tree-get can give
you in certain cases?

I'm asking because I find qom-list-getv *much* simpler.

Re: [PATCH V2 0/5] fast qom tree get

[Markus Armbruster]

Steve,

My sincere apologies for the long, long delay.

It wasn't just for the usual reasons.  It was also because I had a vague
feeling of unease about qom-tree, and had trouble figuring out why.

I'll try do your work justice before the window for 10.1 closes.

QOM maintainers, please have a look, too.

Re: [PATCH V2 0/5] fast qom tree get

[Steven Sistare]

On 7/4/2025 8:26 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> Using qom-list and qom-get to get all the nodes and property values in a
>> QOM tree can take multiple seconds because it requires 1000's of individual
>> QOM requests.  Some managers fetch the entire tree or a large subset
>> of it when starting a new VM, and this cost is a substantial fraction of
>> start up time.
>>
>> To reduce this cost, consider QAPI calls that fetch more information in
>> each call:
>>    * qom-list-get: given a path, return a list of properties and values.
>>    * qom-list-getv: given a list of paths, return a list of properties and
>>      values for each path.
>>    * qom-tree-get: given a path, return all descendant nodes rooted at that
>>      path, with properties and values for each.
>>
>> In all cases, a returned property is represented by ObjectPropertyValue,
>> with fields name, type, and value.  If an error occurs when reading a value
>> the value field is omitted.  Thus an error for one property will not cause a
>> bulk fetch operation to fail.
>>
>> To evaluate each method, I modified scripts/qmp/qom-tree to use the method,
>> verified all methods produce the same output, and timed each using:
>>
>>    qemu-system-x86_64 -display none \
>>      -chardev socket,id=monitor0,path=/tmp/vm1.sock,server=on,wait=off \
>>      -mon monitor0,mode=control &
>>
>>    time qom-tree -s /tmp/vm1.sock > /dev/null
>>
>> I only measured once per method, but the variation is low after a warm up run.
>> The 'real - user - sys' column is a proxy for QEMU CPU time.
>>
>> method               real(s)   user(s)   sys(s)  (real - user - sys)(s)
>> qom-list / qom-get   2.048     0.932     0.057   1.059
>> qom-list-get         0.402     0.230     0.029   0.143
>> qom-list-getv        0.200     0.132     0.015   0.053
>> qom-tree-get         0.143     0.123     0.012   0.008
>>
>> qom-tree-get is the clear winner, reducing elapsed time by a factor of 14X,
>> and reducing QEMU CPU time by 132X.
>>
>> qom-list-getv is slower when fetching the entire tree, but can beat
>> qom-tree-get when only a subset of the tree needs to be fetched (not shown).
>> qom-list-get is shown for comparison only, and is not included in this series.
> 
> How badly do you need the additional performance qom-tree-get can give
> you in certain cases?
> 
> I'm asking because I find qom-list-getv *much* simpler.

I would be content with qom-list-getv, so I will drop qom-tree-get.
qom-list-getv needs ObjectPropertyValue and qom_list_add_property_value
from the qom-tree-get patch, so I will respond to those comments.

- Steve

Re: [PATCH V2 0/5] fast qom tree get

[Steven Sistare]

On 7/4/2025 8:33 AM, Markus Armbruster wrote:
> Steve,
> 
> My sincere apologies for the long, long delay.
> 
> It wasn't just for the usual reasons.  It was also because I had a vague
> feeling of unease about qom-tree, and had trouble figuring out why.
> 
> I'll try do your work justice before the window for 10.1 closes.
> 
> QOM maintainers, please have a look, too.

Thanks, much appreciated - steve

Re: [PATCH V2 4/5] qom: qom-list-getv

[Steven Sistare]

On 7/4/2025 8:22 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> Define the qom-list-getv command, which fetches all the properties and
>> values for a list of paths.  This is faster than qom-tree-get when
>> fetching a subset of the QOM tree.  See qom.json for details.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   qapi/qom.json      | 34 ++++++++++++++++++++++++++++++++++
>>   qom/qom-qmp-cmds.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 74 insertions(+)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 94662ad..dc710d6 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -62,6 +62,16 @@
>>               '*value': 'any' } }
>>   
>>   ##
>> +# @ObjectPropertiesValues:
>> +#
>> +# @properties: a list of properties.
>> +#
>> +# Since 10.1
>> +##
>> +{ 'struct': 'ObjectPropertiesValues',
>> +  'data': { 'properties': [ 'ObjectPropertyValue' ] }}
>> +
>> +##
>>   # @ObjectNode:
>>   #
>>   # @name: the name of the node
>> @@ -158,6 +168,30 @@
>>     'allow-preconfig': true }
>>   
>>   ##
>> +# @qom-list-getv:
>> +#
>> +# This command returns a list of properties and their values for
>> +# each object path in the input list.
> 
> Imperative mood, please: "Return a list of ..."

OK.  (I followed the style of qom-get and qom-list).

>> +#
>> +# @paths: The absolute or partial path for each object, as described
>> +#     in @qom-get
>> +#
>> +# Errors:
>> +#     - If any path is not valid or is ambiguous, returns an error.
>> +#     - If a property cannot be read, the value field is omitted in
>> +#       the corresponding @ObjectPropertyValue.
> 
> My comment on qom-tree-get's Errors: section applies.

Will do.

>> +#
>> +# Returns: A list of @ObjectPropertiesValues.  Each element contains
>> +#     the properties of the corresponding element in @paths.
> 
> Again, ObjectPropertiesValues is an unfortunate name.

See other thread.

>> +#
>> +# Since 10.1
>> +##
>> +{ 'command': 'qom-list-getv',
>> +  'data': { 'paths': [ 'str' ] },
>> +  'returns': [ 'ObjectPropertiesValues' ],
>> +  'allow-preconfig': true }
>> +
>> +##
>>   # @qom-tree-get:
>>   #
>>   # This command returns a tree of objects and their properties,
> 
> I find this command *much* simpler than qom-tree-get.
> 
> qom-list-getv treats all properties the same.  References, whether they
> are children and links, are the same: a QOM path.
> 
> qom-tree-get separates properties into children and non-children.
> Children become nested ObjectNodes, links remain QOM paths.
> 
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index b876681..1f05956 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -90,6 +90,46 @@ static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
>>       }
>>   }
>>   
>> +static ObjectPropertyValueList *qom_get_property_value_list(const char *path,
>> +                                                            Error **errp)
>> +{
>> +    Object *obj;
>> +    ObjectProperty *prop;
>> +    ObjectPropertyIterator iter;
>> +    ObjectPropertyValueList *props = NULL;
>> +
>> +    obj = qom_resolve_path(path, errp);
>> +    if (obj == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    object_property_iter_init(&iter, obj);
>> +    while ((prop = object_property_iter_next(&iter))) {
>> +        qom_list_add_property_value(obj, prop, &props);
>> +    }
>> +
>> +    return props;
>> +}
>> +
>> +ObjectPropertiesValuesList *qmp_qom_list_getv(strList *paths, Error **errp)
>> +{
>> +    ObjectPropertiesValuesList *head = NULL, **tail = &head;
>> +
>> +    for ( ; paths ; paths = paths->next) {
> 
> I'd prefer a separate variable:
> 
>         for (tail = paths; tail; tail = tail->next) {

OK.

- Steve

>> +        ObjectPropertiesValues *item = g_new0(ObjectPropertiesValues, 1);
>> +
>> +        QAPI_LIST_APPEND(tail, item);
>> +
>> +        item->properties = qom_get_property_value_list(paths->value, errp);
>> +        if (!item->properties) {
>> +            qapi_free_ObjectPropertiesValuesList(head);
>> +            return NULL;
>> +        }
>> +    }
>> +
>> +    return head;
>> +}
>> +
>>   static ObjectNode *qom_tree_get(const char *path, Error **errp)
>>   {
>>       Object *obj;
> 
> The implementation is simpler than qom-tree's, too.
> 

Re: [PATCH V2 1/5] qom: qom-tree-get

[Steven Sistare]

On 7/4/2025 8:22 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> Define the qom-tree-get QAPI command, which fetches an entire tree of
>> properties and values with a single QAPI call.  This is much faster
>> than using qom-list plus qom-get for every node and property of the
>> tree.  See qom.json for details.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>>   qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 128 insertions(+)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 28ce24c..94662ad 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -46,6 +46,38 @@
>>               '*default-value': 'any' } }
>>   
>>   ##
>> +# @ObjectPropertyValue:
>> +#
>> +# @name: the name of the property
>> +#
>> +# @type: the type of the property, as described in @ObjectPropertyInfo
> 
> That description is crap.  In part because what it tries to describe is
> crap.  Neither is this patch's problem.
> 
>> +#
>> +# @value: the value of the property.  Omitted if cannot be read.
> 
> Suggest "Absent when the property cannot be read."

OK.

>> +#
>> +# Since 10.1
>> +##
>> +{ 'struct': 'ObjectPropertyValue',
>> +  'data': { 'name': 'str',
>> +            'type': 'str',
>> +            '*value': 'any' } }
> 
> ObjectPropertyValue suggests this describes a property's value.

I would agree with you if the name included "info" or "desc", but it
does not.  To me, "ObjectPropertyValue" says this is an object's
property and value.  But it's subjective.

Perhaps: ObjectPropertyWithValue

>  It does
> not.  It includes the name, i.e. it describes the *property*.
> 
> So does ObjectPropertyInfo.
> 
> The two overlap: both habe name and type.  Only ObjectPropertyValue has
> the current value.  Only ObjectPropertyInfo has the default value and
> description (I suspect the latter is useless in practice).
> 
> ObjectPropertyInfo is used with qom-list and qom-list-properties.
> 
> qom-list takes a QOM path, like your qom-tree-get and qom-list-getv.
> I'd expect your commands to supersede qom-list in practice.
> 
> qom-list-properties is unlike your qom-tree-get and qom-list-getv: it
> takes a type name.  It's unreliable for non-abstract types: it can miss
> dynamically created properties.
> 
> Let's ignore all this for now.
> 
>> +
>> +##
>> +# @ObjectNode:
>> +#
>> +# @name: the name of the node
>> +#
>> +# @children: child nodes
>> +#
>> +# @properties: properties of the node
>> +#
>> +# Since 10.1
>> +##
>> +{ 'struct': 'ObjectNode',
>> +  'data': { 'name': 'str',
>> +            'children': [ 'ObjectNode' ],
>> +            'properties': [ 'ObjectPropertyValue' ] }}
>> +
>> +##
>>   # @qom-list:
>>   #
>>   # This command will list any properties of a object given a path in
>> @@ -126,6 +158,30 @@
>>     'allow-preconfig': true }
>>   
>>   ##
>> +# @qom-tree-get:
>> +#
>> +# This command returns a tree of objects and their properties,
>> +# rooted at the specified path.
>> +#
>> +# @path: The absolute or partial path within the object model, as
>> +#     described in @qom-get
>> +#
>> +# Errors:
>> +#     - If path is not valid or is ambiguous, returns an error.
> 
> By convention, we use "If <condition>, <error>, where <error> is a
> member of QapiErrorClass.

OK.  I was following the minimal Errors examples from this same file.

> What are the possible error classes?  As far as I can tell:
> 
>           - If path is ambiguous, GenericError
>           - If path cannot be resolved, DeviceNotFound
> 
> However, use of error classes other than GenericError is strongly
> discouraged (see error_set() in qapi/error.h).
> 
> Is the ability to distinguish between these two errors useful?
> 
> Existing related commands such as qom-get also use DeviceNotFound.
> Entirely undocumented, exact error conditions unclear.  Awesome.
> 
> Libvirt seems to rely on this undocumented behavior: I can see code
> checking for DeviceNotFound.  Hyrum's law strikes.
> 
> qom-get fails with DeviceNotFound in both of the above cases.  It fails
> with GenericError when @property doesn't exist or cannot be read.  Your
> qom-tree-get fails differently.  Awesome again.
> 
> Choices:
> 
> 1. Leave errors undocumented and inconsistent.
> 
> 2. Document errors for all related commands.  Make the new ones as
> consistent as we can.

Ignoring qom-tree-get since we are dropping it.

Do you prefer that qom-list-getv be consistent with qom-list (GenericError
and DeviceNotFound, as created by the common subroutine qom_resolve_path),
or only return GenericError with a customized message per best practices?

(Regardless, it will still succeed when @property cannot be read).

>> +#     - If a property cannot be read, the value field is omitted in
>> +#       the corresponding @ObjectPropertyValue.
> 
> This is not an error, and therefore doesn't belong here.
> ObjectPropertyValue's documentation also mentions it.  Good enough?

OK.

>> +#
>> +# Returns: A tree of @ObjectNode.  Each node contains its name, list
>> +#     of properties, and list of child nodes.
> 
> Hmm.
> 
> A struct Object has no name.  Only properties have a name.
> 
> An ObjectNode has a name, and an ObjectPropertyValue has a name.
> 
> I may get back to this in a later message.
> 
>> +#
>> +# Since 10.1
>> +##
>> +{ 'command': 'qom-tree-get',
>> +  'data': { 'path': 'str' },
>> +  'returns': 'ObjectNode',
>> +  'allow-preconfig': true }
>> +
>> +##
>>   # @qom-set:
>>   #
>>   # This command will set a property from a object model path.
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index 293755f..b876681 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -69,6 +69,78 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>>       return props;
>>   }
>>   
>> +static void qom_list_add_property_value(Object *obj, ObjectProperty *prop,
>> +                                        ObjectPropertyValueList **props)
>> +{
>> +    ObjectPropertyValue *item = g_new0(ObjectPropertyValue, 1);
>> +    Error *err = NULL;
>> +
>> +    QAPI_LIST_PREPEND(*props, item);
> 
> List elements are in reverse iteration order.  Not wrong.  I would've
> reached for QAPI_LIST_APPEND(), though.
> 
> Wait!  Existing command code uses QAPI_LIST_PREPEND().  Nevermind, carry
> on!

Exactly so.

>> +
>> +    item->name = g_strdup(prop->name);
>> +    item->type = g_strdup(prop->type);
>> +    item->value = object_property_get_qobject(obj, prop->name, &err);
>> +
>> +    if (!item->value) {
>> +        /*
>> +         * For bulk get, the error message is dropped, but the value field
>> +         * is omitted so the caller knows this property could not be read.
>> +         */
>> +        error_free(err);
> 
> Simpler: pass NULL to object_property_get_qobject().

Yes, thanks.

- Steve

>> +    }
>> +}
>> +
>> +static ObjectNode *qom_tree_get(const char *path, Error **errp)
>> +{
>> +    Object *obj;
>> +    ObjectProperty *prop;
>> +    ObjectNode *result, *child;
>> +    ObjectPropertyIterator iter;
>> +
>> +    obj = qom_resolve_path(path, errp);
>> +    if (obj == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    result = g_new0(ObjectNode, 1);
>> +
>> +    object_property_iter_init(&iter, obj);
>> +    while ((prop = object_property_iter_next(&iter))) {
>> +        if (strstart(prop->type, "child<", NULL)) {
>> +            g_autofree char *child_path = g_strdup_printf("%s/%s",
>> +                                                          path, prop->name);
>> +            child = qom_tree_get(child_path, errp);
>> +            if (!child) {
>> +                qapi_free_ObjectNode(result);
>> +                return NULL;
>> +            }
>> +            child->name = g_strdup(prop->name);
> 
> WAT?
> 
>> +            QAPI_LIST_PREPEND(result->children, child);
>> +        } else {
>> +            qom_list_add_property_value(obj, prop, &result->properties);
>> +        }
>> +    }
>> +
> 
> Oh, result->name remains unset, and the caller is expected to fill it
> in.  Two callers, "WAT" above, and ...
> 
>> +    return result;
>> +}
>> +
>> +ObjectNode *qmp_qom_tree_get(const char *path, Error **errp)
>> +{
>> +    ObjectNode *result = qom_tree_get(path, errp);
>> +
>> +    if (result) {
>> +        /* Strip the path prefix if any */
>> +        const char *basename = strrchr(path, '/');
>> +
>> +        if (!basename || !basename[1]) {
>> +            result->name = g_strdup(path);
>> +        } else {
>> +            result->name = g_strdup(basename + 1);
>> +        }
>> +    }
> 
> ... this one.
> 
> Not a fan.  But it works.
> 
>> +    return result;
>> +}
>> +
>>   void qmp_qom_set(const char *path, const char *property, QObject *value,
>>                    Error **errp)
>>   {
> 

Re: [PATCH V2 4/5] qom: qom-list-getv

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>> Steve Sistare <steven.sistare@oracle.com> writes:
>> 
>>> Define the qom-list-getv command, which fetches all the properties and
>>> values for a list of paths.  This is faster than qom-tree-get when
>>> fetching a subset of the QOM tree.  See qom.json for details.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>   qapi/qom.json      | 34 ++++++++++++++++++++++++++++++++++
>>>   qom/qom-qmp-cmds.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 74 insertions(+)
>>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 94662ad..dc710d6 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -62,6 +62,16 @@
>>>               '*value': 'any' } }
>>>   
>>>  ##
>>> +# @ObjectPropertiesValues:
>>> +#
>>> +# @properties: a list of properties.
>>> +#
>>> +# Since 10.1
>>> +##
>>> +{ 'struct': 'ObjectPropertiesValues',
>>> +  'data': { 'properties': [ 'ObjectPropertyValue' ] }}
>>> +
>>> +##
>>>  # @ObjectNode:
>>>  #
>>>  # @name: the name of the node
>>> @@ -158,6 +168,30 @@
>>>     'allow-preconfig': true }
>>>   
>>>  ##
>>> +# @qom-list-getv:
>>> +#
>>> +# This command returns a list of properties and their values for
>>> +# each object path in the input list.
>> 
>> Imperative mood, please: "Return a list of ..."
>
> OK.  (I followed the style of qom-get and qom-list).

Yup.  We have a few more elsewhere.  I'll clean them up.

[...]

Re: [PATCH V2 1/5] qom: qom-tree-get

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>> Steve Sistare <steven.sistare@oracle.com> writes:
>> 
>>> Define the qom-tree-get QAPI command, which fetches an entire tree of
>>> properties and values with a single QAPI call.  This is much faster
>>> than using qom-list plus qom-get for every node and property of the
>>> tree.  See qom.json for details.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>   qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>   qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 128 insertions(+)
>>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 28ce24c..94662ad 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -46,6 +46,38 @@
>>>               '*default-value': 'any' } }
>>>   
>>>  ##
>>> +# @ObjectPropertyValue:
>>> +#
>>> +# @name: the name of the property
>>> +#
>>> +# @type: the type of the property, as described in @ObjectPropertyInfo
>> 
>> That description is crap.  In part because what it tries to describe is
>> crap.  Neither is this patch's problem.
>> 
>>> +#
>>> +# @value: the value of the property.  Omitted if cannot be read.
>> 
>> Suggest "Absent when the property cannot be read."
>
> OK.
>
>>> +#
>>> +# Since 10.1
>>> +##
>>> +{ 'struct': 'ObjectPropertyValue',
>>> +  'data': { 'name': 'str',
>>> +            'type': 'str',
>>> +            '*value': 'any' } }
>> 
>> ObjectPropertyValue suggests this describes a property's value.
>
> I would agree with you if the name included "info" or "desc", but it
> does not.  To me, "ObjectPropertyValue" says this is an object's
> property and value.  But it's subjective.

Naming is hard.

> Perhaps: ObjectPropertyWithValue

I'd be tempted by ObjectProperty if it wasn't already taken by
qom/object.h.

Let's converge on the code, and maybe revisit naming at the end.

>>  It does
>> not.  It includes the name, i.e. it describes the *property*.
>> 
>> So does ObjectPropertyInfo.
>> 
>> The two overlap: both habe name and type.  Only ObjectPropertyValue has
>> the current value.  Only ObjectPropertyInfo has the default value and
>> description (I suspect the latter is useless in practice).
>> 
>> ObjectPropertyInfo is used with qom-list and qom-list-properties.
>> 
>> qom-list takes a QOM path, like your qom-tree-get and qom-list-getv.
>> I'd expect your commands to supersede qom-list in practice.
>> 
>> qom-list-properties is unlike your qom-tree-get and qom-list-getv: it
>> takes a type name.  It's unreliable for non-abstract types: it can miss
>> dynamically created properties.
>> 
>> Let's ignore all this for now.
>> 
>>> +
>>> +##
>>> +# @ObjectNode:
>>> +#
>>> +# @name: the name of the node
>>> +#
>>> +# @children: child nodes
>>> +#
>>> +# @properties: properties of the node
>>> +#
>>> +# Since 10.1
>>> +##
>>> +{ 'struct': 'ObjectNode',
>>> +  'data': { 'name': 'str',
>>> +            'children': [ 'ObjectNode' ],
>>> +            'properties': [ 'ObjectPropertyValue' ] }}
>>> +
>>> +##
>>>  # @qom-list:
>>>  #
>>>  # This command will list any properties of a object given a path in
>>> @@ -126,6 +158,30 @@
>>>     'allow-preconfig': true }
>>>   
>>>  ##
>>> +# @qom-tree-get:
>>> +#
>>> +# This command returns a tree of objects and their properties,
>>> +# rooted at the specified path.
>>> +#
>>> +# @path: The absolute or partial path within the object model, as
>>> +#     described in @qom-get
>>> +#
>>> +# Errors:
>>> +#     - If path is not valid or is ambiguous, returns an error.
>> 
>> By convention, we use "If <condition>, <error>, where <error> is a
>> member of QapiErrorClass.
>
> OK.  I was following the minimal Errors examples from this same file.

Yup.  I'll clean them up.

>> What are the possible error classes?  As far as I can tell:
>> 
>>           - If path is ambiguous, GenericError
>>           - If path cannot be resolved, DeviceNotFound
>> 
>> However, use of error classes other than GenericError is strongly
>> discouraged (see error_set() in qapi/error.h).
>> 
>> Is the ability to distinguish between these two errors useful?
>> 
>> Existing related commands such as qom-get also use DeviceNotFound.
>> Entirely undocumented, exact error conditions unclear.  Awesome.
>> 
>> Libvirt seems to rely on this undocumented behavior: I can see code
>> checking for DeviceNotFound.  Hyrum's law strikes.
>> 
>> qom-get fails with DeviceNotFound in both of the above cases.  It fails
>> with GenericError when @property doesn't exist or cannot be read.  Your
>> qom-tree-get fails differently.  Awesome again.
>> 
>> Choices:
>> 
>> 1. Leave errors undocumented and inconsistent.
>> 
>> 2. Document errors for all related commands.  Make the new ones as
>> consistent as we can.
>
> Ignoring qom-tree-get since we are dropping it.
>
> Do you prefer that qom-list-getv be consistent with qom-list (GenericError
> and DeviceNotFound, as created by the common subroutine qom_resolve_path),
> or only return GenericError with a customized message per best practices?

I like to stick to GenericError, I like consistency, I can't have both.
Go with simpler code?

> (Regardless, it will still succeed when @property cannot be read).

Yes, that's a documented feature.

>>> +#     - If a property cannot be read, the value field is omitted in
>>> +#       the corresponding @ObjectPropertyValue.
>> 
>> This is not an error, and therefore doesn't belong here.
>> ObjectPropertyValue's documentation also mentions it.  Good enough?
>
> OK.
>
>>> +#
>>> +# Returns: A tree of @ObjectNode.  Each node contains its name, list
>>> +#     of properties, and list of child nodes.
>> 
>> Hmm.
>> 
>> A struct Object has no name.  Only properties have a name.
>> 
>> An ObjectNode has a name, and an ObjectPropertyValue has a name.
>> 
>> I may get back to this in a later message.

I propose you respin without qom-tree first.  The patches will get
simpler, and review hopefully more focused.

[...]

Re: [PATCH V2 1/5] qom: qom-tree-get

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Steven Sistare <steven.sistare@oracle.com> writes:
>
>> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>> 
>>>> Define the qom-tree-get QAPI command, which fetches an entire tree of
>>>> properties and values with a single QAPI call.  This is much faster
>>>> than using qom-list plus qom-get for every node and property of the
>>>> tree.  See qom.json for details.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>   qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>   qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 128 insertions(+)
>>>>
>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>> index 28ce24c..94662ad 100644
>>>> --- a/qapi/qom.json
>>>> +++ b/qapi/qom.json

[...]

>>>> ##
>>>> +# @qom-tree-get:
>>>> +#
>>>> +# This command returns a tree of objects and their properties,
>>>> +# rooted at the specified path.
>>>> +#
>>>> +# @path: The absolute or partial path within the object model, as
>>>> +#     described in @qom-get
>>>> +#
>>>> +# Errors:
>>>> +#     - If path is not valid or is ambiguous, returns an error.
>>> 
>>> By convention, we use "If <condition>, <error>, where <error> is a
>>> member of QapiErrorClass.
>>
>> OK.  I was following the minimal Errors examples from this same file.
>
> Yup.  I'll clean them up.

I changed my mind.

Omitting ", <error>" is fairly common, actually.  I don't feel like
chasing down the actual error classes.  Moreover, documenting error
classes we don't want people to use seems counterproductive.

Feel free to just delete ", returns an error." and call it a day.

[...]

Re: [PATCH V2 1/5] qom: qom-tree-get

[Philippe Mathieu-Daudé]

On 12/5/25 15:47, Steve Sistare wrote:
> Define the qom-tree-get QAPI command, which fetches an entire tree of
> properties and values with a single QAPI call.  This is much faster
> than using qom-list plus qom-get for every node and property of the
> tree.  See qom.json for details.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>   qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 128 insertions(+)

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

Re: [PATCH V2 3/5] tests/qtest/qom-test: unit test for qom-tree-get

[Philippe Mathieu-Daudé]

On 12/5/25 15:47, Steve Sistare wrote:
> Add a unit test for qom-tree-get
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   tests/qtest/qom-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)

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

Re: [PATCH V2 1/5] qom: qom-tree-get

[Steven Sistare]

On 7/8/2025 3:14 AM, Philippe Mathieu-Daudé wrote:
> On 12/5/25 15:47, Steve Sistare wrote:
>> Define the qom-tree-get QAPI command, which fetches an entire tree of
>> properties and values with a single QAPI call.  This is much faster
>> than using qom-list plus qom-get for every node and property of the
>> tree.  See qom.json for details.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>>   qom/qom-qmp-cmds.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 128 insertions(+)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Hi Philippe, thank you for reviewing this.

Markus has requested that I drop qom-tree-get, and only provide qom-list-getv,
to simplify things.  Do you prefer that we keep qom-tree-get?

- Steve

Re: [PATCH V2 1/5] qom: qom-tree-get

[Philippe Mathieu-Daudé]

On 8/7/25 13:50, Steven Sistare wrote:
> On 7/8/2025 3:14 AM, Philippe Mathieu-Daudé wrote:
>> On 12/5/25 15:47, Steve Sistare wrote:
>>> Define the qom-tree-get QAPI command, which fetches an entire tree of
>>> properties and values with a single QAPI call.  This is much faster
>>> than using qom-list plus qom-get for every node and property of the
>>> tree.  See qom.json for details.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>   qapi/qom.json      | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>   qom/qom-qmp-cmds.c | 72 +++++++++++++++++++++++++++++++++++++++++++ 
>>> +++++++++++
>>>   2 files changed, 128 insertions(+)
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Hi Philippe, thank you for reviewing this.
> 
> Markus has requested that I drop qom-tree-get, and only provide qom- 
> list-getv,
> to simplify things.  Do you prefer that we keep qom-tree-get?

Doh I missed that, I only noticed his "QOM maintainers please review"
at the end.

If you already addressed Markus comments, please respin to get a chance
to get it merged for 10.1, otherwise I'll try to look at getv in the
next days.

Regards,

Phil.