Steven Sistare <steven.sist...@oracle.com> writes:
> On 7/4/2025 8:22 AM, Markus Armbruster wrote:
>> Steve Sistare <steven.sist...@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.sist...@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.
[...]
