| Message ID | 20180509165530.29561-2-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Try to create well typed json:{} filenames | expand |
On 05/09/2018 11:55 AM, Max Reitz wrote: > This patch allows specifying a discriminator that is an optional member > of the base struct. In such a case, a default value must be provided > that is used when no value is given. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qapi/introspect.json | 8 ++++++ > scripts/qapi/common.py | 57 ++++++++++++++++++++++++++++++++++-------- > scripts/qapi/doc.py | 8 ++++-- > scripts/qapi/introspect.py | 10 +++++--- > scripts/qapi/visit.py | 33 ++++++++++++++++++++++-- > tests/qapi-schema/test-qapi.py | 2 ++ > 6 files changed, 101 insertions(+), 17 deletions(-) I've been threatening that we might need this for some time, so I'm glad to see it being implemented. We'll see if the tests in 2 and 3 cover the code added here. > > diff --git a/qapi/introspect.json b/qapi/introspect.json > index c7f67b7d78..2d7b1e3745 100644 > --- a/qapi/introspect.json > +++ b/qapi/introspect.json > @@ -168,6 +168,13 @@ > # @tag: the name of the member serving as type tag. > # An element of @members with this name must exist. > # > +# @default-variant: if the @tag element of @members is optional, this > +# is the default value for choosing a variant. Its > +# value must be a valid value for @tag. Perhaps s/must/will/ as this struct is used for output (and therefore we always meet the condition, rather than the user having to do something correctly). Nice that you remembered introspection. > +# Present exactly when @tag is present and the > +# associated element of @members is optional. > +# (Since: 2.13) > +# > # @variants: variant members, i.e. additional members that > # depend on the type tag's value. Present exactly when > # @tag is present. The variants are in no particular order, > @@ -181,6 +188,7 @@ > { 'struct': 'SchemaInfoObject', > 'data': { 'members': [ 'SchemaInfoObjectMember' ], > '*tag': 'str', > + '*default-variant': 'str', > '*variants': [ 'SchemaInfoObjectVariant' ] } } > > ## > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index a032cec375..fbf0244f73 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -721,6 +721,7 @@ def check_union(expr, info): > name = expr['union'] > base = expr.get('base') > discriminator = expr.get('discriminator') > + default_variant = expr.get('default-variant') > members = expr['data'] > > # Two types of unions, determined by discriminator. > @@ -745,16 +746,37 @@ def check_union(expr, info): > base_members = find_base_members(base) > assert base_members is not None > > - # The value of member 'discriminator' must name a non-optional > - # member of the base struct. > + # The value of member 'discriminator' must name a member of > + # the base struct. > check_name(info, "Discriminator of flat union '%s'" % name, > discriminator) > - discriminator_type = base_members.get(discriminator) > - if not discriminator_type: > - raise QAPISemError(info, > - "Discriminator '%s' is not a member of base " > - "struct '%s'" > - % (discriminator, base)) > + if default_variant is None: > + discriminator_type = base_members.get(discriminator) > + if not discriminator_type: > + if base_members.get('*' + discriminator) is None: Unrelated to your patch, but this reminds me - we already had a question on list about whether we should allow ANY member of the base struct, rather than the members directly declared in the struct. (The use case was that if you have a grandparent struct, then an intermediate base struct, the code as written does not permit the discriminator to come from the grandparent struct, which is awkward - but if I also recall correctly, the question came up in relation to the query-cpus-fast command, where we ended up not refactoring things after all due to deprecation of query-cpus). > + raise QAPISemError(info, > + "Discriminator '%s' is not a member of " > + "base struct '%s'" > + % (discriminator, base)) > + else: > + raise QAPISemError(info, > + "Default variant must be specified for " > + "optional discriminator '%s'" > + % discriminator) > + else: > + discriminator_type = base_members.get('*' + discriminator) > + if not discriminator_type: > + if base_members.get(discriminator) is None: > + raise QAPISemError(info, > + "Discriminator '%s' is not a member of " > + "base struct '%s'" > + % (discriminator, base)) > + else: > + raise QAPISemError(info, > + "Must not specify a default variant for " > + "non-optional discriminator '%s'" > + % discriminator) > + Looks like you've got good error messages for all the cases of enforcing the rule that default-variant and optional member must be used together. > enum_define = enum_types.get(discriminator_type) > allow_metas = ['struct'] > # Do not allow string discriminator > @@ -763,6 +785,15 @@ def check_union(expr, info): > "Discriminator '%s' must be of enumeration " > "type" % discriminator) > > + if default_variant is not None: > + # Must be a value of the enumeration > + if default_variant not in enum_define['data']: > + raise QAPISemError(info, > + "Default variant '%s' of flat union '%s' is " > + "not part of '%s'" > + % (default_variant, name, > + discriminator_type)) > + > # Check every branch; don't allow an empty union > if len(members) == 0: > raise QAPISemError(info, "Union '%s' cannot have empty 'data'" % name) > @@ -909,7 +940,7 @@ def check_exprs(exprs): > elif 'union' in expr: > meta = 'union' > check_keys(expr_elem, 'union', ['data'], > - ['base', 'discriminator']) > + ['base', 'discriminator', 'default-variant']) > union_types[expr[meta]] = expr > elif 'alternate' in expr: > meta = 'alternate' > @@ -1335,12 +1366,14 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember): > > > class QAPISchemaObjectTypeVariants(object): > - def __init__(self, tag_name, tag_member, variants): > + def __init__(self, tag_name, tag_member, default_tag_value, variants): > # Flat unions pass tag_name but not tag_member. > # Simple unions and alternates pass tag_member but not tag_name. > # After check(), tag_member is always set, and tag_name remains > # a reliable witness of being used by a flat union. > assert bool(tag_member) != bool(tag_name) > + # default_tag_value is only passed for flat unions. > + assert bool(tag_name) or not bool(default_tag_value) Looks correct. > assert (isinstance(tag_name, str) or > isinstance(tag_member, QAPISchemaObjectTypeMember)) > assert len(variants) > 0 > @@ -1348,6 +1381,7 @@ class QAPISchemaObjectTypeVariants(object): > assert isinstance(v, QAPISchemaObjectTypeVariant) > self._tag_name = tag_name > self.tag_member = tag_member > + self.default_tag_value = default_tag_value > self.variants = variants > > def set_owner(self, name): > @@ -1637,6 +1671,7 @@ class QAPISchema(object): > data = expr['data'] > base = expr.get('base') > tag_name = expr.get('discriminator') > + default_tag_value = expr.get('default-variant') > tag_member = None > if isinstance(base, dict): > base = (self._make_implicit_object_type( > @@ -1656,6 +1691,7 @@ class QAPISchema(object): > QAPISchemaObjectType(name, info, doc, base, members, > QAPISchemaObjectTypeVariants(tag_name, > tag_member, > + default_tag_value, > variants))) > > def _def_alternate_type(self, expr, info, doc): > @@ -1668,6 +1704,7 @@ class QAPISchema(object): > QAPISchemaAlternateType(name, info, doc, > QAPISchemaObjectTypeVariants(None, > tag_member, > + None, > variants))) > > def _def_command(self, expr, info, doc): > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py > index 9b312b2c51..91204dc4c6 100644 > --- a/scripts/qapi/doc.py > +++ b/scripts/qapi/doc.py > @@ -160,8 +160,12 @@ def texi_members(doc, what, base, variants, member_func): > items += '@item The members of @code{%s}\n' % base.doc_type() > if variants: > for v in variants.variants: > - when = ' when @code{%s} is @t{"%s"}' % ( > - variants.tag_member.name, v.name) > + if v.name == variants.default_tag_value: > + when = ' when @code{%s} is @t{"%s"} or not given' % ( > + variants.tag_member.name, v.name) Nice, you got the generated docs as well as introspection. > + else: > + when = ' when @code{%s} is @t{"%s"}' % ( > + variants.tag_member.name, v.name) > if v.type.is_implicit(): > assert not v.type.base and not v.type.variants > for m in v.type.local_members: > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index f9e67e8227..2d1d4e320a 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -142,9 +142,12 @@ const QLitObject %(c_name)s = %(c_string)s; > ret['default'] = None > return ret > > - def _gen_variants(self, tag_name, variants): > - return {'tag': tag_name, > - 'variants': [self._gen_variant(v) for v in variants]} > + def _gen_variants(self, tag_name, default_variant, variants): > + ret = {'tag': tag_name, > + 'variants': [self._gen_variant(v) for v in variants]} > + if default_variant: > + ret['default-variant'] = default_variant > + return ret > > def _gen_variant(self, variant): > return {'case': variant.name, 'type': self._use_type(variant.type)} > @@ -163,6 +166,7 @@ const QLitObject %(c_name)s = %(c_string)s; > obj = {'members': [self._gen_member(m) for m in members]} > if variants: > obj.update(self._gen_variants(variants.tag_member.name, > + variants.default_tag_value, > variants.variants)) > self._gen_qlit(name, 'object', obj) > > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 5d72d8936c..ecffc46bd3 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, variants): > void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > { > Error *err = NULL; > - > ''', > c_name=c_name(name)) > > + if variants: > + ret += mcgen(''' > + %(c_type)s %(c_name)s; > +''', > + c_type=variants.tag_member.type.c_name(), > + c_name=c_name(variants.tag_member.name)) > + > + ret += mcgen(''' > + > +''') > + Creating a temp variable makes it easier to handle the default... > if base: > ret += mcgen(''' > visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err); > @@ -75,8 +85,27 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > ''') > > if variants: > + if variants.default_tag_value is None: > + ret += mcgen(''' > + %(c_name)s = obj->%(c_name)s; > +''', > + c_name=c_name(variants.tag_member.name)) > + else: > + ret += mcgen(''' > + if (obj->has_%(c_name)s) { > + %(c_name)s = obj->%(c_name)s; > + } else { > + %(c_name)s = %(enum_const)s; > + } > +''', > + c_name=c_name(variants.tag_member.name), > + enum_const=c_enum_const( > + variants.tag_member.type.name, > + variants.default_tag_value, > + variants.tag_member.type.prefix)) > + > ret += mcgen(''' > - switch (obj->%(c_name)s) { > + switch (%(c_name)s) { ...compared to the old code that just inlined the one thing that used to be assigned to what is now the temporary variable. It might be possible to inline things so that the generated code reads either: switch (obj->discriminator) { switch (!obj->has_discriminator ? DEFAULT : obj->discriminator) { but I don't think it's worth the effort; and the temporary variable is fine even though it makes the generated file bigger. > ''', > c_name=c_name(variants.tag_member.name)) > > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index c1a144ba29..f2a072b92e 100644 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > def _print_variants(variants): > if variants: > print(' tag %s' % variants.tag_member.name) > + if variants.default_tag_value: > + print(' default variant: %s' % variants.default_tag_value) > for v in variants.variants: > print(' case %s: %s' % (v.name, v.type.name)) > > Looks good! Reviewed-by: Eric Blake <eblake@redhat.com>
On 05/10/2018 08:12 AM, Eric Blake wrote: Oh, I just had a thought: >> +++ b/scripts/qapi/visit.py >> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, >> if variants: >> + if variants.default_tag_value is None: >> + ret += mcgen(''' >> + %(c_name)s = obj->%(c_name)s; >> +''', >> + c_name=c_name(variants.tag_member.name)) >> + else: >> + ret += mcgen(''' >> + if (obj->has_%(c_name)s) { >> + %(c_name)s = obj->%(c_name)s; >> + } else { >> + %(c_name)s = %(enum_const)s; In this branch of code, is it worth also generating: %has_(c_name)s = true; That way, the rest of the C code does not have to check has_discriminator, because the process of assigning the default will ensure that has_discriminator is always true later on. It does have the effect that output would never omit the discriminator - but that might be a good thing: if we ever have an output union that used to have a mandatory discriminator and want to now make it optional, we don't want to break older clients that expected the discriminator to be present. It does obscure whether input relied on the default, but I don't think that hurts.
On 2018-05-10 15:12, Eric Blake wrote: > On 05/09/2018 11:55 AM, Max Reitz wrote: >> This patch allows specifying a discriminator that is an optional member >> of the base struct. In such a case, a default value must be provided >> that is used when no value is given. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qapi/introspect.json | 8 ++++++ >> scripts/qapi/common.py | 57 >> ++++++++++++++++++++++++++++++++++-------- >> scripts/qapi/doc.py | 8 ++++-- >> scripts/qapi/introspect.py | 10 +++++--- >> scripts/qapi/visit.py | 33 ++++++++++++++++++++++-- >> tests/qapi-schema/test-qapi.py | 2 ++ >> 6 files changed, 101 insertions(+), 17 deletions(-) > > I've been threatening that we might need this for some time, so I'm glad > to see it being implemented. We'll see if the tests in 2 and 3 cover > the code added here. > >> >> diff --git a/qapi/introspect.json b/qapi/introspect.json >> index c7f67b7d78..2d7b1e3745 100644 >> --- a/qapi/introspect.json >> +++ b/qapi/introspect.json >> @@ -168,6 +168,13 @@ >> # @tag: the name of the member serving as type tag. >> # An element of @members with this name must exist. >> # >> +# @default-variant: if the @tag element of @members is optional, this >> +# is the default value for choosing a variant. Its >> +# value must be a valid value for @tag. > > Perhaps s/must/will/ as this struct is used for output (and therefore we > always meet the condition, rather than the user having to do something > correctly). I mostly copied from the other descriptions which seemed to prefer "must", but I'm happy with either. > Nice that you remembered introspection. I didn't, because I had no idea how introspection works exactly before this series. But one of the tests broke, thus telling me I might have forgotten something. :-) >> +# Present exactly when @tag is present and the >> +# associated element of @members is optional. >> +# (Since: 2.13) >> +# >> # @variants: variant members, i.e. additional members that >> # depend on the type tag's value. Present exactly when >> # @tag is present. The variants are in no particular order, [...] >> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py >> index 5d72d8936c..ecffc46bd3 100644 >> --- a/scripts/qapi/visit.py >> +++ b/scripts/qapi/visit.py >> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, >> variants): >> void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, >> Error **errp) >> { >> Error *err = NULL; >> - >> ''', >> c_name=c_name(name)) >> + if variants: >> + ret += mcgen(''' >> + %(c_type)s %(c_name)s; >> +''', >> + c_type=variants.tag_member.type.c_name(), >> + c_name=c_name(variants.tag_member.name)) >> + >> + ret += mcgen(''' >> + >> +''') >> + > > Creating a temp variable makes it easier to handle the default... > >> if base: >> ret += mcgen(''' >> visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err); >> @@ -75,8 +85,27 @@ void visit_type_%(c_name)s_members(Visitor *v, >> %(c_name)s *obj, Error **errp) >> ''') >> if variants: >> + if variants.default_tag_value is None: >> + ret += mcgen(''' >> + %(c_name)s = obj->%(c_name)s; >> +''', >> + c_name=c_name(variants.tag_member.name)) >> + else: >> + ret += mcgen(''' >> + if (obj->has_%(c_name)s) { >> + %(c_name)s = obj->%(c_name)s; >> + } else { >> + %(c_name)s = %(enum_const)s; >> + } >> +''', >> + c_name=c_name(variants.tag_member.name), >> + enum_const=c_enum_const( >> + variants.tag_member.type.name, >> + variants.default_tag_value, >> + variants.tag_member.type.prefix)) >> + >> ret += mcgen(''' >> - switch (obj->%(c_name)s) { >> + switch (%(c_name)s) { > > ...compared to the old code that just inlined the one thing that used to > be assigned to what is now the temporary variable. > > It might be possible to inline things so that the generated code reads > either: > > switch (obj->discriminator) { > switch (!obj->has_discriminator ? DEFAULT : obj->discriminator) { > > but I don't think it's worth the effort; and the temporary variable is > fine even though it makes the generated file bigger. I don't really mind either way, but depending on the default value and the discriminator name, using ?: may lead to a rather long line. >> ''', >> c_name=c_name(variants.tag_member.name)) >> diff --git a/tests/qapi-schema/test-qapi.py >> b/tests/qapi-schema/test-qapi.py >> index c1a144ba29..f2a072b92e 100644 >> --- a/tests/qapi-schema/test-qapi.py >> +++ b/tests/qapi-schema/test-qapi.py >> @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): >> def _print_variants(variants): >> if variants: >> print(' tag %s' % variants.tag_member.name) >> + if variants.default_tag_value: >> + print(' default variant: %s' % >> variants.default_tag_value) >> for v in variants.variants: >> print(' case %s: %s' % (v.name, v.type.name)) >> > > Looks good! > Reviewed-by: Eric Blake <eblake@redhat.com> Phew. :-) Thanks! Max
On 2018-05-10 15:18, Eric Blake wrote: > On 05/10/2018 08:12 AM, Eric Blake wrote: > > Oh, I just had a thought: > >>> +++ b/scripts/qapi/visit.py >>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, > >>> if variants: >>> + if variants.default_tag_value is None: >>> + ret += mcgen(''' >>> + %(c_name)s = obj->%(c_name)s; >>> +''', >>> + c_name=c_name(variants.tag_member.name)) >>> + else: >>> + ret += mcgen(''' >>> + if (obj->has_%(c_name)s) { >>> + %(c_name)s = obj->%(c_name)s; >>> + } else { >>> + %(c_name)s = %(enum_const)s; > > In this branch of code, is it worth also generating: > > %has_(c_name)s = true; You mean obj->has_%(c_name)s, and then also set obj->%(c_name)s? > That way, the rest of the C code does not have to check > has_discriminator, because the process of assigning the default will > ensure that has_discriminator is always true later on. It does have the > effect that output would never omit the discriminator - but that might > be a good thing: if we ever have an output union that used to have a > mandatory discriminator and want to now make it optional, we don't want > to break older clients that expected the discriminator to be present. It > does obscure whether input relied on the default, but I don't think that > hurts. Also, it would make code a bit simpler because it can cover the !has_X case under X == default_X. But OTOH, you could make that case for any optional value -- except where "is missing" has special value. And that's the tricky part: I think it's hard to say that a missing discriminator never has special meaning. We only need the default-variant so we know which variant of the union to pick; but we don't know that the fact that the discriminator value was missing does not have special meaning. Of course, we can simply foreclose that by setting it here. I don't have money in this game, so I suppose it's yours and Markus's call. :-) Max
On 05/11/2018 12:59 PM, Max Reitz wrote: > On 2018-05-10 15:18, Eric Blake wrote: >> On 05/10/2018 08:12 AM, Eric Blake wrote: >> >> Oh, I just had a thought: >> >>>> +++ b/scripts/qapi/visit.py >>>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, >> >>>> if variants: >>>> + if variants.default_tag_value is None: >>>> + ret += mcgen(''' >>>> + %(c_name)s = obj->%(c_name)s; >>>> +''', >>>> + c_name=c_name(variants.tag_member.name)) >>>> + else: >>>> + ret += mcgen(''' >>>> + if (obj->has_%(c_name)s) { >>>> + %(c_name)s = obj->%(c_name)s; >>>> + } else { >>>> + %(c_name)s = %(enum_const)s; >> >> In this branch of code, is it worth also generating: >> >> %has_(c_name)s = true; > > You mean obj->has_%(c_name)s, and then also set obj->%(c_name)s? Umm, yeah ;) In fact, I think it is as simple as: if variants: if variants.default_tag_value: ret += mcgen(''' if (!obj->has_%(c_name)s) { obj->has_%(c_name)s = true; obj->%(c_name)s = %(enum_const)s; } ''') ret += mcgen(''' switch (obj->%(c_name)s) { ... and you are back to not needing a temporary variable. > >> That way, the rest of the C code does not have to check >> has_discriminator, because the process of assigning the default will >> ensure that has_discriminator is always true later on. It does have the >> effect that output would never omit the discriminator - but that might >> be a good thing: if we ever have an output union that used to have a >> mandatory discriminator and want to now make it optional, we don't want >> to break older clients that expected the discriminator to be present. It >> does obscure whether input relied on the default, but I don't think that >> hurts. > > Also, it would make code a bit simpler because it can cover the !has_X > case under X == default_X. > > But OTOH, you could make that case for any optional value -- except > where "is missing" has special value. My preference - special-casing "is missing" is prone to abuse. I don't want to support that if we can at all avoid it. The sane semantics is that the default is populated as soon as we detect that something is missing, and whether the user relies on the default by leaving the discriminator absent, or explicitly supplies the discriminator set to the default, the behavior should always be the same. > > And that's the tricky part: I think it's hard to say that a missing > discriminator never has special meaning. It won't, if we decide right now that we don't want to let it :) > We only need the > default-variant so we know which variant of the union to pick; but we > don't know that the fact that the discriminator value was missing does > not have special meaning. > > Of course, we can simply foreclose that by setting it here. And that's the way I'm leaning. > > I don't have money in this game, so I suppose it's yours and Markus's > call. :-) Markus, what's your preference?
diff --git a/qapi/introspect.json b/qapi/introspect.json index c7f67b7d78..2d7b1e3745 100644 --- a/qapi/introspect.json +++ b/qapi/introspect.json @@ -168,6 +168,13 @@ # @tag: the name of the member serving as type tag. # An element of @members with this name must exist. # +# @default-variant: if the @tag element of @members is optional, this +# is the default value for choosing a variant. Its +# value must be a valid value for @tag. +# Present exactly when @tag is present and the +# associated element of @members is optional. +# (Since: 2.13) +# # @variants: variant members, i.e. additional members that # depend on the type tag's value. Present exactly when # @tag is present. The variants are in no particular order, @@ -181,6 +188,7 @@ { 'struct': 'SchemaInfoObject', 'data': { 'members': [ 'SchemaInfoObjectMember' ], '*tag': 'str', + '*default-variant': 'str', '*variants': [ 'SchemaInfoObjectVariant' ] } } ## diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index a032cec375..fbf0244f73 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -721,6 +721,7 @@ def check_union(expr, info): name = expr['union'] base = expr.get('base') discriminator = expr.get('discriminator') + default_variant = expr.get('default-variant') members = expr['data'] # Two types of unions, determined by discriminator. @@ -745,16 +746,37 @@ def check_union(expr, info): base_members = find_base_members(base) assert base_members is not None - # The value of member 'discriminator' must name a non-optional - # member of the base struct. + # The value of member 'discriminator' must name a member of + # the base struct. check_name(info, "Discriminator of flat union '%s'" % name, discriminator) - discriminator_type = base_members.get(discriminator) - if not discriminator_type: - raise QAPISemError(info, - "Discriminator '%s' is not a member of base " - "struct '%s'" - % (discriminator, base)) + if default_variant is None: + discriminator_type = base_members.get(discriminator) + if not discriminator_type: + if base_members.get('*' + discriminator) is None: + raise QAPISemError(info, + "Discriminator '%s' is not a member of " + "base struct '%s'" + % (discriminator, base)) + else: + raise QAPISemError(info, + "Default variant must be specified for " + "optional discriminator '%s'" + % discriminator) + else: + discriminator_type = base_members.get('*' + discriminator) + if not discriminator_type: + if base_members.get(discriminator) is None: + raise QAPISemError(info, + "Discriminator '%s' is not a member of " + "base struct '%s'" + % (discriminator, base)) + else: + raise QAPISemError(info, + "Must not specify a default variant for " + "non-optional discriminator '%s'" + % discriminator) + enum_define = enum_types.get(discriminator_type) allow_metas = ['struct'] # Do not allow string discriminator @@ -763,6 +785,15 @@ def check_union(expr, info): "Discriminator '%s' must be of enumeration " "type" % discriminator) + if default_variant is not None: + # Must be a value of the enumeration + if default_variant not in enum_define['data']: + raise QAPISemError(info, + "Default variant '%s' of flat union '%s' is " + "not part of '%s'" + % (default_variant, name, + discriminator_type)) + # Check every branch; don't allow an empty union if len(members) == 0: raise QAPISemError(info, "Union '%s' cannot have empty 'data'" % name) @@ -909,7 +940,7 @@ def check_exprs(exprs): elif 'union' in expr: meta = 'union' check_keys(expr_elem, 'union', ['data'], - ['base', 'discriminator']) + ['base', 'discriminator', 'default-variant']) union_types[expr[meta]] = expr elif 'alternate' in expr: meta = 'alternate' @@ -1335,12 +1366,14 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember): class QAPISchemaObjectTypeVariants(object): - def __init__(self, tag_name, tag_member, variants): + def __init__(self, tag_name, tag_member, default_tag_value, variants): # Flat unions pass tag_name but not tag_member. # Simple unions and alternates pass tag_member but not tag_name. # After check(), tag_member is always set, and tag_name remains # a reliable witness of being used by a flat union. assert bool(tag_member) != bool(tag_name) + # default_tag_value is only passed for flat unions. + assert bool(tag_name) or not bool(default_tag_value) assert (isinstance(tag_name, str) or isinstance(tag_member, QAPISchemaObjectTypeMember)) assert len(variants) > 0 @@ -1348,6 +1381,7 @@ class QAPISchemaObjectTypeVariants(object): assert isinstance(v, QAPISchemaObjectTypeVariant) self._tag_name = tag_name self.tag_member = tag_member + self.default_tag_value = default_tag_value self.variants = variants def set_owner(self, name): @@ -1637,6 +1671,7 @@ class QAPISchema(object): data = expr['data'] base = expr.get('base') tag_name = expr.get('discriminator') + default_tag_value = expr.get('default-variant') tag_member = None if isinstance(base, dict): base = (self._make_implicit_object_type( @@ -1656,6 +1691,7 @@ class QAPISchema(object): QAPISchemaObjectType(name, info, doc, base, members, QAPISchemaObjectTypeVariants(tag_name, tag_member, + default_tag_value, variants))) def _def_alternate_type(self, expr, info, doc): @@ -1668,6 +1704,7 @@ class QAPISchema(object): QAPISchemaAlternateType(name, info, doc, QAPISchemaObjectTypeVariants(None, tag_member, + None, variants))) def _def_command(self, expr, info, doc): diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py index 9b312b2c51..91204dc4c6 100644 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -160,8 +160,12 @@ def texi_members(doc, what, base, variants, member_func): items += '@item The members of @code{%s}\n' % base.doc_type() if variants: for v in variants.variants: - when = ' when @code{%s} is @t{"%s"}' % ( - variants.tag_member.name, v.name) + if v.name == variants.default_tag_value: + when = ' when @code{%s} is @t{"%s"} or not given' % ( + variants.tag_member.name, v.name) + else: + when = ' when @code{%s} is @t{"%s"}' % ( + variants.tag_member.name, v.name) if v.type.is_implicit(): assert not v.type.base and not v.type.variants for m in v.type.local_members: diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index f9e67e8227..2d1d4e320a 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -142,9 +142,12 @@ const QLitObject %(c_name)s = %(c_string)s; ret['default'] = None return ret - def _gen_variants(self, tag_name, variants): - return {'tag': tag_name, - 'variants': [self._gen_variant(v) for v in variants]} + def _gen_variants(self, tag_name, default_variant, variants): + ret = {'tag': tag_name, + 'variants': [self._gen_variant(v) for v in variants]} + if default_variant: + ret['default-variant'] = default_variant + return ret def _gen_variant(self, variant): return {'case': variant.name, 'type': self._use_type(variant.type)} @@ -163,6 +166,7 @@ const QLitObject %(c_name)s = %(c_string)s; obj = {'members': [self._gen_member(m) for m in members]} if variants: obj.update(self._gen_variants(variants.tag_member.name, + variants.default_tag_value, variants.variants)) self._gen_qlit(name, 'object', obj) diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 5d72d8936c..ecffc46bd3 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, variants): void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) { Error *err = NULL; - ''', c_name=c_name(name)) + if variants: + ret += mcgen(''' + %(c_type)s %(c_name)s; +''', + c_type=variants.tag_member.type.c_name(), + c_name=c_name(variants.tag_member.name)) + + ret += mcgen(''' + +''') + if base: ret += mcgen(''' visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err); @@ -75,8 +85,27 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) ''') if variants: + if variants.default_tag_value is None: + ret += mcgen(''' + %(c_name)s = obj->%(c_name)s; +''', + c_name=c_name(variants.tag_member.name)) + else: + ret += mcgen(''' + if (obj->has_%(c_name)s) { + %(c_name)s = obj->%(c_name)s; + } else { + %(c_name)s = %(enum_const)s; + } +''', + c_name=c_name(variants.tag_member.name), + enum_const=c_enum_const( + variants.tag_member.type.name, + variants.default_tag_value, + variants.tag_member.type.prefix)) + ret += mcgen(''' - switch (obj->%(c_name)s) { + switch (%(c_name)s) { ''', c_name=c_name(variants.tag_member.name)) diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index c1a144ba29..f2a072b92e 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): def _print_variants(variants): if variants: print(' tag %s' % variants.tag_member.name) + if variants.default_tag_value: + print(' default variant: %s' % variants.default_tag_value) for v in variants.variants: print(' case %s: %s' % (v.name, v.type.name))
This patch allows specifying a discriminator that is an optional member of the base struct. In such a case, a default value must be provided that is used when no value is given. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qapi/introspect.json | 8 ++++++ scripts/qapi/common.py | 57 ++++++++++++++++++++++++++++++++++-------- scripts/qapi/doc.py | 8 ++++-- scripts/qapi/introspect.py | 10 +++++--- scripts/qapi/visit.py | 33 ++++++++++++++++++++++-- tests/qapi-schema/test-qapi.py | 2 ++ 6 files changed, 101 insertions(+), 17 deletions(-)