Re: [Qemu-devel] [PATCH 1/2] qmp: Add query-qemu-features

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 29.03.2019 um 16:06 hat Eric Blake geschrieben:
>> On 3/29/19 8:52 AM, Markus Armbruster wrote:
>> > Subjective summary:
>> > 
>> > * For the known use cases, query-qemu-features is merely a crutch for
>> >   getting information into the QAPI QMP schema.  Such crutches are ugly,
>> >   but in absence of better ideas, ugly wins by default.
>> 
>> That is, this series qualifies as the crutch (if we can't come up with
>> anything closer in time for 4.0) - but your arguments say we may have
>> something closer:
>
> With your suggestion that we cover things by driver name ('file',
> 'host_device') rather than source file ('file-posix'), we actually can
> return true or false and it's more than just getting things added to the
> schema.
>
> I know that you also suggested making the existence of the field
> conditional, but in my opinion that's just a result of asserting that
> you don't need to actually execute the command. There is no reason to
> make the field conditional unless you never intend to execute the
> command, but just introspect it and draw conclusions from that.

It's a result of my reasoned opinion that properties of the QEMU build
are best conveyed via schema introspection.

One of schema introspection's stated goals was to let us stop adding
more and more ad hoc query commands (see slide 24 of my KVM Forum 2015
"QEMU interface introspection: From hacks to solutions"[*]).

query-qemu-features is not another ad hoc solution for a specific
introspection issue.  It's general solution for another set of problems.

Adding additional general solutions for the same problems would be no
better than additional ad hoc solutions.

However, query-qemu-features can also solve *different* problems, namely
run time properties.

This brings be to the question of extent.

With schema introspection, extent is clear: the same binary will always
give you the same answer.  This enables easy caching of
query-qmp-schema's response.

With query-qemu-features, the returned information's extent could be
anything.  Is it valid for the remainder of this run?  For the next run
of the same QEMU build on the same host?  On another host?  Needs to be
documented for every feature.  For how long can you cache the response
of query-qemu-features?  Complicated.  You could try "until the
shortest-lived feature I'm interested in becomes invalid".

In my opinion, query-qemu-features is the wrong tool for properties of
the build.

Support for dynamic auto-read-only is a property of the build.

>> > * I think I'd prefer adding @dynamic-auto-read-only to
>> >   BlockdevOptionsFile, or perhaps to BlockdevOptions.  Takes precedence
>> >   over @auto-read-only.  Consider deprecating @auto-read-only.
>> 
>> This is interesting. The existing flag keeps its semantics:
>> 
>> Open the file with automatic fallback to read-only, may or may not be
>> dynamic.
>> 
>> And the new flag is documented as:
>> 
>> Overrides auto-read-only, open the file with automatic fallback to
>> read-only, guaranteed to be dynamic (or fails if the guarantee cannot be
>> provided).
>> 
>> I don't know if we need the deprecation or not (especially if our plans
>> over time are to allow the dynamic behavior in more places than just
>> file-posix, where a guaranteed failure if not dynamic vs. a sane
>> fallback to only-on-initial-open may still be useful to some callers),
>> but the new parameter with the guaranteed semantics is definitely
>> introspectible, so it is closer to the problem than creating
>> query-qemu-features.  It also seems like something we could pull
>> together in a short amount of time.
>
> This means that dynamic-auto-read-only is separate from auto-read-only
> and provides different semantics. auto-read-only is one of the features
> that are implemented using a BDRV_O_* flag and that required option
> inheritance. Both are things that I prefer to be avoided whenever we
> can.
>
> I'd rather miss 4.0 than adding another option like this.

Can you explain option inheritance?  I can't find it documented in the
schema.

Why is option inheritance even a thing in the -blockdev part of the
world?  Why would libvirt want to rely on inheritance when using
-blockdev?

Wasn't auto-read-only created just because inheriting read-only didn't
cut it with -blockdev?

> It would also be hard to use because libvirt would have to decide
> whether to use auto-read-only (which is the only option supported by
> most drivers) or dynamic-auto-read-only (file-posix only at the moment).

Libvirt doesn't use auto-read-only so far.  Whether anything else uses
it is unknown.  If I had to guess, I'd guess "unused".  Two reasons.
One, there hasn't been much time for it to pick up users.  Two, the one
user eager to use it (libvirt) couldn't, because it turned out to be too
weak.

Why would libvirt want to start auto-read-only now?  Why is it only too
weak for "file" etc., but not only fine, but actually necessary for
whatever other drivers implement it?

What is it exactly that libvirt needs?

> In other words, in 4.1 we might have the same problem again because
> another driver implemented dynamic-auto-read-only and we need to get
> this information to libvirt.

If we add dynamic-auto-read-only to BlockdevOptionsFile, we won't have
the same problem: when the "foo" driver implements it, we simply add it
to BlockdevOptionsFoo.

> So I think this is not a real solution.

Way too many open questions for me to agree or disagree.

>> > * We need command line introspection (old news).
>
> We do, but that's orthogonal to this one. Introspecting -blockdev
> wouldn't provide more information than introspecting blockdev-add.
>
>> > * A general method to make semantic changes visible syntactically would
>> >   be useful.  The "augment the QAPI schema with feature flags" idea we
>> >   discussed in last KVM Forum's hallway track could be a starting point.
>> 
>> These are not 4.0 goals.
>
> But do we want to build it for 4.1?
>
> If so, I would try to find alternatives to query-qemu-features for the
> short term because we don't need both. But if we don't actually want to
> build it, then I think query-qemu-features is still the next best thing
> and there is no need to find a different solution now.

As always, it's a matter of resources and also of strategically
allocating them.

Even if we decide to use our 4.1 resources for something else (command
line introspection, perhaps), I'd prefer artificial schema changes over
abusing query-qemu-features for build properties.

Here's the first such change that crosses my mind.  It's pretty stupid.
Add @dynamic-auto-read-only to BlockdevOptionsFile.  It does precisely
two things:

1. Signal @auto-read-only is actually usable[**].

2. Get the combination auto-read-only=on,dynamic-auto-read-only=off
   rejected.

Hindsight 20/20: auto-read-only should not have been made a stable
interface without an actual user.  Yes, libvirt doesn't want to use
unstable interfaces.  But the way to break that impasse is not to roll
over and pretend completely unproven stuff was "stable".  The way to
break that impasse is a git branch.

That leads me to my second proposal: rename @auto-read-only to
@dynamic-read-only, call it a day.

I rather like it, to be honest.


[*] 
http://www.linux-kvm.org/images/7/7a/02x05-Aspen-Markus_Armbruster-QEMU_interface_introspection.pdf
[**] For libvirt.  We think.