[Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions

Max Reitz posted 8 patches 6 years, 10 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Max Reitz 6 years, 10 months ago
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         | 58 ++++++++++++++++++++++++++++------
 scripts/qapi/doc.py            | 10 ++++--
 scripts/qapi/introspect.py     | 10 ++++--
 scripts/qapi/visit.py          | 13 ++++++++
 tests/qapi-schema/test-qapi.py |  2 ++
 6 files changed, 86 insertions(+), 15 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index 3d22166b2b..e4740aa7a0 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: 4.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 c89edc0cb0..13fd55d9c9 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -750,6 +750,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.
@@ -774,16 +775,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_value = base_members.get(discriminator)
-        if not discriminator_value:
-            raise QAPISemError(info,
-                               "Discriminator '%s' is not a member of base "
-                               "struct '%s'"
-                               % (discriminator, base))
+        if default_variant is None:
+            discriminator_value = base_members.get(discriminator)
+            if not discriminator_value:
+                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_value = base_members.get('*' + discriminator)
+            if not discriminator_value:
+                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)
+
         if discriminator_value.get('if'):
             raise QAPISemError(info, 'The discriminator %s.%s for union %s '
                                'must not be conditional' %
@@ -796,6 +818,16 @@ 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 not any(value for value in enum_define['data']
+                       if value['name'] == default_variant):
+                raise QAPISemError(info,
+                                   "Default variant '%s' of flat union '%s' is "
+                                   "not part of '%s'"
+                                   % (default_variant, name,
+                                      discriminator_value['type']))
+
     # Check every branch; don't allow an empty union
     if len(members) == 0:
         raise QAPISemError(info, "Union '%s' cannot have empty 'data'" % name)
@@ -976,7 +1008,7 @@ def check_exprs(exprs):
         elif 'union' in expr:
             meta = 'union'
             check_keys(expr_elem, 'union', ['data'],
-                       ['base', 'discriminator', 'if'])
+                       ['base', 'discriminator', 'if', 'default-variant'])
             normalize_members(expr.get('base'))
             normalize_members(expr['data'])
             union_types[expr[meta]] = expr
@@ -1431,12 +1463,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
@@ -1444,6 +1478,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):
@@ -1773,6 +1808,7 @@ class QAPISchema(object):
         base = expr.get('base')
         ifcond = expr.get('if')
         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(
@@ -1794,6 +1830,7 @@ class QAPISchema(object):
             QAPISchemaObjectType(name, info, doc, ifcond, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
+                                                              default_tag_value,
                                                               variants)))
 
     def _def_alternate_type(self, expr, info, doc):
@@ -1807,6 +1844,7 @@ class QAPISchema(object):
             QAPISchemaAlternateType(name, info, doc, ifcond,
                                     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 c03b690161..c7ad23985b 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -168,8 +168,14 @@ 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"}%s' % (
-                variants.tag_member.name, v.name, texi_if(v.ifcond, " (", ")"))
+            if v.name == variants.default_tag_value:
+                when = ' when @code{%s} is @t{"%s"} or not given%s' % (
+                    variants.tag_member.name, v.name,
+                    texi_if(v.ifcond, " (", ")"))
+            else:
+                when = ' when @code{%s} is @t{"%s"}%s' % (
+                    variants.tag_member.name, v.name,
+                    texi_if(v.ifcond, " (", ")"))
             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 f7f2ca07e4..db8af348d2 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -166,9 +166,12 @@ const QLitObject %(c_name)s = %(c_string)s;
             ret = (ret, {'if': member.ifcond})
         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)},
@@ -192,6 +195,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, ifcond)
 
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 82eab72b21..57eccdfdc2 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -77,6 +77,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
         ret += gen_endif(memb.ifcond)
 
     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 d592854601..be223e2cc4 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -66,6 +66,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))
                 QAPISchemaTestVisitor._print_if(v.ifcond, indent=8)
-- 
2.20.1


Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Eric Blake 6 years, 10 months ago
On 2/6/19 1:55 PM, 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         | 58 ++++++++++++++++++++++++++++------
>  scripts/qapi/doc.py            | 10 ++++--
>  scripts/qapi/introspect.py     | 10 ++++--
>  scripts/qapi/visit.py          | 13 ++++++++
>  tests/qapi-schema/test-qapi.py |  2 ++
>  6 files changed, 86 insertions(+), 15 deletions(-)

