Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[]

[Eduardo Habkost]

On Wed, Sep 05, 2018 at 10:32:33AM -0500, Eric Blake wrote:
> On 09/05/2018 09:10 AM, Eduardo Habkost wrote:
> > Question to the QAPI schema maintainers below:
> > 
> 
> > >   ##
> > > -{ 'struct': 'X86CPUFeatureWordInfo',
> > > -  'data': { 'cpuid-input-eax': 'int',
> > > -            '*cpuid-input-ecx': 'int',
> > > -            'cpuid-register': 'X86CPURegister32',
> > > +{ 'struct': 'X86CPUIDFeatureWordInfo',
> > > +  'data': { 'input-eax': 'int',
> > > +            '*input-ecx': 'int',
> > > +            'register': 'X86CPURegister32',
> > >               'features': 'int' } }
> 
> You are renaming the struct (which is not visible over the wire), as well as
> renaming members within the struct (which is visible, if this type appears
> on the wire).
> 
> However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits either
> before or after this patch (well, first you have to build with my
> currently-pending patch as part of Markus' pull request for this trick to
> work, https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05968.html).
> Or, even without my patch, grepping for 'input-eax' has no hits as a member
> name in any struct.  Which means that there are no QMP commands currently
> exposing this struct over the wire.

There is one: qom-get.

> 
> > 
> > 
> > Question to the QAPI maintainers: is this really allowed?  With
> > this change, data that conforms to the new schema may not conform
> > to the old schema, because now the "input-eax" field will become
> > optional.
> 
> Yes, you can do whatever you want to QAPI structs that are not part of the
> QMP API, since they exist merely to make your C code easier.  You don't have
> to worry about backwards compatibility, because the only clients of such
> structs are being compiled at the same time as you make modifications to
> that struct.  Only once the struct appears in QMP introspection do you start
> having to worry about back-compat.  There, the rules are the struct names
> don't matter, but member names do; if the struct is used on input, then
> removing members is bad, adding mandatory members is bad, but adding
> optional members is okay (older clients don't know to pass in the new
> members, and may continue to pass in the member name you no longer want to
> use); if the struct is used on output, then removing non-optional members is
> bad, removing optional members is okay, and adding members is okay (clients
> are supposed to ignore unknown members, but may break if a previously
> mandatory member disappears).
> 
> > 
> > AFAICS, this will break the existing libvirt code: it will make
> > qemuMonitorJSONParseCPUx86Features() error out because it won't
> > find the "input-eax" field on all X86CPUFeatureWordInfo elements.
> 
> No, this change can't break libvirt, since I just proved there is no QMP
> command that libvirt could be using that either supplies an input member or
> expects an output member named "input-eax" in the first place.

I'm sure it will break libvirt.  libvirt uses
"qom-get path=<cpu-path> property=feature-words" to get
X86FeatureWordInfo data, and it expects the returned data to
have a "input-eax" field.

-- 
Eduardo