Re: [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&gt; 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&gt; >> --- >> 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.