Missing a change to docs/devel/qapi-code-gen.txt? [re-reads diffstat in
cover letter...] Oh, you got that later in the series. Maybe those two
patches are worth squashing?

> +++ b/scripts/qapi/visit.py
> @@ -77,6 +77,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>          ret += gen_endif(memb.ifcond)
>  
>      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))
> +

I like that you de-sugar it as early in the input parse as possible.

Markus may have comments, but it looks reasonable to me.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Markus Armbruster 6 years, 10 months ago
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.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/introspect.json           |  8 +++++
>  scripts/qapi/common.py         | 58 ++++++++++++++++++++++++++++------
>  scripts/qapi/doc.py            | 10 ++++--
>  scripts/qapi/introspect.py     | 10 ++++--
>  scripts/qapi/visit.py          | 13 ++++++++
>  tests/qapi-schema/test-qapi.py |  2 ++
>  6 files changed, 86 insertions(+), 15 deletions(-)
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 3d22166b2b..e4740aa7a0 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: 4.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' ] } }
>  
>  ##

I'm afraid this could become awkward later on.  Let me explain.

In many programming languages, absent optional arguments / members
default to a default value specified in the declaration.  Simple.

In others, 'absent' is effectively an additional value.  The code
consuming the argument / member can interpret 'absent' however it wants.
It may eliminate the additional value by mapping it to a default value,
but it can also interpret 'absent' unlike any value.  If there's more
than one consumer, their interpretations need not be consistent.  The
declaration provides no clue on semantics of 'absent'.

QAPI is in the latter camp.  I trust you can already sense how much I
like that.

Now you want to permit optional variant discriminators.  As per general
rule, their interpretation is left to the code consuming it.  One
instance of such code is the generated union visitor, which needs to
decide which variant to visit.  Your default-variant tells it which
variant to visit.  Other code interpreting the discriminator better be
consistent with that, but that's the other code's problem.  Hmm.

If I could go back in time, I'd flip QAPI to "'absent' defaults to a
default value".  Lacking a time machine, we're stuck with cases of
"'absent' means something you can't express with a value" and "'absent'
means different things in different contexts" that have been enshrined
in external interfaces.  Is there anything we could do to improve
matters for saner cases?

I think there is: we could provide for an *optional* default value.  If
the schema specifies it, that's what 'absent' means.  If it doesn't, all
bets are off, just like they are now.

Say we do that (for what it's worth, introspect.json is already prepared
for it).  How would it play with your default-variant?

If an optional discriminator specifies a default value, then that's the
default variant.  But wait, if there's also a default-variant, *that's*
the default variant!  Awkward clash.  To resolve it, we could either
forbid combining the two, or rule default-variant overrides the default.
Feels needlessly complicated.

Could we get away with "if you want a default variant, the discriminator
must specify a default"?

