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