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

[Peter Krempa]

On Mon, Feb 25, 2019 at 17:40:01 +0000, Stefan Hajnoczi wrote:
> 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:

[...]

> > 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.

Yes exactly. There probably is possibility that something would be
detectable only after a shared library load which in turn would depend
on the command line ...

> 
> > 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.

Yeah, such scenario is really breaking our caching even now. I don't
want to say it's bad. It may even be necessary in some scenarios. Both
this and the above scenario may be necessary eventually. Libvirt
certainly can make use of the detection for QMP use if there is such a
thing.

> > 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?

I really don't want to limit the possibilities for the API. My goal is
only that it's obvious both from the docs and preferably also from the
returned data (so that we can filter what to cache to prevent mistakes)
that a given capability bit is static or dynamic.

I think adding the field to the returned data which will be set
according to how the capability was detected should be simple enough,
but I will be okay with just documenting any caveats along with the
inidividual capabilities. In that case an comment should encourage those
coming after you to document it properly.

signature.asc
Description: PGP signature