Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name
Kevin Wolf writes:
> Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
>> The next commit will add feature flags to enum members. There's a
>> problem, though: query-qmp-schema shows an enum type's members as an
>> array of member names (SchemaInfoEnum member @values). If it showed
>> an array of objects with a name member, we could simply add more
>> members to these objects. Since it's just strings, we can't.
>>
>> I can see three ways to correct this design mistake:
>>
>> 1. Do it the way we should have done it, plus compatibility goo.
>>
>>We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum. Since
>>changing @values would be a compatibility break, add a new member
>>@members instead.
>>
>>@values is now redundant. In my testing, output of
>>qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
>>
>>We can deprecate @values now and drop it later. This will break
>>outmoded clients. Well-behaved clients such as libvirt are
>>expected to break cleanly.
>>
>> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>>
>>@values does not become redundant. @members augments it. Somewhat
>>cumbersome, but output of query-qmp-schema grows only as we make
>>enum members non-boring.
>>
>>There is nothing to deprecate here.
>>
>> 3. Versioned query-qmp-schema.
>>
>>query-qmp-schema provides either @values or @members. The QMP
>>client can select which version it wants. There is no redundant
>>output.
>>
>>We can deprecate old versions and eventually drop them. This will
>>break outmoded clients. Breaking cleanly is easier than for 1.
>>
>>While 1 and 2 operate within the common rules for compatible
>>evolution apply (section "Compatibility considerations" in
>>docs/devel/qapi-code-gen.rst), 3 bypasses them. Attractive when
>>operating within the rules is just too awkward. Not the case here.
>>
>> This commit implements 1. Libvirt developers prefer it.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> qapi/introspect.json | 21 +++--
>> scripts/qapi/introspect.py | 18 ++
>> 2 files changed, 33 insertions(+), 6 deletions(-)
>
> docs/devel/qapi-code-gen.rst still says:
>
> The SchemaInfo for an enumeration type has meta-type "enum" and
> variant member "values". The values are listed in no particular
> order; clients must search the entire enum when learning whether a
> particular value is supported.
>
> Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::
>
> { "name": "MyEnum", "meta-type": "enum",
> "values": [ "value1", "value2", "value3" ] }
>
> It probably needs an update.
It sure does. Thanks!
Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name
Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
> The next commit will add feature flags to enum members. There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values). If it showed
> an array of objects with a name member, we could simply add more
> members to these objects. Since it's just strings, we can't.
>
> I can see three ways to correct this design mistake:
>
> 1. Do it the way we should have done it, plus compatibility goo.
>
>We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum. Since
>changing @values would be a compatibility break, add a new member
>@members instead.
>
>@values is now redundant. In my testing, output of
>qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
>
>We can deprecate @values now and drop it later. This will break
>outmoded clients. Well-behaved clients such as libvirt are
>expected to break cleanly.
>
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>
>@values does not become redundant. @members augments it. Somewhat
>cumbersome, but output of query-qmp-schema grows only as we make
>enum members non-boring.
>
>There is nothing to deprecate here.
>
> 3. Versioned query-qmp-schema.
>
>query-qmp-schema provides either @values or @members. The QMP
>client can select which version it wants. There is no redundant
>output.
>
>We can deprecate old versions and eventually drop them. This will
>break outmoded clients. Breaking cleanly is easier than for 1.
>
>While 1 and 2 operate within the common rules for compatible
>evolution apply (section "Compatibility considerations" in
>docs/devel/qapi-code-gen.rst), 3 bypasses them. Attractive when
>operating within the rules is just too awkward. Not the case here.
>
> This commit implements 1. Libvirt developers prefer it.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/introspect.json | 21 +++--
> scripts/qapi/introspect.py | 18 ++
> 2 files changed, 33 insertions(+), 6 deletions(-)
docs/devel/qapi-code-gen.rst still says:
The SchemaInfo for an enumeration type has meta-type "enum" and
variant member "values". The values are listed in no particular
order; clients must search the entire enum when learning whether a
particular value is supported.
Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::
{ "name": "MyEnum", "meta-type": "enum",
"values": [ "value1", "value2", "value3" ] }
It probably needs an update.
Kevin
Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name
On Sat, Oct 09, 2021 at 02:09:40PM +0200, Markus Armbruster wrote:
> The next commit will add feature flags to enum members. There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values). If it showed
> an array of objects with a name member, we could simply add more
> members to these objects. Since it's just strings, we can't.
>
> I can see three ways to correct this design mistake:
>
> 1. Do it the way we should have done it, plus compatibility goo.
...
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
> 3. Versioned query-qmp-schema.
> This commit implements 1. Libvirt developers prefer it.
>
> Signed-off-by: Markus Armbruster
> ---
> qapi/introspect.json | 21 +++--
> scripts/qapi/introspect.py | 18 ++
> 2 files changed, 33 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 39bd303778..f806bd7281 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -142,14 +142,31 @@
> #
> # Additional SchemaInfo members for meta-type 'enum'.
> #
> -# @values: the enumeration type's values, in no particular order.
> +# @members: the enum type's members, in no particular order
> +# (since 6.2).
> +#
> +# @values: the enumeration type's member names, in no particular order.
> +# Redundant with @members. Just for backward compatibility.
> #
> # Values of this type are JSON string on the wire.
> #
> # Since: 2.5
> ##
> { 'struct': 'SchemaInfoEnum',
> - 'data': { 'values': ['str'] } }
> + 'data': { 'members': [ 'SchemaInfoEnumMember' ],
> +'values': ['str'] } }
Not deprecated at this time, I agree with your choice to make the
actual deprecation of 'values' to be in a later patch (if at all).
> +
> +##
> +# @SchemaInfoEnumMember:
> +#
> +# An object member.
> +#
> +# @name: the member's name, as defined in the QAPI schema.
> +#
> +# Since: 6.2
> +##
> +{ 'struct': 'SchemaInfoEnumMember',
> + 'data': { 'name': 'str' } }
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
