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

[Steven Sistare <steven.sistare@oracle.com>]

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