Re: [PATCH V4 1/3] qom: qom-list-get

[Markus Armbruster <armbru@redhat.com>]

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

> Define the qom-list-get command, which fetches all the properties and
> values for a list of paths.  This is faster than qom-list plus qom-get,
> especially when fetching a large subset of the QOM tree.  Some managers
> do so when starting a new VM, and this cost can be a substantial fraction
> of start-up time.

You give such nice rationale in your cover letter...  What about this:

  Using qom-list and qom-get to get all the nodes and property values in
  a QOM tree can take multiple seconds because it requires 1000's of
  individual QOM requests.  Some managers fetch the entire tree or a
  large subset of it when starting a new VM, and this cost is a
  substantial fraction of start up time.

  Define the qom-list-get command, which fetches all the properties and
  values for a list of paths.  This can be much faster than qom-list
  plus qom-get.  When getting an entire QOM tree, I measured a 10x
  speedup in elapsed time.

> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I think you missed

  Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> ---
>  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..49214d2 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.

`ObjectPropertyInfo`

See John Snow's "[PATCH 00/18] QAPI: add cross-references to qapi docs"
rewrites things so they become links in generated HTML.

> +#
> +# @value: the value of the property.  Absent when the property cannot
> +#     be read.
> +#
> +# 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-get:
> +#
> +# 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`.
> +#
> +# Errors:
> +#     - If any path is not valid or is ambiguous
> +#
> +# Returns: A list where each element is the result for the
> +#     corresponding element of @paths.
> +#
> +# Since 10.1
> +##
> +{ 'command': 'qom-list-get',
> +  'data': { 'paths': [ 'str' ] },
> +  'returns': [ 'ObjectPropertiesValues' ],
> +  '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..57f1898 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -69,6 +69,59 @@ 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);
> +
> +    QAPI_LIST_PREPEND(*props, item);
> +
> +    item->name = g_strdup(prop->name);
> +    item->type = g_strdup(prop->type);
> +    item->value = object_property_get_qobject(obj, prop->name, NULL);
> +}
> +
> +static ObjectPropertyValueList *qom_get_property_value_list(const char *path,
> +                                                            Error **errp)
> +{
> +    Object *obj;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +    ObjectPropertyValueList *props = NULL;
> +
> +    obj = qom_resolve_path(path, errp);
> +    if (obj == NULL) {
> +        return NULL;
> +    }
> +
> +    object_property_iter_init(&iter, obj);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        qom_list_add_property_value(obj, prop, &props);
> +    }
> +
> +    return props;
> +}
> +
> +ObjectPropertiesValuesList *qmp_qom_list_get(strList *paths, Error **errp)
> +{
> +    ObjectPropertiesValuesList *head = NULL, **tail = &head;
> +    strList *path;
> +
> +    for (path = paths; path; path = path->next) {
> +        ObjectPropertiesValues *item = g_new0(ObjectPropertiesValues, 1);
> +
> +        QAPI_LIST_APPEND(tail, item);
> +
> +        item->properties = qom_get_property_value_list(path->value, errp);
> +        if (!item->properties) {
> +            qapi_free_ObjectPropertiesValuesList(head);
> +            return NULL;
> +        }
> +    }
> +
> +    return head;
> +}
> +
>  void qmp_qom_set(const char *path, const char *property, QObject *value,
>                   Error **errp)
>  {

Reviewed-by: Markus Armbruster <armbru@redhat.com>