Libguestfs - Sep 2021 - [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name

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.  We should be able to get rid of it
   eventually.

   In my testing, output of qemu-system-x86_64's query-qmp-schema
   grows by 11% (18.5KiB).

2. Like 1, but omit "boring" elements of @member, and empty @member.

   @values does not become redundant.  Output of query-qmp-schema
   grows only as we make enum members non-boring.

3. Versioned query-qmp-schema.

   query-qmp-schema provides either @values or @members.  The QMP
   client can select which version it wants.

This commit implements 1. simply because it's the solution I thought
of first.  I'm prepared to implement one of the others if we decide
that's what we want.

Signed-off-by: Markus Armbruster <armbru at redhat.com>
---
 qapi/introspect.json       | 20 ++++++++++++++++++--
 scripts/qapi/introspect.py | 18 ++++++++++++++----
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index 39bd303778..250748cd95 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -142,14 +142,30 @@
 #
 # 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.
+#
+# @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.1
+##
+{ '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

On Wed, Sep 15, 2021 at 09:24:21PM +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.
> 
>    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.  We should be able to get rid of it
>    eventually.
> 
>    In my testing, output of qemu-system-x86_64's query-qmp-schema
>    grows by 11% (18.5KiB).
This makes sense if we plan to deprecate @values - if so, that
deprecation would make sense as part of this series, although we may
drag our feet for how long before we actually remove it.
> 
> 2. Like 1, but omit "boring" elements of @member, and empty
@member.
> 
>    @values does not become redundant.  Output of query-qmp-schema
>    grows only as we make enum members non-boring.
Does not change whether libvirt would have to learn to query the new
members, but adds a mandatory fallback step for learning about boring
members (although the fallback step will have to be there anyway for
older qemu).  Peter probably has a better idea of which version is
nicer.
> 
> 3. Versioned query-qmp-schema.
> 
>    query-qmp-schema provides either @values or @members.  The QMP
>    client can select which version it wants.
Sounds more complicated to implement.  I'm not opposed to it, but am
leaning towards 1 or 2 myself.
> 
> This commit implements 1. simply because it's the solution I thought
> of first.  I'm prepared to implement one of the others if we decide
> that's what we want.
> 
> Signed-off-by: Markus Armbruster <armbru at redhat.com>
> ---
>  qapi/introspect.json       | 20 ++++++++++++++++++--
>  scripts/qapi/introspect.py | 18 ++++++++++++++----
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 39bd303778..250748cd95 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -142,14 +142,30 @@
>  #
>  # 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.
Missing a '(since 6.2)' tag.
> +#
> +# @values: the enumeration type's member names, in no particular
order.
> +#          Redundant with @members.  Just for backward compatibility.
Worth marking as deprecated in this patch, or in a followup?
>  #
>  # 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.1
6.2
> +##
> +{ 'struct': 'SchemaInfoEnumMember',
> +  'data': { 'name': 'str' } }
>
Definitely more flexible.

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

On Wed, Sep 15, 2021 at 21:24:21 +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.
> 
>    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.  We should be able to get rid of it
>    eventually.
> 
>    In my testing, output of qemu-system-x86_64's query-qmp-schema
>    grows by 11% (18.5KiB).
I prefer this one. While the schema output grows, nobody is really
reading it manually.

The implementation in libvirt is very straightforward:

https://gitlab.com/pipo.sk/libvirt/-/commit/24f1709cfae72cd765d02dc2124d6c954554ea55
https://gitlab.com/pipo.sk/libvirt/-/commit/5909db9d4994327b3f64d5c329edd4b175495bfe
> 2. Like 1, but omit "boring" elements of @member, and empty
@member.
> 
>    @values does not become redundant.  Output of query-qmp-schema
>    grows only as we make enum members non-boring.
This has 2 drawbacks:
- we would never get rid of either of those
- clients would have to check both, one for whether the member is
  present and second whether it's non-boring.

IMO it's simpler for clients just to prefer the new approach when
present as the old is simply a subset.
> 3. Versioned query-qmp-schema.
> 
>    query-qmp-schema provides either @values or @members.  The QMP
>    client can select which version it wants.
At least for libvirt this poses a chicken & egg problem. We'd have to
query the schema to see that it has the switch to do the selection and
then probe with the modern one.