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 | 13 ++++++++
tests/qapi-schema/test-qapi.py | 2 ++
6 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 80a0a3e656..b43c87fe8d 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 will be a valid value for @tag.
+# Present exactly when @tag is present and the
+# associated element of @members is optional.
+# (Since: 3.0)
+#
# @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 e82990f0f2..d15f56b260 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)
@@ -910,7 +941,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'
@@ -1336,12 +1367,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
@@ -1349,6 +1382,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):
@@ -1640,6 +1674,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(
@@ -1659,6 +1694,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):
@@ -1671,6 +1707,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 b5630844f9..7dd14173e5 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 5b6c72c7b2..0133f91792 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..4c432f3ead 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -75,6 +75,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
''')
if variants:
+ if variants.default_tag_value is not None:
+ ret += mcgen('''
+ if (!obj->has_%(c_name)s) {
+ obj->has_%(c_name)s = true;
+ obj->%(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) {
''',
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 4512a41504..3609e98348 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))
--
2.17.1
Max Reitz <mreitz@redhat.com> writes:
> 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.
Hmm. Can you explain why you need this feature?
> 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 | 13 ++++++++
> tests/qapi-schema/test-qapi.py | 2 ++
> 6 files changed, 83 insertions(+), 15 deletions(-)
The documentation update is in the next patch, and tests are in the
patch after next. I'd squash all three.
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 80a0a3e656..b43c87fe8d 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 will be a valid value for @tag.
> +# Present exactly when @tag is present and the
> +# associated element of @members is optional.
> +# (Since: 3.0)
> +#
> # @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' ] } }
>
> ##
Until now, the QAPI schema doesn't let you specify default values.
Instead, code sees "absent", and does whatever needs to be done.
Sometimes, it supplies a default value. "Absent" is shorthand for this
value then. Sometimes, it behaves differently than for any value.
"Absent" is a distinct additional case then. I'm not too fond of the
latter case, but it's what we have, and it's not going away.
We've discussed extending the QAPI schema to let you specify default
values. Two advantages: the default value is *specified* rather than
just documented (or not, when documentation sucks), and we can supply
the default value in generated code. The introspection schema is even
prepared for this:
##
# @SchemaInfoObjectMember:
#
# An object member.
#
# @name: the member's name, as defined in the QAPI schema.
#
# @type: the name of the member's type.
#
--> # @default: default when used as command parameter.
# If absent, the parameter is mandatory.
# If present, the value must be null. The parameter is
# optional, and behavior when it's missing is not specified
# here.
# Future extension: if present and non-null, the parameter
# is optional, and defaults to this value.
#
# Since: 2.5
##
The idea was to turn
'*name': 'type'
into sugar for
'name': { 'type': 'type', 'optional': true }
then extend it to
'name': { 'type': 'type', 'optional': true, 'default': JSON-VALUE }
where JSON-VALUE null means behavior for "absent" is not specified.
Your patch adds default values in a different way, and for just for
union tags. For the QAPI language, that's not necessarily a deal
breaker, as we don't need to maintain backward compatibility there. But
for the introspection schema, we do. Once we add @default-variant,
we're stuck with it. Even though it's redundant with
SchemaInfoObjectMember's @default.
I'm skipping review of the implementation for now.
On 2018-06-12 17:25, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> 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.
>
> Hmm. Can you explain why you need this feature?
Define "need".
Most block drivers support exactly the same syntax with -drive and with
-blockdev. Type confusion occurs, but at least the options are the same.
One notable example is rbd's =keyvalue-pairs, but we all probably agree
on the fact that it's just broken and we don't have to care too much.
The other case I noticed (and so far the only other case I know of
besides =keyvalue-pairs) is qcow(2) encryption. The qcow(2) driver
allows you to specify different parameters under @encrypt depending on
the encryption format, so that naturally becomes a flat union with
encrypt.format being the discriminator.
Now when opening a qcow(2) file, the header already gives you the
encryption format (always qcow AES for qcow v1, qcow AES or LUKS for
qcow2), so the user technically doesn't need to specify it. And that is
what the qcow(2) driver allows on the command line: The user does not
need to specify the format as it can be inferred from the header, and
the union is interpreted accordingly. Of course, that doesn't fly with
-blockdev.
Now luckily both AES and LUKS only allow you to specify a key-secret
option. Therefore, with an optional discriminator, we can define a
default variant that just has this key-secret option. This is what this
patch is needed for. It allows full compatibility to the current
interface, although it means we'll have to watch out when adding
encryption options in the future.
The alternative I saw was to decide that auto-detection was a bad idea
and to make encrypt.format mandatory for -drive. Maybe I would have
taken that approach had I known how little I know about the QAPI
generator at this point. But after I'd started, it was a challenge, so
I continued down that other path. Anyway, the problem with this
approach would be obviously compatibility breakage, and also being a bit
cumbersome. ("Why would you need to specify encrypt.format when it's
all in the image header anyway?")
Short answer: The current QAPI schema for qcow2 is incompatible to what
it allows with -drive, this patch allows making it compatible -- and we
need both to be compatible if we ever want BlockdevOptions throughout
the block layer.
>> 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 | 13 ++++++++
>> tests/qapi-schema/test-qapi.py | 2 ++
>> 6 files changed, 83 insertions(+), 15 deletions(-)
>
> The documentation update is in the next patch, and tests are in the
> patch after next. I'd squash all three.
I don't mind.
(I just like to split patches in advance, because squashing is always easy.)
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index 80a0a3e656..b43c87fe8d 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 will be a valid value for @tag.
>> +# Present exactly when @tag is present and the
>> +# associated element of @members is optional.
>> +# (Since: 3.0)
>> +#
>> # @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' ] } }
>>
>> ##
>
> Until now, the QAPI schema doesn't let you specify default values.
> Instead, code sees "absent", and does whatever needs to be done.
> Sometimes, it supplies a default value. "Absent" is shorthand for this
> value then. Sometimes, it behaves differently than for any value.
> "Absent" is a distinct additional case then. I'm not too fond of the
> latter case, but it's what we have, and it's not going away.
Just a note: In v1 of this series, it was possible to detect when the
variant was indeed absent. Eric proposed just setting the member to the
default value when absent (because he too is not too fond of that case),
so here in v2 there is no difference between setting the value to its
default value or not specifying it.
> We've discussed extending the QAPI schema to let you specify default
> values. Two advantages: the default value is *specified* rather than
> just documented (or not, when documentation sucks), and we can supply
> the default value in generated code. The introspection schema is even
> prepared for this:
>
> ##
> # @SchemaInfoObjectMember:
> #
> # An object member.
> #
> # @name: the member's name, as defined in the QAPI schema.
> #
> # @type: the name of the member's type.
> #
> --> # @default: default when used as command parameter.
> # If absent, the parameter is mandatory.
> # If present, the value must be null. The parameter is
> # optional, and behavior when it's missing is not specified
> # here.
> # Future extension: if present and non-null, the parameter
> # is optional, and defaults to this value.
> #
> # Since: 2.5
> ##
>
> The idea was to turn
>
> '*name': 'type'
>
> into sugar for
>
> 'name': { 'type': 'type', 'optional': true }
>
> then extend it to
>
> 'name': { 'type': 'type', 'optional': true, 'default': JSON-VALUE }
>
> where JSON-VALUE null means behavior for "absent" is not specified.
Seems reasonable.
> Your patch adds default values in a different way, and for just for
> union tags. For the QAPI language, that's not necessarily a deal
> breaker, as we don't need to maintain backward compatibility there. But
> for the introspection schema, we do. Once we add @default-variant,
> we're stuck with it. Even though it's redundant with
> SchemaInfoObjectMember's @default.
I too would like a general solution better. Of course the question is
how far off such a general solution is. It's not like I consider this
series extremely urgent, but I do have a BZ assigned for it, so take
that as you will. O:-)
So between the lines I'd like to think I can read that it would be
"fine" to add default-variant for now, but to omit it from introspection?
Max
> I'm skipping review of the implementation for now
© 2016 - 2026 Red Hat, Inc.