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

[Markus Armbruster <armbru@redhat.com>]

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.

[...]