[...]

Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Max Reitz 6 years, 10 months ago
On 07.02.19 07:56, 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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qapi/introspect.json           |  8 +++++
>>  scripts/qapi/common.py         | 58 ++++++++++++++++++++++++++++------
>>  scripts/qapi/doc.py            | 10 ++++--
>>  scripts/qapi/introspect.py     | 10 ++++--
>>  scripts/qapi/visit.py          | 13 ++++++++
>>  tests/qapi-schema/test-qapi.py |  2 ++
>>  6 files changed, 86 insertions(+), 15 deletions(-)
>>
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index 3d22166b2b..e4740aa7a0 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: 4.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' ] } }
>>  
>>  ##
> 
> I'm afraid this could become awkward later on.  Let me explain.
> 
> In many programming languages, absent optional arguments / members
> default to a default value specified in the declaration.  Simple.
> 
> In others, 'absent' is effectively an additional value.  The code
> consuming the argument / member can interpret 'absent' however it wants.
> It may eliminate the additional value by mapping it to a default value,
> but it can also interpret 'absent' unlike any value.  If there's more
> than one consumer, their interpretations need not be consistent.  The
> declaration provides no clue on semantics of 'absent'.
> 
> QAPI is in the latter camp.  I trust you can already sense how much I
> like that.
> 
> Now you want to permit optional variant discriminators.  As per general
> rule, their interpretation is left to the code consuming it.  One
> instance of such code is the generated union visitor, which needs to
> decide which variant to visit.  Your default-variant tells it which
> variant to visit.  Other code interpreting the discriminator better be
> consistent with that, but that's the other code's problem.  Hmm.
> 
> If I could go back in time, I'd flip QAPI to "'absent' defaults to a
> default value".  Lacking a time machine, we're stuck with cases of
> "'absent' means something you can't express with a value" and "'absent'
> means different things in different contexts" that have been enshrined
> in external interfaces.  Is there anything we could do to improve
> matters for saner cases?
> 
> I think there is: we could provide for an *optional* default value.  If
> the schema specifies it, that's what 'absent' means.  If it doesn't, all
> bets are off, just like they are now.
> 
> Say we do that (for what it's worth, introspect.json is already prepared
> for it).

If somebody(tm) does that, great. :-)

> How would it play with your default-variant?
> 
> If an optional discriminator specifies a default value, then that's the
> default variant.  But wait, if there's also a default-variant, *that's*
> the default variant!  Awkward clash.  To resolve it, we could either
> forbid combining the two, or rule default-variant overrides the default.
> Feels needlessly complicated.

I agree on the needless, not so sure about the complicated.  (Other than
it being double the work, but, well, the default-variant work is already
right here.)

> Could we get away with "if you want a default variant, the discriminator
> must specify a default"?

I think so, yes.  I agree that it'd be the better solution.  But to be
honest I'd really rather not delve any deeper into the QAPI dungeon than
I've done in this series so far...

Max

Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Eric Blake 6 years, 6 months ago
On 2/8/19 12:21 PM, Max Reitz wrote:
> On 07.02.19 07:56, 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.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---

>>> +++ 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: 4.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' ] } }
>>>  
>>>  ##
>>
>> I'm afraid this could become awkward later on.  Let me explain.
>>
>> In many programming languages, absent optional arguments / members
>> default to a default value specified in the declaration.  Simple.
>>
>> In others, 'absent' is effectively an additional value.  The code
>> consuming the argument / member can interpret 'absent' however it wants.
>> It may eliminate the additional value by mapping it to a default value,
>> but it can also interpret 'absent' unlike any value.  If there's more
>> than one consumer, their interpretations need not be consistent.  The
>> declaration provides no clue on semantics of 'absent'.
>>
>> QAPI is in the latter camp.  I trust you can already sense how much I
>> like that.
>>
>> Now you want to permit optional variant discriminators.  As per general
>> rule, their interpretation is left to the code consuming it.  One
>> instance of such code is the generated union visitor, which needs to
>> decide which variant to visit.  Your default-variant tells it which
>> variant to visit.  Other code interpreting the discriminator better be
>> consistent with that, but that's the other code's problem.  Hmm.
>>
>> If I could go back in time, I'd flip QAPI to "'absent' defaults to a
>> default value".  Lacking a time machine, we're stuck with cases of
>> "'absent' means something you can't express with a value" and "'absent'
>> means different things in different contexts" that have been enshrined
>> in external interfaces.  Is there anything we could do to improve
>> matters for saner cases?
>>
>> I think there is: we could provide for an *optional* default value.  If
>> the schema specifies it, that's what 'absent' means.  If it doesn't, all
>> bets are off, just like they are now.
>>
>> Say we do that (for what it's worth, introspect.json is already prepared
>> for it).
> 
> If somebody(tm) does that, great. :-)
> 
>> How would it play with your default-variant?
>>
>> If an optional discriminator specifies a default value, then that's the
>> default variant.  But wait, if there's also a default-variant, *that's*
>> the default variant!  Awkward clash.  To resolve it, we could either
>> forbid combining the two, or rule default-variant overrides the default.
>> Feels needlessly complicated.
> 
> I agree on the needless, not so sure about the complicated.  (Other than
> it being double the work, but, well, the default-variant work is already
> right here.)
> 
>> Could we get away with "if you want a default variant, the discriminator
>> must specify a default"?
> 
> I think so, yes.  I agree that it'd be the better solution.  But to be
> honest I'd really rather not delve any deeper into the QAPI dungeon than
> I've done in this series so far...

