Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Viktor Mihajlovski]

On 12.02.2018 19:15, Luiz Capitulino wrote:
> On Mon, 12 Feb 2018 13:14:32 +0100
> Viktor Mihajlovski <address@hidden> wrote:
> 
>> -{ 'struct': 'CpuInfoFast',
>> -  'data': {'cpu-index': 'int', 'qom-path': 'str',
>> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>> +{ 'union': 'CpuInfoFast',
>> +  'base': {'cpu-index': 'int', 'qom-path': 'str',
>> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
>> +           'arch': 'CpuInfoArch' },
>> +  'discriminator': 'arch',
>> +  'data': { 'x86': 'CpuInfoOther',
>> +            'sparc': 'CpuInfoOther',
>> +            'ppc': 'CpuInfoOther',
>> +            'mips': 'CpuInfoOther',
>> +            'tricore': 'CpuInfoOther',
>> +            's390': 'CpuInfoS390Fast',
>> +            'other': 'CpuInfoOther' } }
> 
> Consider this a minor comment (and QMP maintainers can give a much
> better advice than me), but I think this arch list has problems. For
> one thing, it's incomplete. And the second problem is the 'other'
> field. What happens when QEMU starts supporting a new arch? 'other'
> becomes the new arch. Is this compatible? I don't know.
This seems to be the customary approach, see
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html
> 
> I don't know if this would work with the QAPI, but you could have
> a '*arch-data' field in the CpuInfoFast definition, like:
> 
> 'data': { ..., '*arch-data': 'CpuInfoFastArchData' }
> 
> where 'CpuInfoFastArchData' is defined by each arch that supports
> the field. An arch supporting the field could also export a
> query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast().
> 
I had it like this in my first reply to your initial patch. However,
that adds an additional hierarchy level in the return data. This
prevents clients like libvirt to reuse the data extraction code when
they switch over to using query-cpus-fast.

-- 
Regards,
 Viktor Mihajlovski