Re: [Qemu-devel] [PATCH v10 29/30] cpu: Convert CpuInfo into flat union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/09/2015 08:22 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> When qapi type CpuInfo was originally created for 0.14, we had
>>> no notion of a flat union, and instead just listed a bunch of
>>> optional fields with documentation about the mutually-exclusive
>>> choice of which instruction pointer field(s) would be provided
>>> for a given architecture.  But now that we have flat unions and
>>> introspection, it is better to segregate off which fields will
>>> be provided according to the actual architecture.  With this in
>>> place, we no longer need the fields to be optional, because the
>>> choice of the discriminator serves that role.
>>>
>>> This has an additional benefit: the old all-in-one struct was
>>> the only place in the code base that had a case-sensitive
>>> naming of members 'pc' vs. 'PC'.  Separating these spellings
>>> into different branches of the flat union will allow us to add
>>> restrictions against future case-insensitive collisions, and
>>> make it possible for a future qemu to decide whether to enable
>>> case-insensitive QMP.
>
>>>  ##
>>>  # @query-cpus:
>> 
>> CpuInfo is only used as return type of query-cpus.
>> 
>> If I understand the change correctly, the value of query-cpus is not
>> changed at all.  Its introspection, however, is.
>
> Almost. The output of query-cpus gains a new 'arch':'FOO' string per
> cpu, saying which branch of the flat union is in use.  But as it is
> output-only, adding a new output field shouldn't break any existing
> clients (because QMP already documents that clients should ignore new
> fields); and since the struct is output-only, it can't break any input
> fields.

Understood.

> I probably can spell that out better in the commit message, though.

Yes, please

> Oh, and I need to update qmp-commands.hx, as part of my v11 spin.

Yes, please.

>> Do we need this to make 2.5?
>
> It's true that the introspection will change (instead of seeing flat
> optional members, you now have to chase down variants).  But I don't
> think it is pressing enough to rush into 2.5; the change is
> backwards-compatible no matter when we do it, and introspection clients
> are going to have to get used to the idea of chasing down different
> paths to find all members of a struct (that is, if we don't change this
> until 2.6, I'm sure it won't be the last case of introspection changing
> while keeping QMP wire format back-compatible as formerly non-variant
> fields become variant).

We can mess with the schema as long as the QMP wire format stays
compatible.

With introspection, schema changes get exposed in QMP that weren't
exposed before.  Begs the question whether the ABI promise extends to
the introspection value.

I think we don't want to extend it, at least for now.  In other words,
we refuse to put additional constraints on schema changes to keep the
introspection value stable.  Makes introspection a bit harder to use.
Not ideal, but better than committing to constraints we don't even fully
understand, yet.

I think we should spell this out introspection documentation.

Our only example so far is converting between optional members and flat
unions.  Can we think of others?  Hmm, you found one below.

>> Any other messy optionals that should really be (flat) unions?
>> 
>
> Possibly.
> /me goes and audits all 0.14 interfaces...
>
> add_client is an input command, but it would be nicer as a flat union
> (we can't do that until commands can take unions; but that happens as
> part of my pending queue for netdev_add), so that's not going to make 2.5.

Since the command parameters are just an object type, this is merely
another case of converting between optional members and flat unions.

> client_migrate_info looks odd, since it requires 'protocol':'str' to be
> the value 'spice'.  Probably several functions that take or produce a
> finite set of strings that should be using enums, but conversion from
> 'str' to 'enum' is backwards-compatible according to QMP wire format.
> Doing it sooner rather than later makes introspection nicer.

This is another example: converting between string and enum.  Going from
string to enum only adds information, and should be easy enough for
clients.  Going the other way removes information, and probably should
not be done.

> But I didn't spot any other obvious commands from that far back with
> mutually-exclusive optional parameters, and certainly nothing else with
> case clashes.

Okay.  I guess finding all the examples that matter may take a few
development cycles.  Perhaps we want to commit to some additional
compatibility promises for introspection then.