Re: [Qemu-devel] [PATCH v2 01/10] qapi: Add default-variant for flat unions

[Markus Armbruster <armbru@redhat.com>]

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.