qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Maksim Davydov
Subject: Re: [PATCH v6 2/4] qmp: add dump machine type compatible properties
Date: Wed, 13 Dec 2023 17:46:12 +0300
User-agent: Mozilla Thunderbird

Thanks for reviewing
Sorry for replying late

On 12/1/23 12:49, Markus Armbruster wrote:
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?
I'm not sure that rightly understand your idea about unstable: do you mean
that we can allow this interface to be not robust (e.g. compat-props in
MachineInfo can be not presented) or that this is new thusit can be
unstable?
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?
now, there are 61 machines types, so this qmp command generates a 450KB JSON
+#
  # 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);

--
Best regards,
Maksim Davydov




reply via email to

[Prev in Thread] Current Thread [Next in Thread]