[PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

Markus Armbruster posted 5 patches 1 month, 4 weeks ago

[PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

Posted by Markus Armbruster 1 month, 4 weeks ago
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 <armbru@redhat.com>
---
 qapi/introspect.json       | 21 +++++++++++++++++++--
 scripts/qapi/introspect.py | 18 ++++++++++++++----
 2 files changed, 33 insertions(+), 6 deletions(-)

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'] } }
+
+##
+# @SchemaInfoEnumMember:
+#
+# An object member.
+#
+# @name: the member's name, as defined in the QAPI schema.
+#
+# Since: 6.2
+##
+{ 'struct': 'SchemaInfoEnumMember',
+  'data': { 'name': 'str' } }
 
 ##
 # @SchemaInfoArray:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4c079ee627..6334546363 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -68,6 +68,7 @@
 # TypedDict constructs, so they are broadly typed here as simple
 # Python Dicts.
 SchemaInfo = Dict[str, object]
+SchemaInfoEnumMember = Dict[str, object]
 SchemaInfoObject = Dict[str, object]
 SchemaInfoObjectVariant = Dict[str, object]
 SchemaInfoObjectMember = Dict[str, object]
@@ -274,8 +275,16 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
             obj['features'] = self._gen_features(features)
         self._trees.append(Annotated(obj, ifcond, comment))
 
-    def _gen_member(self, member: QAPISchemaObjectTypeMember
-                    ) -> Annotated[SchemaInfoObjectMember]:
+    @staticmethod
+    def _gen_enum_member(member: QAPISchemaEnumMember
+                         ) -> Annotated[SchemaInfoEnumMember]:
+        obj: SchemaInfoEnumMember = {
+            'name': member.name,
+        }
+        return Annotated(obj, member.ifcond)
+
+    def _gen_object_member(self, member: QAPISchemaObjectTypeMember
+                           ) -> Annotated[SchemaInfoObjectMember]:
         obj: SchemaInfoObjectMember = {
             'name': member.name,
             'type': self._use_type(member.type)
@@ -305,7 +314,8 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
                         prefix: Optional[str]) -> None:
         self._gen_tree(
             name, 'enum',
-            {'values': [Annotated(m.name, m.ifcond) for m in members]},
+            {'members': [self._gen_enum_member(m) for m in members],
+             'values': [Annotated(m.name, m.ifcond) for m in members]},
             ifcond, features
         )
 
@@ -322,7 +332,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
                                members: List[QAPISchemaObjectTypeMember],
                                variants: Optional[QAPISchemaVariants]) -> None:
         obj: SchemaInfoObject = {
-            'members': [self._gen_member(m) for m in members]
+            'members': [self._gen_object_member(m) for m in members]
         }
         if variants:
             obj['tag'] = variants.tag_member.name
-- 
2.31.1

Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

Posted by Kevin Wolf 1 month, 3 weeks ago
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 <armbru@redhat.com>
> ---
>  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

Posted by Markus Armbruster 1 month, 2 weeks ago
Kevin Wolf <kwolf@redhat.com> 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 <armbru@redhat.com>
>> ---
>>  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

Posted by Eric Blake 1 month, 3 weeks ago
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 <armbru@redhat.com>
> ---
>  qapi/introspect.json       | 21 +++++++++++++++++++--
>  scripts/qapi/introspect.py | 18 ++++++++++++++----
>  2 files changed, 33 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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 <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org