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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 3/28/19 1:28 PM, Kevin Wolf wrote:
>> From: Stefan Hajnoczi <address@hidden>
>> 
>> 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-features command. It returns a struct
>> containing booleans for each feature that QEMU can support.
>> 
>> The decision to make this a command rather than something statically
>> defined in the schema is intentional. It allows QEMU to decide which
>> features are available at runtime, if necessary.
>> 
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Kevin Wolf <address@hidden>
>> ---
>>  qapi/misc.json | 23 +++++++++++++++++++++++
>>  qmp.c          | 10 ++++++++++
>>  2 files changed, 33 insertions(+)
>
> Addition of a new QMP command, but justifiable for 4.0 because of the
> nature of the bug which it will enable us to fix.  Still, let's get it
> into -rc2 rather than delaying the release...
>
> Reviewed-by: Eric Blake <address@hidden>

Pending release is reason to hurry (and posting working patches is a
proven way to cut to the chase).  But it's no excuse for skimping on the
design homework, so let's at least try to review the solution space.

This is long, my apologies.  There's a summary at the end.


The general problem is providing management applications the means to
detect external QEMU interface changes relevant to them.

We provide two external interfaces to management applications: command
line and QMP.


Interface changes can be syntactic: a new command, a new parameter,
additional parameter values, and so forth.

Detecting syntactic QMP changes is in principle a solved problem: schema
introspection with query-qmp-schema.  "In principle" because there are
still a few gaps where QMP bypasses the schema.

Detecting syntactic command line changes is solvable the same way:
QAPify, provide schema introspection, done.  Hasn't been done because
the QAPIfy step is hard.  All we have for the command line is
query-command-line-options, which falls well short of requirements.

Sometimes, QMP and the command line share a piece of syntax.
query-qmp-schema cen then serve as a witness for the command line.
Relying on the sharing that way is admittedly a bit unclean.

Sometimes, we artificially make QMP share a piece of command line syntax
just to make it visible in query-qmp-schema.  Ugly, but it can be the
least bad alternative.  Recent example (commit e1ca8f7e191): we created
QMP command query-display-options just to make the argument of -display
visible in QAPI.  We don't have a use case for actually running the
command.


Interface changes can also be purely semantic: we improve behavior in a
backwards compatible way without changing syntax.  Management
applications need to know whether they can rely on the improved
behavior.  This is what motivates this series: BlockdevOptions member
@auto-read-only=on has become dynamic for the file-posix drivers, and
libvirt wants to rely on it.  The file-posix drivers are "file",
"host_device", "host_cdrom", but only on a POSIX host, not on a Windows
host.

We can turn a purely semantic change into a syntactic one by creating a
suitable syntactic change.

For instance, we could add member @dynamic-auto-read-only next to
@auto-read-only, and deprecate @auto-read-only.  Doesn't feel too bad in
this case, but we may not always find something that's similarly
tolerable.

The syntactic change could also be elsewhere.  For instance, we could
have a QMP command whose only purpose is to expose some semantic change
syntactically.  The proposed new command query-qemu-features is like
that: presence of @file-posix-dynamic-auto-read-only in its return type
is the syntactic change we tie to the improved @auto-read-only behavior
in file-posix drivers.

Note that actually running the command provides no additional
information (it could for future extensions; more on that below).  That
means the command is just a crutch for getting information into the QAPI
QMP schema.

Making the syntactic change elsewhere poses the question what semantic
changes exactly it applies to.  Consider @auto-read-only.  It is part of
several external interfaces (because BlockdevOptions is): blockdev-add,
x-blockdev-reopen, blockdev-create, -blockdev, qemu-img create ..., but
only applies to certain drivers and only on certain hosts.  There is no
*syntactic* tie between query-qemu-features and the commands, options
and drivers it applies to.  The management application just has to know.
Workable, I guess, but I can't say I like it.

Digression: what exactly is it the management application has to now in
this particular case?

    The semantic change is actually just for block/file-posix.c's
    drivers, i.e. "file", "host_device", "host_cdrom" on a POSIX host,
    but not on a Windows host.

    In fact, all the other drivers appear to ignore @auto-read-only.
    That's actually within spec:

    # @auto-read-only: if true and @read-only is false, QEMU may automatically
    #                  decide not to open the image read-write as requested, but
    #                  fall back to read-only instead (and switch between the 
