| Message ID | 20190328182810.21497-2-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file-posix: query-qemu-features for auto-read-only | expand |
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>
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?
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.
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; > +}
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 :)
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
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
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.
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; +}