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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|