Steve Sistare <steven.sist...@oracle.com> writes:

> Define the qom-list-getv command, which fetches all the properties and
> values for a list of paths.  This is faster than qom-tree-get when
> fetching a subset of the QOM tree.  See qom.json for details.
>
> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>

You cover letter explains *why* we want this.  Please include the
relevant parts here, so the rationale gets captured in git.

> ---
>  qapi/qom.json      | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qom/qom-qmp-cmds.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index b133b06..c16c2dd 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -46,6 +46,34 @@
>              '*default-value': 'any' } }
>  
>  ##
> +# @ObjectPropertyValue:
> +#
> +# @name: the name of the property
> +#
> +# @type: the type of the property, as described in @ObjectPropertyInfo
> +#
> +# @value: the value of the property.  Absent when the property cannot
> +#     be read.

Best to consistently end the descriptions with a period.

> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectPropertyValue',
> +  'data': { 'name': 'str',
> +            'type': 'str',
> +            '*value': 'any' } }
> +
> +##
> +# @ObjectPropertiesValues:
> +#
> +# @properties: a list of properties.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'ObjectPropertiesValues',
> +  'data': { 'properties': [ 'ObjectPropertyValue' ] }}
> +
> +
> +##
>  # @qom-list:
>  #
>  # This command will list any properties of a object given a path in
> @@ -126,6 +154,28 @@
>    'allow-preconfig': true }
>  
>  ##
> +# @qom-list-getv:
> +#
> +# List properties and their values for each object path in the input
> +# list.
> +#
> +# @paths: The absolute or partial path for each object, as described
> +#     in @qom-get

John Snow's "[PATCH 00/18] QAPI: add cross-references to qapi docs"
rewrites things so they become links in generated HTML.  @qom-get
beccomes `qom-get`.  Please use `qom-get` to avoid the semantic
conflict.

> +#
> +# Errors:
> +#     - If any path is not valid or is ambiguous
> +#
> +# Returns: A list of @ObjectPropertiesValues.  Each element contains
> +#     the properties of the corresponding element in @paths.

I understand you patterned this after qom-get.  It comes out like

   Return:
      "[""ObjectPropertiesValues""]" -- A list of
      "ObjectPropertiesValues".  Each element contains the properties
      of the corresponding element in "paths".

in the generated manual.  'A list of "ObjectPropertiesValues"' is
redundant.  John Snow's "[PATCH v5 4/4] qapi: rephrase return docs to
avoid type name" cleans up existing instances, including qom-get.

Perhaps something like "A list where each element contains information
on the properties of the object referenced by the corresponding element
in @paths."  Or shorter: "A list where each element is the result for
the corresponding element of @paths".

> +#
> +# Since 10.1
> +##
> +{ 'command': 'qom-list-getv',
> +  'data': { 'paths': [ 'str' ] },
> +  'returns': [ 'ObjectPropertiesValues' ],
> +  'allow-preconfig': true }
> +
> +##
>  # @qom-set:
>  #
>  # This command will set a property from a object model path.

The schema looks good.  Not entirely happy with the names.  Naming is
hard!  Type names are not part of the interface, so let's not worry
about them too much.  The command name will be set in stone, though.

When you named the command qom-list-getv, you also had qom-list-get.
qom-list-get works on a single path, and -getv on multiple paths.  The
"v" suffix feels like a natural choice among people used to C.  But does
it make sense without its buddy?

How do you feel about calling it qom-list-get?  qom-get-many?  Other
ideas?

> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c

[...]

The C code looks good to me.


Reply via email to