modes
    #                  later), e.g. depending on whether the image file is 
writable
    #                  or whether a writing user is attached to the node
    #                  (default: false, since 3.1)

    "QEMU may" means @auto-read-only is completely advisory.

    I suspect libvirt wants to know and rely on the fact that the "file"
    driver (and possibly the other two) *always* honor it, and actually
    doesn't care what other drivers do with it.  This goes well beyond
    what's documented in the QAPI schema.

    Commit 23dece19da4 "file-posix: Make auto-read-only dynamic" changes
    how the three drivers implementing @auto-read-only use their leeway.
    According to the commit message, they didn't actually "switch
    between the modes later" before the commit, but do afterwards,
    namely when the first writer appears and when the last writer
    disappears.

    I suspect libvirt wants to know and rely on the fact that the "file"
    driver (and possibly the other two) not only always honor
    @auto-read-only, but also when exactly they switch from read-only to
    read/write and back.  Again, this goes well beyond what's documented
    in the QAPI schema.

What if additional drivers start to honor @auto-read-only?  Would
@file-posix-dynamic-auto-read-only apply to them?  I guess not, or else
it's misnamed.  Would we want another feature flag then?  Perhaps not,
if libvirt is only interested in "file".


We actually discussed the problem of exposing semantic changes
syntactically in the last KVM Forum's hallway track, and came up with
another solution: augment the QAPI schema with feature flags.

The basic idea is simple.  Let me explain with an example.

Instead of what I proposed above:

     { 'union': 'BlockdevOptions',
       'base': { 'driver': 'BlockdevDriver',
                 '*node-name': 'str',
                 '*discard': 'BlockdevDiscardOptions',
                 '*cache': 'BlockdevCacheOptions',
                 '*read-only': 'bool',
                 '*auto-read-only': 'bool',
    +            '*dynamic-auto-read-only': 'bool',
                 '*force-share': 'bool',
                 '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
    [...]
    }

permit something like

     { 'union': 'BlockdevOptions',
    +  'features': { 'dynamic-auto-read-only': true },
    [...]
     }

or even

     { 'union': 'BlockdevOptions',
       'base': { 'driver': 'BlockdevDriver',
                 '*node-name': 'str',
                 '*discard': 'BlockdevDiscardOptions',
                 '*cache': 'BlockdevCacheOptions',
                 '*read-only': 'bool',
                 '*auto-read-only': { 'type': 'bool',
                                      'features': 'dynamic': true },
    [...]
     }

This creates a *syntactic* tie between the feature flag and what it
applies to.

In the case of dynamic auto-read-only, we should perhaps tie to the
actual driver:

     { 'struct': 'BlockdevOptionsFile',
    +  'features': { 'dynamic-auto-read-only': true }
       'data': { 'filename': 'str',
                 '*pr-manager': 'str',
                 '*locking': 'OnOffAuto',
                 '*aio': 'BlockdevAioOptions',
                 '*drop-cache': {'type': 'bool',
                                 'if': 'defined(CONFIG_LINUX)'},
                 '*x-check-cache-dropped': 'bool' } }

If the feature applies only to some users of BlockdevOptions (remember:
blockdev-add, x-blockdev-reopen, blockdev-create, -blockdev, qemu-img
create ...), and that's actually important to know, we could create a
syntactic tie there, e.g.

    -{ 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
    +{ 'command': 'blockdev-add',
    +  'features': { 'dynamic-auto-read-only': true }
    +  'data': 'BlockdevOptions', 'boxed': true }

I pulled the new syntax out of thin air, it's just an example.

Naturally, there's now way we can pull this off for 4.0.


Expressing "features" right in the QAPI schema feels nicer than having
them on the side, in query-qemu-features.

Of course, "can have" beats "nice".

There's one case where only query-qemu-features works: when the feature
cannot be fixed at compile-time.  I discussed this topic in my review of
Stefan's patch

Subject: Re: [PATCH] qmp: add query-qemu-capabilities
Message-ID: <address@hidden>
https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg06951.html

Academic until somebody comes up with an actual use case.


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.

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

* We need command line introspection (old news).

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


Comments?  Opinions?