Re: [PATCH v15 09/11] machine: adding s390 topology to query-cpu-fast

[Thomas Huth <thuth@redhat.com>]

On 06/02/2023 15.50, Daniel P. Berrangé wrote:
> On Mon, Feb 06, 2023 at 02:09:57PM +0100, Thomas Huth wrote:
>> On 06/02/2023 13.49, Daniel P. Berrangé wrote:
>>> On Mon, Feb 06, 2023 at 01:41:44PM +0100, Thomas Huth wrote:
>>>> On 01/02/2023 14.20, Pierre Morel wrote:
>>>>> S390x provides two more topology containers above the sockets,
>>>>> books and drawers.
>>>>>
>>>>> Let's add these CPU attributes to the QAPI command query-cpu-fast.
>>>>>
>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>> ---
>>>>>     qapi/machine.json          | 13 ++++++++++---
>>>>>     hw/core/machine-qmp-cmds.c |  2 ++
>>>>>     2 files changed, 12 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>>>> index 3036117059..e36c39e258 100644
>>>>> --- a/qapi/machine.json
>>>>> +++ b/qapi/machine.json
>>>>> @@ -53,11 +53,18 @@
>>>>>     #
>>>>>     # Additional information about a virtual S390 CPU
>>>>>     #
>>>>> -# @cpu-state: the virtual CPU's state
>>>>> +# @cpu-state: the virtual CPU's state (since 2.12)
>>>>> +# @dedicated: the virtual CPU's dedication (since 8.0)
>>>>> +# @polarity: the virtual CPU's polarity (since 8.0)
>>>>>     #
>>>>>     # Since: 2.12
>>>>>     ##
>>>>> -{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
>>>>> +{ 'struct': 'CpuInfoS390',
>>>>> +    'data': { 'cpu-state': 'CpuS390State',
>>>>> +              'dedicated': 'bool',
>>>>> +              'polarity': 'int'
>>>>
>>>> I think it would also be better to mark the new fields as optional and only
>>>> return them if the guest has the topology enabled, to avoid confusing
>>>> clients that were written before this change.
>>>
>>> FWIW, I would say that the general expectation of QMP clients is that
>>> they must *always* expect new fields to appear in dicts that are
>>> returned in QMP replies. We add new fields at will on a frequent basis.
>>
>> Did we change our policy here? I slightly remember I've been told
>> differently in the past ... but I can't recall where this was, it's
>> certainly been a while.
>>
>> So a question to the QAPI maintainers: What's the preferred handling for new
>> fields nowadays in such situations?
> 
> I think you're mixing it up with policy for adding new fields to *input*
> parameters. You can't add a new mandatory field to input params of a
> command because no existing client will be sending that. Only optional
> params are viable, without a deprecation cycle. For output parameters
> such as this case, there's no compatibilty problem.

Ah, right, I likely mixed it up with that. Thanks for the clarification!

  Thomas