Re: [Qemu-devel] [PATCH 5/6] qapi: extract CpuInfoCommon to mitigate schema duplication

[Laszlo Ersek]

On 04/25/18 09:06, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:
> 
>> @CpuInfo and @CpuInfoFast duplicate the following four fields: @qom-path,
>> @thread-id, @props and @arch. From these, extract the first three to a
>> common structure called @CpuInfoCommon. (More on @arch later.)
>>
>> Introduce two new mid-layer structures, @CpuInfoBase and @CpuInfoFastBase,
>> to soak up the union base struct fields on top of @CpuInfoCommon that are
>> specific to @query-cpus and @query-cpus-fast, respectively.

[[

>> This is
>> necessary because the base struct spec in union definitions does not let
>> us mix named fields with a recursive base struct. (In other words, we
>> couldn't directly use @CpuInfoCommon *plus* some other fields within
>> @CpuInfo and @CpuInfoFast as base struct).

]]

> 
> Yes, you can either specify a base type or inline common members.  If
> "union's common members = base type plus additional inline members"
> turns out to be desirable in more places, we can try to add the feature.
> I'm not asking *you* to find out, let alone try :)

[[

>> @arch cannot be hoisted higher than to @CpuInfoBase and @CpuInfoFastBase
>> because the union descriminator is only accepted from a direct base
>> struct, not from an indirect one.

]]

> That's a bit of a blemish.  Again, I'm not asking you to do anything
> about it.

If these characteristics of the generator are operating knowledge for
QAPI developers, I can trim the commit message -- those traits don't
bother me at all, I just felt that I needed to justify the code.

IOW, should I drop:
- the sentences marked with [[ ]] above,
- and/or the "Notes:" on @arch in the schema itself (which are updated
  to @target, in the next patch)
?

[snip]

>> @@ -512,70 +541,77 @@
>>  #
>>  # Since: 0.14.0
>>  #
>>  # Example:
>>  #
>>  # -> { "execute": "query-cpus" }
>>  # <- { "return": [
>>  #          {
>>  #             "CPU":0,
>>  #             "current":true,
>>  #             "halted":false,
>> -#             "qom_path":"/machine/unattached/device[0]",
>> +#             "qom-path":"/machine/unattached/device[0]",
>>  #             "arch":"x86",
>>  #             "pc":3227107138,
>> -#             "thread_id":3134
>> +#             "thread-id":3134
>>  #          },
>>  #          {
>>  #             "CPU":1,
>>  #             "current":false,
>>  #             "halted":true,
>> -#             "qom_path":"/machine/unattached/device[2]",
>> +#             "qom-path":"/machine/unattached/device[2]",
>>  #             "arch":"x86",
>>  #             "pc":7108165,
>> -#             "thread_id":3135
>> +#             "thread-id":3135
>>  #          }
>>  #       ]
>>  #    }
> 
> Compatibility break, whoops!

But, at least, I updated the example! :)

> 
> CpuInfo and CpuInfoFast do share qom-path and thread-id *values*, but
> the keys differ in '_' vs. '-'.  Sad.
> 
> What now?  Is there enough common stuff left to justify the refactoring?

While working on the schema changes, I saw the '_' vs. '-' difference
immediately, but I thought these two characters were treated
equivalently by all QAPI clients.

Later I found (in the test suite) that this wasn't the case, so I
thought my memories must have applied to libvirtd only. (Because,
libvirt does map "_" to "-", right?) Anyway, I figured the best way to
ask was to post the patch like this :)


So, if I drop @qom-path and @thread-id from CpuInfoCommon, then only
@props remains subject to hoisting, in this patch. In the next patch,
@arch joins @props.

I believe the refactoring is still worth doing, because otherwise, after
the addition of @target, we'd end up with:

{ 'union' : 'CpuInfo',
  'base'  : { 'CPU'       : 'int',
              'current'   : 'bool',
              'halted'    : 'bool',
              'qom_path'  : 'str',
              'thread_id' : 'int',
              '*props'    : 'CpuInstanceProperties',
              'arch'      : 'CpuInfoArch',
              'target'    : 'SysEmuTarget' },
...

{ 'union' : 'CpuInfoFast',
  'base'  : { 'cpu-index' : 'int',
              'qom-path'  : 'str',
              'thread-id' : 'int',
              '*props'    : 'CpuInstanceProperties',
              'arch'      : 'CpuInfoArch',
              'target'    : 'SysEmuTarget' },
...

and people would ask themselves ever after, "are there some common
fields in there that we could extract ... hmmm, @props and @arch, okay,
maybe, maybe not, grey area". Let's do it now and save them the thinking.

[snip]

>> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> index 25bce78352b8..5bfd2ef1dd3b 100644
>> --- a/qapi/qapi-schema.json
>> +++ b/qapi/qapi-schema.json
>> @@ -61,23 +61,23 @@
>>          'query-migrate-cache-size',
>>          'query-tpm-models',
>>          'query-tpm-types',
>>          'ringbuf-read' ],
>>      'name-case-whitelist': [
>>          'ACPISlotType',         # DIMM, visible through 
>> query-acpi-ospm-status
>>          'CpuInfoMIPS',          # PC, visible through query-cpu
>>          'CpuInfoTricore',       # PC, visible through query-cpu
>>          'QapiErrorClass',       # all members, visible through errors
>>          'UuidInfo',             # UUID, visible through query-uuid
>>          'X86CPURegister32',     # all members, visible indirectly through 
>> qom-get
>> -        'q_obj_CpuInfo-base'    # CPU, visible through query-cpu
>> +        'CpuInfoBase'           # CPU, visible through query-cpu
> 
> Let's update this to "visible through query-cpus, query-cpus-fast" while
> there.

Right, I noticed the typo in the preexistent comment too late (and I
didn't notice the "query-cpus-fast" omission at all).

Thanks!
Laszlo