Re: [Qemu-devel] [PATCH] qmp: add query-qemu-capabilities

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6018 bytes --]

On Mon, Feb 25, 2019 at 10:28:46AM +0100, Peter Krempa wrote:
> On Mon, Feb 25, 2019 at 09:50:26 +0100, Markus Armbruster wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> > 
> > > QMP clients can usually detect the presence of features via schema
> > > introspection.  There are rare features that do not involve schema
> > > changes and are therefore impossible to detect with schema
> > > introspection.
> > >
> > > This patch adds the query-qemu-capabilities command.  It returns a list
> > > of capabilities that this QEMU supports.
> > 
> > The name "capabilities" could be confusing, because we already have QMP
> > capabilities, complete with command qmp_capabilities.  Would "features"
> > work?
> > 
> > > The decision to make this a command rather than something statically
> > > defined in the schema is intentional.  It allows QEMU to decide which
> > > capabilities are available at runtime, if necessary.
> > >
> > > This new interface is necessary so that QMP clients can discover that
> > > migrating disk image files is safe with cache.direct=off on Linux.
> > > There is no other way to detect whether or not QEMU supports this.
> > 
> > I think what's unsaid here is that we don't want to make a completely
> > arbitrary schema change just to carry this bit of information.  We
> > could, but we don't want to.  Correct?
> 
> One example of such 'unrelated' change was a recent query- command which
> is used by libvirt just to detect presence of the feature but the
> queried result is never used and not very useful.
> 
> > > Cc: Markus Armbruster <armbru@redhat.com>
> > > Cc: Peter Krempa <pkrempa@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  qapi/misc.json | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  qmp.c          | 18 ++++++++++++++++++
> > >  2 files changed, 60 insertions(+)
> 
> [...]
> 
> > > +QemuCapabilities *qmp_query_qemu_capabilities(Error **errp)
> > > +{
> > > +    QemuCapabilities *caps = g_new0(QemuCapabilities, 1);
> > > +    QemuCapabilityList **prev = &caps->capabilities;
> > > +    QemuCapability cap_val;
> > > +
> > > +    /* Add all QemuCapability enum values defined in the schema */
> > > +    for (cap_val = 0; cap_val < QEMU_CAPABILITY__MAX; cap_val++) {
> > > +        QemuCapabilityList *cap = g_new0(QemuCapabilityList, 1);
> > > +
> > > +        cap->value = cap_val;
> > > +        *prev = cap;
> > > +        prev = &cap->next;
> > > +    }
> > > +
> > > +    return caps;
> > > +}
> > 
> > All capabilities known to this build of QEMU are always present.  Okay;
> > no need to complicate things until we need to.  I just didn't expect it
> > to be this simple after the commit message's "It allows QEMU to decide
> > which capabilities are available at runtime, if necessary."
> 
> I'm slightly worried of misuse of the possibility to change the behavior
> on runtime. In libvirt we cache the capabilities to prevent a "chicken
> and egg" problem where we need to know what qemu is able to do when
> generating the command line which is obviously prior to starting qemu.
> This means that we will want to cache even information determined by
> interpreting results of this API.
> 
> If any further addition is not as simple as this one it may be
> challenging to be able to interpret the result correctly and be able to
> cache it.
> 
> Libvirt's use of capabilities is split to pre-startup steps and runtime
> usage. For the pre-startup case [A]  we obviously want to use the cached
> capabilities which are obtained by running same qemu with a different
> configuration that will be used later. After qemu is started we use
> QMP to interact [B] with it also depending to the capabilities
> determined from the cache. Scenario [B] also allows us to re-probe some
> things but they need to be strictly usable only with QMP afterwards.
> 
> The proposed API with the 'runtime' behaviour allows for these 3
> scenarios for a capability:
> 1) Static capability (as this patch shows)
>     This is easy to cache and also supports both [A] and [B]
> 
> 2) Capability depending on configuration
>     [A] is out of the window but if QMP only use is necessary we can
>     adapt.

Does "configuration" mean "QEMU command-line"?  The result of the query
command should not depend on command-line arguments.

> 3) Capability depending on host state.
>     Same commandline might not result in same behaviour. Obviously can't
>     be cached at all, but libvirt would not do it anyways. [B] is
>     possible, but it's unpleasant.

Say the kernel or a library dependency is updated, and this enables a
feature that QEMU was capable of but couldn't advertise before.  I guess
this might happen and this is why I noted that the features could be
selected at runtime.

> I propose that the docs for the function filling the result (and perhaps
> also the documentation for the QMP interface) clarify and/or guide devs
> to avoid situations 2) and 3) if possible and motivate them to document
> the limitations on when the capability is detactable.
> 
> Additionally a new field could be added that e.g. pledges that the given
> capability is of type 1) as described above and thus can be easily
> cached. That way we can make sure programatically that we pre-cache only
> the 'good' capabilities.
> 
> Other than the above, this is a welcome improvement as I've personally
> ran into scenarios where a feature in qemu was fixed but it was
> impossible to detect whether given qemu version contains the fix as it
> did not have any influence on the QMP schema.

I'd like to make things as simpler as possible, but no simpler :).

The simplest option is that the advertised features are set in stone at
build time (e.g. selected with #ifdef if necessary).  But then we have
no way of selecting features at runtime (e.g. based on kernel features).

What do you think?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]