3:22 p.m.
Steve Sistare <steven.sistare(a)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(a)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)
{
5:44 p.m.
New subject: [PATCH V2 1/5] qom: qom-tree-get
On 7/4/2025 8:22 AM, Markus Armbruster wrote:
Steve Sistare <steven.sistare(a)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(a)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)
> {
8:06 a.m.
New subject: [PATCH V2 1/5] qom: qom-tree-get
Steven Sistare <steven.sistare(a)oracle.com> writes:
On 7/4/2025 8:22 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare(a)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(a)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.
[...]
9:53 a.m.
New subject: [PATCH V2 1/5] qom: qom-tree-get
Markus Armbruster <armbru(a)redhat.com> writes:
Steven Sistare <steven.sistare(a)oracle.com> writes:
> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>> Steve Sistare <steven.sistare(a)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(a)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.
[...]
9
days inactive
13
days old
3 comments
2 participants
participants (2)
-
Markus Armbruster -
Steven Sistare