[PATCH v1 0/9] Machine type compatible properties

[PATCH v1 0/9] Machine type compatible properties

[Maxim Davydov]

We need to be able to check machine type after its definition. It's
necessary when using complicated inheritance of compatible features. For
instance, this tool can help to find bugs in the machine type definition
if the name of the device has been changed. Also, this tool was created
to help with MTs of other projects such as vz branches.

Maxim Davydov (9):
  qmp: Add dump machine type compatible properties
  pci: add null-pointer check
  mem: appropriate handling getting mem region
  msmouse: add appropriate unregister handler
  wctablet: add appropriate unregister handler
  chardev: add appropriate getting address
  colo-compare: safe finalization
  qom: add command to print initial properties
  scripts: printing machine type compat properties

 chardev/char-socket.c       |   9 ++
 chardev/msmouse.c           |   4 +-
 chardev/wctablet.c          |   4 +-
 hw/core/machine-qmp-cmds.c  |  25 +++-
 hw/i386/sgx-epc.c           |   5 +-
 hw/mem/nvdimm.c             |   6 +
 hw/mem/pc-dimm.c            |   5 +
 hw/pci-host/i440fx.c        |  17 ++-
 hw/pci-host/q35.c           |  17 ++-
 net/colo-compare.c          |  25 ++--
 qapi/machine.json           |  58 +++++++-
 qapi/qom.json               |  69 +++++++++
 qom/qom-qmp-cmds.c          | 121 ++++++++++++++++
 scripts/print_MT.py         | 274 ++++++++++++++++++++++++++++++++++++
 tests/qtest/fuzz/qos_fuzz.c |   2 +-
 15 files changed, 613 insertions(+), 28 deletions(-)
 create mode 100755 scripts/print_MT.py

-- 
2.31.1

[PATCH v1 1/9] qmp: Add dump machine type compatible properties

[Maxim Davydov]

This patch adds the ability to get all the compat_props of the
corresponding supported machines for their comparison.

Example:
{ "execute" : "query-machines", "arguments" : { "is-full" : true } }

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 hw/core/machine-qmp-cmds.c  | 25 +++++++++++++++-
 qapi/machine.json           | 58 +++++++++++++++++++++++++++++++++++--
 tests/qtest/fuzz/qos_fuzz.c |  2 +-
 3 files changed, 81 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 4f4ab30f8c..8f3206ba8d 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -74,7 +74,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
     return head;
 }
 
-MachineInfoList *qmp_query_machines(Error **errp)
+MachineInfoList *qmp_query_machines(bool has_is_full, bool is_full,
+                                    Error **errp)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
     MachineInfoList *mach_list = NULL;
@@ -107,6 +108,28 @@ MachineInfoList *qmp_query_machines(Error **errp)
             info->default_ram_id = g_strdup(mc->default_ram_id);
             info->has_default_ram_id = true;
         }
+        if (has_is_full && is_full && mc->compat_props) {
+            int i;
+            info->compat_props = NULL;
+            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);
+                ObjectClass *klass = object_class_by_name(mt_prop->driver);
+                CompatProperty *prop;
+
+                prop = g_malloc0(sizeof(*prop));
+                if (klass && object_class_is_abstract(klass)) {
+                    prop->abstract = true;
+                }
+                prop->driver = g_strdup(mt_prop->driver);
+                prop->property = g_strdup(mt_prop->property);
+                prop->value = g_strdup(mt_prop->value);
+
+                QAPI_LIST_PREPEND(info->compat_props, prop);
+            }
+        }
 
         QAPI_LIST_PREPEND(mach_list, info);
     }
diff --git a/qapi/machine.json b/qapi/machine.json
index 42fc68403d..16e961477c 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -130,6 +130,28 @@
 ##
 { '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)
+#
+# @driver: name of the driver that has GlobalProperty
+#
+# @abstract: Bool value that shows that property is belonged to abstract class
+#
+# @property: global property name
+#
+# @value: global property value
+#
+# Since: 7.0
+##
+{ 'struct': 'CompatProperty',
+  'data': { 'driver': 'str',
+            'abstract': 'bool',
+            'property': 'str',
+            'value': 'str' } }
+
 ##
 # @MachineInfo:
 #
@@ -158,6 +180,9 @@
 #
 # @default-ram-id: the default ID of initial RAM memory backend (since 5.2)
 #
+# @compat-props: List of compatible properties that defines machine type
+#                (since 7.0)
+#
 # Since: 1.2
 ##
 { 'struct': 'MachineInfo',
@@ -165,18 +190,47 @@
             '*is-default': 'bool', 'cpu-max': 'int',
             'hotpluggable-cpus': 'bool',  'numa-mem-supported': 'bool',
             'deprecated': 'bool', '*default-cpu-type': 'str',
-            '*default-ram-id': 'str' } }
+            '*default-ram-id': 'str', '*compat-props': ['CompatProperty'] } }
 
 ##
 # @query-machines:
 #
 # Return a list of supported machines
 #
+# @is-full: if true return will contain information about machine type 
+#           compatible properties (since 7.0)
+#
 # Returns: a list of MachineInfo
 #
 # Since: 1.2
+#
+# Example:
+#
+# -> { "execute" : "query-machines", "arguments" : { "is-full" : true } }
+# <- { "return": [
+#          {
+#              "hotpluggable-cpus": true,
+#              "name": "pc-q35-6.2",
+#              "compat-props": [
+#                  {
+#                      "abstract": false,
+#                      "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': { '*is-full': 'bool' },
+  'returns': ['MachineInfo'] }
 
 ##
 # @CurrentMachineParams:
diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
index 7a244c951e..3f9c1ede6e 100644
--- a/tests/qtest/fuzz/qos_fuzz.c
+++ b/tests/qtest/fuzz/qos_fuzz.c
@@ -47,7 +47,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);
 
-- 
2.31.1

Re: [PATCH v1 1/9] qmp: Add dump machine type compatible properties

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> This patch adds the ability to get all the compat_props of the
> corresponding supported machines for their comparison.
> 
> Example:
> { "execute" : "query-machines", "arguments" : { "is-full" : true } }
> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   hw/core/machine-qmp-cmds.c  | 25 +++++++++++++++-
>   qapi/machine.json           | 58 +++++++++++++++++++++++++++++++++++--
>   tests/qtest/fuzz/qos_fuzz.c |  2 +-
>   3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 4f4ab30f8c..8f3206ba8d 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -74,7 +74,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>       return head;
>   }
>   
> -MachineInfoList *qmp_query_machines(Error **errp)
> +MachineInfoList *qmp_query_machines(bool has_is_full, bool is_full,
> +                                    Error **errp)
>   {
>       GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>       MachineInfoList *mach_list = NULL;
> @@ -107,6 +108,28 @@ MachineInfoList *qmp_query_machines(Error **errp)
>               info->default_ram_id = g_strdup(mc->default_ram_id);
>               info->has_default_ram_id = true;
>           }
> +        if (has_is_full && is_full && mc->compat_props) {

is_full is guaranteed to be zero when has_is_full is zero. So, it's enough to write:

    if (is_full && mc->compat_props) {

> +            int i;
> +            info->compat_props = NULL;
> +            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);
> +                ObjectClass *klass = object_class_by_name(mt_prop->driver);
> +                CompatProperty *prop;
> +
> +                prop = g_malloc0(sizeof(*prop));
> +                if (klass && object_class_is_abstract(klass)) {
> +                    prop->abstract = true;
> +                }
> +                prop->driver = g_strdup(mt_prop->driver);
> +                prop->property = g_strdup(mt_prop->property);
> +                prop->value = g_strdup(mt_prop->value);
> +
> +                QAPI_LIST_PREPEND(info->compat_props, prop);
> +            }
> +        }
>   
>           QAPI_LIST_PREPEND(mach_list, info);
>       }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 42fc68403d..16e961477c 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -130,6 +130,28 @@
>   ##
>   { '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)
> +#
> +# @driver: name of the driver that has GlobalProperty
> +#
> +# @abstract: Bool value that shows that property is belonged to abstract class
> +#
> +# @property: global property name
> +#
> +# @value: global property value
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'CompatProperty',
> +  'data': { 'driver': 'str',
> +            'abstract': 'bool',
> +            'property': 'str',
> +            'value': 'str' } }
> +
>   ##
>   # @MachineInfo:
>   #
> @@ -158,6 +180,9 @@
>   #
>   # @default-ram-id: the default ID of initial RAM memory backend (since 5.2)
>   #
> +# @compat-props: List of compatible properties that defines machine type
> +#                (since 7.0)
> +#
>   # Since: 1.2
>   ##
>   { 'struct': 'MachineInfo',
> @@ -165,18 +190,47 @@
>               '*is-default': 'bool', 'cpu-max': 'int',
>               'hotpluggable-cpus': 'bool',  'numa-mem-supported': 'bool',
>               'deprecated': 'bool', '*default-cpu-type': 'str',
> -            '*default-ram-id': 'str' } }
> +            '*default-ram-id': 'str', '*compat-props': ['CompatProperty'] } }
>   
>   ##
>   # @query-machines:
>   #
>   # Return a list of supported machines
>   #
> +# @is-full: if true return will contain information about machine type
> +#           compatible properties (since 7.0)

Should be 7.1.

Also, maybe call it "compat-props" to be consistent with output and with documentation?

> +#
>   # Returns: a list of MachineInfo
>   #
>   # Since: 1.2
> +#
> +# Example:
> +#
> +# -> { "execute" : "query-machines", "arguments" : { "is-full" : true } }
> +# <- { "return": [
> +#          {
> +#              "hotpluggable-cpus": true,
> +#              "name": "pc-q35-6.2",
> +#              "compat-props": [
> +#                  {
> +#                      "abstract": false,
> +#                      "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': { '*is-full': 'bool' },
> +  'returns': ['MachineInfo'] }
>   
>   ##
>   # @CurrentMachineParams:
> diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
> index 7a244c951e..3f9c1ede6e 100644
> --- a/tests/qtest/fuzz/qos_fuzz.c
> +++ b/tests/qtest/fuzz/qos_fuzz.c
> @@ -47,7 +47,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);
>   

weak:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>

-- 
Best regards,
Vladimir

Re: [PATCH v1 1/9] qmp: Add dump machine type compatible properties

[Maxim Davydov]

On 3/30/22 14:03, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> This patch adds the ability to get all the compat_props of the
>> corresponding supported machines for their comparison.
>>
>> Example:
>> { "execute" : "query-machines", "arguments" : { "is-full" : true } }
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   hw/core/machine-qmp-cmds.c  | 25 +++++++++++++++-
>>   qapi/machine.json           | 58 +++++++++++++++++++++++++++++++++++--
>>   tests/qtest/fuzz/qos_fuzz.c |  2 +-
>>   3 files changed, 81 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
>> index 4f4ab30f8c..8f3206ba8d 100644
>> --- a/hw/core/machine-qmp-cmds.c
>> +++ b/hw/core/machine-qmp-cmds.c
>> @@ -74,7 +74,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>>       return head;
>>   }
>>   -MachineInfoList *qmp_query_machines(Error **errp)
>> +MachineInfoList *qmp_query_machines(bool has_is_full, bool is_full,
>> +                                    Error **errp)
>>   {
>>       GSList *el, *machines = object_class_get_list(TYPE_MACHINE, 
>> false);
>>       MachineInfoList *mach_list = NULL;
>> @@ -107,6 +108,28 @@ MachineInfoList *qmp_query_machines(Error **errp)
>>               info->default_ram_id = g_strdup(mc->default_ram_id);
>>               info->has_default_ram_id = true;
>>           }
>> +        if (has_is_full && is_full && mc->compat_props) {
>
> is_full is guaranteed to be zero when has_is_full is zero. So, it's 
> enough to write:
>
>    if (is_full && mc->compat_props) {
>
>> +            int i;
>> +            info->compat_props = NULL;
>> +            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);
>> +                ObjectClass *klass = 
>> object_class_by_name(mt_prop->driver);
>> +                CompatProperty *prop;
>> +
>> +                prop = g_malloc0(sizeof(*prop));
>> +                if (klass && object_class_is_abstract(klass)) {
>> +                    prop->abstract = true;
>> +                }
>> +                prop->driver = g_strdup(mt_prop->driver);
>> +                prop->property = g_strdup(mt_prop->property);
>> +                prop->value = g_strdup(mt_prop->value);
>> +
>> +                QAPI_LIST_PREPEND(info->compat_props, prop);
>> +            }
>> +        }
>>             QAPI_LIST_PREPEND(mach_list, info);
>>       }
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 42fc68403d..16e961477c 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -130,6 +130,28 @@
>>   ##
>>   { '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)
>> +#
>> +# @driver: name of the driver that has GlobalProperty
>> +#
>> +# @abstract: Bool value that shows that property is belonged to 
>> abstract class
>> +#
>> +# @property: global property name
>> +#
>> +# @value: global property value
>> +#
>> +# Since: 7.0
>> +##
>> +{ 'struct': 'CompatProperty',
>> +  'data': { 'driver': 'str',
>> +            'abstract': 'bool',
>> +            'property': 'str',
>> +            'value': 'str' } }
>> +
>>   ##
>>   # @MachineInfo:
>>   #
>> @@ -158,6 +180,9 @@
>>   #
>>   # @default-ram-id: the default ID of initial RAM memory backend 
>> (since 5.2)
>>   #
>> +# @compat-props: List of compatible properties that defines machine 
>> type
>> +#                (since 7.0)
>> +#
>>   # Since: 1.2
>>   ##
>>   { 'struct': 'MachineInfo',
>> @@ -165,18 +190,47 @@
>>               '*is-default': 'bool', 'cpu-max': 'int',
>>               'hotpluggable-cpus': 'bool', 'numa-mem-supported': 'bool',
>>               'deprecated': 'bool', '*default-cpu-type': 'str',
>> -            '*default-ram-id': 'str' } }
>> +            '*default-ram-id': 'str', '*compat-props': 
>> ['CompatProperty'] } }
>>     ##
>>   # @query-machines:
>>   #
>>   # Return a list of supported machines
>>   #
>> +# @is-full: if true return will contain information about machine type
>> +#           compatible properties (since 7.0)
>
> Should be 7.1.
>
> Also, maybe call it "compat-props" to be consistent with output and 
> with documentation?
>
>> +#
>>   # Returns: a list of MachineInfo
>>   #
>>   # Since: 1.2
>> +#
>> +# Example:
>> +#
>> +# -> { "execute" : "query-machines", "arguments" : { "is-full" : 
>> true } }
>> +# <- { "return": [
>> +#          {
>> +#              "hotpluggable-cpus": true,
>> +#              "name": "pc-q35-6.2",
>> +#              "compat-props": [
>> +#                  {
>> +#                      "abstract": false,
>> +#                      "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': { '*is-full': 'bool' },
>> +  'returns': ['MachineInfo'] }
>>     ##
>>   # @CurrentMachineParams:
>> diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
>> index 7a244c951e..3f9c1ede6e 100644
>> --- a/tests/qtest/fuzz/qos_fuzz.c
>> +++ b/tests/qtest/fuzz/qos_fuzz.c
>> @@ -47,7 +47,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);
>
> weak:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>
Thank you for the review. Yes, the name "compat-props" seems more 
appropriate than "is-full". It will be changed in the next version

-- 
Best regards,
Maxim Davydov

[PATCH v1 2/9] pci: add null-pointer check

[Maxim Davydov]

Call pci_bus_get_w64_range can fail with the segmentation fault. For
example, this can happen during attempt to get pci-hole64-end immediately
after initialization.

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 hw/pci-host/i440fx.c | 17 +++++++++++------
 hw/pci-host/q35.c    | 17 +++++++++++------
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index e08716142b..71a114e551 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -158,10 +158,12 @@ static uint64_t i440fx_pcihost_get_pci_hole64_start_value(Object *obj)
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
     Range w64;
-    uint64_t value;
+    uint64_t value = 0;
 
