| Message ID | 20231108153827.39692-3-davydov-max@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | compare machine type compat_props | expand |
I apologize for the lateness of my review. Maksim Davydov <davydov-max@yandex-team.ru> writes: > To control that creating new machine type doesn't affect the previous > types (their compat_props) and to check complex compat_props inheritance > we need qmp command to print machine type compatible properties. > > This patch adds the ability to get list of all the compat_props of the > corresponding supported machines for their comparison via new optional > argument of "query-machines" command. Sounds like this is to let developers prevent unwanted change. Such testing interfaces need not and should not be stable interfaces. Thoughts? > Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > hw/core/machine-qmp-cmds.c | 23 +++++++++++++++- > qapi/machine.json | 54 +++++++++++++++++++++++++++++++++++-- > tests/qtest/fuzz/qos_fuzz.c | 2 +- > 3 files changed, 75 insertions(+), 4 deletions(-) > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c > index 3860a50c3b..a49d0dc362 100644 > --- a/hw/core/machine-qmp-cmds.c > +++ b/hw/core/machine-qmp-cmds.c > @@ -66,7 +66,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) > return head; > } > > -MachineInfoList *qmp_query_machines(Error **errp) > +MachineInfoList *qmp_query_machines(bool has_compat_props, bool compat_props, > + Error **errp) > { > GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false); > MachineInfoList *mach_list = NULL; > @@ -98,6 +99,26 @@ MachineInfoList *qmp_query_machines(Error **errp) > info->default_ram_id = g_strdup(mc->default_ram_id); > } > > + if (compat_props && mc->compat_props) { > + int i; > + info->compat_props = NULL; > + CompatPropertyList **tail = &(info->compat_props); > + info->has_compat_props = true; > + > + for (i = 0; i < mc->compat_props->len; i++) { > + GlobalProperty *mt_prop = g_ptr_array_index(mc->compat_props, > + i); > + CompatProperty *prop; > + > + prop = g_malloc0(sizeof(*prop)); > + prop->driver = g_strdup(mt_prop->driver); > + prop->property = g_strdup(mt_prop->property); > + prop->value = g_strdup(mt_prop->value); > + > + QAPI_LIST_APPEND(tail, prop); > + } > + } > + > QAPI_LIST_PREPEND(mach_list, info); > } > > diff --git a/qapi/machine.json b/qapi/machine.json > index b6d634b30d..8ca0c134a2 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -135,6 +135,25 @@ > ## > { 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] } > > +## > +# @CompatProperty: > +# > +# Machine type compatible property. It's based on GlobalProperty and created > +# for machine type compat properties (see scripts) "compatibility property" Doc comments are user documentation. Can we describe this without referencing C data types like GlobalProperty? I'd start with the purpose: specify a default value for a certain property of certain kind of device. The reference (see scripts) needs to be more precise to be useful. Limit line length to 70, please. Two spaces between sentences for consistency, please. > +# > +# @driver: name of the driver that has GlobalProperty > +# > +# @property: global property name > +# > +# @value: global property value I don't like these descriptions, but let's improve the paragraph above them first. > +# > +# Since: 8.2 Going to be 9.0. More of the same below. > +## > +{ 'struct': 'CompatProperty', > + 'data': { 'driver': 'str', > + 'property': 'str', > + 'value': 'str' } } > + > ## > # @MachineInfo: > # > @@ -166,6 +185,9 @@ > # > # @acpi: machine type supports ACPI (since 8.0) > # > +# @compat-props: List of compatible properties that defines machine type "The machine type's compatibility properties" Leaves unsaid when the member is present. Let's worry about that later. > +# (since 8.2) > +# > # Since: 1.2 > ## > { 'struct': 'MachineInfo', > @@ -173,18 +195,46 @@ > '*is-default': 'bool', 'cpu-max': 'int', > 'hotpluggable-cpus': 'bool', 'numa-mem-supported': 'bool', > 'deprecated': 'bool', '*default-cpu-type': 'str', > - '*default-ram-id': 'str', 'acpi': 'bool' } } > + '*default-ram-id': 'str', 'acpi': 'bool', '*compat-props': ['CompatProperty'] } } Long line, break it before '*compat-props', please. To mark the interface unstable, do something like '*compat-props': { 'type': ['CompatProperty'], 'features': [ 'unstable' ] }, You may want to rename it to x-compat-props, just to make "unstable" obvious to human users. > > ## > # @query-machines: > # > # Return a list of supported machines > # > +# @compat-props: if true return will contain information about machine type > +# compatible properties (since 8.2) "compatibility properties" Suppressing parts of the output makes sense only if it's fairly big. How much additional compat-props output do you observe? > +# > # Returns: a list of MachineInfo > # > # Since: 1.2 > +# > +# Example: > +# > +# -> { "execute": "query-machines", "arguments": { "compat-props": true } } > +# <- { "return": [ > +# { > +# "hotpluggable-cpus": true, > +# "name": "pc-q35-6.2", > +# "compat-props": [ > +# { > +# "driver": "virtio-mem", > +# "property": "unplugged-inaccessible", > +# "value": "off" > +# } > +# ], > +# "numa-mem-supported": false, > +# "default-cpu-type": "qemu64-x86_64-cpu", > +# "cpu-max": 288, > +# "deprecated": false, > +# "default-ram-id": "pc.ram" > +# }, > +# ... > +# } > ## > -{ 'command': 'query-machines', 'returns': ['MachineInfo'] } > +{ 'command': 'query-machines', > + 'data': { '*compat-props': 'bool' }, > + 'returns': ['MachineInfo'] } > > ## > # @CurrentMachineParams: > diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c > index e403d373a0..b71e945c5f 100644 > --- a/tests/qtest/fuzz/qos_fuzz.c > +++ b/tests/qtest/fuzz/qos_fuzz.c > @@ -46,7 +46,7 @@ static void qos_set_machines_devices_available(void) > MachineInfoList *mach_info; > ObjectTypeInfoList *type_info; > > - mach_info = qmp_query_machines(&error_abort); > + mach_info = qmp_query_machines(false, false, &error_abort); > machines_apply_to_node(mach_info); > qapi_free_MachineInfoList(mach_info);
Thanks for reviewing Sorry for replying late On 12/1/23 12:49, Markus Armbruster wrote: > I apologize for the lateness of my review. > > Maksim Davydov <davydov-max@yandex-team.ru> writes: > >> To control that creating new machine type doesn't affect the previous >> types (their compat_props) and to check complex compat_props inheritance >> we need qmp command to print machine type compatible properties. >> >> This patch adds the ability to get list of all the compat_props of the >> corresponding supported machines for their comparison via new optional >> argument of "query-machines" command. > Sounds like this is to let developers prevent unwanted change. Such > testing interfaces need not and should not be stable interfaces. > Thoughts? I'm not sure that rightly understand your idea about unstable: do you mean that we can allow this interface to be not robust (e.g. compat-props in MachineInfo can be not presented) or that this is new thusit can be unstable? >> Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> hw/core/machine-qmp-cmds.c | 23 +++++++++++++++- >> qapi/machine.json | 54 +++++++++++++++++++++++++++++++++++-- >> tests/qtest/fuzz/qos_fuzz.c | 2 +- >> 3 files changed, 75 insertions(+), 4 deletions(-) >> >> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c >> index 3860a50c3b..a49d0dc362 100644 >> --- a/hw/core/machine-qmp-cmds.c >> +++ b/hw/core/machine-qmp-cmds.c >> @@ -66,7 +66,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) >> return head; >> } >> >> -MachineInfoList *qmp_query_machines(Error **errp) >> +MachineInfoList *qmp_query_machines(bool has_compat_props, bool compat_props, >> + Error **errp) >> { >> GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false); >> MachineInfoList *mach_list = NULL; >> @@ -98,6 +99,26 @@ MachineInfoList *qmp_query_machines(Error **errp) >> info->default_ram_id = g_strdup(mc->default_ram_id); >> } >> >> + if (compat_props && mc->compat_props) { >> + int i; >> + info->compat_props = NULL; >> + CompatPropertyList **tail = &(info->compat_props); >> + info->has_compat_props = true; >> + >> + for (i = 0; i < mc->compat_props->len; i++) { >> + GlobalProperty *mt_prop = g_ptr_array_index(mc->compat_props, >> + i); >> + CompatProperty *prop; >> + >> + prop = g_malloc0(sizeof(*prop)); >> + prop->driver = g_strdup(mt_prop->driver); >> + prop->property = g_strdup(mt_prop->property); >> + prop->value = g_strdup(mt_prop->value); >> + >> + QAPI_LIST_APPEND(tail, prop); >> + } >> + } >> + >> QAPI_LIST_PREPEND(mach_list, info); >> } >> >> diff --git a/qapi/machine.json b/qapi/machine.json >> index b6d634b30d..8ca0c134a2 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -135,6 +135,25 @@ >> ## >> { 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] } >> >> +## >> +# @CompatProperty: >> +# >> +# Machine type compatible property. It's based on GlobalProperty and created >> +# for machine type compat properties (see scripts) > "compatibility property" > > Doc comments are user documentation. Can we describe this without > referencing C data types like GlobalProperty? I'd start with the > purpose: specify a default value for a certain property of certain kind > of device. > > The reference (see scripts) needs to be more precise to be useful. > > Limit line length to 70, please. > > Two spaces between sentences for consistency, please. > >> +# >> +# @driver: name of the driver that has GlobalProperty >> +# >> +# @property: global property name >> +# >> +# @value: global property value > I don't like these descriptions, but let's improve the paragraph above > them first. > >> +# >> +# Since: 8.2 > Going to be 9.0. More of the same below. > >> +## >> +{ 'struct': 'CompatProperty', >> + 'data': { 'driver': 'str', >> + 'property': 'str', >> + 'value': 'str' } } >> + >> ## >> # @MachineInfo: >> # >> @@ -166,6 +185,9 @@ >> # >> # @acpi: machine type supports ACPI (since 8.0) >> # >> +# @compat-props: List of compatible properties that defines machine type > "The machine type's compatibility properties" > > Leaves unsaid when the member is present. Let's worry about that later. > >> +# (since 8.2) >> +# >> # Since: 1.2 >> ## >> { 'struct': 'MachineInfo', >> @@ -173,18 +195,46 @@ >> '*is-default': 'bool', 'cpu-max': 'int', >> 'hotpluggable-cpus': 'bool', 'numa-mem-supported': 'bool', >> 'deprecated': 'bool', '*default-cpu-type': 'str', >> - '*default-ram-id': 'str', 'acpi': 'bool' } } >> + '*default-ram-id': 'str', 'acpi': 'bool', '*compat-props': ['CompatProperty'] } } > Long line, break it before '*compat-props', please. > > To mark the interface unstable, do something like > > '*compat-props': { 'type': ['CompatProperty'], > 'features': [ 'unstable' ] }, > > You may want to rename it to x-compat-props, just to make "unstable" > obvious to human users. > >> >> ## >> # @query-machines: >> # >> # Return a list of supported machines >> # >> +# @compat-props: if true return will contain information about machine type >> +# compatible properties (since 8.2) > "compatibility properties" > > Suppressing parts of the output makes sense only if it's fairly big. > How much additional compat-props output do you observe? now, there are 61 machines types, so this qmp command generates a 450KB JSON >> +# >> # Returns: a list of MachineInfo >> # >> # Since: 1.2 >> +# >> +# Example: >> +# >> +# -> { "execute": "query-machines", "arguments": { "compat-props": true } } >> +# <- { "return": [ >> +# { >> +# "hotpluggable-cpus": true, >> +# "name": "pc-q35-6.2", >> +# "compat-props": [ >> +# { >> +# "driver": "virtio-mem", >> +# "property": "unplugged-inaccessible", >> +# "value": "off" >> +# } >> +# ], >> +# "numa-mem-supported": false, >> +# "default-cpu-type": "qemu64-x86_64-cpu", >> +# "cpu-max": 288, >> +# "deprecated": false, >> +# "default-ram-id": "pc.ram" >> +# }, >> +# ... >> +# } >> ## >> -{ 'command': 'query-machines', 'returns': ['MachineInfo'] } >> +{ 'command': 'query-machines', >> + 'data': { '*compat-props': 'bool' }, >> + 'returns': ['MachineInfo'] } >> >> ## >> # @CurrentMachineParams: >> diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c >> index e403d373a0..b71e945c5f 100644 >> --- a/tests/qtest/fuzz/qos_fuzz.c >> +++ b/tests/qtest/fuzz/qos_fuzz.c >> @@ -46,7 +46,7 @@ static void qos_set_machines_devices_available(void) >> MachineInfoList *mach_info; >> ObjectTypeInfoList *type_info; >> >> - mach_info = qmp_query_machines(&error_abort); >> + mach_info = qmp_query_machines(false, false, &error_abort); >> machines_apply_to_node(mach_info); >> qapi_free_MachineInfoList(mach_info);
Maksim Davydov <davydov-max@yandex-team.ru> writes: > Thanks for reviewing > Sorry for replying late > > On 12/1/23 12:49, Markus Armbruster wrote: >> I apologize for the lateness of my review. >> >> Maksim Davydov <davydov-max@yandex-team.ru> writes: >> >>> To control that creating new machine type doesn't affect the previous >>> types (their compat_props) and to check complex compat_props inheritance >>> we need qmp command to print machine type compatible properties. >>> >>> This patch adds the ability to get list of all the compat_props of the >>> corresponding supported machines for their comparison via new optional >>> argument of "query-machines" command. >> >> Sounds like this is to let developers prevent unwanted change. Such >> testing interfaces need not and should not be stable interfaces. >> Thoughts? > > I'm not sure that rightly understand your idea about unstable: do you mean > that we can allow this interface to be not robust (e.g. compat-props in > MachineInfo can be not presented) or that this is new thusit can be > unstable? It's about external interface stability: incompatible change requires deprecation and a grace period. See docs/about/deprecated.rst first section. QMP is a stable external interface, except for parts marked unstable. In my review, I wondered whether the QMP interface you add needs to be stable in that sense. Does it? If no, I'll gladly show you how to mark it unstable. >>> Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> [...] >>> diff --git a/qapi/machine.json b/qapi/machine.json >>> index b6d634b30d..8ca0c134a2 100644 >>> --- a/qapi/machine.json >>> +++ b/qapi/machine.json [...] >>> ## >>> # @query-machines: >>> # >>> # Return a list of supported machines >>> # >>> +# @compat-props: if true return will contain information about machine type >>> +# compatible properties (since 8.2) >> >> "compatibility properties" >> >> Suppressing parts of the output makes sense only if it's fairly big. >> How much additional compat-props output do you observe? > > now, there are 61 machines types, so this qmp command generates a 450KB JSON Uff. Recommend to explain this briefly in the commit message. Something like "Since information on compatibility properties can increase the command output by a factor of 40, add an argument to enable it, default off." Does make me wonder whether we want a separate command, though. [...]
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c index 3860a50c3b..a49d0dc362 100644 --- a/hw/core/machine-qmp-cmds.c +++ b/hw/core/machine-qmp-cmds.c @@ -66,7 +66,8 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) return head; } -MachineInfoList *qmp_query_machines(Error **errp) +MachineInfoList *qmp_query_machines(bool has_compat_props, bool compat_props, + Error **errp) { GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false); MachineInfoList *mach_list = NULL; @@ -98,6 +99,26 @@ MachineInfoList *qmp_query_machines(Error **errp) info->default_ram_id = g_strdup(mc->default_ram_id); } + if (compat_props && mc->compat_props) { + int i; + info->compat_props = NULL; + CompatPropertyList **tail = &(info->compat_props); + info->has_compat_props = true; + + for (i = 0; i < mc->compat_props->len; i++) { + GlobalProperty *mt_prop = g_ptr_array_index(mc->compat_props, + i); + CompatProperty *prop; + + prop = g_malloc0(sizeof(*prop)); + prop->driver = g_strdup(mt_prop->driver); + prop->property = g_strdup(mt_prop->property); + prop->value = g_strdup(mt_prop->value); + + QAPI_LIST_APPEND(tail, prop); + } + } + QAPI_LIST_PREPEND(mach_list, info); } diff --git a/qapi/machine.json b/qapi/machine.json index b6d634b30d..8ca0c134a2 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -135,6 +135,25 @@ ## { 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] } +## +# @CompatProperty: +# +# Machine type compatible property. It's based on GlobalProperty and created +# for machine type compat properties (see scripts) +# +# @driver: name of the driver that has GlobalProperty +# +# @property: global property name +# +# @value: global property value +# +# Since: 8.2 +## +{ 'struct': 'CompatProperty', + 'data': { 'driver': 'str', + 'property': 'str', + 'value': 'str' } } + ## # @MachineInfo: # @@ -166,6 +185,9 @@ # # @acpi: machine type supports ACPI (since 8.0) # +# @compat-props: List of compatible properties that defines machine type +# (since 8.2) +# # Since: 1.2 ## { 'struct': 'MachineInfo', @@ -173,18 +195,46 @@ '*is-default': 'bool', 'cpu-max': 'int', 'hotpluggable-cpus': 'bool', 'numa-mem-supported': 'bool', 'deprecated': 'bool', '*default-cpu-type': 'str', - '*default-ram-id': 'str', 'acpi': 'bool' } } + '*default-ram-id': 'str', 'acpi': 'bool', '*compat-props': ['CompatProperty'] } } ## # @query-machines: # # Return a list of supported machines # +# @compat-props: if true return will contain information about machine type +# compatible properties (since 8.2) +# # Returns: a list of MachineInfo # # Since: 1.2 +# +# Example: +# +# -> { "execute": "query-machines", "arguments": { "compat-props": true } } +# <- { "return": [ +# { +# "hotpluggable-cpus": true, +# "name": "pc-q35-6.2", +# "compat-props": [ +# { +# "driver": "virtio-mem", +# "property": "unplugged-inaccessible", +# "value": "off" +# } +# ], +# "numa-mem-supported": false, +# "default-cpu-type": "qemu64-x86_64-cpu", +# "cpu-max": 288, +# "deprecated": false, +# "default-ram-id": "pc.ram" +# }, +# ... +# } ## -{ 'command': 'query-machines', 'returns': ['MachineInfo'] } +{ 'command': 'query-machines', + 'data': { '*compat-props': 'bool' }, + 'returns': ['MachineInfo'] } ## # @CurrentMachineParams: diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c index e403d373a0..b71e945c5f 100644 --- a/tests/qtest/fuzz/qos_fuzz.c +++ b/tests/qtest/fuzz/qos_fuzz.c @@ -46,7 +46,7 @@ static void qos_set_machines_devices_available(void) MachineInfoList *mach_info; ObjectTypeInfoList *type_info; - mach_info = qmp_query_machines(&error_abort); + mach_info = qmp_query_machines(false, false, &error_abort); machines_apply_to_node(mach_info); qapi_free_MachineInfoList(mach_info);