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

[Eric Blake]

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.

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

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

> 
> 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).

> 
> 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.

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.

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature