Laszlo Ersek <ler...@redhat.com> writes:
> On 04/25/18 09:06, Markus Armbruster wrote:
>> Laszlo Ersek <ler...@redhat.com> 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!
