Re: [PATCH v3 12/34] qapi: Add feature flags to remaining definitions
On 3/17/20 12:46 AM, Markus Armbruster wrote: +++ 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 Good. I was getting confused between the doc comment (which generally should have spaces, rather than being one word) and the feature name itself. Sorry for the distraction. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 12/34] qapi: Add feature flags to remaining definitions
Eric Blake 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 >> --- > > >> +++ 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 > > >> +++ 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!
Re: [PATCH v3 12/34] qapi: Add feature flags to remaining definitions
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 --- +++ 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 +++ 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
[PATCH v3 12/34] qapi: Add feature flags to remaining definitions
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 --- docs/devel/qapi-code-gen.txt| 54 +- tests/qapi-schema/doc-good.texi | 30 qapi/introspect.json| 20 +++--- tests/test-qmp-cmds.c | 6 +- scripts/qapi/doc.py | 6 +- scripts/qapi/events.py | 2 +- scripts/qapi/expr.py| 11 ++- scripts/qapi/introspect.py | 31 scripts/qapi/schema.py | 96 ++--- scripts/qapi/types.py | 4 +- scripts/qapi/visit.py | 4 +- tests/qapi-schema/alternate-base.err| 2 +- tests/qapi-schema/doc-good.json | 17 + tests/qapi-schema/doc-good.out | 15 tests/qapi-schema/qapi-schema-test.json | 29 ++-- tests/qapi-schema/qapi-schema-test.out | 27 +-- tests/qapi-schema/test-qapi.py | 9 ++- 17 files changed, 242 insertions(+), 121 deletions(-) diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 297a725084..9fce78dcad 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -172,7 +172,8 @@ Syntax: ENUM = { 'enum': STRING, 'data': [ ENUM-VALUE, ... ], '*prefix': STRING, - '*if': COND } + '*if': COND, + '*features': FEATURES } ENUM-VALUE = STRING | { 'name': STRING, '*if': COND } @@ -207,6 +208,9 @@ the job satisfactorily. The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. +The optional 'features' member specifies features. See "Features" +below for more on this. + === Type references and array types === @@ -279,12 +283,14 @@ below for more on this. Syntax: UNION = { 'union': STRING, 'data': BRANCHES, - '*if': COND } + '*if': COND, + '*features': FEATURES } | { 'union': STRING, 'data': BRANCHES, 'base': ( MEMBERS | STRING ), 'discriminator': STRING, - '*if': COND } + '*if': COND, + '*features': FEATURES } BRANCHES = { BRANCH, ... } BRANCH = STRING : TYPE-REF | STRING : { 'type': TYPE-REF, '*if': COND } @@ -391,13 +397,17 @@ is identical on the wire to: The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. +The optional 'features' member specifies features. See "Features" +below for more on this. + === Alternate types === Syntax: ALTERNATE = { 'alternate': STRING, 'data': ALTERNATIVES, - '*if': COND } + '*if': COND, + '*features': FEATURES } ALTERNATIVES = { ALTERNATIVE, ... } ALTERNATIVE = STRING : TYPE-REF | STRING : { 'type': STRING, '*if': COND } @@ -441,6 +451,9 @@ following example objects: The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. +The optional 'features' member specifies features. See "Features" +below for more on this. + === Commands === @@ -584,6 +597,9 @@ started with --preconfig. The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. +The optional 'features' member specifies features. See "Features" +below for more on this. + === Events === @@ -595,7 +611,8 @@ Syntax: 'data': STRING, 'boxed': true, ) - '*if': COND } + '*if': COND, + '*features': FEATURES } Member 'event' names the event. This is the event name used in the Client JSON Protocol. @@ -628,6 +645,9 @@ complex type. See section "Code generated for events" for examples. The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. +The optional 'features' member specifies features. See "Features" +below for more on this. + === Features === @@ -966,8 +986,9 @@ schema, along with the SchemaInfo type. This text attempts to give an overview how things work. For details you need to consult the QAPI schema. -SchemaInfo objects have common members "name", "meta-type", and -additional variant members depending on the value of meta-type. +SchemaInfo objects have common members "name",