Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name

[Markus Armbruster <armbru@redhat.com>]

Peter Krempa <pkrempa@redhat.com> writes:

> On Mon, Sep 20, 2021 at 11:08:59 +0200, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>> 
>> > On Wed, Sep 15, 2021 at 21:24:21 +0200, Markus Armbruster wrote:
>> >> The next commit will add feature flags to enum members.  There's a
>> >> problem, though: query-qmp-schema shows an enum type's members as an
>> >> array of member names (SchemaInfoEnum member @values).  If it showed
>> >> an array of objects with a name member, we could simply add more
>> >> members to these objects.  Since it's just strings, we can't.
>> >> 
>> >> I can see three ways to correct this design mistake:
>> >> 
>> >> 1. Do it the way we should have done it, plus compatibility goo.
>> >> 
>> >>    We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
>> >>    changing @values would be a compatibility break, add a new member
>> >>    @members instead.
>> >> 
>> >>    @values is now redundant.  We should be able to get rid of it
>> >>    eventually.
>> >> 
>> >>    In my testing, output of qemu-system-x86_64's query-qmp-schema
>> >>    grows by 11% (18.5KiB).
>> >
>> > I prefer this one. While the schema output grows, nobody is really
>> > reading it manually.
>> 
>> True, but growing schema output can only slow down client startup.
>> Negligible for libvirt, I presume?
>
> Libvirt employs caching, so unless it's the first VM started after a
> qemu/libvirt upgrade, the results are already processed and cached.

Good!

> In fact we don't even keep the full schema around, we just extract
> information and store them as capability bits. For now we didn't run
> into the need to have the full schema around when starting a VM.
>
> [...]
>
>> >> 3. Versioned query-qmp-schema.
>> >> 
>> >>    query-qmp-schema provides either @values or @members.  The QMP
>> >>    client can select which version it wants.
>> >
>> > At least for libvirt this poses a chicken & egg problem. We'd have to
>> > query the schema to see that it has the switch to do the selection and
>> > then probe with the modern one.
>> 
>> The simplest solution is to try the versions the management application
>> can understand in order of preference (newest to oldest) until one
>> succeeds.  I'd expect the first try to work most of the time.  Only when
>> you combine new libvirt with old QEMU, the fallback has to kick in.
>> 
>> Other parts of the management application should remain oblivous of the
>> differences.
>
> That would certainly work and be reasonably straightforward for libvirt
> to implement, but:
>  1) libvirt's code for using the QMP schema would be exactly the same as
>     with approach 1), as we need to handle old clients too and the new
>     way is simply a superset of what we have

Yes, libvirt would need the same code for processing old and new.  The
only difference would be how it decides which method to use.  With 1,
it's "if @members is present, use it, else @values".  With 2, it's "if
the version we use is new enough, use @members, else @values".

>  2) qemu's deprecation approach itself wouldn't be any easier in either
>     of those scenarios
>
> Basically the only thing this would gain us is that if the deprecation
> period is over old clients which were not fixed could fail silently:
>
> Assuming that 'query-qmp-schema' gains a boolean option such as
> 'fancier-enums' and setting that to true returns the new format of
> schema, after the deprecation is over you could simply return an error
> if a caller omits 'fancier-enums' or sets it to false, which creates a
> clean cut for the removal.

Yes.

> With approach 1) itself, clients which were not adapted would start
> lacking information based on enum values.
>
> Now for those it depends on how they actually handled it until now. E.g.
> old libvirt would report that the QMP schema is broken if 'values' would
> be missing.

Which I consider the sensible thing to do.

> Whether that's a worthwhile thing to do? I'm not really persuaded. (And
> I'm biased since libvirt handles it correctly).

I think 3 has the following advantages over 1:

* As you noted, it ensures outmoded clients fail cleanly.  Not much of
  an advantage for clients that handle missing @values sensibly.
  Perhaps it could enable better error messages.

* It avoids duplicated contents in old an new format.  Not much of an
  advantage for clients that cache their schema interrogation.

* It can enable more radical introspection changes.  Without versioning,
  the common rules for compatible evolution apply (section
  "Compatibility considerations" in qapi-code-gen.rst).  With
  versioning, they don't.

I agree this is not really compelling just for the problem at hand.  We
can reconsider when we run into more problems.

>> We could of course try to reduce the number of roundtrips, say by
>> putting sufficient information into the QMP greeting (one roundtrip), or
>> the output of query-qmp-schema (try oldest to find the best one, then
>> try the best one unless it's the oldest).  I doubt that's worthwhile.
>
> In this particular scenario, I'd doubt that it's worthwhile as the
> change isn't really fundamental and issuing one extra QMP call isn't as
> problematic as other cases, e.g probing of CPU features which results in
> a QMP call per feature when starting a VM.
>
> Currently libvirt issues 50 + 5 QMP commands(kvm, and non-kvm) for
> probing capabilities.
>
>> I'm not trying to talk you into this solution.  We're just exploring the
>> solution space together, and with an open mind.
>
> The idea of unconditionally trying a newer approach is a good one to
> hold onto when we'll need it in the future!

Only where the failure modes are simple enough to make misinterpretation
basically impossible.