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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

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

Let's keep them.

>
> [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! :)

Thanks :)

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

I wish...

QEMU could do that on QMP input.  We've talked about it, but no patches.

On QMP output, we're screwed.

Tolerating '_' was one of the dumber mistakes in QAPI.

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

Heh.  I would've appreciated a hint in the commit message, though.

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

Works for me.

> [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!

You're welcome!