Re: [PATCH v3 12/34] qapi: Add feature flags to remaining definitions

[Eric Blake <eblake@redhat.com>]

On 3/15/20 9:46 AM, Markus Armbruster wrote:
> In v4.1.0, we added feature flags just to struct types (commit
> 6a8c0b5102^..f3ed93d545), to satisfy an immediate need (commit
> c9d4070991 "file-posix: Add dynamic-auto-read-only QAPI feature").  In
> v4.2.0, we added them to commands (commit 23394b4c39 "qapi: Add
> feature flags to commands") to satisfy another immediate need (commit
> d76744e65e "qapi: Allow introspecting fix for savevm's cooperation
> with blockdev").
> 
> Add them to the remaining definitions: enumeration types, union types,
> alternate types, and events.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---


> +++ b/qapi/introspect.json
> @@ -89,12 +89,18 @@
>   #
>   # @meta-type: the entity's meta type, inherited from @base.
>   #
> +# @features: names of features associated with the entity, in no
> +#            particular order.
> +#            (since 4.1 for object types, 4.2 for commands, 5.0 for
> +#            the rest)

Odd versioning hint, but accurate, and I don't see any way to improve it.

> +#
>   # Additional members depend on the value of @meta-type.
>   #
>   # Since: 2.5
>   ##
>   { 'union': 'SchemaInfo',
> -  'base': { 'name': 'str', 'meta-type': 'SchemaMetaType' },
> +  'base': { 'name': 'str', 'meta-type': 'SchemaMetaType',
> +            '*features': [ 'str' ] },
>     'discriminator': 'meta-type',
>     'data': {
>         'builtin': 'SchemaInfoBuiltin',
> @@ -174,9 +180,6 @@
>   #            and may even differ from the order of the values of the
>   #            enum type of the @tag.
>   #
> -# @features: names of features associated with the type, in no particular
> -#            order. (since: 4.1)
> -#
>   # Values of this type are JSON object on the wire.
>   #
>   # Since: 2.5
> @@ -184,8 +187,7 @@
>   { 'struct': 'SchemaInfoObject',
>     'data': { 'members': [ 'SchemaInfoObjectMember' ],
>               '*tag': 'str',
> -            '*variants': [ 'SchemaInfoObjectVariant' ],
> -            '*features': [ 'str' ] } }
> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }

The code motion from use in some of the union branches to now being 
present in the base class of all of the branches is backwards-compatible.

The generator changes also look correct, and have enough testsuite 
coverage to make it easier to be confident about the patch.

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


> +++ b/tests/qapi-schema/doc-good.json
> @@ -53,10 +53,14 @@
>   # @Enum:
>   # @one: The _one_ {and only}
>   #
> +# Features:
> +# @enum-feat: Also _one_ {and only}

All our existing public features are a single word (matching naming 
conventions elsewhere in QAPI).  Are we sure we want to allow feature 
names that include whitespace?  Of course, the fact that our testsuite 
covers it (even if we don't use it publically) means that we are sure 
that our generator can handle it, regardless of whether we decide that a 
separate patch should restrict feature names.  But I don't see it 
holding up this patch.

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