Re: [PATCH v2 11/32] qom: report & filter on security status in qom-list-types

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This adds:
>
>  * a new boolean 'secure' field to the type info returned by
>    qom-list-types, which will be set if the type provides a
>    security boundary
>
>  * a new boolean 'secure' parameter to the arguments of
>    qom-list-types, which can be used to filter types based
>    on their security status
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

I was about to ask for this feature in reply to PATCH 2 when I found
this patch.  Consider moving it right after PATCH 2, or
forward-referencing it in PATCH 2's commit message.

> ---
>  qapi/qom.json      | 10 ++++++++--
>  qom/qom-qmp-cmds.c | 30 ++++++++++++++++++++++++------
>  2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 830cb2ffe7..3e5e7e6f6f 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -210,12 +210,15 @@
>  # @abstract: the type is abstract and can't be directly instantiated.
>  #     Omitted if false.  (since 2.10)
>  #
> +# @secure: the type provides a security boundary.
> +#     Omitted if false.  (since 10.2)

Please wrap like this:

   # @secure: the type provides a security boundary.  Omitted if false.
   #     (since 10.2)

> +#
>  # @parent: Name of parent type, if any (since 2.10)
>  #
>  # Since: 1.1
>  ##
>  { 'struct': 'ObjectTypeInfo',
> -  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
> +  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str', '*secure': 'bool' } }

Long line.  I think it's time to put each member on its own line.

>  
>  ##
>  # @qom-list-types:
> @@ -227,12 +230,15 @@
>  #
>  # @abstract: if true, include abstract types in the results
>  #
> +# @secure: if set, filter to only include types with matching security status
> +#     (since 10.2)

Hmm.

                absent          false           true
    @abstract   only concrete   only concrete   all
    @secure     all             only insecure   only secure     (I think)

The difference is grating.  Any ideas?

If we decide to keep it as is, please wrap like this:

   # @secure: if set, filter to only include types with matching security
   #     status (since 10.2)

> +#
>  # Returns: a list of types, or an empty list if no results are found
>  #
>  # Since: 1.1
>  ##
>  { 'command': 'qom-list-types',
> -  'data': { '*implements': 'str', '*abstract': 'bool' },
> +  'data': { '*implements': 'str', '*abstract': 'bool', '*secure': 'bool' },
>    'returns': [ 'ObjectTypeInfo' ],
>    'allow-preconfig': true }
>  
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 57f1898cf6..9e221bb332 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -151,33 +151,51 @@ QObject *qmp_qom_get(const char *path, const char *property, Error **errp)
>      return object_property_get_qobject(obj, property, errp);
>  }
>  
> -static void qom_list_types_tramp(ObjectClass *klass, void *data)
> +typedef struct {
> +    ObjectTypeInfoList *list;
> +    bool has_secure;
> +    bool secure;
> +} ObjectTypeInfoData;
> +
> +static void qom_list_types_tramp(ObjectClass *klass, void *opaque)
>  {
> -    ObjectTypeInfoList **pret = data;
> +    ObjectTypeInfoData *data = opaque;
>      ObjectTypeInfo *info;
>      ObjectClass *parent = object_class_get_parent(klass);
>  
> +    if (data->has_secure &&
> +        data->secure != object_class_is_secure(klass)) {
> +        return;
> +    }
> +
>      info = g_malloc0(sizeof(*info));
>      info->name = g_strdup(object_class_get_name(klass));
>      info->has_abstract = info->abstract = object_class_is_abstract(klass);
> +    info->has_secure = info->secure = object_class_is_secure(klass);
>      if (parent) {
>          info->parent = g_strdup(object_class_get_name(parent));
>      }
>  
> -    QAPI_LIST_PREPEND(*pret, info);
> +    QAPI_LIST_PREPEND(data->list, info);
>  }
>  
>  ObjectTypeInfoList *qmp_qom_list_types(const char *implements,
>                                         bool has_abstract,
>                                         bool abstract,
> +                                       bool has_secure,
> +                                       bool secure,
>                                         Error **errp)
>  {
> -    ObjectTypeInfoList *ret = NULL;
> +    ObjectTypeInfoData data = {
> +        .list = NULL,
> +        .has_secure = has_secure,
> +        .secure = secure,
> +    };
>  
>      module_load_qom_all();
> -    object_class_foreach(qom_list_types_tramp, implements, abstract, &ret);
> +    object_class_foreach(qom_list_types_tramp, implements, abstract, &data);
>  
> -    return ret;
> +    return data.list;
>  }
>  
>  ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,

This fuses a change of how the list value is built with the addition of
a new list element member.  I'd prefer them separate.