[1/2] qmp: Add query-qemu-features
diff mbox series

Message ID 20190328182810.21497-2-kwolf@redhat.com
State New
Headers show
Series
  • file-posix: query-qemu-features for auto-read-only
Related show

Commit Message

Kevin Wolf March 28, 2019, 6:28 p.m. UTC
From: Stefan Hajnoczi <stefanha@redhat.com>

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 <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/misc.json | 23 +++++++++++++++++++++++
 qmp.c          | 10 ++++++++++
 2 files changed, 33 insertions(+)

Comments

Eric Blake March 28, 2019, 6:43 p.m. UTC | #1
On 3/28/19 1:28 PM, Kevin Wolf wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> 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 <stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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 <eblake@redhat.com>
Markus Armbruster March 29, 2019, 1:52 p.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 3/28/19 1:28 PM, Kevin Wolf wrote:
>> From: Stefan Hajnoczi <stefanha@redhat.com>
>> 
>> 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 <stefanha@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  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 <eblake@redhat.com>

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: <87a7iincg0.fsf@dusky.pond.sub.org>
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?
Eric Blake March 29, 2019, 3:06 p.m. UTC | #3
On 3/29/19 8:52 AM, Markus Armbruster wrote:

> 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' },
>     [...]
>     }

If I'm summarizing correctly, this is the minimum we could pull off to
get a syntactic witness of the change, in time for 4.0, while leaving:

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

as future work.


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

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

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

These are not 4.0 goals.

> 
> 
> Comments?  Opinions?

Thanks for working on a nice summary, and forcing us to think about the
long-term before taking a short-term hack that we may regret.
Markus Armbruster March 29, 2019, 4:06 p.m. UTC | #4
Kevin Wolf <kwolf@redhat.com> writes:

> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> 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 <stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/misc.json | 23 +++++++++++++++++++++++
>  qmp.c          | 10 ++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 8b3ca4fdd3..d892f37633 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -3051,3 +3051,26 @@
>    'data': 'NumaOptions',
>    'allow-preconfig': true
>  }
> +
> +##
> +# @QemuFeatures:
> +#
> +# Information about support for QEMU features that isn't available through
> +# schema introspection.

This becomes incorrect the moment you apply the patch :)

> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'QemuFeatures',
> +  'data': { } }
> +
> +##
> +# @query-qemu-features:
> +#
> +# Return the features supported by this QEMU. Most features can be detected
> +# via schema introspection but some are not observable from the schema. This
> +# command offers a way to check for the presence of such features.

Likewise.

To be useful, the command must document each feature's extent.

The feature added in the next patch is a property of the QEMU build.
This means:

* Running the command provides no additional information over
  query-qmp-schema.

* You can safely cache the result for future invocations of the same
  QEMU build.

We'll figure out implications of different feature extents once we have
them.

> +#
> +# Since: 4.0
> +##
> +{ 'command': 'query-qemu-features',
> +  'returns': 'QemuFeatures' }
> diff --git a/qmp.c b/qmp.c
> index b92d62cd5f..0aad533eca 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -717,3 +717,13 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
>  
>      return mem_info;
>  }
> +
> +QemuFeatures *qmp_query_qemu_features(Error **errp)
> +{
> +    QemuFeatures *caps = g_new(QemuFeatures, 1);
> +
> +    *caps = (QemuFeatures) {
> +    };
> +
> +    return caps;
> +}
Markus Armbruster March 29, 2019, 4:43 p.m. UTC | #5
Eric Blake <eblake@redhat.com> writes:

> On 3/29/19 8:52 AM, Markus Armbruster wrote:
>
>> 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' },
>>     [...]
>>     }
>
> If I'm summarizing correctly, this is the minimum we could pull off to
> get a syntactic witness of the change, in time for 4.0, while leaving:
>
>> 
>> permit something like
>> 
>>      { 'union': 'BlockdevOptions',
>>     +  'features': { 'dynamic-auto-read-only': true },
>>     [...]
>>      }
> ...
>
> as future work.

Yes.

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

Yes.

Of course, crutchiness is no excuse for not addressing the need in 4.0.
Either we come up with something we like better, or we accept the
crutch.

>                                  - but your arguments say we may have
> something closer:
>
>> 
>> * 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).

Yes, adding @dynamic-auto-read-only is also an opportunity to specify
the semantics libvirt actually wants to rely on.  Not knowing enough
about that, I'd approximate by spelling out current actual behavior,
like this:

# @dynamic-auto-read-only: if true, QEMU opens the file read-only
#                  regardless of @read-only.
#                  If @read-only is false, QEMU reopens the file read/write
#                  when a writing user attaches to the node, and
#                  reopens it read-only again when the last writin user
#                  detaches from the node.
#                  (default: false, since 4.0)

We can either add it to BlockdevOptionsFile or to BlockdevOptions (next
to @auto-read-only).  If we choose the latter, the doc comment needs to
spell out that @dynamic-auto-read-only applies only to drivers "file",
"host_device" and "host_cdrom".

Regardless of where we put it, we should guard it with CONFIG_POSIX.

> 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),

@auto-read-only as specified feels too weak to be useful.  But it's hard
to predict the future.

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

Agree.

>> * 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.
>
> These are not 4.0 goals.

True!

>> Comments?  Opinions?
>
> Thanks for working on a nice summary, and forcing us to think about the
> long-term before taking a short-term hack that we may regret.

You're welcome :)
Kevin Wolf March 29, 2019, 5:04 p.m. UTC | #6
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.

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

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

So I think this is not a real solution.

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

Kevin
Kevin Wolf March 29, 2019, 5:29 p.m. UTC | #7
Am 29.03.2019 um 18:04 hat Kevin Wolf geschrieben:
> 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.
> 
> > > * 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.
> 
> 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).
> 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.
> 
> So I think this is not a real solution.
> 
> > > * 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.

One way to buy us some more time until 4.1 would be to extend
BlockdevOptionsFile with an optional field x-auto-read-only-is-dynamic
(type null), which is not to be used except for introspection and which
will be dropped again as soon as we get real feature flags in the schema
in 4.1.

Kevin
Markus Armbruster March 30, 2019, 1:24 p.m. UTC | #8
Kevin Wolf <kwolf@redhat.com> 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.

Patch
diff mbox series

diff --git a/qapi/misc.json b/qapi/misc.json
index 8b3ca4fdd3..d892f37633 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3051,3 +3051,26 @@ 
   'data': 'NumaOptions',
   'allow-preconfig': true
 }
+
+##
+# @QemuFeatures:
+#
+# Information about support for QEMU features that isn't available through
+# schema introspection.
+#
+# Since: 4.0
+##
+{ 'struct': 'QemuFeatures',
+  'data': { } }
+
+##
+# @query-qemu-features:
+#
+# Return the features supported by this QEMU. Most features can be detected
+# via schema introspection but some are not observable from the schema. This
+# command offers a way to check for the presence of such features.
+#
+# Since: 4.0
+##
+{ 'command': 'query-qemu-features',
+  'returns': 'QemuFeatures' }
diff --git a/qmp.c b/qmp.c
index b92d62cd5f..0aad533eca 100644
--- a/qmp.c
+++ b/qmp.c
@@ -717,3 +717,13 @@  MemoryInfo *qmp_query_memory_size_summary(Error **errp)
 
     return mem_info;
 }
+
+QemuFeatures *qmp_query_qemu_features(Error **errp)
+{
+    QemuFeatures *caps = g_new(QemuFeatures, 1);
+
+    *caps = (QemuFeatures) {
+    };
+
+    return caps;
+}