I've now hit a case where I'd like a default-variant (namely, improving
nbd-server-add to avoid SocketAddressLegacy); maybe I'll find time to
revive at least this part of the series.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Eric Blake 6 years, 10 months ago
On 2/7/19 12:56 AM, 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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---

> I'm afraid this could become awkward later on.  Let me explain.
> 
> In many programming languages, absent optional arguments / members
> default to a default value specified in the declaration.  Simple.
> 
> In others, 'absent' is effectively an additional value.  The code
> consuming the argument / member can interpret 'absent' however it wants.
> It may eliminate the additional value by mapping it to a default value,
> but it can also interpret 'absent' unlike any value.  If there's more
> than one consumer, their interpretations need not be consistent.  The
> declaration provides no clue on semantics of 'absent'.
> 
> QAPI is in the latter camp.  I trust you can already sense how much I
> like that.
> 
> Now you want to permit optional variant discriminators.  As per general
> rule, their interpretation is left to the code consuming it.  One
> instance of such code is the generated union visitor, which needs to
> decide which variant to visit.  Your default-variant tells it which
> variant to visit.  Other code interpreting the discriminator better be
> consistent with that, but that's the other code's problem.  Hmm.
> 
> If I could go back in time, I'd flip QAPI to "'absent' defaults to a
> default value".  Lacking a time machine, we're stuck with cases of
> "'absent' means something you can't express with a value" and "'absent'
> means different things in different contexts" that have been enshrined
> in external interfaces.  Is there anything we could do to improve
> matters for saner cases?
> 
> I think there is: we could provide for an *optional* default value.  If
> the schema specifies it, that's what 'absent' means.  If it doesn't, all
> bets are off, just like they are now.

And we already have the planned syntax, thanks to our recent work on
adding conditionals - where we now have:

{ '*field': 'mytype' }

we can also do long-hand:

{ { 'name': '*field', 'type': 'mytype' } }

which also lends itself well to declaring a default:

{ { 'name': '*field', 'type': 'mytype', 'default': 'xyz' } }

> 
> Say we do that (for what it's worth, introspect.json is already prepared
> for it).  How would it play with your default-variant?
> 
> If an optional discriminator specifies a default value, then that's the
> default variant.  But wait, if there's also a default-variant, *that's*
> the default variant!  Awkward clash.  To resolve it, we could either
> forbid combining the two, or rule default-variant overrides the default.
> Feels needlessly complicated.
> 
> Could we get away with "if you want a default variant, the discriminator
> must specify a default"?

I like the idea. It means finally biting the bullet and implementing
default values, but we've known we've wanted them for a while (as
evidenced by the existing introspection output, and by the syntax that
we have ready to put into use for it).

It means that representing the default variant in a union now depends on
the base class declaring a default for the optional parameter (that is,
an optional parameter can only be used as discriminator if it has a
declared default), and gives us variable defaults in more uses than just
unions.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/8] qapi: Add default-variant for flat unions
Posted by Eric Blake 6 years, 10 months ago
On 2/7/19 8:01 AM, Eric Blake wrote:

>> I think there is: we could provide for an *optional* default value.  If
>> the schema specifies it, that's what 'absent' means.  If it doesn't, all
>> bets are off, just like they are now.
> 
> And we already have the planned syntax, thanks to our recent work on
> adding conditionals - where we now have:
> 
> { '*field': 'mytype' }
> 
> we can also do long-hand:
> 
> { { 'name': '*field', 'type': 'mytype' } }

I'd better use the actual syntax, instead of inventing non-JSON off the
top of my head:

{ '*field': { 'type': 'mytype' } }

> 
> which also lends itself well to declaring a default:
> 
> { { 'name': '*field', 'type': 'mytype', 'default': 'xyz' } }

{ '*field': { 'type': 'mytype', 'default': 'xyz' } }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org