Re: [Qemu-devel] [PATCH RFC v3 30/32] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 08/24/2015 10:55 AM, Markus Armbruster wrote:
>
>> Our motivation for dropping nested structs was to avoid burning the
>> 'name': {} struct member syntax on a trivial and rarely used
>> convenience, and instead make it available for a way to specify member
>> attributes beyond name and type.
>> 
>> Is there a chance we want to define simple union cases with attributes
>> beyond tag value and type?
>
> You may have a valid point there.  It's hard to predict the future, but
> leaving dictionary open for future use is the most extensible approach.
>
> But in the patches I'm currently working on, I had only been adding
> support for anonymous types for the branches of flat unions; I
> intentionally left simple unions to REQUIRE a type name for the branches
> (because of the way we create a wrapper type around the single data
> member for introspection purposes).

I asked only about simple unions, but my question actually applies to
any kind of union.  In fact, we could decide to reserve the {} syntax
for extensions in the longhand syntactical form, and still burn it on
convenience in shorthand, sugared forms.

>> I think we have a better chance to answer that question after we clean
>> non-simple unions.
>
> Well, my proposed cleanup was figuring out a way to explicitly specify
> that for a given enum value, we add no additional members to the wire
> struct.  But there is a possible alternative syntax for that:
>
> { 'union': 'Union', 'base': 'Base', 'discriminator': 'type',
>   'data': { 'branch1': 'AdditionalMembers',
>             'branch2': null } }
>
> which uses 'null' in place of '{}' for the explicitly empty case, while
> still requiring a type name for all other branches.

Let's revisit this once we've figured out how to clean up union syntax.

>                                                      I still think that
> requiring a user to explicitly list all branches of a union is a nice
> fail-safe (if the enum is extended, we are then reminded to update the
> union to match) that we don't currently have.

Missing case reminders are obviously useful for code switching over an
enumeration.  They're less useful for data.  A forgotten case in code
compiles fine, then fails (often catastrophically) at run time.  A
forgotten case in data simply won't compile (assuming a statically
checked language).

>>>> Both Abort and ChardevDummy exist only because you need a type to
>>>> declare a simple union case.  I'd like to explore cleaning up the
>>>> convoluted union syntax first.  If we then still have a need for
>>>> empty structs, we can consider optimizing them.
>>>
>>> And that's where my patches were headed - by allowing a dict instead of
>>> a type name for the branches of a flat union, the syntax for flat unions
>>> becomes simpler, and allows us to sanely represent a
>>> "no-additional-members" variant without needing 'Abort' as an empty type.
>> 
>> Empty cases in flat unions are not a problem: simply don't mention the
>> tag value.
>
> But that's opposite of the direction I want to move, where we require
> all branches to be listed, but have a clean way to document the branches
> that add no additional members to the variant object.

Let's figure out how to clean up union syntax first, and how to do empty
cases second.

>> I'd like to explore doing unions in schema syntax the way we represent
>> them internally and in introspection.  Basically get rid of the "need to
>> inherit the common members from a base" nonsense.
>
> I've already posted patches that would allow:
>
> { 'union': 'Union', 'base': { ... }, 'discriminator': 'type',
>   'data': { ... } }
>
> that is, allowing the base fields to be specified inline as an anonymous
> struct rather than having to create a one-off named struct for that task.
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02346.html

In my opinion, the whole 'base' business is a hack to inject additional
common members into a union.  If I remember correctly, Kevin did that
just to keep his flat union work minimally invasive.  Considering what
it took us to do introspection the not minimally invasive way, I can't
fault him for taking a shortcut.

In my recent KVM Forum talk, I showed the QAPI schema and introspection
value for SchemaInfo.  The former is a flat union with a struct base,
i.e. two types connected by a (non-trivial) inheritance relation.  The
latter is simpler: a single, straightforward variant record.  That's
what I'd like to have in the schema, too.

https://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf

> But there's still the question of whether we want to always tie the
> union branches to an explicitly named enum, or whether it is nice to
> preserve the current simple union semantics that an implicit enum is
> created to cover all branches when an explicit enum type is not named.
> Conversely, I still want to get to the point that even a simple union
> can optionally document that it reuses an existing enum (along with the
> corresponding qapi-generator enforced rules that all enum branches are
> properly covered), rather than always being forced to use an implicit
> enum type (where mismatches between an intended explicit enum are too
> likely).

I suspect that once we cut the excess baggage from unions, simple unions
will add only marginal convenience.  Perhaps enough to keep them for the
truly simple cases.  Probably not enough to complicate them so they can
cover a few more cases.

>> Once that's done, we can decice whether simple unions are still
>> worthwhile syntactical sugar.
>
> Agreed - there's still some room to play with things, and flushing our
> existing patch queue to have a stable base to play on will make it a bit
> nicer.

Flushing the patch queue: yes.  Next step: respin this series, non-RFC.

Thanks!