-    pci_bus_get_w64_range(h->bus, &w64);
-    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
+    if (h->bus) {
+        pci_bus_get_w64_range(h->bus, &w64);
+        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
+    }
     if (!value && s->pci_hole64_fix) {
         value = pc_pci_hole64_start();
     }
@@ -191,10 +193,13 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
     uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
     Range w64;
-    uint64_t value, hole64_end;
+    uint64_t value = 0;
+    uint64_t hole64_end;
 
-    pci_bus_get_w64_range(h->bus, &w64);
-    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    if (h->bus) {
+        pci_bus_get_w64_range(h->bus, &w64);
+        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    }
     hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
     if (s->pci_hole64_fix && value < hole64_end) {
         value = hole64_end;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index ab5a47aff5..d679fd85ef 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -124,10 +124,12 @@ static uint64_t q35_host_get_pci_hole64_start_value(Object *obj)
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
     Range w64;
-    uint64_t value;
+    uint64_t value = 0;
 
-    pci_bus_get_w64_range(h->bus, &w64);
-    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
+    if (h->bus) {
+        pci_bus_get_w64_range(h->bus, &w64);
+        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
+    }
     if (!value && s->pci_hole64_fix) {
         value = pc_pci_hole64_start();
     }
@@ -157,10 +159,13 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
     uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
     Range w64;
-    uint64_t value, hole64_end;
+    uint64_t value = 0;
+    uint64_t hole64_end;
 
-    pci_bus_get_w64_range(h->bus, &w64);
-    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    if (h->bus) {
+        pci_bus_get_w64_range(h->bus, &w64);
+        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
+    }
     hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
     if (s->pci_hole64_fix && value < hole64_end) {
         value = hole64_end;
-- 
2.31.1

Re: [PATCH v1 2/9] pci: add null-pointer check

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> Call pci_bus_get_w64_range can fail with the segmentation fault. For
> example, this can happen during attempt to get pci-hole64-end immediately
> after initialization.

So, immediately after initialization, h->bus is NULL?

The significant bit is, is the value which we calculate without h->bus is correct or not? That should be covered by commit message.

> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   hw/pci-host/i440fx.c | 17 +++++++++++------
>   hw/pci-host/q35.c    | 17 +++++++++++------
>   2 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index e08716142b..71a114e551 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -158,10 +158,12 @@ static uint64_t i440fx_pcihost_get_pci_hole64_start_value(Object *obj)
>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>       Range w64;
> -    uint64_t value;
> +    uint64_t value = 0;
>   
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    }
>       if (!value && s->pci_hole64_fix) {
>           value = pc_pci_hole64_start();
>       }
> @@ -191,10 +193,13 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>       uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
>       Range w64;
> -    uint64_t value, hole64_end;
> +    uint64_t value = 0;
> +    uint64_t hole64_end;
>   
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    }
>       hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
>       if (s->pci_hole64_fix && value < hole64_end) {
>           value = hole64_end;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index ab5a47aff5..d679fd85ef 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -124,10 +124,12 @@ static uint64_t q35_host_get_pci_hole64_start_value(Object *obj)
>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>       Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>       Range w64;
> -    uint64_t value;
> +    uint64_t value = 0;
>   
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    }
>       if (!value && s->pci_hole64_fix) {
>           value = pc_pci_hole64_start();
>       }
> @@ -157,10 +159,13 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>       Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>       uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
>       Range w64;
> -    uint64_t value, hole64_end;
> +    uint64_t value = 0;
> +    uint64_t hole64_end;
>   
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    }
>       hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
>       if (s->pci_hole64_fix && value < hole64_end) {
>           value = hole64_end;


-- 
Best regards,
Vladimir

Re: [PATCH v1 2/9] pci: add null-pointer check

[Maxim Davydov]

On 3/30/22 14:07, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> Call pci_bus_get_w64_range can fail with the segmentation fault. For
>> example, this can happen during attempt to get pci-hole64-end 
>> immediately
>> after initialization.
>
> So, immediately after initialization, h->bus is NULL?
>
> The significant bit is, is the value which we calculate without h->bus 
> is correct or not? That should be covered by commit message.
For example, object_new_with_class() returns only initialized object 
(after calling instance_init). It means that pci_root_bus_new() in 
q35_host_realize() hasn't been called for returned object and pci->bus 
== NULL. So, if then we try to call q35_host_get_pci_hole64_end() it 
will fail with segmentation fault in the pci_for_each_device_under_bus() 
(d = bus->devices[devfn], but bus == NULL). Similarly for i440fx. I'm 
not sure that it's the correct behavior.
To reproduce this situation, run "{'execute' : 'query-init-properties'}" 
or qmp_query_init_properties() from 8th patch of this series without 
applying fixes for pci-host.
After this fix, the behavior is the similar as if range_is_empty(&w64) 
== True, but without SEGFAULT. Although, we can check flag 
DeviceState.realized to detect unrealized device.
>
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   hw/pci-host/i440fx.c | 17 +++++++++++------
>>   hw/pci-host/q35.c    | 17 +++++++++++------
>>   2 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>> index e08716142b..71a114e551 100644
>> --- a/hw/pci-host/i440fx.c
>> +++ b/hw/pci-host/i440fx.c
>> @@ -158,10 +158,12 @@ static uint64_t 
>> i440fx_pcihost_get_pci_hole64_start_value(Object *obj)
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>>       Range w64;
>> -    uint64_t value;
>> +    uint64_t value = 0;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    }
>>       if (!value && s->pci_hole64_fix) {
>>           value = pc_pci_hole64_start();
>>       }
>> @@ -191,10 +193,13 @@ static void 
>> i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>>       uint64_t hole64_start = 
>> i440fx_pcihost_get_pci_hole64_start_value(obj);
>>       Range w64;
>> -    uint64_t value, hole64_end;
>> +    uint64_t value = 0;
>> +    uint64_t hole64_end;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    }
>>       hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL 
>> << 30);
>>       if (s->pci_hole64_fix && value < hole64_end) {
>>           value = hole64_end;
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index ab5a47aff5..d679fd85ef 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -124,10 +124,12 @@ static uint64_t 
>> q35_host_get_pci_hole64_start_value(Object *obj)
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>       Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>       Range w64;
>> -    uint64_t value;
>> +    uint64_t value = 0;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    }
>>       if (!value && s->pci_hole64_fix) {
>>           value = pc_pci_hole64_start();
>>       }
>> @@ -157,10 +159,13 @@ static void q35_host_get_pci_hole64_end(Object 
>> *obj, Visitor *v,
>>       Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>       uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
>>       Range w64;
>> -    uint64_t value, hole64_end;
>> +    uint64_t value = 0;
>> +    uint64_t hole64_end;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    }
>>       hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 
>> 1ULL << 30);
>>       if (s->pci_hole64_fix && value < hole64_end) {
>>           value = hole64_end;
>
>

-- 
Best regards,
Maxim Davydov

Re: [PATCH v1 2/9] pci: add null-pointer check

[Igor Mammedov]

On Tue, 29 Mar 2022 00:15:32 +0300
Maxim Davydov <maxim.davydov@openvz.org> wrote:

> Call pci_bus_get_w64_range can fail with the segmentation fault. For
> example, this can happen during attempt to get pci-hole64-end

>" immediately after initialization"
this too vague, pls provide a better description 
and is possible a reproducer.

> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>  hw/pci-host/i440fx.c | 17 +++++++++++------
>  hw/pci-host/q35.c    | 17 +++++++++++------
>  2 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index e08716142b..71a114e551 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -158,10 +158,12 @@ static uint64_t i440fx_pcihost_get_pci_hole64_start_value(Object *obj)
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
>      I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>      Range w64;
> -    uint64_t value;
> +    uint64_t value = 0;
>  
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    }
>      if (!value && s->pci_hole64_fix) {
>          value = pc_pci_hole64_start();
>      }
> @@ -191,10 +193,13 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>      I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>      uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj);
>      Range w64;
> -    uint64_t value, hole64_end;
> +    uint64_t value = 0;
> +    uint64_t hole64_end;
>  
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    }
>      hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL << 30);
>      if (s->pci_hole64_fix && value < hole64_end) {
>          value = hole64_end;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index ab5a47aff5..d679fd85ef 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -124,10 +124,12 @@ static uint64_t q35_host_get_pci_hole64_start_value(Object *obj)
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>      Range w64;
> -    uint64_t value;
> +    uint64_t value = 0;
>  
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
> +    }
>      if (!value && s->pci_hole64_fix) {
>          value = pc_pci_hole64_start();
>      }
> @@ -157,10 +159,13 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>      uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
>      Range w64;
> -    uint64_t value, hole64_end;
> +    uint64_t value = 0;
> +    uint64_t hole64_end;
>  
> -    pci_bus_get_w64_range(h->bus, &w64);
> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    if (h->bus) {
> +        pci_bus_get_w64_range(h->bus, &w64);
> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
> +    }
>      hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 1ULL << 30);
>      if (s->pci_hole64_fix && value < hole64_end) {
>          value = hole64_end;

[PATCH v1 3/9] mem: appropriate handling getting mem region

[Maxim Davydov]

Attempt to get memory region if the device doesn't have hostmem may not be
an error. This can be happen immediately after initialization (getting
value without default one).

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 hw/i386/sgx-epc.c | 5 ++++-
 hw/mem/nvdimm.c   | 6 ++++++
 hw/mem/pc-dimm.c  | 5 +++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
index d664829d35..1a4c8acdcc 100644
--- a/hw/i386/sgx-epc.c
+++ b/hw/i386/sgx-epc.c
@@ -121,9 +121,12 @@ static MemoryRegion *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
 {
     SGXEPCDevice *epc = SGX_EPC(md);
     HostMemoryBackend *hostmem;
+    DeviceState *dev = DEVICE(epc);
 
     if (!epc->hostmem) {
-        error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
+        if (dev->realized) {
+            error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
+        }
         return NULL;
     }
 
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7c7d777781..61e77e5476 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,9 +166,15 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
                                                  Error **errp)
 {
     NVDIMMDevice *nvdimm = NVDIMM(md);
+    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
     Error *local_err = NULL;
 
     if (!nvdimm->nvdimm_mr) {
+        /* Not error if we try get memory region after init */
+        if (!dimm->hostmem) {
+            return NULL;
+        }
+
         nvdimm_prepare_memory_region(nvdimm, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index f27e1a11ba..6fd74de97f 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
 static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
                                                   Error **errp)
 {
+    PCDIMMDevice *dimm = PC_DIMM(md);
+    /* Not error if we try get memory region after init */
+    if (!dimm->hostmem) {
+        return NULL;
+    }
     return pc_dimm_get_memory_region(PC_DIMM(md), errp);
 }
 
-- 
2.31.1

Re: [PATCH v1 3/9] mem: appropriate handling getting mem region

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> Attempt to get memory region if the device doesn't have hostmem may not be
> an error. This can be happen immediately after initialization (getting
> value without default one).
> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   hw/i386/sgx-epc.c | 5 ++++-
>   hw/mem/nvdimm.c   | 6 ++++++
>   hw/mem/pc-dimm.c  | 5 +++++
>   3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
> index d664829d35..1a4c8acdcc 100644
> --- a/hw/i386/sgx-epc.c
> +++ b/hw/i386/sgx-epc.c
> @@ -121,9 +121,12 @@ static MemoryRegion *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
>   {
>       SGXEPCDevice *epc = SGX_EPC(md);
>       HostMemoryBackend *hostmem;
> +    DeviceState *dev = DEVICE(epc);
>   
>       if (!epc->hostmem) {
> -        error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +        if (dev->realized) {
> +            error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +        }
>           return NULL;
>       }

I can't judge, is it really and error or not.

But the way you change the logic is not correct, as you change the semantics:

Old semantics: on error return NULL and set errp, on success return non-NULL and not set errp

New semantics: on error return NULL and set errp, on success return anything (may be NULL) and not set errp.

Callers are not prepared to this. For example, look at memory_device_unplug:
it does

   mr = mdc->get_memory_region(md, &error_abort);

assume it returns NULL, which is not an error (so we don't crash on error_abort)

and then pass mr  to memory_region_del_subregion(), which in turn access mr->container, which will crash if mr is NULL.

Most probably the situation I describe is not possible, but I just want to illustrate the idea.

Moreover, in QEMU functions which has "Error **errp" argument and return pointer are recommended to return NULL on failure and nonNULL on success. In other words, return value of function with "Error **errp" argument should report success/failure information. And having NULL as possible success return value is not recommended, as it's ambiguous and leads to bugs (see big comment at start of include/qapi/error.h).

So, if it's really needed to change the semantics in such not-recommended way, you should check that all callers are OK with it and also describe new semantics in a comment near get_memory_region declaration. But better is deal with returned error as it is.. What is an exact problem you trying to solve with this commit?

>   
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7c7d777781..61e77e5476 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -166,9 +166,15 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
>                                                    Error **errp)
>   {
>       NVDIMMDevice *nvdimm = NVDIMM(md);
> +    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
>       Error *local_err = NULL;
>   
>       if (!nvdimm->nvdimm_mr) {
> +        /* Not error if we try get memory region after init */
> +        if (!dimm->hostmem) {
> +            return NULL;
> +        }
> +
>           nvdimm_prepare_memory_region(nvdimm, &local_err);
>           if (local_err) {
>               error_propagate(errp, local_err);
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index f27e1a11ba..6fd74de97f 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
>   static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
>                                                     Error **errp)
>   {
> +    PCDIMMDevice *dimm = PC_DIMM(md);
> +    /* Not error if we try get memory region after init */
> +    if (!dimm->hostmem) {
> +        return NULL;
> +    }
>       return pc_dimm_get_memory_region(PC_DIMM(md), errp);
>   }
>   


-- 
Best regards,
Vladimir

Re: [PATCH v1 3/9] mem: appropriate handling getting mem region

[Maxim Davydov]

On 3/30/22 14:27, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> Attempt to get memory region if the device doesn't have hostmem may 
>> not be
>> an error. This can be happen immediately after initialization (getting
>> value without default one).
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   hw/i386/sgx-epc.c | 5 ++++-
>>   hw/mem/nvdimm.c   | 6 ++++++
>>   hw/mem/pc-dimm.c  | 5 +++++
>>   3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
>> index d664829d35..1a4c8acdcc 100644
>> --- a/hw/i386/sgx-epc.c
>> +++ b/hw/i386/sgx-epc.c
>> @@ -121,9 +121,12 @@ static MemoryRegion 
>> *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
>>   {
>>       SGXEPCDevice *epc = SGX_EPC(md);
>>       HostMemoryBackend *hostmem;
>> +    DeviceState *dev = DEVICE(epc);
>>         if (!epc->hostmem) {
>> -        error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be 
>> set");
>> +        if (dev->realized) {
>> +            error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property 
>> must be set");
>> +        }
>>           return NULL;
>>       }
>
> I can't judge, is it really and error or not.
>
> But the way you change the logic is not correct, as you change the 
> semantics:
>
> Old semantics: on error return NULL and set errp, on success return 
> non-NULL and not set errp
>
> New semantics: on error return NULL and set errp, on success return 
> anything (may be NULL) and not set errp.
>
> Callers are not prepared to this. For example, look at 
> memory_device_unplug:
> it does
>
>   mr = mdc->get_memory_region(md, &error_abort);
>
> assume it returns NULL, which is not an error (so we don't crash on 
> error_abort)
>
> and then pass mr  to memory_region_del_subregion(), which in turn 
> access mr->container, which will crash if mr is NULL.
>
> Most probably the situation I describe is not possible, but I just 
> want to illustrate the idea.
>
> Moreover, in QEMU functions which has "Error **errp" argument and 
> return pointer are recommended to return NULL on failure and nonNULL 
> on success. In other words, return value of function with "Error 
> **errp" argument should report success/failure information. And having 
> NULL as possible success return value is not recommended, as it's 
> ambiguous and leads to bugs (see big comment at start of 
> include/qapi/error.h).
>
> So, if it's really needed to change the semantics in such 
> not-recommended way, you should check that all callers are OK with it 
> and also describe new semantics in a comment near get_memory_region 
> declaration. But better is deal with returned error as it is.. What is 
> an exact problem you trying to solve with this commit?
I tried to solve the problem with errors from request MemoryRegion (via 
*md_get_memory_region()) that was called immediately after 
object_new_with_class(). But it does seem to change the semantics. 
Perhaps better solution would be to ignore these errors or to add an 
exception to handle the object properties correctly.
>
>>   diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
>> index 7c7d777781..61e77e5476 100644
>> --- a/hw/mem/nvdimm.c
>> +++ b/hw/mem/nvdimm.c
>> @@ -166,9 +166,15 @@ static MemoryRegion 
>> *nvdimm_md_get_memory_region(MemoryDeviceState *md,
>>                                                    Error **errp)
>>   {
>>       NVDIMMDevice *nvdimm = NVDIMM(md);
>> +    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
>>       Error *local_err = NULL;
>>         if (!nvdimm->nvdimm_mr) {
>> +        /* Not error if we try get memory region after init */
>> +        if (!dimm->hostmem) {
>> +            return NULL;
>> +        }
>> +
>>           nvdimm_prepare_memory_region(nvdimm, &local_err);
>>           if (local_err) {
>>               error_propagate(errp, local_err);
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index f27e1a11ba..6fd74de97f 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -240,6 +240,11 @@ static void 
>> pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
>>   static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState 
>> *md,
>>                                                     Error **errp)
>>   {
>> +    PCDIMMDevice *dimm = PC_DIMM(md);
>> +    /* Not error if we try get memory region after init */
>> +    if (!dimm->hostmem) {
>> +        return NULL;
>> +    }
>>       return pc_dimm_get_memory_region(PC_DIMM(md), errp);
>>   }
>
>
-- 
Best regards,
Maxim Davydov

Re: [PATCH v1 3/9] mem: appropriate handling getting mem region

[Igor Mammedov]

On Tue, 29 Mar 2022 00:15:33 +0300
Maxim Davydov <maxim.davydov@openvz.org> wrote:

> Attempt to get memory region if the device doesn't have hostmem may not be
> an error. This can be happen immediately after initialization (getting
> value without default one).

Above statement begs for expanded explanation
Pls rephrase and explain why it's needed and how it will be used.

> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>  hw/i386/sgx-epc.c | 5 ++++-
>  hw/mem/nvdimm.c   | 6 ++++++
>  hw/mem/pc-dimm.c  | 5 +++++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
> index d664829d35..1a4c8acdcc 100644
> --- a/hw/i386/sgx-epc.c
> +++ b/hw/i386/sgx-epc.c
> @@ -121,9 +121,12 @@ static MemoryRegion *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
>  {
>      SGXEPCDevice *epc = SGX_EPC(md);
>      HostMemoryBackend *hostmem;
> +    DeviceState *dev = DEVICE(epc);
>  
>      if (!epc->hostmem) {
> -        error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +        if (dev->realized) {
> +            error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +        }
>          return NULL;
>      }
>  
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7c7d777781..61e77e5476 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -166,9 +166,15 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
>                                                   Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(md);
> +    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
>      Error *local_err = NULL;
>  
>      if (!nvdimm->nvdimm_mr) {
> +        /* Not error if we try get memory region after init */
> +        if (!dimm->hostmem) {
> +            return NULL;
> +        }
> +
>          nvdimm_prepare_memory_region(nvdimm, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index f27e1a11ba..6fd74de97f 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
>  static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
>                                                    Error **errp)
>  {
> +    PCDIMMDevice *dimm = PC_DIMM(md);
> +    /* Not error if we try get memory region after init */
> +    if (!dimm->hostmem) {
> +        return NULL;
> +    }
>      return pc_dimm_get_memory_region(PC_DIMM(md), errp);
>  }
>  

[PATCH v1 4/9] msmouse: add appropriate unregister handler

[Maxim Davydov]

Attempt to finalize msmouse after initalization brings to segmentation
fault in QTAILQ_REMOVE.

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 chardev/msmouse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index eb9231dcdb..2cc1b16561 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -146,7 +146,9 @@ static void char_msmouse_finalize(Object *obj)
 {
     MouseChardev *mouse = MOUSE_CHARDEV(obj);
 
-    qemu_input_handler_unregister(mouse->hs);
+    if (mouse->hs) {
+        qemu_input_handler_unregister(mouse->hs);
+    }
 }
 
 static QemuInputHandler msmouse_handler = {
-- 
2.31.1

Re: [PATCH v1 4/9] msmouse: add appropriate unregister handler

[Marc-André Lureau]

On Tue, Mar 29, 2022 at 1:15 AM Maxim Davydov <maxim.davydov@openvz.org> wrote:
>
> Attempt to finalize msmouse after initalization brings to segmentation
> fault in QTAILQ_REMOVE.
>
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/msmouse.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/msmouse.c b/chardev/msmouse.c
> index eb9231dcdb..2cc1b16561 100644
> --- a/chardev/msmouse.c
> +++ b/chardev/msmouse.c
> @@ -146,7 +146,9 @@ static void char_msmouse_finalize(Object *obj)
>  {
>      MouseChardev *mouse = MOUSE_CHARDEV(obj);
>
> -    qemu_input_handler_unregister(mouse->hs);
> +    if (mouse->hs) {
> +        qemu_input_handler_unregister(mouse->hs);
> +    }
>  }
>
>  static QemuInputHandler msmouse_handler = {
> --
> 2.31.1
>

[PATCH v1 5/9] wctablet: add appropriate unregister handler

[Maxim Davydov]

Attempt to finalize msmouse after initalization brings to segmentation
fault in QTAILQ_REMOVE.

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 chardev/wctablet.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/chardev/wctablet.c b/chardev/wctablet.c
index e8b292c43c..43bdf6b608 100644
--- a/chardev/wctablet.c
+++ b/chardev/wctablet.c
@@ -319,7 +319,9 @@ static void wctablet_chr_finalize(Object *obj)
 {
     TabletChardev *tablet = WCTABLET_CHARDEV(obj);
 
-    qemu_input_handler_unregister(tablet->hs);
+    if (tablet->hs) {
+        qemu_input_handler_unregister(tablet->hs);
+    }
 }
 
 static void wctablet_chr_open(Chardev *chr,
-- 
2.31.1

Re: [PATCH v1 5/9] wctablet: add appropriate unregister handler

[Marc-André Lureau]

On Tue, Mar 29, 2022 at 1:15 AM Maxim Davydov <maxim.davydov@openvz.org> wrote:
>
> Attempt to finalize msmouse after initalization brings to segmentation
> fault in QTAILQ_REMOVE.
>
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/wctablet.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/wctablet.c b/chardev/wctablet.c
> index e8b292c43c..43bdf6b608 100644
> --- a/chardev/wctablet.c
> +++ b/chardev/wctablet.c
> @@ -319,7 +319,9 @@ static void wctablet_chr_finalize(Object *obj)
>  {
>      TabletChardev *tablet = WCTABLET_CHARDEV(obj);
>
> -    qemu_input_handler_unregister(tablet->hs);
> +    if (tablet->hs) {
> +        qemu_input_handler_unregister(tablet->hs);
> +    }
>  }
>
>  static void wctablet_chr_open(Chardev *chr,
> --
> 2.31.1
>

[PATCH v1 6/9] chardev: add appropriate getting address

[Maxim Davydov]

Attempt to get address after initialization shouldn't fail on assert in
the qapi automatically generated code. As a possible solution, it can
return null type.

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 chardev/char-socket.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index fab2d791d4..f851e3346b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -33,6 +33,7 @@
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
 #include "qemu/yank.h"
+#include "qapi/qmp/qnull.h"
 
 #include "chardev/char-io.h"
 #include "chardev/char-socket.h"
@@ -1509,6 +1510,14 @@ char_socket_get_addr(Object *obj, Visitor *v, const char *name,
 {
     SocketChardev *s = SOCKET_CHARDEV(obj);
 
+    QNull *null = NULL;
+
+    /* Return NULL type if getting addr was called after init */
+    if (!s->addr) {
+        visit_type_null(v, NULL, &null, errp);
+        return;
+    }
+
     visit_type_SocketAddress(v, name, &s->addr, errp);
 }
 
-- 
2.31.1

Re: [PATCH v1 6/9] chardev: add appropriate getting address

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> Attempt to get address after initialization shouldn't fail on assert in
> the qapi automatically generated code. As a possible solution, it can
> return null type.

But at some point this address appears? May be we try to query it too early, or we need some more initialization steps?

Isn't it better to report failure, when we try to query things that are not yet initialized?

> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   chardev/char-socket.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index fab2d791d4..f851e3346b 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -33,6 +33,7 @@
>   #include "qapi/clone-visitor.h"
>   #include "qapi/qapi-visit-sockets.h"
>   #include "qemu/yank.h"
> +#include "qapi/qmp/qnull.h"
>   
>   #include "chardev/char-io.h"
>   #include "chardev/char-socket.h"
> @@ -1509,6 +1510,14 @@ char_socket_get_addr(Object *obj, Visitor *v, const char *name,
>   {
>       SocketChardev *s = SOCKET_CHARDEV(obj);
>   
> +    QNull *null = NULL;
> +
> +    /* Return NULL type if getting addr was called after init */
> +    if (!s->addr) {
> +        visit_type_null(v, NULL, &null, errp);
> +        return;
> +    }
> +
>       visit_type_SocketAddress(v, name, &s->addr, errp);
>   }
>   


-- 
Best regards,
Vladimir

Re: [PATCH v1 6/9] chardev: add appropriate getting address

[Maxim Davydov]

On 3/30/22 14:32, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> Attempt to get address after initialization shouldn't fail on assert in
>> the qapi automatically generated code. As a possible solution, it can
>> return null type.
>
> But at some point this address appears? May be we try to query it too 
> early, or we need some more initialization steps?
For example, query address after object_new_with_class(). Without the 
patch it triggers assert(). I tried to implement the same solution as in 
hw/ppc/spapr_drc.c:prop_get_fdt
>
> Isn't it better to report failure, when we try to query things that 
> are not yet initialized?
Yes, maybe it should set errp after visit_type_null. And it should be a 
common error for unrealized devices to fix the same problem with 
MemoryRegion, etc.
>
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   chardev/char-socket.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index fab2d791d4..f851e3346b 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -33,6 +33,7 @@
>>   #include "qapi/clone-visitor.h"
>>   #include "qapi/qapi-visit-sockets.h"
>>   #include "qemu/yank.h"
>> +#include "qapi/qmp/qnull.h"
>>     #include "chardev/char-io.h"
>>   #include "chardev/char-socket.h"
>> @@ -1509,6 +1510,14 @@ char_socket_get_addr(Object *obj, Visitor *v, 
>> const char *name,
>>   {
>>       SocketChardev *s = SOCKET_CHARDEV(obj);
>>   +    QNull *null = NULL;
>> +
>> +    /* Return NULL type if getting addr was called after init */
>> +    if (!s->addr) {
>> +        visit_type_null(v, NULL, &null, errp);
>> +        return;
>> +    }
>> +
>>       visit_type_SocketAddress(v, name, &s->addr, errp);
>>   }
>
>
-- 
Best regards,
Maxim Davydov

[PATCH v1 7/9] colo-compare: safe finalization

[Maxim Davydov]

Fixes some possible issues with finalization. For example, finalization
immediately after instance_init fails on the assert.

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 net/colo-compare.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 62554b5b3c..81d8de0aaa 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -1426,7 +1426,7 @@ static void colo_compare_finalize(Object *obj)
             break;
         }
     }
-    if (QTAILQ_EMPTY(&net_compares)) {
+    if (QTAILQ_EMPTY(&net_compares) && colo_compare_active) {
         colo_compare_active = false;
         qemu_mutex_destroy(&event_mtx);
         qemu_cond_destroy(&event_complete_cond);
@@ -1442,19 +1442,26 @@ static void colo_compare_finalize(Object *obj)
 
     colo_compare_timer_del(s);
 
-    qemu_bh_delete(s->event_bh);
+    if (s->event_bh) {
+        qemu_bh_delete(s->event_bh);
+    }
 
-    AioContext *ctx = iothread_get_aio_context(s->iothread);
-    aio_context_acquire(ctx);
-    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
-    if (s->notify_dev) {
-        AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
+    if (s->iothread) {
+        AioContext *ctx = iothread_get_aio_context(s->iothread);
+        aio_context_acquire(ctx);
+        AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
+        if (s->notify_dev) {
+            AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
+        }
+        aio_context_release(ctx);
     }
-    aio_context_release(ctx);
 
     /* Release all unhandled packets after compare thead exited */
     g_queue_foreach(&s->conn_list, colo_flush_packets, s);
-    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
+    /* Without colo_compare_complete done == false without packets */
+    if (!g_queue_is_empty(&s->out_sendco.send_list)) {
+        AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
+    }
 
     g_queue_clear(&s->conn_list);
     g_queue_clear(&s->out_sendco.send_list);
-- 
2.31.1

Re: [PATCH v1 7/9] colo-compare: safe finalization

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> Fixes some possible issues with finalization. For example, finalization
> immediately after instance_init fails on the assert.
> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   net/colo-compare.c | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 62554b5b3c..81d8de0aaa 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -1426,7 +1426,7 @@ static void colo_compare_finalize(Object *obj)
>               break;
>           }
>       }
> -    if (QTAILQ_EMPTY(&net_compares)) {
> +    if (QTAILQ_EMPTY(&net_compares) && colo_compare_active) {
>           colo_compare_active = false;
>           qemu_mutex_destroy(&event_mtx);
>           qemu_cond_destroy(&event_complete_cond);
> @@ -1442,19 +1442,26 @@ static void colo_compare_finalize(Object *obj)
>   
>       colo_compare_timer_del(s);
>   
> -    qemu_bh_delete(s->event_bh);
> +    if (s->event_bh) {
> +        qemu_bh_delete(s->event_bh);
> +    }
>   
> -    AioContext *ctx = iothread_get_aio_context(s->iothread);
> -    aio_context_acquire(ctx);
> -    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> -    if (s->notify_dev) {
> -        AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
> +    if (s->iothread) {
> +        AioContext *ctx = iothread_get_aio_context(s->iothread);
> +        aio_context_acquire(ctx);
> +        AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> +        if (s->notify_dev) {
> +            AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
> +        }
> +        aio_context_release(ctx);
>       }
> -    aio_context_release(ctx);
>   
>       /* Release all unhandled packets after compare thead exited */
>       g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> -    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> +    /* Without colo_compare_complete done == false without packets */
> +    if (!g_queue_is_empty(&s->out_sendco.send_list)) {
> +        AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> +    }

I think, would be good to add more description for this last change. It's not as obvious as previous two changes.

>   
>       g_queue_clear(&s->conn_list);
>       g_queue_clear(&s->out_sendco.send_list);


-- 
Best regards,
Vladimir

Re: [PATCH v1 7/9] colo-compare: safe finalization

[Maxim Davydov]

The main problem that if we call object_new_with_class() and then 
object_unref(), it fails. First of all, this is due to the fact that 
finalize expects that net/colo-compare.c:colo_compare_complete() has 
been called before.

On 3/30/22 17:54, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> Fixes some possible issues with finalization. For example, finalization
>> immediately after instance_init fails on the assert.
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   net/colo-compare.c | 25 ++++++++++++++++---------
>>   1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index 62554b5b3c..81d8de0aaa 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -1426,7 +1426,7 @@ static void colo_compare_finalize(Object *obj)
>>               break;
>>           }
>>       }
>> -    if (QTAILQ_EMPTY(&net_compares)) {
if colo_compare_active == false, event_mtx and event_complete_cond 
didn't inited in colo_compare_complete()
>> +    if (QTAILQ_EMPTY(&net_compares) && colo_compare_active) {
>>           colo_compare_active = false;
>>           qemu_mutex_destroy(&event_mtx);
>>           qemu_cond_destroy(&event_complete_cond);
>> @@ -1442,19 +1442,26 @@ static void colo_compare_finalize(Object *obj)
>>         colo_compare_timer_del(s);
>>   -    qemu_bh_delete(s->event_bh);
s->event_bh wasn't allocated in colo_compare_iothread() in 
colo_compare_complete()
>> +    if (s->event_bh) {
>> +        qemu_bh_delete(s->event_bh);
>> +    }
>>   -    AioContext *ctx = iothread_get_aio_context(s->iothread);
>> -    aio_context_acquire(ctx);
>> -    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
>> -    if (s->notify_dev) {
>> -        AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
s->iothread == NULL after .instance_init (it can be detected in 
colo_compare_complete(), if it has been called)
>> +    if (s->iothread) {
>> +        AioContext *ctx = iothread_get_aio_context(s->iothread);
>> +        aio_context_acquire(ctx);
>> +        AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
>> +        if (s->notify_dev) {
>> +            AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
>> +        }
>> +        aio_context_release(ctx);
>>       }
>> -    aio_context_release(ctx);
>>         /* Release all unhandled packets after compare thead exited */
>>       g_queue_foreach(&s->conn_list, colo_flush_packets, s);
>> -    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
In normal situation, it flushes all packets and sets s->out_sendco.done 
= true via compare_chr_send (we wait this event). But s->conn_list isn't 
initialized, s->out_sendco.done == false and won't become true. So, it's 
infinite waiting.
>> +    /* Without colo_compare_complete done == false without packets */
>> +    if (!g_queue_is_empty(&s->out_sendco.send_list)) {
>> +        AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
>> +    }
>
> I think, would be good to add more description for this last change. 
> It's not as obvious as previous two changes.
>
>> g_queue_clear(&s->conn_list);
>>       g_queue_clear(&s->out_sendco.send_list);
>
>
-- 
Best regards,
Maxim Davydov

[PATCH v1 8/9] qom: add command to print initial properties

[Maxim Davydov]

The command "query-init-properties" is needed to get values of properties
after initialization (not only default value). It makes sense, for example,
when working with x86_64-cpu.
All machine types (and x-remote-object, because its init uses machime
type's infrastructure) should be skipped, because only the one instance can
be correctly initialized.

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 qapi/qom.json      |  69 ++++++++++++++++++++++++++
 qom/qom-qmp-cmds.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 190 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index eeb5395ff3..1eedc441eb 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -949,3 +949,72 @@
 ##
 { 'command': 'object-del', 'data': {'id': 'str'},
   'allow-preconfig': true }
+
+##
+# @InitValue:
+#
+# Not all objects have default values but they have "initial" values.
+#
+# @name: property name
+#
+# @value: Current value (default or after initialization. It makes sence,
+#         for example, for x86-cpus)
+#
+# Since: 7.0
+#
+##
+{ 'struct': 'InitValue',
+  'data': { 'name': 'str',
+            '*value': 'any' } }
+
+##
+# @ClassProperties:
+#
+# Initial values of properties that are owned by the class
+#
+# @classname: name of the class that owns appropriate properties
+#
+# @classprops: List of class properties
+#
+# Since: 7.0
+#
+##
+{ 'struct': 'ClassProperties',
+  'data': { 'classname': 'str',
+            '*classprops': [ 'InitValue' ] } }
+
+##
+# @InitProps:
+#
+# List of properties and their values that are available after class
+# initialization. So it important to know default value of the property
+# even if it doesn't have "QObject *defval"
+#
+# @name: Object name
+#
+# @props: List of properties
+#
+# Notes: a value in each property was defval if it's available
+#        otherwise it's obtained via "(ObjectPropertyAccessor*) get"
+#        immediately after initialization of device object.
+#
+# Since: 7.0
+#
+##
+{ 'struct': 'InitProps',
+  'data': { 'name': 'str',
+            'props': [ 'ClassProperties' ] } }
+
+##
+# @query-init-properties:
+#
+# Returns list of all objects (except all types related with machine type)
+# with all properties and their "default" values that  will be available
+# after initialization. The main purpose of this command is to be used to
+# build table with all machine-type-specific properties
+#
+# Since: 7.0
+#
+##
+{ 'command': 'query-init-properties',
+  'returns': [ 'InitProps' ] }
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 2d6f41ecc7..c1bb3f1f8b 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -27,6 +27,7 @@
 #include "qemu/cutils.h"
 #include "qom/object_interfaces.h"
 #include "qom/qom-qobject.h"
+#include "hw/boards.h"
 
 ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
 {
@@ -235,3 +236,123 @@ void qmp_object_del(const char *id, Error **errp)
 {
     user_creatable_del(id, errp);
 }
+
+static void query_object_prop(InitValueList **props_list, ObjectProperty *prop,
+                              Object *obj, Error **errp)
+{
+    InitValue *prop_info = NULL;
+
+    /* Skip inconsiderable properties */
+    if (strcmp(prop->name, "type") == 0 ||
+        strcmp(prop->name, "realized") == 0 ||
+        strcmp(prop->name, "hotpluggable") == 0 ||
+        strcmp(prop->name, "hotplugged") == 0 ||
+        strcmp(prop->name, "parent_bus") == 0) {
+        return;
+    }
+
+    prop_info = g_malloc0(sizeof(*prop_info));
+    prop_info->name = g_strdup(prop->name);
+    prop_info->value = NULL;
+    if (prop->defval) {
+        prop_info->value = qobject_ref(prop->defval);
+    } else if (prop->get) {
+        /*
+         * crash-information in x86-cpu uses errp to return current state.
+         * So, after requesting this property it returns  GenericError:
+         * "No crash occured"
+         */
+        if (strcmp(prop->name, "crash-information") != 0) {
+            prop_info->value = object_property_get_qobject(obj, prop->name,
+                                                           errp);
+        }
+    }
+    prop_info->has_value = !!prop_info->value;
+
+    QAPI_LIST_PREPEND(*props_list, prop_info);
+}
+
+typedef struct QIPData {
+    InitPropsList **dev_list;
+    Error **errp;
+} QIPData;
+
+static void query_init_properties_tramp(gpointer list_data, gpointer opaque)
+{
+    ObjectClass *k = list_data;
+    Object *obj;
+    ObjectClass *parent;
+    GHashTableIter iter;
+
+    QIPData *data = opaque;
+    ClassPropertiesList *class_props_list = NULL;
+    InitProps *dev_info;
+
+    /* Only one machine can be initialized correctly (it's already happened) */
+    if (object_class_dynamic_cast(k, TYPE_MACHINE)) {
+        return;
+    }
+
+    const char *klass_name = object_class_get_name(k);
+    /*
+     * Uses machine type infrastructure with notifiers. It causes immediate
+     * notify and SEGSEGV during remote_object_machine_done
+     */
+    if (strcmp(klass_name, "x-remote-object") == 0) {
+        return;
+    }
+
+    dev_info = g_malloc0(sizeof(*dev_info));
+    dev_info->name = g_strdup(klass_name);
+
+    obj = object_new_with_class(k);
+
+    /*
+     * Part of ObjectPropertyIterator infrastructure, but we need more precise
+     * control of current class to dump appropriate features
+     * This part was taken out from loop because first initialization differ
+     * from other reinitializations
+     */
+    parent = object_get_class(obj);
+    g_hash_table_iter_init(&iter, obj->properties);
+    const char *prop_owner_name = object_get_typename(obj);
+    do {
+        InitValueList *prop_list = NULL;
+        ClassProperties *class_data;
+
+        gpointer key, val;
+        while (g_hash_table_iter_next(&iter, &key, &val)) {
+            query_object_prop(&prop_list, (ObjectProperty *)val, obj,
+                              data->errp);
+        }
+        class_data = g_malloc0(sizeof(*class_data));
+        class_data->classname = g_strdup(prop_owner_name);
+        class_data->classprops = prop_list;
+        class_data->has_classprops = !!prop_list;
+
+        QAPI_LIST_PREPEND(class_props_list, class_data);
+
+        if (!parent) {
+            break;
+        }
+        g_hash_table_iter_init(&iter, parent->properties);
+        prop_owner_name = object_class_get_name(parent);
+        parent = object_class_get_parent(parent);
+    } while (true);
+    dev_info->props = class_props_list;
+    object_unref(OBJECT(obj));
+
+    QAPI_LIST_PREPEND(*(data->dev_list), dev_info);
+}
+
+InitPropsList *qmp_query_init_properties(Error **errp)
+{
+    GSList *typename_list = object_class_get_list(TYPE_OBJECT, false);
+
+    InitPropsList *dev_list = NULL;
+    QIPData data = { &dev_list, errp };
+    g_slist_foreach(typename_list, query_init_properties_tramp, &data);
+    g_slist_free(typename_list);
+
+    return dev_list;
+}
-- 
2.31.1

Re: [PATCH v1 8/9] qom: add command to print initial properties

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> The command "query-init-properties" is needed to get values of properties
> after initialization (not only default value). It makes sense, for example,
> when working with x86_64-cpu.
> All machine types (and x-remote-object, because its init uses machime
> type's infrastructure) should be skipped, because only the one instance can
> be correctly initialized.
> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   qapi/qom.json      |  69 ++++++++++++++++++++++++++
>   qom/qom-qmp-cmds.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 190 insertions(+)
> 
> diff --git a/qapi/qom.json b/qapi/qom.json
> index eeb5395ff3..1eedc441eb 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -949,3 +949,72 @@
>   ##
>   { 'command': 'object-del', 'data': {'id': 'str'},
>     'allow-preconfig': true }
> +
> +##
> +# @InitValue:
> +#
> +# Not all objects have default values but they have "initial" values.
> +#
> +# @name: property name
> +#
> +# @value: Current value (default or after initialization. It makes sence,
> +#         for example, for x86-cpus)
> +#
> +# Since: 7.0

7.1 (here and below)

> +#
> +##
> +{ 'struct': 'InitValue',
> +  'data': { 'name': 'str',
> +            '*value': 'any' } }
> +

[..]

> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 2d6f41ecc7..c1bb3f1f8b 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -27,6 +27,7 @@
>   #include "qemu/cutils.h"
>   #include "qom/object_interfaces.h"
>   #include "qom/qom-qobject.h"
> +#include "hw/boards.h"
>   
>   ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>   {
> @@ -235,3 +236,123 @@ void qmp_object_del(const char *id, Error **errp)
>   {
>       user_creatable_del(id, errp);
>   }
> +
> +static void query_object_prop(InitValueList **props_list, ObjectProperty *prop,
> +                              Object *obj, Error **errp)
> +{
> +    InitValue *prop_info = NULL;
> +
> +    /* Skip inconsiderable properties */
> +    if (strcmp(prop->name, "type") == 0 ||
> +        strcmp(prop->name, "realized") == 0 ||
> +        strcmp(prop->name, "hotpluggable") == 0 ||
> +        strcmp(prop->name, "hotplugged") == 0 ||
> +        strcmp(prop->name, "parent_bus") == 0) {
> +        return;
> +    }
> +
> +    prop_info = g_malloc0(sizeof(*prop_info));
> +    prop_info->name = g_strdup(prop->name);
> +    prop_info->value = NULL;
> +    if (prop->defval) {
> +        prop_info->value = qobject_ref(prop->defval);
> +    } else if (prop->get) {
> +        /*
> +         * crash-information in x86-cpu uses errp to return current state.
> +         * So, after requesting this property it returns  GenericError:
> +         * "No crash occured"
> +         */
> +        if (strcmp(prop->name, "crash-information") != 0) {
> +            prop_info->value = object_property_get_qobject(obj, prop->name,
> +                                                           errp);
> +        }
> +    }

Hmmm. Should we instead call prop->get() when is is available, and only if not use prep->defval?

> +    prop_info->has_value = !!prop_info->value;
> +
> +    QAPI_LIST_PREPEND(*props_list, prop_info);
> +}
> +
> +typedef struct QIPData {
> +    InitPropsList **dev_list;
> +    Error **errp;
> +} QIPData;
> +
> +static void query_init_properties_tramp(gpointer list_data, gpointer opaque)
> +{
> +    ObjectClass *k = list_data;
> +    Object *obj;
> +    ObjectClass *parent;
> +    GHashTableIter iter;
> +
> +    QIPData *data = opaque;
> +    ClassPropertiesList *class_props_list = NULL;
> +    InitProps *dev_info;
> +
> +    /* Only one machine can be initialized correctly (it's already happened) */
> +    if (object_class_dynamic_cast(k, TYPE_MACHINE)) {
> +        return;
> +    }
> +
> +    const char *klass_name = object_class_get_name(k);
> +    /*
> +     * Uses machine type infrastructure with notifiers. It causes immediate
> +     * notify and SEGSEGV during remote_object_machine_done
> +     */
> +    if (strcmp(klass_name, "x-remote-object") == 0) {
> +        return;
> +    }
> +
> +    dev_info = g_malloc0(sizeof(*dev_info));
> +    dev_info->name = g_strdup(klass_name);
> +
> +    obj = object_new_with_class(k);
> +
> +    /*
> +     * Part of ObjectPropertyIterator infrastructure, but we need more precise
> +     * control of current class to dump appropriate features
> +     * This part was taken out from loop because first initialization differ
> +     * from other reinitializations
> +     */
> +    parent = object_get_class(obj);

hmm.. obj = object_new_with_class(k); parent = object_get_class(obj);.. Looks for me like parent should be equal to k. Or object_ API is rather unobvious.

> +    g_hash_table_iter_init(&iter, obj->properties);
> +    const char *prop_owner_name = object_get_typename(obj);
> +    do {
> +        InitValueList *prop_list = NULL;
> +        ClassProperties *class_data;
> +
> +        gpointer key, val;
> +        while (g_hash_table_iter_next(&iter, &key, &val)) {
> +            query_object_prop(&prop_list, (ObjectProperty *)val, obj,
> +                              data->errp);
> +        }
> +        class_data = g_malloc0(sizeof(*class_data));
> +        class_data->classname = g_strdup(prop_owner_name);
> +        class_data->classprops = prop_list;
> +        class_data->has_classprops = !!prop_list;
> +
> +        QAPI_LIST_PREPEND(class_props_list, class_data);
> +
> +        if (!parent) {
> +            break;
> +        }
> +        g_hash_table_iter_init(&iter, parent->properties);
> +        prop_owner_name = object_class_get_name(parent);
> +        parent = object_class_get_parent(parent);
> +    } while (true);
> +    dev_info->props = class_props_list;
> +    object_unref(OBJECT(obj));
> +
> +    QAPI_LIST_PREPEND(*(data->dev_list), dev_info);
> +}
> +
> +InitPropsList *qmp_query_init_properties(Error **errp)
> +{
> +    GSList *typename_list = object_class_get_list(TYPE_OBJECT, false);
> +
> +    InitPropsList *dev_list = NULL;
> +    QIPData data = { &dev_list, errp };
> +    g_slist_foreach(typename_list, query_init_properties_tramp, &data);
> +    g_slist_free(typename_list);
> +
> +    return dev_list;
> +}


-- 
Best regards,
Vladimir

Re: [PATCH v1 8/9] qom: add command to print initial properties

[Maxim Davydov]

On 3/30/22 18:17, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> The command "query-init-properties" is needed to get values of 
>> properties
>> after initialization (not only default value). It makes sense, for 
>> example,
>> when working with x86_64-cpu.
>> All machine types (and x-remote-object, because its init uses machime
>> type's infrastructure) should be skipped, because only the one 
>> instance can
>> be correctly initialized.
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   qapi/qom.json      |  69 ++++++++++++++++++++++++++
>>   qom/qom-qmp-cmds.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 190 insertions(+)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index eeb5395ff3..1eedc441eb 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -949,3 +949,72 @@
>>   ##
>>   { 'command': 'object-del', 'data': {'id': 'str'},
>>     'allow-preconfig': true }
>> +
>> +##
>> +# @InitValue:
>> +#
>> +# Not all objects have default values but they have "initial" values.
>> +#
>> +# @name: property name
>> +#
>> +# @value: Current value (default or after initialization. It makes 
>> sence,
>> +#         for example, for x86-cpus)
>> +#
>> +# Since: 7.0
>
> 7.1 (here and below)
>
>> +#
>> +##
>> +{ 'struct': 'InitValue',
>> +  'data': { 'name': 'str',
>> +            '*value': 'any' } }
>> +
>
> [..]
>
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index 2d6f41ecc7..c1bb3f1f8b 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -27,6 +27,7 @@
>>   #include "qemu/cutils.h"
>>   #include "qom/object_interfaces.h"
>>   #include "qom/qom-qobject.h"
>> +#include "hw/boards.h"
>>     ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>>   {
>> @@ -235,3 +236,123 @@ void qmp_object_del(const char *id, Error **errp)
>>   {
>>       user_creatable_del(id, errp);
>>   }
>> +
>> +static void query_object_prop(InitValueList **props_list, 
>> ObjectProperty *prop,
>> +                              Object *obj, Error **errp)
>> +{
>> +    InitValue *prop_info = NULL;
>> +
>> +    /* Skip inconsiderable properties */
>> +    if (strcmp(prop->name, "type") == 0 ||
>> +        strcmp(prop->name, "realized") == 0 ||
>> +        strcmp(prop->name, "hotpluggable") == 0 ||
>> +        strcmp(prop->name, "hotplugged") == 0 ||
>> +        strcmp(prop->name, "parent_bus") == 0) {
>> +        return;
>> +    }
>> +
>> +    prop_info = g_malloc0(sizeof(*prop_info));
>> +    prop_info->name = g_strdup(prop->name);
>> +    prop_info->value = NULL;
>> +    if (prop->defval) {
>> +        prop_info->value = qobject_ref(prop->defval);
>> +    } else if (prop->get) {
>> +        /*
>> +         * crash-information in x86-cpu uses errp to return current 
>> state.
>> +         * So, after requesting this property it returns GenericError:
>> +         * "No crash occured"
>> +         */
>> +        if (strcmp(prop->name, "crash-information") != 0) {
>> +            prop_info->value = object_property_get_qobject(obj, 
>> prop->name,
>> + errp);
>> +        }
>> +    }
>
> Hmmm. Should we instead call prop->get() when is is available, and 
> only if not use prep->defval?
default properties more rare and sometimes can give more information (if 
the device developer thought that there should be a default value). And 
I think that if prop->get() isn't available, prop->defval() isn't too.
>
>> +    prop_info->has_value = !!prop_info->value;
>> +
>> +    QAPI_LIST_PREPEND(*props_list, prop_info);
>> +}
>> +
>> +typedef struct QIPData {
>> +    InitPropsList **dev_list;
>> +    Error **errp;
>> +} QIPData;
>> +
>> +static void query_init_properties_tramp(gpointer list_data, gpointer 
>> opaque)
>> +{
>> +    ObjectClass *k = list_data;
>> +    Object *obj;
>> +    ObjectClass *parent;
>> +    GHashTableIter iter;
>> +
>> +    QIPData *data = opaque;
>> +    ClassPropertiesList *class_props_list = NULL;
>> +    InitProps *dev_info;
>> +
>> +    /* Only one machine can be initialized correctly (it's already 
>> happened) */
>> +    if (object_class_dynamic_cast(k, TYPE_MACHINE)) {
>> +        return;
>> +    }
>> +
>> +    const char *klass_name = object_class_get_name(k);
>> +    /*
>> +     * Uses machine type infrastructure with notifiers. It causes 
>> immediate
>> +     * notify and SEGSEGV during remote_object_machine_done
>> +     */
>> +    if (strcmp(klass_name, "x-remote-object") == 0) {
>> +        return;
>> +    }
>> +
>> +    dev_info = g_malloc0(sizeof(*dev_info));
>> +    dev_info->name = g_strdup(klass_name);
>> +
>> +    obj = object_new_with_class(k);
>> +
>> +    /*
>> +     * Part of ObjectPropertyIterator infrastructure, but we need 
>> more precise
>> +     * control of current class to dump appropriate features
>> +     * This part was taken out from loop because first 
>> initialization differ
>> +     * from other reinitializations
>> +     */
>> +    parent = object_get_class(obj);
>
> hmm.. obj = object_new_with_class(k); parent = 
> object_get_class(obj);.. Looks for me like parent should be equal to 
> k. Or object_ API is rather unobvious.
I'll change it)
>
>> +    g_hash_table_iter_init(&iter, obj->properties);
>> +    const char *prop_owner_name = object_get_typename(obj);
>> +    do {
>> +        InitValueList *prop_list = NULL;
>> +        ClassProperties *class_data;
>> +
>> +        gpointer key, val;
>> +        while (g_hash_table_iter_next(&iter, &key, &val)) {
>> +            query_object_prop(&prop_list, (ObjectProperty *)val, obj,
>> +                              data->errp);
>> +        }
>> +        class_data = g_malloc0(sizeof(*class_data));
>> +        class_data->classname = g_strdup(prop_owner_name);
>> +        class_data->classprops = prop_list;
>> +        class_data->has_classprops = !!prop_list;
>> +
>> +        QAPI_LIST_PREPEND(class_props_list, class_data);
>> +
>> +        if (!parent) {
>> +            break;
>> +        }
>> +        g_hash_table_iter_init(&iter, parent->properties);
>> +        prop_owner_name = object_class_get_name(parent);
>> +        parent = object_class_get_parent(parent);
>> +    } while (true);
>> +    dev_info->props = class_props_list;
>> +    object_unref(OBJECT(obj));
>> +
>> +    QAPI_LIST_PREPEND(*(data->dev_list), dev_info);
>> +}
>> +
>> +InitPropsList *qmp_query_init_properties(Error **errp)
>> +{
>> +    GSList *typename_list = object_class_get_list(TYPE_OBJECT, false);
>> +
>> +    InitPropsList *dev_list = NULL;
>> +    QIPData data = { &dev_list, errp };
>> +    g_slist_foreach(typename_list, query_init_properties_tramp, &data);
>> +    g_slist_free(typename_list);
>> +
>> +    return dev_list;
>> +}
>
>
-- 
Best regards,
Maxim Davydov

Re: [PATCH v1 8/9] qom: add command to print initial properties

[Igor Mammedov]

On Tue, 29 Mar 2022 00:15:38 +0300
Maxim Davydov <maxim.davydov@openvz.org> wrote:

> The command "query-init-properties" is needed to get values of properties
> after initialization (not only default value). It makes sense, for example,
> when working with x86_64-cpu.
> All machine types (and x-remote-object, because its init uses machime
> type's infrastructure) should be skipped, because only the one instance can
> be correctly initialized.

It might be obvious to you but I couldn't parse above commit message at all.
Pls rephrase and explain in more detail what you are trying to achieve.

> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>  qapi/qom.json      |  69 ++++++++++++++++++++++++++
>  qom/qom-qmp-cmds.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 190 insertions(+)
> 
> diff --git a/qapi/qom.json b/qapi/qom.json
> index eeb5395ff3..1eedc441eb 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -949,3 +949,72 @@
>  ##
>  { 'command': 'object-del', 'data': {'id': 'str'},
>    'allow-preconfig': true }
> +
> +##
> +# @InitValue:
> +#
> +# Not all objects have default values but they have "initial" values.
> +#
> +# @name: property name
> +#
> +# @value: Current value (default or after initialization. It makes sence,
> +#         for example, for x86-cpus)
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'struct': 'InitValue',
> +  'data': { 'name': 'str',
> +            '*value': 'any' } }
> +
> +##
> +# @ClassProperties:
> +#
> +# Initial values of properties that are owned by the class
> +#
> +# @classname: name of the class that owns appropriate properties
> +#
> +# @classprops: List of class properties
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'struct': 'ClassProperties',
> +  'data': { 'classname': 'str',
> +            '*classprops': [ 'InitValue' ] } }
> +
> +##
> +# @InitProps:
> +#
> +# List of properties and their values that are available after class
> +# initialization. So it important to know default value of the property
> +# even if it doesn't have "QObject *defval"
> +#
> +# @name: Object name
> +#
> +# @props: List of properties
> +#
> +# Notes: a value in each property was defval if it's available
> +#        otherwise it's obtained via "(ObjectPropertyAccessor*) get"
> +#        immediately after initialization of device object.
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'struct': 'InitProps',
> +  'data': { 'name': 'str',
> +            'props': [ 'ClassProperties' ] } }
> +
> +##
> +# @query-init-properties:
> +#
> +# Returns list of all objects (except all types related with machine type)
> +# with all properties and their "default" values that  will be available
> +# after initialization. The main purpose of this command is to be used to
> +# build table with all machine-type-specific properties
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'command': 'query-init-properties',
> +  'returns': [ 'InitProps' ] }
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 2d6f41ecc7..c1bb3f1f8b 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -27,6 +27,7 @@
>  #include "qemu/cutils.h"
>  #include "qom/object_interfaces.h"
>  #include "qom/qom-qobject.h"
> +#include "hw/boards.h"
>  
>  ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>  {
> @@ -235,3 +236,123 @@ void qmp_object_del(const char *id, Error **errp)
>  {
>      user_creatable_del(id, errp);
>  }
> +
> +static void query_object_prop(InitValueList **props_list, ObjectProperty *prop,
> +                              Object *obj, Error **errp)
> +{
> +    InitValue *prop_info = NULL;
> +
> +    /* Skip inconsiderable properties */
> +    if (strcmp(prop->name, "type") == 0 ||
> +        strcmp(prop->name, "realized") == 0 ||
> +        strcmp(prop->name, "hotpluggable") == 0 ||
> +        strcmp(prop->name, "hotplugged") == 0 ||
> +        strcmp(prop->name, "parent_bus") == 0) {
> +        return;
> +    }
> +
> +    prop_info = g_malloc0(sizeof(*prop_info));
> +    prop_info->name = g_strdup(prop->name);
> +    prop_info->value = NULL;
> +    if (prop->defval) {
> +        prop_info->value = qobject_ref(prop->defval);
> +    } else if (prop->get) {
> +        /*
> +         * crash-information in x86-cpu uses errp to return current state.
> +         * So, after requesting this property it returns  GenericError:
> +         * "No crash occured"
> +         */
> +        if (strcmp(prop->name, "crash-information") != 0) {
> +            prop_info->value = object_property_get_qobject(obj, prop->name,
> +                                                           errp);
> +        }
> +    }
> +    prop_info->has_value = !!prop_info->value;
> +
> +    QAPI_LIST_PREPEND(*props_list, prop_info);
> +}
> +
> +typedef struct QIPData {
> +    InitPropsList **dev_list;
> +    Error **errp;
> +} QIPData;
> +
> +static void query_init_properties_tramp(gpointer list_data, gpointer opaque)
> +{
> +    ObjectClass *k = list_data;
> +    Object *obj;
> +    ObjectClass *parent;
> +    GHashTableIter iter;
> +
> +    QIPData *data = opaque;
> +    ClassPropertiesList *class_props_list = NULL;
> +    InitProps *dev_info;
> +
> +    /* Only one machine can be initialized correctly (it's already happened) */
> +    if (object_class_dynamic_cast(k, TYPE_MACHINE)) {
> +        return;
> +    }
> +
> +    const char *klass_name = object_class_get_name(k);
> +    /*
> +     * Uses machine type infrastructure with notifiers. It causes immediate
> +     * notify and SEGSEGV during remote_object_machine_done
> +     */
> +    if (strcmp(klass_name, "x-remote-object") == 0) {
> +        return;
> +    }
> +
> +    dev_info = g_malloc0(sizeof(*dev_info));
> +    dev_info->name = g_strdup(klass_name);
> +
> +    obj = object_new_with_class(k);
> +
> +    /*
> +     * Part of ObjectPropertyIterator infrastructure, but we need more precise
> +     * control of current class to dump appropriate features
> +     * This part was taken out from loop because first initialization differ
> +     * from other reinitializations
> +     */
> +    parent = object_get_class(obj);
> +    g_hash_table_iter_init(&iter, obj->properties);
> +    const char *prop_owner_name = object_get_typename(obj);
> +    do {
> +        InitValueList *prop_list = NULL;
> +        ClassProperties *class_data;
> +
> +        gpointer key, val;
> +        while (g_hash_table_iter_next(&iter, &key, &val)) {
> +            query_object_prop(&prop_list, (ObjectProperty *)val, obj,
> +                              data->errp);
> +        }
> +        class_data = g_malloc0(sizeof(*class_data));
> +        class_data->classname = g_strdup(prop_owner_name);
> +        class_data->classprops = prop_list;
> +        class_data->has_classprops = !!prop_list;
> +
> +        QAPI_LIST_PREPEND(class_props_list, class_data);
> +
> +        if (!parent) {
> +            break;
> +        }
> +        g_hash_table_iter_init(&iter, parent->properties);
> +        prop_owner_name = object_class_get_name(parent);
> +        parent = object_class_get_parent(parent);
> +    } while (true);
> +    dev_info->props = class_props_list;
> +    object_unref(OBJECT(obj));
> +
> +    QAPI_LIST_PREPEND(*(data->dev_list), dev_info);
> +}
> +
> +InitPropsList *qmp_query_init_properties(Error **errp)
> +{
> +    GSList *typename_list = object_class_get_list(TYPE_OBJECT, false);
> +
> +    InitPropsList *dev_list = NULL;
> +    QIPData data = { &dev_list, errp };
> +    g_slist_foreach(typename_list, query_init_properties_tramp, &data);
> +    g_slist_free(typename_list);
> +
> +    return dev_list;
> +}

Re: [PATCH v1 8/9] qom: add command to print initial properties

[Maxim Davydov]

On 3/31/22 14:55, Igor Mammedov wrote:
> On Tue, 29 Mar 2022 00:15:38 +0300
> Maxim Davydov <maxim.davydov@openvz.org> wrote:
>
>> The command "query-init-properties" is needed to get values of properties
>> after initialization (not only default value). It makes sense, for example,
>> when working with x86_64-cpu.
>> All machine types (and x-remote-object, because its init uses machime
>> type's infrastructure) should be skipped, because only the one instance can
>> be correctly initialized.
> It might be obvious to you but I couldn't parse above commit message at all.
> Pls rephrase and explain in more detail what you are trying to achieve.
I want to dump all "default" object properties to compare it with 
compat_props of MachineState. It means that I need values from 
ObjectProperty even it doesn't have default value. For many devices it 
can give useless information. But, for example, x86_64-cpu sets "real" 
default values for specific model only during initialization. 
x86_cpu_properties[] can't give information about kvm default features.
>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   qapi/qom.json      |  69 ++++++++++++++++++++++++++
>>   qom/qom-qmp-cmds.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 190 insertions(+)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index eeb5395ff3..1eedc441eb 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -949,3 +949,72 @@
>>   ##
>>   { 'command': 'object-del', 'data': {'id': 'str'},
>>     'allow-preconfig': true }
>> +
>> +##
>> +# @InitValue:
>> +#
>> +# Not all objects have default values but they have "initial" values.
>> +#
>> +# @name: property name
>> +#
>> +# @value: Current value (default or after initialization. It makes sence,
>> +#         for example, for x86-cpus)
>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'struct': 'InitValue',
>> +  'data': { 'name': 'str',
>> +            '*value': 'any' } }
>> +
>> +##
>> +# @ClassProperties:
>> +#
>> +# Initial values of properties that are owned by the class
>> +#
>> +# @classname: name of the class that owns appropriate properties
>> +#
>> +# @classprops: List of class properties
>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'struct': 'ClassProperties',
>> +  'data': { 'classname': 'str',
>> +            '*classprops': [ 'InitValue' ] } }
>> +
>> +##
>> +# @InitProps:
>> +#
>> +# List of properties and their values that are available after class
>> +# initialization. So it important to know default value of the property
>> +# even if it doesn't have "QObject *defval"
>> +#
>> +# @name: Object name
>> +#
>> +# @props: List of properties
>> +#
>> +# Notes: a value in each property was defval if it's available
>> +#        otherwise it's obtained via "(ObjectPropertyAccessor*) get"
>> +#        immediately after initialization of device object.
>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'struct': 'InitProps',
>> +  'data': { 'name': 'str',
>> +            'props': [ 'ClassProperties' ] } }
>> +
>> +##
>> +# @query-init-properties:
>> +#
>> +# Returns list of all objects (except all types related with machine type)
>> +# with all properties and their "default" values that  will be available
>> +# after initialization. The main purpose of this command is to be used to
>> +# build table with all machine-type-specific properties
>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'command': 'query-init-properties',
>> +  'returns': [ 'InitProps' ] }
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index 2d6f41ecc7..c1bb3f1f8b 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -27,6 +27,7 @@
>>   #include "qemu/cutils.h"
>>   #include "qom/object_interfaces.h"
>>   #include "qom/qom-qobject.h"
>> +#include "hw/boards.h"
>>   
>>   ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>>   {
>> @@ -235,3 +236,123 @@ void qmp_object_del(const char *id, Error **errp)
>>   {
>>       user_creatable_del(id, errp);
>>   }
>> +
>> +static void query_object_prop(InitValueList **props_list, ObjectProperty *prop,
>> +                              Object *obj, Error **errp)
>> +{
>> +    InitValue *prop_info = NULL;
>> +
>> +    /* Skip inconsiderable properties */
>> +    if (strcmp(prop->name, "type") == 0 ||
>> +        strcmp(prop->name, "realized") == 0 ||
>> +        strcmp(prop->name, "hotpluggable") == 0 ||
>> +        strcmp(prop->name, "hotplugged") == 0 ||
>> +        strcmp(prop->name, "parent_bus") == 0) {
>> +        return;
>> +    }
>> +
>> +    prop_info = g_malloc0(sizeof(*prop_info));
>> +    prop_info->name = g_strdup(prop->name);
>> +    prop_info->value = NULL;
>> +    if (prop->defval) {
>> +        prop_info->value = qobject_ref(prop->defval);
>> +    } else if (prop->get) {
>> +        /*
>> +         * crash-information in x86-cpu uses errp to return current state.
>> +         * So, after requesting this property it returns  GenericError:
>> +         * "No crash occured"
>> +         */
>> +        if (strcmp(prop->name, "crash-information") != 0) {
>> +            prop_info->value = object_property_get_qobject(obj, prop->name,
>> +                                                           errp);
>> +        }
>> +    }
>> +    prop_info->has_value = !!prop_info->value;
>> +
>> +    QAPI_LIST_PREPEND(*props_list, prop_info);
>> +}
>> +
>> +typedef struct QIPData {
>> +    InitPropsList **dev_list;
>> +    Error **errp;
>> +} QIPData;
>> +
>> +static void query_init_properties_tramp(gpointer list_data, gpointer opaque)
>> +{
>> +    ObjectClass *k = list_data;
>> +    Object *obj;
>> +    ObjectClass *parent;
>> +    GHashTableIter iter;
>> +
>> +    QIPData *data = opaque;
>> +    ClassPropertiesList *class_props_list = NULL;
>> +    InitProps *dev_info;
>> +
>> +    /* Only one machine can be initialized correctly (it's already happened) */
>> +    if (object_class_dynamic_cast(k, TYPE_MACHINE)) {
>> +        return;
>> +    }
>> +
>> +    const char *klass_name = object_class_get_name(k);
>> +    /*
>> +     * Uses machine type infrastructure with notifiers. It causes immediate
>> +     * notify and SEGSEGV during remote_object_machine_done
>> +     */
>> +    if (strcmp(klass_name, "x-remote-object") == 0) {
>> +        return;
>> +    }
>> +
>> +    dev_info = g_malloc0(sizeof(*dev_info));
>> +    dev_info->name = g_strdup(klass_name);
>> +
>> +    obj = object_new_with_class(k);
>> +
>> +    /*
>> +     * Part of ObjectPropertyIterator infrastructure, but we need more precise
>> +     * control of current class to dump appropriate features
>> +     * This part was taken out from loop because first initialization differ
>> +     * from other reinitializations
>> +     */
>> +    parent = object_get_class(obj);
>> +    g_hash_table_iter_init(&iter, obj->properties);
>> +    const char *prop_owner_name = object_get_typename(obj);
>> +    do {
>> +        InitValueList *prop_list = NULL;
>> +        ClassProperties *class_data;
>> +
>> +        gpointer key, val;
>> +        while (g_hash_table_iter_next(&iter, &key, &val)) {
>> +            query_object_prop(&prop_list, (ObjectProperty *)val, obj,
>> +                              data->errp);
>> +        }
>> +        class_data = g_malloc0(sizeof(*class_data));
>> +        class_data->classname = g_strdup(prop_owner_name);
>> +        class_data->classprops = prop_list;
>> +        class_data->has_classprops = !!prop_list;
>> +
>> +        QAPI_LIST_PREPEND(class_props_list, class_data);
>> +
>> +        if (!parent) {
>> +            break;
>> +        }
>> +        g_hash_table_iter_init(&iter, parent->properties);
>> +        prop_owner_name = object_class_get_name(parent);
>> +        parent = object_class_get_parent(parent);
>> +    } while (true);
>> +    dev_info->props = class_props_list;
>> +    object_unref(OBJECT(obj));
>> +
>> +    QAPI_LIST_PREPEND(*(data->dev_list), dev_info);
>> +}
>> +
>> +InitPropsList *qmp_query_init_properties(Error **errp)
>> +{
>> +    GSList *typename_list = object_class_get_list(TYPE_OBJECT, false);
>> +
>> +    InitPropsList *dev_list = NULL;
>> +    QIPData data = { &dev_list, errp };
>> +    g_slist_foreach(typename_list, query_init_properties_tramp, &data);
>> +    g_slist_free(typename_list);
>> +
>> +    return dev_list;
>> +}

-- 
Best regards,
Maxim Davydov

[PATCH v1 9/9] scripts: printing machine type compat properties

[Maxim Davydov]

This script makes the information that can be obtained from
query-init-properties and query-machines easy to read.

Note: some init values from the devices can't be available like properties
from virtio-9p when configure has --disable-virtfs. Such values are
replaced by "DEFAULT". Another exception is properties of abstract
classes. The default value of the abstract class property is common
value of all child classes. But if the values of the child classes are
different the default value will be "BASED_ON_CHILD" (for example, old
x86_64-cpu can have unsupported feature).

Example:

    1) virsh qemu-monitor-command VM --pretty \
       '{"execute" : "query-init-properties"}' > init_props.json

    2) virsh qemu-monitor-command VM --pretty \
       '{"execute" : "query-machines", "arguments" : {"is-full" : true}}' \
       > compat_props.json

    3) scripts/print_MT.py --MT_compat_props compat_props.json\
        --init_props init_props.json --mt pc-q35-7.0 pc-q35-6.1

Output:
╒═══════════════════════════════════╤══════════════╤══════════════╕
│           property_name           │  pc-q35-7.0  │  pc-q35-6.1  │
╞═══════════════════════════════════╪══════════════╪══════════════╡
│   ICH9-LPC-x-keep-pci-slot-hpc    │     True     │    False     │
├───────────────────────────────────┼──────────────┼──────────────┤
│          nvme-ns-shared           │     True     │     off      │
├───────────────────────────────────┼──────────────┼──────────────┤
│ vhost-user-vsock-device-seqpacket │     auto     │     off      │
├───────────────────────────────────┼──────────────┼──────────────┤
│ virtio-mem-unplugged-inaccessible │     auto     │     off      │
├───────────────────────────────────┼──────────────┼──────────────┤
│  x86_64-cpu-hv-version-id-build   │    14393     │    0x1bbc    │
├───────────────────────────────────┼──────────────┼──────────────┤
│  x86_64-cpu-hv-version-id-major   │      10      │    0x0006    │
├───────────────────────────────────┼──────────────┼──────────────┤
│  x86_64-cpu-hv-version-id-minor   │      0       │    0x0001    │
╘═══════════════════════════════════╧══════════════╧══════════════╛

Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
---
 scripts/print_MT.py | 274 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 274 insertions(+)
 create mode 100755 scripts/print_MT.py

diff --git a/scripts/print_MT.py b/scripts/print_MT.py
new file mode 100755
index 0000000000..8be13be8d7
--- /dev/null
+++ b/scripts/print_MT.py
@@ -0,0 +1,274 @@
+#! /usr/bin/python3
+#
+# Script for printing machine type compatible features. It uses two JSON files
+# that should be generated by qmp-init-properties and query-machines.
+#
+# Copyright (c) 2022 Maxim Davydov <maxim.davydov@openvz.org>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import pandas as pd
+import json
+from tabulate import tabulate
+from argparse import ArgumentParser
+
+
+# used for aliases and other names that can be changed
+aliases = { "e1000-82540em": "e1000" }
+
+
+def get_major(mt):
+    splited = mt.split('.')
+    if (len(splited) >= 2):
+        return int(mt.split('.')[1])
+    else:
+        return 0
+
+
+def get_prefix(mt):
+    splited = mt.split('.')
+    if (len(splited) >= 1):
+        return mt.split('.')[0]
+    else:
+        return mt
+
+
+def get_mt_sequence(mt_data):
+    mt_list = [mt['name'] for mt in mt_data['return']]
+    mt_list.remove('none')
+
+    mt_list.sort(key=get_major, reverse=True)
+    mt_list.sort(key=get_prefix, reverse=True)
+
+    return mt_list
+
+
+def get_req_props(defval_data, prop_set, abstr_class_to_features):
+    req_prop_values = dict()
+    req_abstr_prop_values = dict()
+
+    for device in defval_data['return']:
+        # Skip cpu devices that will break all default values for cpus
+        if device['name'] == 'base-x86_64-cpu':
+            continue
+        if device['name'] == 'max-x86_64-cpu':
+            continue
+
+        # some features in mt set as one absract class
+        # but this features are owned by another class
+        device_props_owners = dict()
+        for props_class in device['props']:
+            if not 'classprops' in props_class: # for example, Object class
+                continue
+
+            for prop in props_class['classprops']:
+                if not 'value' in prop:
+                    continue
+
+                prop_name = device['name'] + '-' + prop['name']
+                device_props_owners[prop['name']] = prop['value']
+                if prop_name in prop_set:
+                    req_prop_values[prop_name] = prop['value']
+
+        for props_class in device['props']:
+            if not props_class['classname'] in abstr_class_to_features:
+                continue
+
+            for req_prop in abstr_class_to_features[props_class['classname']]:
+                if not req_prop in device_props_owners:
+                    continue
+
+                prop_value = device_props_owners[req_prop]
+                prop_name = props_class['classname'] + '-' + req_prop
+                if req_abstr_prop_values.setdefault(prop_name, prop_value) \
+                    != prop_value:
+                    req_abstr_prop_values[prop_name] = 'BASED_ON_CHILD'
+
+    return req_prop_values, req_abstr_prop_values
+
+
+def make_definition_table(mt_to_compat_props, prop_set,
+                          req_props, req_abstr_props, is_full):
+    mt_table = dict()
+    for prop in sorted(prop_set):
+        if not is_full:
+            values = set()
+            for mt in mt_to_compat_props:
+                if prop in mt_to_compat_props[mt]:
+                    values.add(mt_to_compat_props[mt][prop])
+                else:
+                    if prop in req_props:
+                        values.add(req_props[prop])
+                    else:
+                        values.add('DEFAULT')
+            # Skip the property if its value is the same for
+            # all required machines
+            if len(values) == 1:
+                continue
+
+        mt_table.setdefault('property_name', []).append(prop)
+        for mt in mt_to_compat_props:
+            if prop in mt_to_compat_props[mt]:
+                value = mt_to_compat_props[mt][prop]
+                mt_table.setdefault(mt, []).append(value)
+            else:
+                if prop in req_props:
+                    mt_table.setdefault(mt, []).append(req_props[prop])
+                else:
+                    value = req_abstr_props.get(prop, 'DEFAULT')
+                    mt_table.setdefault(mt, []).append(value)
+
+    return mt_table
+
+
+def get_standard_form(value):
+    if type(value) is str:
+        out = value.upper()
+        if out.isnumeric():
+            return int(out)
+        if out == 'TRUE':
+            return True
+        if out == 'FALSE':
+            return False
+
+    return value
+
+
+def get_features(mt_data, defval_data, mts, is_full):
+    prop_set = set()
+    abstr_prop_set = set()
+    mt_to_compat_props = dict()
+    # It will be used for searching appropriate feature (sometimes class name
+    # in machine type definition and real class name are different)
+    abstr_class_to_features = dict()
+
+    for mt in mt_data['return']:
+        if mt['name'] == 'none':
+            continue
+
+        if not mt['name'] in mts:
+            continue
+
+        mt_to_compat_props[mt['name']] = dict()
+        for prop in mt['compat-props']:
+            driver_name = aliases.get(prop['driver'], prop['driver'])
+            prop_name = prop['driver'] + '-' + prop['property']
+            real_name = driver_name + '-' + prop['property']
+            # value is always string
+            prop_val  = get_standard_form(prop['value'])
+            if prop['abstract']:
+                mt_to_compat_props[mt['name']][real_name] = prop_val
+                abstr_prop_set.add(real_name)
+                abstr_class_to_features.setdefault(driver_name,
+                                                   set()).add(prop['property'])
+            else:
+                mt_to_compat_props[mt['name']][real_name] = prop_val
+                prop_set.add(real_name)
+
+    req_props, req_abstr_props = get_req_props(defval_data, prop_set,
+                                               abstr_class_to_features)
+
+    # join sets for global sorting by name
+    prop_set.update(abstr_prop_set)
+    mt_table = make_definition_table(mt_to_compat_props, prop_set, req_props,
+                                     req_abstr_props, is_full)
+    # to save mt sequence
+    df = pd.DataFrame({'property_name': mt_table['property_name']})
+    for mt in mts:
+        if not mt in mt_table:
+            print('Error: {0} no found'.format(mt))
+            continue
+        df[mt] = mt_table[mt]
+
+    return df
+
+
+def main():
+    parser = ArgumentParser(description='''Print definition of machine
+                                           type (compatible features)''')
+    parser.add_argument('--MT_compat_props', type=str, required=True,
+                        help='''Path to JSON file with current machine type
+                                definition. It must be generated via
+                                query-machines with is-full option.''')
+    parser.add_argument('--init_props', type=str, required=True,
+                        help='''Path to JSON file with initial features. It
+                                must be generated via
+                                query-init-properties.''')
+    parser.add_argument('--format', type=str,
+                        choices=['table', 'csv', 'html', 'json'],
+                        default='table', help='Format of the output file')
+    parser.add_argument('--file', type=str,
+                        help='''Path to output file. It must be set with csv
+                                and html formats.''')
+    parser.add_argument('--all', action='store_true',
+                        help='''Print all available machine types (list of
+                                machine types will be ignored''')
+    parser.add_argument('--mt', nargs="*", type=str,
+                        help='List of machine types that will be compared')
+    parser.add_argument('--full', action='store_true',
+                        help='''Print all defined properties (by default,
+                                only properties with different values are
+                                printed)''')
+
+    args = parser.parse_args()
+
+    if args.all == 0 and args.mt == None:
+        print('Enter the list of required machine types (list of all '\
+              'machine types : qemu-system-x86_64 --machine help)')
+        return
+
+    with open(args.MT_compat_props) as mt_json_file:
+        mt_data = json.load(mt_json_file)
+
+    with open(args.init_props) as defval_json_file:
+        defval_data = json.load(defval_json_file)
+
+    if args.all:
+        df = get_features(mt_data, defval_data, get_mt_sequence(mt_data),
+                          args.full)
+    else:
+        df = get_features(mt_data, defval_data, args.mt, args.full)
+
+    if args.format == 'csv':
+        if args.file == None:
+            print('Error: csv format requires path to output file')
+            return
+        df.to_csv(args.file)
+    elif args.format == 'html':
+        if args.file == None:
+            print('Error: html format requires path to output file')
+            return
+        with open(args.file, 'w') as output_html:
+            output_html.write(df.to_html(justify='center', col_space='400px',
+                                         index=False))
+    elif args.format == 'json':
+        json_table = df.to_json()
+        if args.file == None:
+            print(json_table)
+        else:
+            with open(args.file, 'w') as output_json:
+                output_json.write(json_table)
+    elif args.format == 'table':
+        table = tabulate(df, showindex=False, stralign='center',
+                         tablefmt='fancy_grid', headers='keys')
+        if args.file == None:
+            print(table)
+        else:
+            with open(args.file, 'w') as output_table:
+                output_table.write(table)
+
+
+if __name__ == '__main__':
+    main()
-- 
2.31.1

Re: [PATCH v1 9/9] scripts: printing machine type compat properties

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> This script makes the information that can be obtained from
> query-init-properties and query-machines easy to read.
> 
> Note: some init values from the devices can't be available like properties
> from virtio-9p when configure has --disable-virtfs. Such values are
> replaced by "DEFAULT". Another exception is properties of abstract
> classes. The default value of the abstract class property is common
> value of all child classes. But if the values of the child classes are
> different the default value will be "BASED_ON_CHILD" (for example, old
> x86_64-cpu can have unsupported feature).
> 
> Example:
> 
>      1) virsh qemu-monitor-command VM --pretty \
>         '{"execute" : "query-init-properties"}' > init_props.json
> 
>      2) virsh qemu-monitor-command VM --pretty \
>         '{"execute" : "query-machines", "arguments" : {"is-full" : true}}' \
>         > compat_props.json
> 
>      3) scripts/print_MT.py --MT_compat_props compat_props.json\
>          --init_props init_props.json --mt pc-q35-7.0 pc-q35-6.1
> 
> Output:
> ╒═══════════════════════════════════╤══════════════╤══════════════╕
> │           property_name           │  pc-q35-7.0  │  pc-q35-6.1  │
> ╞═══════════════════════════════════╪══════════════╪══════════════╡
> │   ICH9-LPC-x-keep-pci-slot-hpc    │     True     │    False     │
> ├───────────────────────────────────┼──────────────┼──────────────┤
> │          nvme-ns-shared           │     True     │     off      │
> ├───────────────────────────────────┼──────────────┼──────────────┤
> │ vhost-user-vsock-device-seqpacket │     auto     │     off      │
> ├───────────────────────────────────┼──────────────┼──────────────┤
> │ virtio-mem-unplugged-inaccessible │     auto     │     off      │
> ├───────────────────────────────────┼──────────────┼──────────────┤
> │  x86_64-cpu-hv-version-id-build   │    14393     │    0x1bbc    │
> ├───────────────────────────────────┼──────────────┼──────────────┤
> │  x86_64-cpu-hv-version-id-major   │      10      │    0x0006    │
> ├───────────────────────────────────┼──────────────┼──────────────┤
> │  x86_64-cpu-hv-version-id-minor   │      0       │    0x0001    │
> ╘═══════════════════════════════════╧══════════════╧══════════════╛
> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   scripts/print_MT.py | 274 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 274 insertions(+)
>   create mode 100755 scripts/print_MT.py
> 
> diff --git a/scripts/print_MT.py b/scripts/print_MT.py
> new file mode 100755
> index 0000000000..8be13be8d7
> --- /dev/null
> +++ b/scripts/print_MT.py
> @@ -0,0 +1,274 @@
> +#! /usr/bin/python3

Standard shebang line for python3 is:

#!/usr/bin/env python3


> +#
> +# Script for printing machine type compatible features. It uses two JSON files
> +# that should be generated by qmp-init-properties and query-machines.
> +#
> +# Copyright (c) 2022 Maxim Davydov <maxim.davydov@openvz.org>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import pandas as pd
> +import json
> +from tabulate import tabulate
> +from argparse import ArgumentParser
> +
> +
> +# used for aliases and other names that can be changed
> +aliases = { "e1000-82540em": "e1000" }
> +
> +
> +def get_major(mt):
> +    splited = mt.split('.')
> +    if (len(splited) >= 2):
> +        return int(mt.split('.')[1])

why to call split() again? You may use splited[1]

> +    else:
> +        return 0
> +
> +
> +def get_prefix(mt):
> +    splited = mt.split('.')
> +    if (len(splited) >= 1):

In python you don't need braces around if condition:

    if len(splited) >= 1:

is the same thing.

> +        return mt.split('.')[0]
> +    else:
> +        return mt

seems, split() never return empty list, so len is always >=1.

You can do simply

return mt.split(',', 1)[0]

> +
> +
> +def get_mt_sequence(mt_data):
> +    mt_list = [mt['name'] for mt in mt_data['return']]
> +    mt_list.remove('none')
> +
> +    mt_list.sort(key=get_major, reverse=True)
> +    mt_list.sort(key=get_prefix, reverse=True)

Aha. You may use one sort() call, if use tuple as key. Something like this should work:

def parse_mt(mt):
   spl = mt.split('.')
   if len(spl) >= 2:
     return spl[0], spl[1]
   else:
     return mt, 0

...

mt_list.sort(key=parse_mt, ...)

> +
> +    return mt_list
> +
> +
> +def get_req_props(defval_data, prop_set, abstr_class_to_features):
> +    req_prop_values = dict()
> +    req_abstr_prop_values = dict()

{} construction is native way to create empty dict:

   req_prop_values = {}

> +
> +    for device in defval_data['return']:
> +        # Skip cpu devices that will break all default values for cpus
> +        if device['name'] == 'base-x86_64-cpu':
> +            continue
> +        if device['name'] == 'max-x86_64-cpu':
> +            continue
> +
> +        # some features in mt set as one absract class
> +        # but this features are owned by another class

Hmm. That all hard for me to review, because I don't know the whole model of machine types.. Don't we have a documentation somewhere? Which objects, classes, abstart classes and properties we have and what that all mean.

> +        device_props_owners = dict()
> +        for props_class in device['props']:
> +            if not 'classprops' in props_class: # for example, Object class

More pythonic is:

   if 'classprops' not in props_class:

> +                continue
> +
> +            for prop in props_class['classprops']:
> +                if not 'value' in prop:
> +                    continue
> +
> +                prop_name = device['name'] + '-' + prop['name']
> +                device_props_owners[prop['name']] = prop['value']
> +                if prop_name in prop_set:
> +                    req_prop_values[prop_name] = prop['value']
> +
> +        for props_class in device['props']:
> +            if not props_class['classname'] in abstr_class_to_features:
> +                continue
> +
> +            for req_prop in abstr_class_to_features[props_class['classname']]:
> +                if not req_prop in device_props_owners:
> +                    continue
> +
> +                prop_value = device_props_owners[req_prop]
> +                prop_name = props_class['classname'] + '-' + req_prop
> +                if req_abstr_prop_values.setdefault(prop_name, prop_value) \
> +                    != prop_value:
> +                    req_abstr_prop_values[prop_name] = 'BASED_ON_CHILD'
> +
> +    return req_prop_values, req_abstr_prop_values
> +
> +
> +def make_definition_table(mt_to_compat_props, prop_set,
> +                          req_props, req_abstr_props, is_full):
> +    mt_table = dict()
> +    for prop in sorted(prop_set):
> +        if not is_full:
> +            values = set()
> +            for mt in mt_to_compat_props:
> +                if prop in mt_to_compat_props[mt]:
> +                    values.add(mt_to_compat_props[mt][prop])
> +                else:
> +                    if prop in req_props:
> +                        values.add(req_props[prop])
> +                    else:
> +                        values.add('DEFAULT')
> +            # Skip the property if its value is the same for
> +            # all required machines
> +            if len(values) == 1:
> +                continue
> +
> +        mt_table.setdefault('property_name', []).append(prop)
> +        for mt in mt_to_compat_props:
> +            if prop in mt_to_compat_props[mt]:
> +                value = mt_to_compat_props[mt][prop]
> +                mt_table.setdefault(mt, []).append(value)
> +            else:
> +                if prop in req_props:
> +                    mt_table.setdefault(mt, []).append(req_props[prop])
> +                else:
> +                    value = req_abstr_props.get(prop, 'DEFAULT')
> +                    mt_table.setdefault(mt, []).append(value)
> +
> +    return mt_table
> +
> +
> +def get_standard_form(value):
> +    if type(value) is str:
> +        out = value.upper()
> +        if out.isnumeric():
> +            return int(out)
> +        if out == 'TRUE':
> +            return True
> +        if out == 'FALSE':
> +            return False
> +
> +    return value
> +
> +
> +def get_features(mt_data, defval_data, mts, is_full):
> +    prop_set = set()
> +    abstr_prop_set = set()
> +    mt_to_compat_props = dict()
> +    # It will be used for searching appropriate feature (sometimes class name
> +    # in machine type definition and real class name are different)
> +    abstr_class_to_features = dict()
> +
> +    for mt in mt_data['return']:
> +        if mt['name'] == 'none':
> +            continue
> +
> +        if not mt['name'] in mts:
> +            continue
> +
> +        mt_to_compat_props[mt['name']] = dict()
> +        for prop in mt['compat-props']:
> +            driver_name = aliases.get(prop['driver'], prop['driver'])
> +            prop_name = prop['driver'] + '-' + prop['property']
> +            real_name = driver_name + '-' + prop['property']
> +            # value is always string
> +            prop_val  = get_standard_form(prop['value'])
> +            if prop['abstract']:
> +                mt_to_compat_props[mt['name']][real_name] = prop_val
> +                abstr_prop_set.add(real_name)
> +                abstr_class_to_features.setdefault(driver_name,
> +                                                   set()).add(prop['property'])
> +            else:
> +                mt_to_compat_props[mt['name']][real_name] = prop_val
> +                prop_set.add(real_name)
> +
> +    req_props, req_abstr_props = get_req_props(defval_data, prop_set,
> +                                               abstr_class_to_features)
> +
> +    # join sets for global sorting by name
> +    prop_set.update(abstr_prop_set)
> +    mt_table = make_definition_table(mt_to_compat_props, prop_set, req_props,
> +                                     req_abstr_props, is_full)
> +    # to save mt sequence
> +    df = pd.DataFrame({'property_name': mt_table['property_name']})
> +    for mt in mts:
> +        if not mt in mt_table:
> +            print('Error: {0} no found'.format(mt))
> +            continue
> +        df[mt] = mt_table[mt]
> +
> +    return df
> +
> +
> +def main():
> +    parser = ArgumentParser(description='''Print definition of machine
> +                                           type (compatible features)''')
> +    parser.add_argument('--MT_compat_props', type=str, required=True,
> +                        help='''Path to JSON file with current machine type
> +                                definition. It must be generated via
> +                                query-machines with is-full option.''')
> +    parser.add_argument('--init_props', type=str, required=True,
> +                        help='''Path to JSON file with initial features. It
> +                                must be generated via
> +                                query-init-properties.''')
> +    parser.add_argument('--format', type=str,
> +                        choices=['table', 'csv', 'html', 'json'],
> +                        default='table', help='Format of the output file')
> +    parser.add_argument('--file', type=str,
> +                        help='''Path to output file. It must be set with csv
> +                                and html formats.''')
> +    parser.add_argument('--all', action='store_true',
> +                        help='''Print all available machine types (list of
> +                                machine types will be ignored''')
> +    parser.add_argument('--mt', nargs="*", type=str,
> +                        help='List of machine types that will be compared')
> +    parser.add_argument('--full', action='store_true',
> +                        help='''Print all defined properties (by default,
> +                                only properties with different values are
> +                                printed)''')
> +
> +    args = parser.parse_args()
> +
> +    if args.all == 0 and args.mt == None:

Don't compare boolean value with zero, use it as is (I'm about args.all, but comparing mt with None doesn't make real sense here too):

   if not(args.all or args.mt):

> +        print('Enter the list of required machine types (list of all '\
> +              'machine types : qemu-system-x86_64 --machine help)')
> +        return
> +
> +    with open(args.MT_compat_props) as mt_json_file:
> +        mt_data = json.load(mt_json_file)
> +
> +    with open(args.init_props) as defval_json_file:
> +        defval_data = json.load(defval_json_file)
> +
> +    if args.all:
> +        df = get_features(mt_data, defval_data, get_mt_sequence(mt_data),
> +                          args.full)
> +    else:
> +        df = get_features(mt_data, defval_data, args.mt, args.full)

I'd write it like this:

mt = args.mt or get_mt_sequence(mt_data)
df = get_feature(..., mt, args.full)

> +
> +    if args.format == 'csv':
> +        if args.file == None:
> +            print('Error: csv format requires path to output file')
> +            return
> +        df.to_csv(args.file)
> +    elif args.format == 'html':
> +        if args.file == None:
> +            print('Error: html format requires path to output file')
> +            return
> +        with open(args.file, 'w') as output_html:
> +            output_html.write(df.to_html(justify='center', col_space='400px',
> +                                         index=False))
> +    elif args.format == 'json':
> +        json_table = df.to_json()
> +        if args.file == None:
> +            print(json_table)
> +        else:
> +            with open(args.file, 'w') as output_json:
> +                output_json.write(json_table)
> +    elif args.format == 'table':
> +        table = tabulate(df, showindex=False, stralign='center',
> +                         tablefmt='fancy_grid', headers='keys')
> +        if args.file == None:
> +            print(table)
> +        else:
> +            with open(args.file, 'w') as output_table:
> +                output_table.write(table)

For me this looks like extra logic.

Why to restrict printing csv/html directly to stdout? It's native to use output redirection to save output to some file. I think you can simply drop --file argument always print to stdout.

Or, if you still want this option, it's better to prepare the whole output in string variable, and then handle it once:

if ...
elif ...
elif ...

...

if args.file:
   with open(...) as f:
     f.write(output)
else:
   print(output)

> +
> +
> +if __name__ == '__main__':
> +    main()


-- 
Best regards,
Vladimir

Re: [PATCH v1 9/9] scripts: printing machine type compat properties

[John Snow]

On Wed, Mar 30, 2022 at 11:55 AM Vladimir Sementsov-Ogievskiy
<v.sementsov-og@mail.ru> wrote:
>
> 29.03.2022 00:15, Maxim Davydov wrote:
> > This script makes the information that can be obtained from
> > query-init-properties and query-machines easy to read.
> >
> > Note: some init values from the devices can't be available like properties
> > from virtio-9p when configure has --disable-virtfs. Such values are
> > replaced by "DEFAULT". Another exception is properties of abstract
> > classes. The default value of the abstract class property is common
> > value of all child classes. But if the values of the child classes are
> > different the default value will be "BASED_ON_CHILD" (for example, old
> > x86_64-cpu can have unsupported feature).
> >
> > Example:
> >
> >      1) virsh qemu-monitor-command VM --pretty \
> >         '{"execute" : "query-init-properties"}' > init_props.json
> >
> >      2) virsh qemu-monitor-command VM --pretty \
> >         '{"execute" : "query-machines", "arguments" : {"is-full" : true}}' \
> >         > compat_props.json
> >
> >      3) scripts/print_MT.py --MT_compat_props compat_props.json\
> >          --init_props init_props.json --mt pc-q35-7.0 pc-q35-6.1
> >

Being able to parse existing JSON files is nice, but have you also
considered baking the QMP querying directly into this script?

> > Output:
> > ╒═══════════════════════════════════╤══════════════╤══════════════╕
> > │           property_name           │  pc-q35-7.0  │  pc-q35-6.1  │
> > ╞═══════════════════════════════════╪══════════════╪══════════════╡
> > │   ICH9-LPC-x-keep-pci-slot-hpc    │     True     │    False     │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │          nvme-ns-shared           │     True     │     off      │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ vhost-user-vsock-device-seqpacket │     auto     │     off      │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ virtio-mem-unplugged-inaccessible │     auto     │     off      │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │  x86_64-cpu-hv-version-id-build   │    14393     │    0x1bbc    │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │  x86_64-cpu-hv-version-id-major   │      10      │    0x0006    │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │  x86_64-cpu-hv-version-id-minor   │      0       │    0x0001    │
> > ╘═══════════════════════════════════╧══════════════╧══════════════╛
> >
> > Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> > ---
> >   scripts/print_MT.py | 274 ++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 274 insertions(+)
> >   create mode 100755 scripts/print_MT.py
> >
> > diff --git a/scripts/print_MT.py b/scripts/print_MT.py
> > new file mode 100755
> > index 0000000000..8be13be8d7
> > --- /dev/null
> > +++ b/scripts/print_MT.py
> > @@ -0,0 +1,274 @@
> > +#! /usr/bin/python3
>
> Standard shebang line for python3 is:
>
> #!/usr/bin/env python3
>
>
> > +#
> > +# Script for printing machine type compatible features. It uses two JSON files
> > +# that should be generated by qmp-init-properties and query-machines.
> > +#
> > +# Copyright (c) 2022 Maxim Davydov <maxim.davydov@openvz.org>
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +import pandas as pd
> > +import json
> > +from tabulate import tabulate
> > +from argparse import ArgumentParser
> > +
> > +
> > +# used for aliases and other names that can be changed
> > +aliases = { "e1000-82540em": "e1000" }
> > +
> > +
> > +def get_major(mt):
> > +    splited = mt.split('.')
> > +    if (len(splited) >= 2):
> > +        return int(mt.split('.')[1])
>
> why to call split() again? You may use splited[1]
>
> > +    else:
> > +        return 0
> > +
> > +
> > +def get_prefix(mt):
> > +    splited = mt.split('.')
> > +    if (len(splited) >= 1):
>
> In python you don't need braces around if condition:
>
>     if len(splited) >= 1:
>
> is the same thing.
>
> > +        return mt.split('.')[0]
> > +    else:
> > +        return mt
>
> seems, split() never return empty list, so len is always >=1.
>
> You can do simply
>
> return mt.split(',', 1)[0]
>
> > +
> > +
> > +def get_mt_sequence(mt_data):
> > +    mt_list = [mt['name'] for mt in mt_data['return']]
> > +    mt_list.remove('none')
> > +
> > +    mt_list.sort(key=get_major, reverse=True)
> > +    mt_list.sort(key=get_prefix, reverse=True)
>
> Aha. You may use one sort() call, if use tuple as key. Something like this should work:
>
> def parse_mt(mt):
>    spl = mt.split('.')
>    if len(spl) >= 2:
>      return spl[0], spl[1]
>    else:
>      return mt, 0
>
> ...
>
> mt_list.sort(key=parse_mt, ...)
>
> > +
> > +    return mt_list
> > +
> > +
> > +def get_req_props(defval_data, prop_set, abstr_class_to_features):
> > +    req_prop_values = dict()
> > +    req_abstr_prop_values = dict()
>
> {} construction is native way to create empty dict:
>
>    req_prop_values = {}
>
> > +
> > +    for device in defval_data['return']:
> > +        # Skip cpu devices that will break all default values for cpus
> > +        if device['name'] == 'base-x86_64-cpu':
> > +            continue
> > +        if device['name'] == 'max-x86_64-cpu':
> > +            continue
> > +
> > +        # some features in mt set as one absract class
> > +        # but this features are owned by another class
>
> Hmm. That all hard for me to review, because I don't know the whole model of machine types.. Don't we have a documentation somewhere? Which objects, classes, abstart classes and properties we have and what that all mean.
>
> > +        device_props_owners = dict()
> > +        for props_class in device['props']:
> > +            if not 'classprops' in props_class: # for example, Object class
>
> More pythonic is:
>
>    if 'classprops' not in props_class:
>
> > +                continue
> > +
> > +            for prop in props_class['classprops']:
> > +                if not 'value' in prop:
> > +                    continue
> > +
> > +                prop_name = device['name'] + '-' + prop['name']
> > +                device_props_owners[prop['name']] = prop['value']
> > +                if prop_name in prop_set:
> > +                    req_prop_values[prop_name] = prop['value']
> > +
> > +        for props_class in device['props']:
> > +            if not props_class['classname'] in abstr_class_to_features:
> > +                continue
> > +
> > +            for req_prop in abstr_class_to_features[props_class['classname']]:
> > +                if not req_prop in device_props_owners:
> > +                    continue
> > +
> > +                prop_value = device_props_owners[req_prop]
> > +                prop_name = props_class['classname'] + '-' + req_prop
> > +                if req_abstr_prop_values.setdefault(prop_name, prop_value) \
> > +                    != prop_value:
> > +                    req_abstr_prop_values[prop_name] = 'BASED_ON_CHILD'
> > +
> > +    return req_prop_values, req_abstr_prop_values
> > +
> > +
> > +def make_definition_table(mt_to_compat_props, prop_set,
> > +                          req_props, req_abstr_props, is_full):
> > +    mt_table = dict()
> > +    for prop in sorted(prop_set):
> > +        if not is_full:
> > +            values = set()
> > +            for mt in mt_to_compat_props:
> > +                if prop in mt_to_compat_props[mt]:
> > +                    values.add(mt_to_compat_props[mt][prop])
> > +                else:
> > +                    if prop in req_props:
> > +                        values.add(req_props[prop])
> > +                    else:
> > +                        values.add('DEFAULT')
> > +            # Skip the property if its value is the same for
> > +            # all required machines
> > +            if len(values) == 1:
> > +                continue
> > +
> > +        mt_table.setdefault('property_name', []).append(prop)
> > +        for mt in mt_to_compat_props:
> > +            if prop in mt_to_compat_props[mt]:
> > +                value = mt_to_compat_props[mt][prop]
> > +                mt_table.setdefault(mt, []).append(value)
> > +            else:
> > +                if prop in req_props:
> > +                    mt_table.setdefault(mt, []).append(req_props[prop])
> > +                else:
> > +                    value = req_abstr_props.get(prop, 'DEFAULT')
> > +                    mt_table.setdefault(mt, []).append(value)
> > +
> > +    return mt_table
> > +
> > +
> > +def get_standard_form(value):
> > +    if type(value) is str:
> > +        out = value.upper()
> > +        if out.isnumeric():
> > +            return int(out)
> > +        if out == 'TRUE':
> > +            return True
> > +        if out == 'FALSE':
> > +            return False
> > +
> > +    return value
> > +
> > +
> > +def get_features(mt_data, defval_data, mts, is_full):
> > +    prop_set = set()
> > +    abstr_prop_set = set()
> > +    mt_to_compat_props = dict()
> > +    # It will be used for searching appropriate feature (sometimes class name
> > +    # in machine type definition and real class name are different)
> > +    abstr_class_to_features = dict()
> > +
> > +    for mt in mt_data['return']:
> > +        if mt['name'] == 'none':
> > +            continue
> > +
> > +        if not mt['name'] in mts:
> > +            continue
> > +
> > +        mt_to_compat_props[mt['name']] = dict()
> > +        for prop in mt['compat-props']:
> > +            driver_name = aliases.get(prop['driver'], prop['driver'])
> > +            prop_name = prop['driver'] + '-' + prop['property']
> > +            real_name = driver_name + '-' + prop['property']
> > +            # value is always string
> > +            prop_val  = get_standard_form(prop['value'])
> > +            if prop['abstract']:
> > +                mt_to_compat_props[mt['name']][real_name] = prop_val
> > +                abstr_prop_set.add(real_name)
> > +                abstr_class_to_features.setdefault(driver_name,
> > +                                                   set()).add(prop['property'])
> > +            else:
> > +                mt_to_compat_props[mt['name']][real_name] = prop_val
> > +                prop_set.add(real_name)
> > +
> > +    req_props, req_abstr_props = get_req_props(defval_data, prop_set,
> > +                                               abstr_class_to_features)
> > +
> > +    # join sets for global sorting by name
> > +    prop_set.update(abstr_prop_set)
> > +    mt_table = make_definition_table(mt_to_compat_props, prop_set, req_props,
> > +                                     req_abstr_props, is_full)
> > +    # to save mt sequence
> > +    df = pd.DataFrame({'property_name': mt_table['property_name']})
> > +    for mt in mts:
> > +        if not mt in mt_table:
> > +            print('Error: {0} no found'.format(mt))
> > +            continue
> > +        df[mt] = mt_table[mt]
> > +
> > +    return df
> > +
> > +
> > +def main():
> > +    parser = ArgumentParser(description='''Print definition of machine
> > +                                           type (compatible features)''')
> > +    parser.add_argument('--MT_compat_props', type=str, required=True,
> > +                        help='''Path to JSON file with current machine type
> > +                                definition. It must be generated via
> > +                                query-machines with is-full option.''')
> > +    parser.add_argument('--init_props', type=str, required=True,
> > +                        help='''Path to JSON file with initial features. It
> > +                                must be generated via
> > +                                query-init-properties.''')
> > +    parser.add_argument('--format', type=str,
> > +                        choices=['table', 'csv', 'html', 'json'],
> > +                        default='table', help='Format of the output file')
> > +    parser.add_argument('--file', type=str,
> > +                        help='''Path to output file. It must be set with csv
> > +                                and html formats.''')
> > +    parser.add_argument('--all', action='store_true',
> > +                        help='''Print all available machine types (list of
> > +                                machine types will be ignored''')
> > +    parser.add_argument('--mt', nargs="*", type=str,
> > +                        help='List of machine types that will be compared')
> > +    parser.add_argument('--full', action='store_true',
> > +                        help='''Print all defined properties (by default,
> > +                                only properties with different values are
> > +                                printed)''')
> > +
> > +    args = parser.parse_args()
> > +
> > +    if args.all == 0 and args.mt == None:
>
> Don't compare boolean value with zero, use it as is (I'm about args.all, but comparing mt with None doesn't make real sense here too):
>
>    if not(args.all or args.mt):
>
> > +        print('Enter the list of required machine types (list of all '\
> > +              'machine types : qemu-system-x86_64 --machine help)')
> > +        return
> > +
> > +    with open(args.MT_compat_props) as mt_json_file:
> > +        mt_data = json.load(mt_json_file)
> > +
> > +    with open(args.init_props) as defval_json_file:
> > +        defval_data = json.load(defval_json_file)
> > +
> > +    if args.all:
> > +        df = get_features(mt_data, defval_data, get_mt_sequence(mt_data),
> > +                          args.full)
> > +    else:
> > +        df = get_features(mt_data, defval_data, args.mt, args.full)
>
> I'd write it like this:
>
> mt = args.mt or get_mt_sequence(mt_data)
> df = get_feature(..., mt, args.full)
>
> > +
> > +    if args.format == 'csv':
> > +        if args.file == None:
> > +            print('Error: csv format requires path to output file')
> > +            return
> > +        df.to_csv(args.file)
> > +    elif args.format == 'html':
> > +        if args.file == None:
> > +            print('Error: html format requires path to output file')
> > +            return
> > +        with open(args.file, 'w') as output_html:
> > +            output_html.write(df.to_html(justify='center', col_space='400px',
> > +                                         index=False))
> > +    elif args.format == 'json':
> > +        json_table = df.to_json()
> > +        if args.file == None:
> > +            print(json_table)
> > +        else:
> > +            with open(args.file, 'w') as output_json:
> > +                output_json.write(json_table)
> > +    elif args.format == 'table':
> > +        table = tabulate(df, showindex=False, stralign='center',
> > +                         tablefmt='fancy_grid', headers='keys')
> > +        if args.file == None:
> > +            print(table)
> > +        else:
> > +            with open(args.file, 'w') as output_table:
> > +                output_table.write(table)
>
> For me this looks like extra logic.
>
> Why to restrict printing csv/html directly to stdout? It's native to use output redirection to save output to some file. I think you can simply drop --file argument always print to stdout.
>
> Or, if you still want this option, it's better to prepare the whole output in string variable, and then handle it once:
>
> if ...
> elif ...
> elif ...
>
> ...
>
> if args.file:
>    with open(...) as f:
>      f.write(output)
> else:
>    print(output)
>
> > +
> > +
> > +if __name__ == '__main__':
> > +    main()
>
>
> --
> Best regards,
> Vladimir
>

I trust Vladimir's review on python scripting otherwise.

--js

Re: [PATCH v1 0/9] Machine type compatible properties

[Igor Mammedov]

On Tue, 29 Mar 2022 00:15:30 +0300
Maxim Davydov <maxim.davydov@openvz.org> wrote:

> We need to be able to check machine type after its definition. It's
> necessary when using complicated inheritance of compatible features. For
> instance, this tool can help to find bugs in the machine type definition
> if the name of the device has been changed. Also, this tool was created
> to help with MTs of other projects such as vz branches.

a comment to all patches where "after initialization" is mentioned.
Pls rewrite commit messages so that it would be clear what that means
and describe in detail why current code needs fixing.

> 
> Maxim Davydov (9):
>   qmp: Add dump machine type compatible properties
>   pci: add null-pointer check
>   mem: appropriate handling getting mem region
>   msmouse: add appropriate unregister handler
>   wctablet: add appropriate unregister handler
>   chardev: add appropriate getting address
>   colo-compare: safe finalization
>   qom: add command to print initial properties
>   scripts: printing machine type compat properties
> 
>  chardev/char-socket.c       |   9 ++
>  chardev/msmouse.c           |   4 +-
>  chardev/wctablet.c          |   4 +-
>  hw/core/machine-qmp-cmds.c  |  25 +++-
>  hw/i386/sgx-epc.c           |   5 +-
>  hw/mem/nvdimm.c             |   6 +
>  hw/mem/pc-dimm.c            |   5 +
>  hw/pci-host/i440fx.c        |  17 ++-
>  hw/pci-host/q35.c           |  17 ++-
>  net/colo-compare.c          |  25 ++--
>  qapi/machine.json           |  58 +++++++-
>  qapi/qom.json               |  69 +++++++++
>  qom/qom-qmp-cmds.c          | 121 ++++++++++++++++
>  scripts/print_MT.py         | 274 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/fuzz/qos_fuzz.c |   2 +-
>  15 files changed, 613 insertions(+), 28 deletions(-)
>  create mode 100755 scripts/print_MT.py
> 

Re: [PATCH v1 0/9] Machine type compatible properties

[Vladimir Sementsov-Ogievskiy]

29.03.2022 00:15, Maxim Davydov wrote:
> We need to be able to check machine type after its definition. It's
> necessary when using complicated inheritance of compatible features. For
> instance, this tool can help to find bugs in the machine type definition
> if the name of the device has been changed. Also, this tool was created
> to help with MTs of other projects such as vz branches.


We discussed verbally two points to change:

1. Instead of workarounds to avoid crashes, which are unsafe (we avoid crash, but is data that we generate this way valid or not?), let's require to call QEMU process with such options so everything that needed is initialized and we avoid crashes and still get valid information

2. We want not "default" values, but "current" ones. I.e., we start QEMU with same options like in production, and we clearly get all real values of machine properties.


> 
> Maxim Davydov (9):
>    qmp: Add dump machine type compatible properties
>    pci: add null-pointer check
>    mem: appropriate handling getting mem region
>    msmouse: add appropriate unregister handler
>    wctablet: add appropriate unregister handler
>    chardev: add appropriate getting address
>    colo-compare: safe finalization
>    qom: add command to print initial properties
>    scripts: printing machine type compat properties
> 
>   chardev/char-socket.c       |   9 ++
>   chardev/msmouse.c           |   4 +-
>   chardev/wctablet.c          |   4 +-
>   hw/core/machine-qmp-cmds.c  |  25 +++-
>   hw/i386/sgx-epc.c           |   5 +-
>   hw/mem/nvdimm.c             |   6 +
>   hw/mem/pc-dimm.c            |   5 +
>   hw/pci-host/i440fx.c        |  17 ++-
>   hw/pci-host/q35.c           |  17 ++-
>   net/colo-compare.c          |  25 ++--
>   qapi/machine.json           |  58 +++++++-
>   qapi/qom.json               |  69 +++++++++
>   qom/qom-qmp-cmds.c          | 121 ++++++++++++++++
>   scripts/print_MT.py         | 274 ++++++++++++++++++++++++++++++++++++
>   tests/qtest/fuzz/qos_fuzz.c |   2 +-
>   15 files changed, 613 insertions(+), 28 deletions(-)
>   create mode 100755 scripts/print_MT.py
> 


-- 
Best regards,
Vladimir