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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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 <address@hidden>
>> ---
>
>
>> +++ 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 <address@hidden>
>
>
>> +++ b/tests/qapi-schema/doc-good.json
>> @@ -53,10 +53,14 @@
>>   # @Enum:
>>   # @one: The _one_ {and only}
>>   #
>> +# Features:
>> +# @enum-feat: Also _one_ {and only}
   +#
    # @two is undocumented
    ##
    { 'enum': 'Enum', 'data':
      [ { 'name': 'one', 'if': 'defined(IFONE)' }, 'two' ],
   +  'features': [ 'enum-feat' ],
      'if': 'defined(IFCOND)' }

>
> 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.

We definitely do not want to exempt feature names from the QAPI naming
rules.

The code enforces this.  If I change '-' to ' ' in 'features': [
'enum-feat' ], I get

    doc-good.json:61: 'features' member 'enum feat' has an invalid name

Thanks!