Re: [PATCH v6 2/4] qmp: add dump machine type compatible properties

[Markus Armbruster <armbru@redhat.com>]

I apologize for the lateness of my review.

Maksim Davydov <davydov-max@yandex-team.ru> writes:

> To control that creating new machine type doesn't affect the previous
> types (their compat_props) and to check complex compat_props inheritance
> we need qmp command to print machine type compatible properties.
>
> This patch adds the ability to get list of all the compat_props of the
> corresponding supported machines for their comparison via new optional
> argument of "query-machines" command.

Sounds like this is to let developers prevent unwanted change.  Such
testing interfaces need not and should not be stable interfaces.
Thoughts?

> Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/core/machine-qmp-cmds.c  | 23 +++++++++++++++-
>  qapi/machine.json           | 54 +++++++++++++++++++++++++++++++++++--
>  tests/qtest/fuzz/qos_fuzz.c |  2 +-
>  3 files changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 3860a50c3b..a49d0dc362 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -66,7 +66,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>      return head;
>  }
>  
> -MachineInfoList *qmp_query_machines(Error **errp)
> +MachineInfoList *qmp_query_machines(bool has_compat_props, bool compat_props,
> +                                    Error **errp)
>  {
>      GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>      MachineInfoList *mach_list = NULL;
> @@ -98,6 +99,26 @@ MachineInfoList *qmp_query_machines(Error **errp)
>              info->default_ram_id = g_strdup(mc->default_ram_id);
>          }
>  
> +        if (compat_props && mc->compat_props) {
> +            int i;
> +            info->compat_props = NULL;
> +            CompatPropertyList **tail = &(info->compat_props);
> +            info->has_compat_props = true;
> +
> +            for (i = 0; i < mc->compat_props->len; i++) {
> +                GlobalProperty *mt_prop = g_ptr_array_index(mc->compat_props,
> +                                                            i);
> +                CompatProperty *prop;
> +
> +                prop = g_malloc0(sizeof(*prop));
> +                prop->driver = g_strdup(mt_prop->driver);
> +                prop->property = g_strdup(mt_prop->property);
> +                prop->value = g_strdup(mt_prop->value);
> +
> +                QAPI_LIST_APPEND(tail, prop);
> +            }
> +        }
> +
>          QAPI_LIST_PREPEND(mach_list, info);
>      }
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index b6d634b30d..8ca0c134a2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -135,6 +135,25 @@
>  ##
>  { 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] }
>  
> +##
> +# @CompatProperty:
> +#
> +# Machine type compatible property. It's based on GlobalProperty and created
> +# for machine type compat properties (see scripts)

"compatibility property"

Doc comments are user documentation.  Can we describe this without
referencing C data types like GlobalProperty?  I'd start with the
purpose: specify a default value for a certain property of certain kind
of device.

The reference (see scripts) needs to be more precise to be useful.

Limit line length to 70, please.

Two spaces between sentences for consistency, please.

> +#
> +# @driver: name of the driver that has GlobalProperty
> +#
> +# @property: global property name
> +#
> +# @value: global property value

I don't like these descriptions, but let's improve the paragraph above
them first.

> +#
> +# Since: 8.2

Going to be 9.0.  More of the same below.

> +##
> +{ 'struct': 'CompatProperty',
> +  'data': { 'driver': 'str',
> +            'property': 'str',
> +            'value': 'str' } }
> +
>  ##
>  # @MachineInfo:
>  #
> @@ -166,6 +185,9 @@
>  #
>  # @acpi: machine type supports ACPI (since 8.0)
>  #
> +# @compat-props: List of compatible properties that defines machine type

"The machine type's compatibility properties"

Leaves unsaid when the member is present.  Let's worry about that later.

> +#                (since 8.2)
> +#
>  # Since: 1.2
>  ##
>  { 'struct': 'MachineInfo',
> @@ -173,18 +195,46 @@
>              '*is-default': 'bool', 'cpu-max': 'int',
>              'hotpluggable-cpus': 'bool',  'numa-mem-supported': 'bool',
>              'deprecated': 'bool', '*default-cpu-type': 'str',
> -            '*default-ram-id': 'str', 'acpi': 'bool' } }
> +            '*default-ram-id': 'str', 'acpi': 'bool', '*compat-props': ['CompatProperty'] } }

Long line, break it before '*compat-props', please.

To mark the interface unstable, do something like

               '*compat-props': { 'type': ['CompatProperty'],
                                  'features': [ 'unstable' ] },

You may want to rename it to x-compat-props, just to make "unstable"
obvious to human users.

>  
>  ##
>  # @query-machines:
>  #
>  # Return a list of supported machines
>  #
> +# @compat-props: if true return will contain information about machine type
> +#                compatible properties (since 8.2)

"compatibility properties"

Suppressing parts of the output makes sense only if it's fairly big.
How much additional compat-props output do you observe?

> +#
>  # Returns: a list of MachineInfo
>  #
>  # Since: 1.2
> +#
> +# Example:
> +#
> +# -> { "execute": "query-machines", "arguments": { "compat-props": true } }
> +# <- { "return": [
> +#          {
> +#              "hotpluggable-cpus": true,
> +#              "name": "pc-q35-6.2",
> +#              "compat-props": [
> +#                  {
> +#                      "driver": "virtio-mem",
> +#                      "property": "unplugged-inaccessible",
> +#                      "value": "off"
> +#                   }
> +#               ],
> +#               "numa-mem-supported": false,
> +#               "default-cpu-type": "qemu64-x86_64-cpu",
> +#               "cpu-max": 288,
> +#               "deprecated": false,
> +#               "default-ram-id": "pc.ram"
> +#           },
> +#           ...
> +#    }
>  ##
> -{ 'command': 'query-machines', 'returns': ['MachineInfo'] }
> +{ 'command': 'query-machines',
> +  'data': { '*compat-props': 'bool' },
> +  'returns': ['MachineInfo'] }
>  
>  ##
>  # @CurrentMachineParams:
> diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
> index e403d373a0..b71e945c5f 100644
> --- a/tests/qtest/fuzz/qos_fuzz.c
> +++ b/tests/qtest/fuzz/qos_fuzz.c
> @@ -46,7 +46,7 @@ static void qos_set_machines_devices_available(void)
>      MachineInfoList *mach_info;
>      ObjectTypeInfoList *type_info;
>  
> -    mach_info = qmp_query_machines(&error_abort);
> +    mach_info = qmp_query_machines(false, false, &error_abort);
>      machines_apply_to_node(mach_info);
>      qapi_free_MachineInfoList(mach_info);