| Message ID | 20201112172850.401925-6-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: Add support for aliases | expand |
On 11/12/20 11:28 AM, Kevin Wolf wrote: > Introduce alias definitions for object types (structs and unions). This > allows using the same QAPI type and visitor for many syntax variations > that exist in the external representation, like between QMP and the > command line. It also provides a new tool for evolving the schema while > maintaining backwards compatibility during a deprecation period. Cool! Obvious followup patch series: deprecate all QAPI members spelled with _ by making them aliases of new members spelled with -, so that we can finally have consistent spellings. > +=== Aliases === > + > +Object types, including structs and unions, can contain alias > +definitions. > + > +Aliases define alternative member names that may be used in the > +external representation to provide a value for a member in the same > +object or in a nested object. > + > +Syntax: > + ALIAS = { '*alias': STRING, > + 'source': [ STRING, ... ] } > + > +'source' is a list of member names representing the path to an object > +member, starting from the type where the alias definition is > +specified. It may refer to another alias name. It is allowed to use > +a path that doesn't necessarily match an existing member in every > +variant or even at all; in this case, the alias remains unused. > + > +If 'alias' is present, then the single member referred to by 'source' > +is made accessible with the name given in 'alias' in the type where > +the alias definition is specified. > + > +If 'alias' is not present, then all members in the object referred to > +by 'source' are made accessible in the type where the alias definition > +is specified with the same name as they have in 'source'. Is it worth an example of how to use this?
Am 12.11.2020 um 19:34 hat Eric Blake geschrieben: > On 11/12/20 11:28 AM, Kevin Wolf wrote: > > Introduce alias definitions for object types (structs and unions). This > > allows using the same QAPI type and visitor for many syntax variations > > that exist in the external representation, like between QMP and the > > command line. It also provides a new tool for evolving the schema while > > maintaining backwards compatibility during a deprecation period. > > Cool! Obvious followup patch series: deprecate all QAPI members spelled > with _ by making them aliases of new members spelled with -, so that we > can finally have consistent spellings. Ah, that's a nice one, too. I didn't even think of it. Another one I'd like to see is deprecation of SocketAddressLegacy. There is one part missing in this series that we would first need to address before we can actually use it to evolve parts of the schema that are visible in QMP: Exposing aliases in introspection and expressing that the original name of something is deprecated, but the alias will stay around (and probably also deprecating an alias without the original name or other aliases). If we can easily figure out a way to express this that everyone agrees with, I'm happy to include it in this series. Otherwise, since the first user is the command line and not QMP, I'd leave that for the future. For example, imagine we have an option 'foo' with a new alias 'bar'. If we just directly expose the alias rule (which would be the simplest solution from the QEMU perspective), management will check if the alias exists before accessing 'bar'. But in the final state, we remove 'foo' and 'bar' is not an alias any more, so the introspection for 'bar' would change. Is this desirable? On the other hand, we can't specify both as normal options because you must set (at most) one of them, but not both. Also exposing things as normal options becomes hard with wildcard aliases (mapping all fields from a nested struct), especially if unions are involved where some options exist in one or two variants, but not in others. Given this, I think just exposing the alias rules and letting the management tool check both alternatives - if the alias rule or the future option exists - might actually still be the least bad option. Hmm, I guess I should CC libvirt for this discussion, actually. :-) > > +=== Aliases === > > + > > +Object types, including structs and unions, can contain alias > > +definitions. > > + > > +Aliases define alternative member names that may be used in the > > +external representation to provide a value for a member in the same > > +object or in a nested object. > > + > > +Syntax: > > + ALIAS = { '*alias': STRING, > > + 'source': [ STRING, ... ] } > > + > > +'source' is a list of member names representing the path to an object > > +member, starting from the type where the alias definition is > > +specified. It may refer to another alias name. It is allowed to use > > +a path that doesn't necessarily match an existing member in every > > +variant or even at all; in this case, the alias remains unused. > > + > > +If 'alias' is present, then the single member referred to by 'source' > > +is made accessible with the name given in 'alias' in the type where > > +the alias definition is specified. > > + > > +If 'alias' is not present, then all members in the object referred to > > +by 'source' are made accessible in the type where the alias definition > > +is specified with the same name as they have in 'source'. > > Is it worth an example of how to use this? Yes, I should have included an example. Or actually, probably one example for aliasing a single field and another one for a wildcard alias that maps all fields in a struct. Kevin
On Fri, Nov 13, 2020 at 10:46:02 +0100, Kevin Wolf wrote: > Am 12.11.2020 um 19:34 hat Eric Blake geschrieben: > > On 11/12/20 11:28 AM, Kevin Wolf wrote: > > > Introduce alias definitions for object types (structs and unions). This > > > allows using the same QAPI type and visitor for many syntax variations > > > that exist in the external representation, like between QMP and the > > > command line. It also provides a new tool for evolving the schema while > > > maintaining backwards compatibility during a deprecation period. > > > > Cool! Obvious followup patch series: deprecate all QAPI members spelled > > with _ by making them aliases of new members spelled with -, so that we > > can finally have consistent spellings. > > Ah, that's a nice one, too. I didn't even think of it. Another one I'd > like to see is deprecation of SocketAddressLegacy. > > There is one part missing in this series that we would first need to > address before we can actually use it to evolve parts of the schema that > are visible in QMP: Exposing aliases in introspection and expressing > that the original name of something is deprecated, but the alias will > stay around (and probably also deprecating an alias without the original > name or other aliases). > > If we can easily figure out a way to express this that everyone agrees > with, I'm happy to include it in this series. Otherwise, since the first > user is the command line and not QMP, I'd leave that for the future. > > For example, imagine we have an option 'foo' with a new alias 'bar'. If > we just directly expose the alias rule (which would be the simplest > solution from the QEMU perspective), management will check if the alias > exists before accessing 'bar'. But in the final state, we remove 'foo' > and 'bar' is not an alias any more, so the introspection for 'bar' would > change. Is this desirable? > > On the other hand, we can't specify both as normal options because you > must set (at most) one of them, but not both. Also exposing things as > normal options becomes hard with wildcard aliases (mapping all fields > from a nested struct), especially if unions are involved where some > options exist in one or two variants, but not in others. > > Given this, I think just exposing the alias rules and letting the > management tool check both alternatives - if the alias rule or the > future option exists - might actually still be the least bad option. > > Hmm, I guess I should CC libvirt for this discussion, actually. :-) I can see how that simplifies things for qemu in the long run, but to be honest, if you then deprecate the old name, libvirt will need to add a translation table for it. Either explicit by detecting the new name and adapting the code or by adding a lookup table or something. This would be needed as if you then remove the alias itself we'd no longer be able to use it, so I'm not entirely a fan of it, especially the deprecation bit.
On Fri, Nov 20, 2020 at 03:41:54PM +0100, Peter Krempa wrote: > On Fri, Nov 13, 2020 at 10:46:02 +0100, Kevin Wolf wrote: > > Am 12.11.2020 um 19:34 hat Eric Blake geschrieben: > > > On 11/12/20 11:28 AM, Kevin Wolf wrote: > > > > Introduce alias definitions for object types (structs and unions). This > > > > allows using the same QAPI type and visitor for many syntax variations > > > > that exist in the external representation, like between QMP and the > > > > command line. It also provides a new tool for evolving the schema while > > > > maintaining backwards compatibility during a deprecation period. > > > > > > Cool! Obvious followup patch series: deprecate all QAPI members spelled > > > with _ by making them aliases of new members spelled with -, so that we > > > can finally have consistent spellings. > > > > Ah, that's a nice one, too. I didn't even think of it. Another one I'd > > like to see is deprecation of SocketAddressLegacy. > > > > There is one part missing in this series that we would first need to > > address before we can actually use it to evolve parts of the schema that > > are visible in QMP: Exposing aliases in introspection and expressing > > that the original name of something is deprecated, but the alias will > > stay around (and probably also deprecating an alias without the original > > name or other aliases). > > > > If we can easily figure out a way to express this that everyone agrees > > with, I'm happy to include it in this series. Otherwise, since the first > > user is the command line and not QMP, I'd leave that for the future. > > > > For example, imagine we have an option 'foo' with a new alias 'bar'. If > > we just directly expose the alias rule (which would be the simplest > > solution from the QEMU perspective), management will check if the alias > > exists before accessing 'bar'. But in the final state, we remove 'foo' > > and 'bar' is not an alias any more, so the introspection for 'bar' would > > change. Is this desirable? > > > > On the other hand, we can't specify both as normal options because you > > must set (at most) one of them, but not both. Also exposing things as > > normal options becomes hard with wildcard aliases (mapping all fields > > from a nested struct), especially if unions are involved where some > > options exist in one or two variants, but not in others. > > > > Given this, I think just exposing the alias rules and letting the > > management tool check both alternatives - if the alias rule or the > > future option exists - might actually still be the least bad option. > > > > Hmm, I guess I should CC libvirt for this discussion, actually. :-) > > I can see how that simplifies things for qemu in the long run, but to be > honest, if you then deprecate the old name, libvirt will need to add a > translation table for it. Either explicit by detecting the new name and > adapting the code or by adding a lookup table or something. This would > be needed as if you then remove the alias itself we'd no longer be able > to use it, so I'm not entirely a fan of it, especially the deprecation > bit. To be fair, a key part of libvirt's value add is that it provides the stable API to applications, insulating them from hypervisor changes. This enables QEMU to make incompatible changes in a reasonable timeframe, without breaking the end applications. Of course there is a trade off here because every time QEMU changes, libvirt gets to add back compat code to its maint burden, but the premise is that this is still cheaper overall. We don't want QEMU to gratuitously break things for no reason, but if there's a compelling reason to change QEMU, libvirt is there to pick up the pieces to protect applications. Regards, Daniel
Kevin Wolf <kwolf@redhat.com> writes: > Introduce alias definitions for object types (structs and unions). This > allows using the same QAPI type and visitor for many syntax variations > that exist in the external representation, like between QMP and the > command line. It also provides a new tool for evolving the schema while > maintaining backwards compatibility during a deprecation period. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++--- > docs/sphinx/qapidoc.py | 2 +- > scripts/qapi/expr.py | 34 +++++++++++++++++++++-- > scripts/qapi/schema.py | 27 +++++++++++++++---- > scripts/qapi/types.py | 4 ++- > scripts/qapi/visit.py | 33 ++++++++++++++++++++--- > tests/qapi-schema/test-qapi.py | 7 ++++- > tests/qapi-schema/double-type.err | 2 +- > tests/qapi-schema/unknown-expr-key.err | 2 +- > 9 files changed, 130 insertions(+), 18 deletions(-) > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > index 6906a06ad2..6da14d5275 100644 > --- a/docs/devel/qapi-code-gen.txt > +++ b/docs/devel/qapi-code-gen.txt > @@ -231,7 +231,8 @@ Syntax: > 'data': MEMBERS, > '*base': STRING, > '*if': COND, > - '*features': FEATURES } > + '*features': FEATURES, > + '*aliases': ALIASES } > MEMBERS = { MEMBER, ... } > MEMBER = STRING : TYPE-REF > | STRING : { 'type': TYPE-REF, Missing: a forward reference, like we have for 'if' and 'features'. Here's the obvious one: 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. +The optional 'aliases' member specifies aliases. See "Aliases" below +for more on this. > @@ -286,13 +287,15 @@ Syntax: > UNION = { 'union': STRING, > 'data': BRANCHES, > '*if': COND, > - '*features': FEATURES } > + '*features': FEATURES, > + '*aliases': ALIASES } > | { 'union': STRING, > 'data': BRANCHES, > 'base': ( MEMBERS | STRING ), > 'discriminator': STRING, > '*if': COND, > - '*features': FEATURES } > + '*features': FEATURES, > + '*aliases': ALIASES } > BRANCHES = { BRANCH, ... } > BRANCH = STRING : TYPE-REF > | STRING : { 'type': TYPE-REF, '*if': COND } Likewise. > @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in > this particular build. > > > +=== Aliases === > + > +Object types, including structs and unions, can contain alias > +definitions. > + > +Aliases define alternative member names that may be used in the > +external representation to provide a value for a member in the same > +object or in a nested object. "or one if its sub-objects"? Not sure which is better. > + > +Syntax: > + ALIAS = { '*alias': STRING, > + 'source': [ STRING, ... ] } You used non-terminal ALIASES above. Please define it here. I have doubts about the name 'alias'. The alias is the complete thing, and 'alias' is just one property of the complete thing. I think 'name' would be better. Further evidence: below, you write "If 'alias' is present" and "If 'alias' is not present". I think both read better with 'name' instead of 'alias'. > + > +'source' is a list of member names representing the path to an object > +member, starting from the type where the alias definition is > +specified. May 'source' be empty? More on that below. "where the definition is specified" feels a bit awkward, and "path" slightly hand-wavy. Let me try induction: 'source' is a list of member names. The first name is resolved in the same object. Each subsequent member is resolved in the object named by the preceding member. Differently awkward, I guess. > It may refer to another alias name. It is allowed to use > +a path that doesn't necessarily match an existing member in every > +variant or even at all; in this case, the alias remains unused. Aha! Knowing this would've saved me some trouble in reviewing code. I wrote on PATCH 1: I think updating the big comment in visitor.h for aliases would help. Let's postpone it until I've seen the rest of the series. We can cover unused aliases right there. Whether they also need to go into contracts we'll see. What if only a *prefix* of 'source' matches? E.g. 'source': ['eins', 'zwei', 'drei'] and we have an object-valued member 'eins' (match), which has a member 'zwei' (match), which is *not* an object. Is that an error? Is it caught? > + > +If 'alias' is present, then the single member referred to by 'source' > +is made accessible with the name given in 'alias' in the type where > +the alias definition is specified. 'source' may not be empty here. Correct? If yes, please spell it out. Double-checking I got it... Say we have 'alias': 'foo', 'source': ['bar', 'baz'] where 'bar' is an object with a member 'baz'. Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}. If input also contains "bar", we merge. Duplicates are an error. This is the case even when 'baz' is an object. If you want to alias member 'foo' of 'baz', you have to say 'alias': 'foo', 'source': ['bar', 'baz', 'foo'] Correct? > + > +If 'alias' is not present, then all members in the object referred to > +by 'source' are made accessible in the type where the alias definition > +is specified with the same name as they have in 'source'. 'source' may not be empty here, either. Correct? If yes, please spell it out, and make sure the code catches it. What if it resolve to a non-object? Is that an error? Is it caught? Continuing the double-checking... Say we have # alias missing 'source': ['gnu'] where 'gnu' is an object with a member 'gnat'. Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}. Correct? The document could use examples. Feel free to steal mine. I think we should talk about 'alias' first, and only then about 'source'. It matches their order in the schema, and also matches how I think about aliases, namely "this name actually means that". Here, "this name" is 'alias', and "that" is 'source'. > + > + Don't get deceived by my comments; this is a pretty good start. I wish I had studied this part before PATCH 1. > === Documentation comments === > > A multi-line comment that starts and ends with a '##' line is a I intend to look at the remainder shortly. [...]
Am 10.02.2021 um 10:17 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Introduce alias definitions for object types (structs and unions). This > > allows using the same QAPI type and visitor for many syntax variations > > that exist in the external representation, like between QMP and the > > command line. It also provides a new tool for evolving the schema while > > maintaining backwards compatibility during a deprecation period. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++--- > > docs/sphinx/qapidoc.py | 2 +- > > scripts/qapi/expr.py | 34 +++++++++++++++++++++-- > > scripts/qapi/schema.py | 27 +++++++++++++++---- > > scripts/qapi/types.py | 4 ++- > > scripts/qapi/visit.py | 33 ++++++++++++++++++++--- > > tests/qapi-schema/test-qapi.py | 7 ++++- > > tests/qapi-schema/double-type.err | 2 +- > > tests/qapi-schema/unknown-expr-key.err | 2 +- > > 9 files changed, 130 insertions(+), 18 deletions(-) > > > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > > index 6906a06ad2..6da14d5275 100644 > > --- a/docs/devel/qapi-code-gen.txt > > +++ b/docs/devel/qapi-code-gen.txt > > @@ -231,7 +231,8 @@ Syntax: > > 'data': MEMBERS, > > '*base': STRING, > > '*if': COND, > > - '*features': FEATURES } > > + '*features': FEATURES, > > + '*aliases': ALIASES } > > MEMBERS = { MEMBER, ... } > > MEMBER = STRING : TYPE-REF > > | STRING : { 'type': TYPE-REF, > > Missing: a forward reference, like we have for 'if' and 'features'. > Here's the obvious one: > > 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. > > +The optional 'aliases' member specifies aliases. See "Aliases" below > +for more on this. > > > @@ -286,13 +287,15 @@ Syntax: > > UNION = { 'union': STRING, > > 'data': BRANCHES, > > '*if': COND, > > - '*features': FEATURES } > > + '*features': FEATURES, > > + '*aliases': ALIASES } > > | { 'union': STRING, > > 'data': BRANCHES, > > 'base': ( MEMBERS | STRING ), > > 'discriminator': STRING, > > '*if': COND, > > - '*features': FEATURES } > > + '*features': FEATURES, > > + '*aliases': ALIASES } > > BRANCHES = { BRANCH, ... } > > BRANCH = STRING : TYPE-REF > > | STRING : { 'type': TYPE-REF, '*if': COND } > > Likewise. > > > @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in > > this particular build. > > > > > > +=== Aliases === > > + > > +Object types, including structs and unions, can contain alias > > +definitions. > > + > > +Aliases define alternative member names that may be used in the > > +external representation to provide a value for a member in the same > > +object or in a nested object. > > "or one if its sub-objects"? Not sure which is better. "nested object" feels a little clearer to me, but not that it's a big difference. If you feel "sub-object" is better, I can use that. > > + > > +Syntax: > > + ALIAS = { '*alias': STRING, > > + 'source': [ STRING, ... ] } > > You used non-terminal ALIASES above. Please define it here. > > I have doubts about the name 'alias'. The alias is the complete thing, > and 'alias' is just one property of the complete thing. I think 'name' > would be better. Further evidence: below, you write "If 'alias' is > present" and "If 'alias' is not present". I think both read better with > 'name' instead of 'alias'. Works for me. > > + > > +'source' is a list of member names representing the path to an object > > +member, starting from the type where the alias definition is > > +specified. > > May 'source' be empty? More on that below. No. Empty 'source' isn't the path to any object member, so it doesn't meet the requirement. If you prefer, we can explicitly specify a "non-empty list". > "where the definition is specified" feels a bit awkward, and "path" > slightly hand-wavy. Let me try induction: > > 'source' is a list of member names. The first name is resolved in > the same object. Each subsequent member is resolved in the object > named by the preceding member. > > Differently awkward, I guess. Now you've left out what the purpose of it is. I think I'll combine your version with my first part ("'source' is a list of member names representing the path to an object member"). > > It may refer to another alias name. It is allowed to use > > +a path that doesn't necessarily match an existing member in every > > +variant or even at all; in this case, the alias remains unused. > > Aha! Knowing this would've saved me some trouble in reviewing code. > > I wrote on PATCH 1: > > I think updating the big comment in visitor.h for aliases would help. > Let's postpone it until I've seen the rest of the series. > > We can cover unused aliases right there. Whether they also need to go > into contracts we'll see. Ok. I assume updating that big comment is still postponed because you saw the series, but didn't actually review all of it yet? > What if only a *prefix* of 'source' matches? E.g. > > 'source': ['eins', 'zwei', 'drei'] > > and we have an object-valued member 'eins' (match), which has a member > 'zwei' (match), which is *not* an object. Is that an error? Is it > caught? This feels like a realistic case to me when 'eins' is a union type where some variants contain an object 'zwei' with a member 'drei' and others have 'zwei' as a non-object member. In this case, we want the alias not to match in the non-object 'zwei' case, but we do want it to match in another variant. So it is intentionally not an error. The QAPI generator could try to prove that there is at least one variant where the alias would actually be applied, but just leaving it unused when it doesn't match anywhere seemed good enough to me. > > + > > +If 'alias' is present, then the single member referred to by 'source' > > +is made accessible with the name given in 'alias' in the type where > > +the alias definition is specified. > > 'source' may not be empty here. Correct? > > If yes, please spell it out. Yes. Does spelling it out more explicitly in the description of 'source' suffice? > Double-checking I got it... Say we have > > 'alias': 'foo', > 'source': ['bar', 'baz'] > > where 'bar' is an object with a member 'baz'. > > Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}. > > If input also contains "bar", we merge. Duplicates are an error. > > This is the case even when 'baz' is an object. If you want to alias > member 'foo' of 'baz', you have to say > > 'alias': 'foo', > 'source': ['bar', 'baz', 'foo'] > > Correct? Correct. > > + > > +If 'alias' is not present, then all members in the object referred to > > +by 'source' are made accessible in the type where the alias definition > > +is specified with the same name as they have in 'source'. > > 'source' may not be empty here, either. Correct? > > If yes, please spell it out, and make sure the code catches it. Yes, as above. It's checked in check_aliases(): if not a['source']: raise QAPISemError(info, "'source' must not be empty") > What if it resolve to a non-object? Is that an error? Is it caught? Same as above, it just doesn't match. > Continuing the double-checking... Say we have > > # alias missing > 'source': ['gnu'] > > where 'gnu' is an object with a member 'gnat'. > > Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}. > > Correct? Yes. > The document could use examples. Feel free to steal mine. > > I think we should talk about 'alias' first, and only then about > 'source'. It matches their order in the schema, and also matches how I > think about aliases, namely "this name actually means that". Here, > "this name" is 'alias', and "that" is 'source'. > > > + > > + > > Don't get deceived by my comments; this is a pretty good start. > > I wish I had studied this part before PATCH 1. > > > === Documentation comments === > > > > A multi-line comment that starts and ends with a '##' line is a > > I intend to look at the remainder shortly. Ok. I'll prepare for a context switch to actually be able to address your comments on the other patches and to figure out what I had already addressed in my branch during your last review attempt. I thought I had done a better than average job on documenting the code (at least compare to my other patches), but doesn't seem so... Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 10.02.2021 um 10:17 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Introduce alias definitions for object types (structs and unions). This >> > allows using the same QAPI type and visitor for many syntax variations >> > that exist in the external representation, like between QMP and the >> > command line. It also provides a new tool for evolving the schema while >> > maintaining backwards compatibility during a deprecation period. >> > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > --- >> > docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++--- >> > docs/sphinx/qapidoc.py | 2 +- >> > scripts/qapi/expr.py | 34 +++++++++++++++++++++-- >> > scripts/qapi/schema.py | 27 +++++++++++++++---- >> > scripts/qapi/types.py | 4 ++- >> > scripts/qapi/visit.py | 33 ++++++++++++++++++++--- >> > tests/qapi-schema/test-qapi.py | 7 ++++- >> > tests/qapi-schema/double-type.err | 2 +- >> > tests/qapi-schema/unknown-expr-key.err | 2 +- >> > 9 files changed, 130 insertions(+), 18 deletions(-) >> > >> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt >> > index 6906a06ad2..6da14d5275 100644 >> > --- a/docs/devel/qapi-code-gen.txt >> > +++ b/docs/devel/qapi-code-gen.txt >> > @@ -231,7 +231,8 @@ Syntax: >> > 'data': MEMBERS, >> > '*base': STRING, >> > '*if': COND, >> > - '*features': FEATURES } >> > + '*features': FEATURES, >> > + '*aliases': ALIASES } >> > MEMBERS = { MEMBER, ... } >> > MEMBER = STRING : TYPE-REF >> > | STRING : { 'type': TYPE-REF, >> >> Missing: a forward reference, like we have for 'if' and 'features'. >> Here's the obvious one: >> >> 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. >> >> +The optional 'aliases' member specifies aliases. See "Aliases" below >> +for more on this. >> >> > @@ -286,13 +287,15 @@ Syntax: >> > UNION = { 'union': STRING, >> > 'data': BRANCHES, >> > '*if': COND, >> > - '*features': FEATURES } >> > + '*features': FEATURES, >> > + '*aliases': ALIASES } >> > | { 'union': STRING, >> > 'data': BRANCHES, >> > 'base': ( MEMBERS | STRING ), >> > 'discriminator': STRING, >> > '*if': COND, >> > - '*features': FEATURES } >> > + '*features': FEATURES, >> > + '*aliases': ALIASES } >> > BRANCHES = { BRANCH, ... } >> > BRANCH = STRING : TYPE-REF >> > | STRING : { 'type': TYPE-REF, '*if': COND } >> >> Likewise. >> >> > @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in >> > this particular build. >> > >> > >> > +=== Aliases === >> > + >> > +Object types, including structs and unions, can contain alias >> > +definitions. >> > + >> > +Aliases define alternative member names that may be used in the >> > +external representation to provide a value for a member in the same >> > +object or in a nested object. >> >> "or one if its sub-objects"? Not sure which is better. > > "nested object" feels a little clearer to me, but not that it's a big > difference. If you feel "sub-object" is better, I can use that. > >> > + >> > +Syntax: >> > + ALIAS = { '*alias': STRING, >> > + 'source': [ STRING, ... ] } >> >> You used non-terminal ALIASES above. Please define it here. >> >> I have doubts about the name 'alias'. The alias is the complete thing, >> and 'alias' is just one property of the complete thing. I think 'name' >> would be better. Further evidence: below, you write "If 'alias' is >> present" and "If 'alias' is not present". I think both read better with >> 'name' instead of 'alias'. > > Works for me. > >> > + >> > +'source' is a list of member names representing the path to an object >> > +member, starting from the type where the alias definition is >> > +specified. >> >> May 'source' be empty? More on that below. > > No. Empty 'source' isn't the path to any object member, so it doesn't > meet the requirement. If you prefer, we can explicitly specify a > "non-empty list". I think it's best to be tediously explicit here. >> "where the definition is specified" feels a bit awkward, and "path" >> slightly hand-wavy. Let me try induction: >> >> 'source' is a list of member names. The first name is resolved in >> the same object. Each subsequent member is resolved in the object >> named by the preceding member. >> >> Differently awkward, I guess. > > Now you've left out what the purpose of it is. I think I'll combine your > version with my first part ("'source' is a list of member names > representing the path to an object member"). > >> > It may refer to another alias name. It is allowed to use >> > +a path that doesn't necessarily match an existing member in every >> > +variant or even at all; in this case, the alias remains unused. >> >> Aha! Knowing this would've saved me some trouble in reviewing code. >> >> I wrote on PATCH 1: >> >> I think updating the big comment in visitor.h for aliases would help. >> Let's postpone it until I've seen the rest of the series. >> >> We can cover unused aliases right there. Whether they also need to go >> into contracts we'll see. > > Ok. I assume updating that big comment is still postponed because you > saw the series, but didn't actually review all of it yet? Writing documentation before I understand the code is probably not a good use of my time, and my reviewer's time, too. Getting there. If you want to try, go right ahead. >> What if only a *prefix* of 'source' matches? E.g. >> >> 'source': ['eins', 'zwei', 'drei'] >> >> and we have an object-valued member 'eins' (match), which has a member >> 'zwei' (match), which is *not* an object. Is that an error? Is it >> caught? > > This feels like a realistic case to me when 'eins' is a union type where > some variants contain an object 'zwei' with a member 'drei' and others > have 'zwei' as a non-object member. > > In this case, we want the alias not to match in the non-object 'zwei' > case, but we do want it to match in another variant. So it is > intentionally not an error. > > The QAPI generator could try to prove that there is at least one variant > where the alias would actually be applied, but just leaving it unused > when it doesn't match anywhere seemed good enough to me. I see. A typo can get your alias silently ignored. A bit of a trap. Testing should catch this, of course. Consider adding a comment in the QAPI generator along the lines "could check this, but not sure it's worth our while", and a short note in qapi-code-gen.txt to warn users about the trap. >> > + >> > +If 'alias' is present, then the single member referred to by 'source' >> > +is made accessible with the name given in 'alias' in the type where >> > +the alias definition is specified. >> >> 'source' may not be empty here. Correct? >> >> If yes, please spell it out. > > Yes. Does spelling it out more explicitly in the description of 'source' > suffice? I think so, yes. >> Double-checking I got it... Say we have >> >> 'alias': 'foo', >> 'source': ['bar', 'baz'] >> >> where 'bar' is an object with a member 'baz'. >> >> Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}. >> >> If input also contains "bar", we merge. Duplicates are an error. >> >> This is the case even when 'baz' is an object. If you want to alias >> member 'foo' of 'baz', you have to say >> >> 'alias': 'foo', >> 'source': ['bar', 'baz', 'foo'] >> >> Correct? > > Correct. > >> > + >> > +If 'alias' is not present, then all members in the object referred to >> > +by 'source' are made accessible in the type where the alias definition >> > +is specified with the same name as they have in 'source'. >> >> 'source' may not be empty here, either. Correct? >> >> If yes, please spell it out, and make sure the code catches it. > > Yes, as above. It's checked in check_aliases(): > > if not a['source']: > raise QAPISemError(info, "'source' must not be empty") > >> What if it resolve to a non-object? Is that an error? Is it caught? > > Same as above, it just doesn't match. > >> Continuing the double-checking... Say we have >> >> # alias missing >> 'source': ['gnu'] >> >> where 'gnu' is an object with a member 'gnat'. >> >> Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}. >> >> Correct? > > Yes. > >> The document could use examples. Feel free to steal mine. >> >> I think we should talk about 'alias' first, and only then about >> 'source'. It matches their order in the schema, and also matches how I >> think about aliases, namely "this name actually means that". Here, >> "this name" is 'alias', and "that" is 'source'. >> >> > + >> > + >> >> Don't get deceived by my comments; this is a pretty good start. >> >> I wish I had studied this part before PATCH 1. >> >> > === Documentation comments === >> > >> > A multi-line comment that starts and ends with a '##' line is a >> >> I intend to look at the remainder shortly. > > Ok. I'll prepare for a context switch to actually be able to address > your comments on the other patches and to figure out what I had already > addressed in my branch during your last review attempt. I intend to look at the remainder of PATCH 5 this afternoon. > I thought I had done a better than average job on documenting the code > (at least compare to my other patches), but doesn't seem so... Writing excellent documentation for code you just wrote is *hard*! I think yours was pretty good, actually.
Kevin Wolf <kwolf@redhat.com> writes: > Introduce alias definitions for object types (structs and unions). This > allows using the same QAPI type and visitor for many syntax variations > that exist in the external representation, like between QMP and the > command line. It also provides a new tool for evolving the schema while > maintaining backwards compatibility during a deprecation period. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- [...] > A multi-line comment that starts and ends with a '##' line is a > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > index e03abcbb95..6c94c01148 100644 > --- a/docs/sphinx/qapidoc.py > +++ b/docs/sphinx/qapidoc.py > @@ -310,7 +310,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor): > + self._nodes_for_if_section(ifcond)) > > def visit_object_type(self, name, info, ifcond, features, > - base, members, variants): > + base, members, variants, aliases): > doc = self._cur_doc > if base and base.is_implicit(): > base = None > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 2fcaaa2497..21fded529b 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -198,6 +198,32 @@ def check_features(features, info): > check_if(f, info, source) > > > +def check_aliases(aliases, info): > + if aliases is None: > + return > + if not isinstance(aliases, list): > + raise QAPISemError(info, "'aliases' must be an array") > + for a in aliases: > + if not isinstance(a, dict): > + raise QAPISemError(info, "'aliases' entries must be objects") > + check_keys(a, info, "alias", ['source'], ['alias']) > + > + if 'alias' in a: > + source = "alias member 'alias'" > + check_name_is_str(a['alias'], info, source) > + check_name_str(a['alias'], info, source) > + > + if not isinstance(a['source'], list): > + raise QAPISemError(info, "'source' must be an array") > + if not a['source']: > + raise QAPISemError(info, "'source' must not be empty") > + > + source = "element of alias member 'source'" > + for s in a['source']: > + check_name_is_str(s, info, source) > + check_name_str(s, info, source) > + > + > def check_enum(expr, info): > name = expr['enum'] > members = expr['data'] > @@ -228,6 +254,7 @@ def check_struct(expr, info): > > check_type(members, info, "'data'", allow_dict=name) > check_type(expr.get('base'), info, "'base'") > + check_aliases(expr.get('aliases'), info) > > > def check_union(expr, info): > @@ -245,6 +272,8 @@ def check_union(expr, info): > raise QAPISemError(info, "'discriminator' requires 'base'") > check_name_is_str(discriminator, info, "'discriminator'") > > + check_aliases(expr.get('aliases'), info) > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > check_name_str(key, info, source) > @@ -331,7 +360,7 @@ def check_exprs(exprs): > elif meta == 'union': > check_keys(expr, info, meta, > ['union', 'data'], > - ['base', 'discriminator', 'if', 'features']) > + ['base', 'discriminator', 'if', 'features', 'aliases']) > normalize_members(expr.get('base')) > normalize_members(expr['data']) > check_union(expr, info) > @@ -342,7 +371,8 @@ def check_exprs(exprs): > check_alternate(expr, info) > elif meta == 'struct': > check_keys(expr, info, meta, > - ['struct', 'data'], ['base', 'if', 'features']) > + ['struct', 'data'], > + ['base', 'if', 'features', 'aliases']) > normalize_members(expr['data']) > check_struct(expr, info) > elif meta == 'command': > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 720449feee..5daa137163 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -117,7 +117,7 @@ class QAPISchemaVisitor: > pass > > def visit_object_type(self, name, info, ifcond, features, > - base, members, variants): > + base, members, variants, aliases): > pass > > def visit_object_type_flat(self, name, info, ifcond, features, > @@ -331,9 +331,16 @@ class QAPISchemaArrayType(QAPISchemaType): > return "%s type ['%s']" % (self.meta, self._element_type_name) > > > +class QAPISchemaAlias: > + def __init__(self, alias, source): > + assert source > + self.alias = alias > + self.source = source The existing QAPISchemaFOO.__init__() assert the arguments are sane. I expect John's type hinting work to replace these assertions. Adding type hints is much easier when you have assertions to replace. Let's add them here, too. > + > + > class QAPISchemaObjectType(QAPISchemaType): > def __init__(self, name, info, doc, ifcond, features, > - base, local_members, variants): > + base, local_members, variants, aliases=None): > # struct has local_members, optional base, and no variants > # flat union has base, variants, and no local_members > # simple union has local_members, variants, and no base > @@ -351,6 +358,7 @@ class QAPISchemaObjectType(QAPISchemaType): > self.local_members = local_members > self.variants = variants > self.members = None > + self.aliases = aliases or [] > > def check(self, schema): > # This calls another type T's .check() exactly when the C > @@ -443,7 +451,7 @@ class QAPISchemaObjectType(QAPISchemaType): > super().visit(visitor) > visitor.visit_object_type( > self.name, self.info, self.ifcond, self.features, > - self.base, self.local_members, self.variants) > + self.base, self.local_members, self.variants, self.aliases) > visitor.visit_object_type_flat( > self.name, self.info, self.ifcond, self.features, > self.members, self.variants) > @@ -934,6 +942,12 @@ class QAPISchema: > return [QAPISchemaFeature(f['name'], info, f.get('if')) > for f in features] > > + def _make_aliases(self, aliases): > + if aliases is None: > + return [] > + return [QAPISchemaAlias(a.get('alias'), a['source']) > + for a in aliases] > + > def _make_enum_members(self, values, info): > return [QAPISchemaEnumMember(v['name'], info, v.get('if')) > for v in values] > @@ -1008,11 +1022,12 @@ class QAPISchema: > base = expr.get('base') > data = expr['data'] > ifcond = expr.get('if') > + aliases = self._make_aliases(expr.get('aliases')) > features = self._make_features(expr.get('features'), info) > self._def_entity(QAPISchemaObjectType( > name, info, doc, ifcond, features, base, > self._make_members(data, info), > - None)) > + None, aliases)) > > def _make_variant(self, case, typ, ifcond, info): > return QAPISchemaVariant(case, info, typ, ifcond) > @@ -1031,6 +1046,7 @@ class QAPISchema: > data = expr['data'] > base = expr.get('base') > ifcond = expr.get('if') > + aliases = self._make_aliases(expr.get('aliases')) > features = self._make_features(expr.get('features'), info) > tag_name = expr.get('discriminator') > tag_member = None > @@ -1055,7 +1071,8 @@ class QAPISchema: > QAPISchemaObjectType(name, info, doc, ifcond, features, > base, members, > QAPISchemaVariants( > - tag_name, info, tag_member, variants))) > + tag_name, info, tag_member, variants), > + aliases)) > > def _def_alternate_type(self, expr, info, doc): > name = expr['alternate'] > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 2b4916cdaa..e870bebb7e 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -25,6 +25,7 @@ from .common import ( > from .gen import QAPISchemaModularCVisitor, ifcontext > from .schema import ( > QAPISchema, > + QAPISchemaAlias, > QAPISchemaEnumMember, > QAPISchemaFeature, > QAPISchemaObjectType, > @@ -332,7 +333,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): > features: List[QAPISchemaFeature], > base: Optional[QAPISchemaObjectType], > members: List[QAPISchemaObjectTypeMember], > - variants: Optional[QAPISchemaVariants]) -> None: > + variants: Optional[QAPISchemaVariants], > + aliases: List[QAPISchemaAlias]) -> None: > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 339f152152..a35921ef2c 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -26,6 +26,7 @@ from .common import ( > from .gen import QAPISchemaModularCVisitor, ifcontext > from .schema import ( > QAPISchema, > + QAPISchemaAlias, > QAPISchemaEnumMember, > QAPISchemaEnumType, > QAPISchemaFeature, > @@ -60,7 +61,8 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp); > def gen_visit_object_members(name: str, > base: Optional[QAPISchemaObjectType], > members: List[QAPISchemaObjectTypeMember], > - variants: Optional[QAPISchemaVariants]) -> str: > + variants: Optional[QAPISchemaVariants], > + aliases: List[QAPISchemaAlias]) -> str: > ret = mcgen(''' > > bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > @@ -68,6 +70,25 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > ''', > c_name=c_name(name)) > > + if aliases: > + ret += mcgen(''' > + visit_start_alias_scope(v); > +''') > + > + for a in aliases: > + if a.alias: > + alias = '"%s"' % a.alias > + else: > + alias = "NULL" > + > + source_list = ", ".join('"%s"' % x for x in a.source) > + source = "(const char * []) { %s, NULL }" % source_list > + > + ret += mcgen(''' > + visit_define_alias(v, %(alias)s, %(source)s); > +''', > + alias=alias, source=source) > + I prefer putting C code into mcgen() as much as practical. Like this: if a.alias: alias = '"%s"' % a.alias else: alias = "NULL" source = ", ".join('"%s"' % x for x in a.source) ret += mcgen(''' visit_define_alias(v, %(alias)s, (const char * []){ %(source), NULL }); ''', alias=alias, source=source) > if base: > ret += mcgen(''' > if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) { > @@ -133,6 +154,11 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > } > ''') > > + if aliases: > + ret += mcgen(''' > + visit_end_alias_scope(v); > +''') > + > ret += mcgen(''' > return true; > } > @@ -361,14 +387,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): > features: List[QAPISchemaFeature], > base: Optional[QAPISchemaObjectType], > members: List[QAPISchemaObjectTypeMember], > - variants: Optional[QAPISchemaVariants]) -> None: > + variants: Optional[QAPISchemaVariants], > + aliases: List[QAPISchemaAlias]) -> None: > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > with ifcontext(ifcond, self._genh, self._genc): > self._genh.add(gen_visit_members_decl(name)) > self._genc.add(gen_visit_object_members(name, base, > - members, variants)) > + members, variants, aliases)) Long line. Recommend hanging indent: self._genc.add(gen_visit_object_members( name, base, members, variants, aliases)) > # TODO Worth changing the visitor signature, so we could > # directly use rather than repeat type.is_implicit()? > if not name.startswith('q_'): > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index e8db9d09d9..adf8bda89a 100755 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -47,7 +47,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > self._print_if(ifcond) > > def visit_object_type(self, name, info, ifcond, features, > - base, members, variants): > + base, members, variants, aliases): > print('object %s' % name) > if base: > print(' base %s' % base.name) > @@ -56,6 +56,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > % (m.name, m.type.name, m.optional)) > self._print_if(m.ifcond, 8) > self._print_features(m.features, indent=8) > + for a in aliases: > + if a.alias: > + print(' alias %s -> %s' % (a.alias, '/'.join(a.source))) > + else: > + print(' alias * -> %s/*' % '/'.join(a.source)) For better or worse, we use '.' as a separator elsewhere. > self._print_variants(variants) > self._print_if(ifcond) > self._print_features(features) > diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err > index 71fc4dbb52..5d25d7623c 100644 > --- a/tests/qapi-schema/double-type.err > +++ b/tests/qapi-schema/double-type.err > @@ -1,3 +1,3 @@ > double-type.json: In struct 'bar': > double-type.json:2: struct has unknown key 'command' > -Valid keys are 'base', 'data', 'features', 'if', 'struct'. > +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'. > diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err > index c5f395bf79..7429d1ff03 100644 > --- a/tests/qapi-schema/unknown-expr-key.err > +++ b/tests/qapi-schema/unknown-expr-key.err > @@ -1,3 +1,3 @@ > unknown-expr-key.json: In struct 'bar': > unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony' > -Valid keys are 'base', 'data', 'features', 'if', 'struct'. > +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'. Straightforward, easy to review. Appreciated!
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 6906a06ad2..6da14d5275 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -231,7 +231,8 @@ Syntax: 'data': MEMBERS, '*base': STRING, '*if': COND, - '*features': FEATURES } + '*features': FEATURES, + '*aliases': ALIASES } MEMBERS = { MEMBER, ... } MEMBER = STRING : TYPE-REF | STRING : { 'type': TYPE-REF, @@ -286,13 +287,15 @@ Syntax: UNION = { 'union': STRING, 'data': BRANCHES, '*if': COND, - '*features': FEATURES } + '*features': FEATURES, + '*aliases': ALIASES } | { 'union': STRING, 'data': BRANCHES, 'base': ( MEMBERS | STRING ), 'discriminator': STRING, '*if': COND, - '*features': FEATURES } + '*features': FEATURES, + '*aliases': ALIASES } BRANCHES = { BRANCH, ... } BRANCH = STRING : TYPE-REF | STRING : { 'type': TYPE-REF, '*if': COND } @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in this particular build. +=== Aliases === + +Object types, including structs and unions, can contain alias +definitions. + +Aliases define alternative member names that may be used in the +external representation to provide a value for a member in the same +object or in a nested object. + +Syntax: + ALIAS = { '*alias': STRING, + 'source': [ STRING, ... ] } + +'source' is a list of member names representing the path to an object +member, starting from the type where the alias definition is +specified. It may refer to another alias name. It is allowed to use +a path that doesn't necessarily match an existing member in every +variant or even at all; in this case, the alias remains unused. + +If 'alias' is present, then the single member referred to by 'source' +is made accessible with the name given in 'alias' in the type where +the alias definition is specified. + +If 'alias' is not present, then all members in the object referred to +by 'source' are made accessible in the type where the alias definition +is specified with the same name as they have in 'source'. + + === Documentation comments === A multi-line comment that starts and ends with a '##' line is a diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py index e03abcbb95..6c94c01148 100644 --- a/docs/sphinx/qapidoc.py +++ b/docs/sphinx/qapidoc.py @@ -310,7 +310,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor): + self._nodes_for_if_section(ifcond)) def visit_object_type(self, name, info, ifcond, features, - base, members, variants): + base, members, variants, aliases): doc = self._cur_doc if base and base.is_implicit(): base = None diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 2fcaaa2497..21fded529b 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -198,6 +198,32 @@ def check_features(features, info): check_if(f, info, source) +def check_aliases(aliases, info): + if aliases is None: + return + if not isinstance(aliases, list): + raise QAPISemError(info, "'aliases' must be an array") + for a in aliases: + if not isinstance(a, dict): + raise QAPISemError(info, "'aliases' entries must be objects") + check_keys(a, info, "alias", ['source'], ['alias']) + + if 'alias' in a: + source = "alias member 'alias'" + check_name_is_str(a['alias'], info, source) + check_name_str(a['alias'], info, source) + + if not isinstance(a['source'], list): + raise QAPISemError(info, "'source' must be an array") + if not a['source']: + raise QAPISemError(info, "'source' must not be empty") + + source = "element of alias member 'source'" + for s in a['source']: + check_name_is_str(s, info, source) + check_name_str(s, info, source) + + def check_enum(expr, info): name = expr['enum'] members = expr['data'] @@ -228,6 +254,7 @@ def check_struct(expr, info): check_type(members, info, "'data'", allow_dict=name) check_type(expr.get('base'), info, "'base'") + check_aliases(expr.get('aliases'), info) def check_union(expr, info): @@ -245,6 +272,8 @@ def check_union(expr, info): raise QAPISemError(info, "'discriminator' requires 'base'") check_name_is_str(discriminator, info, "'discriminator'") + check_aliases(expr.get('aliases'), info) + for (key, value) in members.items(): source = "'data' member '%s'" % key check_name_str(key, info, source) @@ -331,7 +360,7 @@ def check_exprs(exprs): elif meta == 'union': check_keys(expr, info, meta, ['union', 'data'], - ['base', 'discriminator', 'if', 'features']) + ['base', 'discriminator', 'if', 'features', 'aliases']) normalize_members(expr.get('base')) normalize_members(expr['data']) check_union(expr, info) @@ -342,7 +371,8 @@ def check_exprs(exprs): check_alternate(expr, info) elif meta == 'struct': check_keys(expr, info, meta, - ['struct', 'data'], ['base', 'if', 'features']) + ['struct', 'data'], + ['base', 'if', 'features', 'aliases']) normalize_members(expr['data']) check_struct(expr, info) elif meta == 'command': diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 720449feee..5daa137163 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -117,7 +117,7 @@ class QAPISchemaVisitor: pass def visit_object_type(self, name, info, ifcond, features, - base, members, variants): + base, members, variants, aliases): pass def visit_object_type_flat(self, name, info, ifcond, features, @@ -331,9 +331,16 @@ class QAPISchemaArrayType(QAPISchemaType): return "%s type ['%s']" % (self.meta, self._element_type_name) +class QAPISchemaAlias: + def __init__(self, alias, source): + assert source + self.alias = alias + self.source = source + + class QAPISchemaObjectType(QAPISchemaType): def __init__(self, name, info, doc, ifcond, features, - base, local_members, variants): + base, local_members, variants, aliases=None): # struct has local_members, optional base, and no variants # flat union has base, variants, and no local_members # simple union has local_members, variants, and no base @@ -351,6 +358,7 @@ class QAPISchemaObjectType(QAPISchemaType): self.local_members = local_members self.variants = variants self.members = None + self.aliases = aliases or [] def check(self, schema): # This calls another type T's .check() exactly when the C @@ -443,7 +451,7 @@ class QAPISchemaObjectType(QAPISchemaType): super().visit(visitor) visitor.visit_object_type( self.name, self.info, self.ifcond, self.features, - self.base, self.local_members, self.variants) + self.base, self.local_members, self.variants, self.aliases) visitor.visit_object_type_flat( self.name, self.info, self.ifcond, self.features, self.members, self.variants) @@ -934,6 +942,12 @@ class QAPISchema: return [QAPISchemaFeature(f['name'], info, f.get('if')) for f in features] + def _make_aliases(self, aliases): + if aliases is None: + return [] + return [QAPISchemaAlias(a.get('alias'), a['source']) + for a in aliases] + def _make_enum_members(self, values, info): return [QAPISchemaEnumMember(v['name'], info, v.get('if')) for v in values] @@ -1008,11 +1022,12 @@ class QAPISchema: base = expr.get('base') data = expr['data'] ifcond = expr.get('if') + aliases = self._make_aliases(expr.get('aliases')) features = self._make_features(expr.get('features'), info) self._def_entity(QAPISchemaObjectType( name, info, doc, ifcond, features, base, self._make_members(data, info), - None)) + None, aliases)) def _make_variant(self, case, typ, ifcond, info): return QAPISchemaVariant(case, info, typ, ifcond) @@ -1031,6 +1046,7 @@ class QAPISchema: data = expr['data'] base = expr.get('base') ifcond = expr.get('if') + aliases = self._make_aliases(expr.get('aliases')) features = self._make_features(expr.get('features'), info) tag_name = expr.get('discriminator') tag_member = None @@ -1055,7 +1071,8 @@ class QAPISchema: QAPISchemaObjectType(name, info, doc, ifcond, features, base, members, QAPISchemaVariants( - tag_name, info, tag_member, variants))) + tag_name, info, tag_member, variants), + aliases)) def _def_alternate_type(self, expr, info, doc): name = expr['alternate'] diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 2b4916cdaa..e870bebb7e 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -25,6 +25,7 @@ from .common import ( from .gen import QAPISchemaModularCVisitor, ifcontext from .schema import ( QAPISchema, + QAPISchemaAlias, QAPISchemaEnumMember, QAPISchemaFeature, QAPISchemaObjectType, @@ -332,7 +333,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): features: List[QAPISchemaFeature], base: Optional[QAPISchemaObjectType], members: List[QAPISchemaObjectTypeMember], - variants: Optional[QAPISchemaVariants]) -> None: + variants: Optional[QAPISchemaVariants], + aliases: List[QAPISchemaAlias]) -> None: # Nothing to do for the special empty builtin if name == 'q_empty': return diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 339f152152..a35921ef2c 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -26,6 +26,7 @@ from .common import ( from .gen import QAPISchemaModularCVisitor, ifcontext from .schema import ( QAPISchema, + QAPISchemaAlias, QAPISchemaEnumMember, QAPISchemaEnumType, QAPISchemaFeature, @@ -60,7 +61,8 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp); def gen_visit_object_members(name: str, base: Optional[QAPISchemaObjectType], members: List[QAPISchemaObjectTypeMember], - variants: Optional[QAPISchemaVariants]) -> str: + variants: Optional[QAPISchemaVariants], + aliases: List[QAPISchemaAlias]) -> str: ret = mcgen(''' bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) @@ -68,6 +70,25 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) ''', c_name=c_name(name)) + if aliases: + ret += mcgen(''' + visit_start_alias_scope(v); +''') + + for a in aliases: + if a.alias: + alias = '"%s"' % a.alias + else: + alias = "NULL" + + source_list = ", ".join('"%s"' % x for x in a.source) + source = "(const char * []) { %s, NULL }" % source_list + + ret += mcgen(''' + visit_define_alias(v, %(alias)s, %(source)s); +''', + alias=alias, source=source) + if base: ret += mcgen(''' if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) { @@ -133,6 +154,11 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) } ''') + if aliases: + ret += mcgen(''' + visit_end_alias_scope(v); +''') + ret += mcgen(''' return true; } @@ -361,14 +387,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): features: List[QAPISchemaFeature], base: Optional[QAPISchemaObjectType], members: List[QAPISchemaObjectTypeMember], - variants: Optional[QAPISchemaVariants]) -> None: + variants: Optional[QAPISchemaVariants], + aliases: List[QAPISchemaAlias]) -> None: # Nothing to do for the special empty builtin if name == 'q_empty': return with ifcontext(ifcond, self._genh, self._genc): self._genh.add(gen_visit_members_decl(name)) self._genc.add(gen_visit_object_members(name, base, - members, variants)) + members, variants, aliases)) # TODO Worth changing the visitor signature, so we could # directly use rather than repeat type.is_implicit()? if not name.startswith('q_'): diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index e8db9d09d9..adf8bda89a 100755 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -47,7 +47,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): self._print_if(ifcond) def visit_object_type(self, name, info, ifcond, features, - base, members, variants): + base, members, variants, aliases): print('object %s' % name) if base: print(' base %s' % base.name) @@ -56,6 +56,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): % (m.name, m.type.name, m.optional)) self._print_if(m.ifcond, 8) self._print_features(m.features, indent=8) + for a in aliases: + if a.alias: + print(' alias %s -> %s' % (a.alias, '/'.join(a.source))) + else: + print(' alias * -> %s/*' % '/'.join(a.source)) self._print_variants(variants) self._print_if(ifcond) self._print_features(features) diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err index 71fc4dbb52..5d25d7623c 100644 --- a/tests/qapi-schema/double-type.err +++ b/tests/qapi-schema/double-type.err @@ -1,3 +1,3 @@ double-type.json: In struct 'bar': double-type.json:2: struct has unknown key 'command' -Valid keys are 'base', 'data', 'features', 'if', 'struct'. +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'. diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err index c5f395bf79..7429d1ff03 100644 --- a/tests/qapi-schema/unknown-expr-key.err +++ b/tests/qapi-schema/unknown-expr-key.err @@ -1,3 +1,3 @@ unknown-expr-key.json: In struct 'bar': unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony' -Valid keys are 'base', 'data', 'features', 'if', 'struct'. +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
Introduce alias definitions for object types (structs and unions). This allows using the same QAPI type and visitor for many syntax variations that exist in the external representation, like between QMP and the command line. It also provides a new tool for evolving the schema while maintaining backwards compatibility during a deprecation period. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++--- docs/sphinx/qapidoc.py | 2 +- scripts/qapi/expr.py | 34 +++++++++++++++++++++-- scripts/qapi/schema.py | 27 +++++++++++++++---- scripts/qapi/types.py | 4 ++- scripts/qapi/visit.py | 33 ++++++++++++++++++++--- tests/qapi-schema/test-qapi.py | 7 ++++- tests/qapi-schema/double-type.err | 2 +- tests/qapi-schema/unknown-expr-key.err | 2 +- 9 files changed, 130 insertions(+), 18 deletions(-)