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.
+#
+# 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;
+}
--
2.20.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> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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.
Let's review our options for 4.0.
Please note my analysis is handicapped by incomplete information, in
particular on libvirt's needs.
Terminology:
* "Hard read-write" semantics: open read/write.
* "Hard read-only" semantics: open read-only.
* "Dynamic read-only" semantics: open read-only, reopen read/write when
a writer attaches, reopen read-only when the last writer detaches.
* "Fallback read-only" semantics:. try to open read/write, fall back to
read-only.
We have a use case for dynamic read-only: libvirt. I'm not aware of a
use case for fallback read-only.
Behavior before 3.1:
* read-only=on: hard read-only
* read-only=off: hard read-write
Behavior in 3.1: new parameter auto-read-only, default off.
* read-only=on: hard read-only (no change)
* read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
silently ignored)
* read-only=off: hard read-write
* read-only=off,auto-read-only=on: depends on the driver
- file-posix.c's drivers: fallback read-only
- some other drivers: fallback read-only
- remaining drivers: hard read/write
Behavior in 4.0 so far:
* read-only=on: hard read-only (no change)
* read-only=on,auto-read-only=on: hard read-only (no change)
* read-only=off: hard read-write (no change)
* read-only=off,auto-read-only=on: depends on the driver
- file-posix.c's drivers: dynamic read-only
- some other drivers: fallback read-only (no change)
- remaining drivers: hard read/write (no change)
Option 1: Rename @auto-read-only.
Rationale: we released auto-read-only in v3.1 with unproven fallback
read-only semantics. It turned out not to be useful. Admit the
mistake, retract it. Release our next attempt in 4.0 under a suitable
new name with fallback read-only semantics.
CON:
* Compatibility break. No-go if there are users. Users seem quite
unlikely, though.
* Still unproven, albeit less so: this time we have some unreleased
(unfinished?) libvirt code.
* Semantics are still largely left to drivers, and the schema can't tell
which one does what. Awkward.
* Unless we're fairly confident we won't upgrade drivers from "hard" to
"fallback" to "dynamic", this merely kicks the can down the road:
we'll face the exact same "how can libvirt find out" problem on every
upgrade.
PRO:
* When libvirt sees the new name, it can rely on file-posix.c's drivers
providing dynamic read-only semantics.
Option 2: Add query-qemu-features command, return
file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
Rationale: we released auto-read-only in v3.1 with unproven fallback
read-only semantics. It turned out not to be useful. Admit the
mistake, and patch it up in 4.0. Libirt needs to know whether it's
patche up, and this is a simple way to tell it.
CON:
* All of option 1's, except for the compatibility break
* Uses query-qemu-features to expose a property of the build. I
consider that a mistake.
PRO
* Libvirt can use either query-qmp-schema or query-qemu-features to find
out whether it can can rely on file-posix.c's drivers providing
dynamic read-only semantics. To make query-qemu-features usable, we
need to promise query-qemu-features never returns false for it. To
make query-qemu-features usable, we need to promise the value will
remain valid for the remainder of the QEMU run (defeats caching) or
for any future run of the same QEMU binary (enables caching).
Option 3: Add @dynamic-read-only to the drivers that provide dynamic
read-only, default to value of auto-read-only, promise we'll never add
it to drivers that don't.
Rationale: give users explicit control over dynamic vs. fallback for all
drivers that can provide dynamic. This makes some sense as an
interface, as long as you ignore the fact that no driver implements both
dynamic and fallback. I can't see why a driver couldn't implement both.
It also makes dynamic support visible in the schema.
Behavior (three bools -> eight cases):
* read-only=on: hard read-only (no change)
Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
* read-only=on,auto-read-only=on: hard read-only (no change)
Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
* read-only=off: hard read-write (no change)
Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
* read-only=off,auto-read-only=on:
Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
- file-posix.c's drivers: dynamic read-only (no change,
dynamic-read-only=on is the default)
- some other drivers: fallback read-only (no change)
- remaining drivers: hard read/write (no change)
* read-only=off,auto-read-only=on,dynamic-read-only=off
- file-posix.c's drivers: error
- all other drivers: N/A
* read-only=off,auto-read-only=off,dynamic-read-only=on
- file-posix.c's drivers: error
- all other drivers: N/A
* read-only=on,dynamic-read-only=on
Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
- file-posix.c's drivers: error
- all other drivers: N/A
* read-only=on,auto-read-only=on,dynamic-read-only=off
Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
- file-posix.c's drivers: error
- all other drivers: N/A
CON:
* Two bools (read-only and auto-read-only) to select from three choices
was already ugly. Three bools (the same plus dynamic-read-only) to
select from four choices is even uglier.
* The explicit control is just a facade so far: since only the default
setting is implemented, it doesn't actually control anything.
PRO:
* When libvirt sees a driver providing a dynamic-read-only parameter, it
knows it can rely on the driver providing dynamic read-only semantics.
* Adding dynamic read-only capability to drivers creates no
introspection problems: we simply add dynamic-read-only to their
parameters.
Code sketch appended
From 6c5f539818a817947545bed6457a8971dccb6464 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Sun, 31 Mar 2019 09:19:11 +0200
Subject: [PATCH] dynamic-read-only WIP
---
block/file-posix.c | 12 ++++++++++++
qapi/block-core.json | 3 +++
2 files changed, 15 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index db4cccbe51..d62681df14 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -420,6 +420,11 @@ static QemuOptsList raw_runtime_opts = {
.type = QEMU_OPT_STRING,
.help = "File name of the image",
},
+ {
+ .name = "dynamic-read-only",
+ .type = QEMU_OPT_BOOL,
+ .help = "FIXME",
+ },
{
.name = "aio",
.type = QEMU_OPT_STRING,
@@ -540,6 +545,13 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
s->open_flags = open_flags;
raw_parse_flags(bdrv_flags, &s->open_flags, false);
+ if (qemu_opt_get_bool(opts, "dynamic-read-only",
+ bdrv_flags & BDRV_O_AUTO_RDONLY)
+ == !(bdrv_flags & BDRV_O_AUTO_RDONLY)) {
+ error_setg(errp, "FIXME");
+ ret = - EINVAL;
+ }
+
s->fd = -1;
fd = qemu_open(filename, s->open_flags, 0644);
ret = fd < 0 ? -errno : 0;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..0cf15477ce 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2827,6 +2827,7 @@
# Driver specific block device options for the file backend.
#
# @filename: path to the image file
+# @dynamic-read-only: FIXME document (since 4.0)
# @pr-manager: the id for the object that will handle persistent reservations
# for this device (default: none, forward the commands via SG_IO;
# since 2.11)
@@ -2847,6 +2848,8 @@
##
{ 'struct': 'BlockdevOptionsFile',
'data': { 'filename': 'str',
+ '*dynamic-read-only': {'type': 'bool',
+ 'if': 'defined(CONFIG_POSIX)'},
'*pr-manager': 'str',
'*locking': 'OnOffAuto',
'*aio': 'BlockdevAioOptions',
--
2.17.2
Patchew URL: https://patchew.org/QEMU/87y34v42z0.fsf_-_@dusky.pond.sub.org/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 87y34v42z0.fsf_-_@dusky.pond.sub.org
Subject: [Qemu-devel] Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features)
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/87y34v42z0.fsf_-_@dusky.pond.sub.org -> patchew/87y34v42z0.fsf_-_@dusky.pond.sub.org
Switched to a new branch 'test'
9e30b1b Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features)
=== OUTPUT BEGIN ===
ERROR: space prohibited after that '-' (ctx:WxW)
#223: FILE: block/file-posix.c:552:
+ ret = - EINVAL;
^
ERROR: Missing Signed-off-by: line(s)
total: 2 errors, 0 warnings, 39 lines checked
Commit 9e30b1b63472 (Options for 4.0 (was: [PATCH 1/2] qmp: Add query-qemu-features)) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/87y34v42z0.fsf_-_@dusky.pond.sub.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Am 31.03.2019 um 14:56 hat Markus Armbruster geschrieben:
> Let's review our options for 4.0.
>
> Please note my analysis is handicapped by incomplete information, in
> particular on libvirt's needs.
>
> Terminology:
>
> * "Hard read-write" semantics: open read/write.
>
> * "Hard read-only" semantics: open read-only.
>
> * "Dynamic read-only" semantics: open read-only, reopen read/write when
> a writer attaches, reopen read-only when the last writer detaches.
>
> * "Fallback read-only" semantics:. try to open read/write, fall back to
> read-only.
>
> We have a use case for dynamic read-only: libvirt. I'm not aware of a
> use case for fallback read-only.
The situation is a bit more complex than this:
libvirt requires dynamic read-only for file-posix. It prefers dynamic
read-only for other drivers, but can live with fallback read-only there.
Only file-posix implements dynamic read-only today. Other drivers
implement only fallback read-only.
Therefore libvirt has a use case for using fallback read-only for
non-file-posix drivers.
> Behavior before 3.1:
>
> * read-only=on: hard read-only
>
> * read-only=off: hard read-write
>
> Behavior in 3.1: new parameter auto-read-only, default off.
>
> * read-only=on: hard read-only (no change)
>
> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
> silently ignored)
>
> * read-only=off: hard read-write
>
> * read-only=off,auto-read-only=on: depends on the driver
>
> - file-posix.c's drivers: fallback read-only
> - some other drivers: fallback read-only
> - remaining drivers: hard read/write
>
> Behavior in 4.0 so far:
>
> * read-only=on: hard read-only (no change)
>
> * read-only=on,auto-read-only=on: hard read-only (no change)
>
> * read-only=off: hard read-write (no change)
>
> * read-only=off,auto-read-only=on: depends on the driver
>
> - file-posix.c's drivers: dynamic read-only
> - some other drivers: fallback read-only (no change)
> - remaining drivers: hard read/write (no change)
>
>
> Option 1: Rename @auto-read-only.
>
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics. It turned out not to be useful.
It turned out not to be useful for file-posix in the context of libvirt
with sVirt enabled. That's a different thing than just "not useful".
> Admit the mistake, retract it. Release our next attempt in 4.0 under
> a suitable new name with fallback read-only semantics.
I'm confused. Do you mean s/fallback/dynamic/?
file-posix is the only driver than implements dynamic read-only, so this
is not a suitable replacement for auto-read-only, which is supported by
many other drivers, too (with fallback read-only semantics, which is
useful enough for libvirt for those drivers). Removing auto-read-only
without adding a replacement for all drivers that support it would be a
regression.
> CON:
>
> * Compatibility break. No-go if there are users. Users seem quite
> unlikely, though.
>
> * Still unproven, albeit less so: this time we have some unreleased
> (unfinished?) libvirt code.
We had this last time, too. Peter just didn't realise that he hadn't
sVirt enabled in his development build...
> * Semantics are still largely left to drivers, and the schema can't tell
> which one does what. Awkward.
>
> * Unless we're fairly confident we won't upgrade drivers from "hard" to
> "fallback" to "dynamic", this merely kicks the can down the road:
> we'll face the exact same "how can libvirt find out" problem on every
> upgrade.
Hm, maybe you meant something different above. Would the new command
actually be dynamic read-only only for file-posix and fallback read-only
for everything else?
> PRO:
>
> * When libvirt sees the new name, it can rely on file-posix.c's drivers
> providing dynamic read-only semantics.
>
>
> Option 2: Add query-qemu-features command, return
> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>
> Rationale: we released auto-read-only in v3.1 with unproven fallback
> read-only semantics. It turned out not to be useful. Admit the
> mistake, and patch it up in 4.0. Libirt needs to know whether it's
> patche up, and this is a simple way to tell it.
>
> CON:
>
> * All of option 1's, except for the compatibility break
>
> * Uses query-qemu-features to expose a property of the build. I
> consider that a mistake.
>
> PRO
>
> * Libvirt can use either query-qmp-schema or query-qemu-features to find
> out whether it can can rely on file-posix.c's drivers providing
> dynamic read-only semantics. To make query-qemu-features usable, we
> need to promise query-qemu-features never returns false for it. To
> make query-qemu-features usable, we need to promise the value will
> remain valid for the remainder of the QEMU run (defeats caching) or
> for any future run of the same QEMU binary (enables caching).
I don't agree with your assessment here.
query-qmp-schema, as its name says, queries the QMP schema, which
describes the structure of QMP communication, not generally features of
the QEMU build.
Features are often related in some way to a QAPI type and clients query
the schema anyway, so it's convenient to add additional information
about features there. But it's not the only place where it makes sense.
The schema for query-qemu-features should tell you whether this QEMU
version even knows about a feature like this, i.e. whether it is capable
of providing this information in a QMP response. Only executing the
command returns whether the feature is actually supported in this build.
Inferring the value of a field from its presence in the schema feels
wrong.
> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
> read-only, default to value of auto-read-only, promise we'll never add
> it to drivers that don't.
>
> Rationale: give users explicit control over dynamic vs. fallback for all
> drivers that can provide dynamic. This makes some sense as an
> interface, as long as you ignore the fact that no driver implements both
> dynamic and fallback. I can't see why a driver couldn't implement both.
> It also makes dynamic support visible in the schema.
Except that we don't even give users more control (the drivers still
provide what they provide), but we require users to specify that they
actually expect what the driver provides.
> Behavior (three bools -> eight cases):
>
> * read-only=on: hard read-only (no change)
>
> Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>
> * read-only=on,auto-read-only=on: hard read-only (no change)
>
> Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>
> * read-only=off: hard read-write (no change)
>
> Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>
> * read-only=off,auto-read-only=on:
>
> Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>
> - file-posix.c's drivers: dynamic read-only (no change,
> dynamic-read-only=on is the default)
> - some other drivers: fallback read-only (no change)
> - remaining drivers: hard read/write (no change)
>
> * read-only=off,auto-read-only=on,dynamic-read-only=off
>
> - file-posix.c's drivers: error
> - all other drivers: N/A
>
> * read-only=off,auto-read-only=off,dynamic-read-only=on
>
> - file-posix.c's drivers: error
> - all other drivers: N/A
>
> * read-only=on,dynamic-read-only=on
>
> Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>
> - file-posix.c's drivers: error
> - all other drivers: N/A
>
> * read-only=on,auto-read-only=on,dynamic-read-only=off
>
> Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>
> - file-posix.c's drivers: error
> - all other drivers: N/A
>
> CON:
>
> * Two bools (read-only and auto-read-only) to select from three choices
> was already ugly. Three bools (the same plus dynamic-read-only) to
> select from four choices is even uglier.
>
> * The explicit control is just a facade so far: since only the default
> setting is implemented, it doesn't actually control anything.
>
> PRO:
>
> * When libvirt sees a driver providing a dynamic-read-only parameter, it
> knows it can rely on the driver providing dynamic read-only semantics.
>
> * Adding dynamic read-only capability to drivers creates no
> introspection problems: we simply add dynamic-read-only to their
> parameters.
Option 4:
Add a dummy option to BlockdevOptionsFile:
'*x-auto-read-only-is-dynamic': { 'type': 'null',
'if': 'defined(CONFIG_POSIX)' }
Specifying it has no effect, so the ridiculous complexity of three bools
to select from three options is avoided. Its presence in the schema
indicates that file-posix implements dynamic auto-read-only.
Basically this is features flags in the schema without having proper
feature flags yet.
Once we add real annotations (hopefully 4.1), this dummy field can be
removed again.
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 31.03.2019 um 14:56 hat Markus Armbruster geschrieben:
>> Let's review our options for 4.0.
>>
>> Please note my analysis is handicapped by incomplete information, in
>> particular on libvirt's needs.
>>
>> Terminology:
>>
>> * "Hard read-write" semantics: open read/write.
>>
>> * "Hard read-only" semantics: open read-only.
>>
>> * "Dynamic read-only" semantics: open read-only, reopen read/write when
>> a writer attaches, reopen read-only when the last writer detaches.
>>
>> * "Fallback read-only" semantics:. try to open read/write, fall back to
>> read-only.
>>
>> We have a use case for dynamic read-only: libvirt. I'm not aware of a
>> use case for fallback read-only.
>
> The situation is a bit more complex than this:
>
> libvirt requires dynamic read-only for file-posix. It prefers dynamic
> read-only for other drivers, but can live with fallback read-only there.
Aha!
> Only file-posix implements dynamic read-only today. Other drivers
> implement only fallback read-only.
>
> Therefore libvirt has a use case for using fallback read-only for
> non-file-posix drivers.
>
>> Behavior before 3.1:
>>
>> * read-only=on: hard read-only
>>
>> * read-only=off: hard read-write
>>
>> Behavior in 3.1: new parameter auto-read-only, default off.
>>
>> * read-only=on: hard read-only (no change)
>>
>> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>> silently ignored)
>>
>> * read-only=off: hard read-write
>>
>> * read-only=off,auto-read-only=on: depends on the driver
>>
>> - file-posix.c's drivers: fallback read-only
>> - some other drivers: fallback read-only
>> - remaining drivers: hard read/write
>>
>> Behavior in 4.0 so far:
>>
>> * read-only=on: hard read-only (no change)
>>
>> * read-only=on,auto-read-only=on: hard read-only (no change)
>>
>> * read-only=off: hard read-write (no change)
>>
>> * read-only=off,auto-read-only=on: depends on the driver
>>
>> - file-posix.c's drivers: dynamic read-only
>> - some other drivers: fallback read-only (no change)
>> - remaining drivers: hard read/write (no change)
>>
>>
>> Option 1: Rename @auto-read-only.
>>
>> Rationale: we released auto-read-only in v3.1 with unproven fallback
>> read-only semantics. It turned out not to be useful.
>
> It turned out not to be useful for file-posix in the context of libvirt
> with sVirt enabled. That's a different thing than just "not useful".
Right. It turned out to be insufficient, which is not the same as "not
useful".
>> Admit the mistake, retract it. Release our next attempt in 4.0 under
>> a suitable new name with fallback read-only semantics.
>
> I'm confused. Do you mean s/fallback/dynamic/?
Messed up prose, sorry.
> file-posix is the only driver than implements dynamic read-only, so this
> is not a suitable replacement for auto-read-only, which is supported by
> many other drivers, too (with fallback read-only semantics, which is
> useful enough for libvirt for those drivers). Removing auto-read-only
> without adding a replacement for all drivers that support it would be a
> regression.
Our next attempt is actually "dynamic read-only with file-posix.c's
drivers, fallback read-only with some other drivers, hard read/write
with the remaining drivers". All enabled by a single flag.
Documenting this with sufficient precision without making a mess of it
won't be easy.
In my opinion, we did make a mess of it in 3.1: we specified only what
QEMU *may* do, not what it actually does. Am I confused or was libvirt
expected to rely on actual behavior and not just the specification?
>> CON:
>>
>> * Compatibility break. No-go if there are users. Users seem quite
>> unlikely, though.
>>
>> * Still unproven, albeit less so: this time we have some unreleased
>> (unfinished?) libvirt code.
>
> We had this last time, too. Peter just didn't realise that he hadn't
> sVirt enabled in his development build...
I see. High pressure is bound to lead to mistakes.
>> * Semantics are still largely left to drivers, and the schema can't tell
>> which one does what. Awkward.
>>
>> * Unless we're fairly confident we won't upgrade drivers from "hard" to
>> "fallback" to "dynamic", this merely kicks the can down the road:
>> we'll face the exact same "how can libvirt find out" problem on every
>> upgrade.
>
> Hm, maybe you meant something different above. Would the new command
> actually be dynamic read-only only for file-posix and fallback read-only
> for everything else?
Yes.
>> PRO:
>>
>> * When libvirt sees the new name, it can rely on file-posix.c's drivers
>> providing dynamic read-only semantics.
>>
>>
>> Option 2: Add query-qemu-features command, return
>> file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>>
>> Rationale: we released auto-read-only in v3.1 with unproven fallback
>> read-only semantics. It turned out not to be useful. Admit the
>> mistake, and patch it up in 4.0. Libirt needs to know whether it's
>> patche up, and this is a simple way to tell it.
>>
>> CON:
>>
>> * All of option 1's, except for the compatibility break
>>
>> * Uses query-qemu-features to expose a property of the build. I
>> consider that a mistake.
>>
>> PRO
>>
>> * Libvirt can use either query-qmp-schema or query-qemu-features to find
>> out whether it can can rely on file-posix.c's drivers providing
>> dynamic read-only semantics. To make query-qemu-features usable, we
"To make query-qmp-schema usable", of course.
>> need to promise query-qemu-features never returns false for it. To
>> make query-qemu-features usable, we need to promise the value will
>> remain valid for the remainder of the QEMU run (defeats caching) or
>> for any future run of the same QEMU binary (enables caching).
>
> I don't agree with your assessment here.
>
> query-qmp-schema, as its name says, queries the QMP schema, which
> describes the structure of QMP communication, not generally features of
> the QEMU build.
>
> Features are often related in some way to a QAPI type and clients query
> the schema anyway, so it's convenient to add additional information
> about features there. But it's not the only place where it makes sense.
I've never denied the existence of use cases for query-qemu-features.
I just believe this ain't one :)
> The schema for query-qemu-features should tell you whether this QEMU
> version even knows about a feature like this, i.e. whether it is capable
> of providing this information in a QMP response. Only executing the
> command returns whether the feature is actually supported in this build.
> Inferring the value of a field from its presence in the schema feels
> wrong.
That depends.
If we specify that query-qemu-features never returns false for a certain
feature, there is no need to actually run it to obtain the feature's
value. If you prefer to run query-qemu-features anyway, go ahead and
run it to your heart's content. You'll still rely on "never returns
false", because that's the part that tells you for how long the response
remains valid.
If we don't specify "never returns false", we get to specify directly
for how long the response remains valid.
>> Option 3: Add @dynamic-read-only to the drivers that provide dynamic
>> read-only, default to value of auto-read-only, promise we'll never add
>> it to drivers that don't.
>>
>> Rationale: give users explicit control over dynamic vs. fallback for all
>> drivers that can provide dynamic. This makes some sense as an
>> interface, as long as you ignore the fact that no driver implements both
>> dynamic and fallback. I can't see why a driver couldn't implement both.
>> It also makes dynamic support visible in the schema.
>
> Except that we don't even give users more control (the drivers still
> provide what they provide), but we require users to specify that they
> actually expect what the driver provides.
That would be a feature. Declaring needs explicitly is *good*.
We don't actually require it here because of defaults.
>> Behavior (three bools -> eight cases):
>>
>> * read-only=on: hard read-only (no change)
>>
>> Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>>
>> * read-only=on,auto-read-only=on: hard read-only (no change)
>>
>> Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>>
>> * read-only=off: hard read-write (no change)
>>
>> Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>>
>> * read-only=off,auto-read-only=on:
>>
>> Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>>
>> - file-posix.c's drivers: dynamic read-only (no change,
>> dynamic-read-only=on is the default)
>> - some other drivers: fallback read-only (no change)
>> - remaining drivers: hard read/write (no change)
>>
>> * read-only=off,auto-read-only=on,dynamic-read-only=off
>>
>> - file-posix.c's drivers: error
>> - all other drivers: N/A
>>
>> * read-only=off,auto-read-only=off,dynamic-read-only=on
>>
>> - file-posix.c's drivers: error
>> - all other drivers: N/A
>>
>> * read-only=on,dynamic-read-only=on
>>
>> Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>>
>> - file-posix.c's drivers: error
>> - all other drivers: N/A
>>
>> * read-only=on,auto-read-only=on,dynamic-read-only=off
>>
>> Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>>
>> - file-posix.c's drivers: error
>> - all other drivers: N/A
>>
>> CON:
>>
>> * Two bools (read-only and auto-read-only) to select from three choices
>> was already ugly. Three bools (the same plus dynamic-read-only) to
>> select from four choices is even uglier.
>>
>> * The explicit control is just a facade so far: since only the default
>> setting is implemented, it doesn't actually control anything.
>>
>> PRO:
>>
>> * When libvirt sees a driver providing a dynamic-read-only parameter, it
>> knows it can rely on the driver providing dynamic read-only semantics.
>>
>> * Adding dynamic read-only capability to drivers creates no
>> introspection problems: we simply add dynamic-read-only to their
>> parameters.
>
> Option 4:
>
> Add a dummy option to BlockdevOptionsFile:
>
> '*x-auto-read-only-is-dynamic': { 'type': 'null',
> 'if': 'defined(CONFIG_POSIX)' }
>
> Specifying it has no effect, so the ridiculous complexity of three bools
> to select from three options is avoided. Its presence in the schema
> indicates that file-posix implements dynamic auto-read-only.
>
> Basically this is features flags in the schema without having proper
> feature flags yet.
>
> Once we add real annotations (hopefully 4.1), this dummy field can be
> removed again.
Exact same patch as for option 3, with the parameter renamed and the
sanity check for non-sensical use dropped. *Shrug*
Puts more pressure on me to deliver QAPI feature flags sooner rather
than later. My QAPI pressure control valve has been shedding pressure
for a while, so "bring it on". I'd advise against holding your breath,
though.
SIGCHLD, back later.
Am 01.04.2019 um 15:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > Option 4:
> >
> > Add a dummy option to BlockdevOptionsFile:
> >
> > '*x-auto-read-only-is-dynamic': { 'type': 'null',
> > 'if': 'defined(CONFIG_POSIX)' }
> >
> > Specifying it has no effect, so the ridiculous complexity of three bools
> > to select from three options is avoided. Its presence in the schema
> > indicates that file-posix implements dynamic auto-read-only.
> >
> > Basically this is features flags in the schema without having proper
> > feature flags yet.
> >
> > Once we add real annotations (hopefully 4.1), this dummy field can be
> > removed again.
>
> Exact same patch as for option 3, with the parameter renamed and the
> sanity check for non-sensical use dropped. *Shrug*
>
> Puts more pressure on me to deliver QAPI feature flags sooner rather
> than later. My QAPI pressure control valve has been shedding pressure
> for a while, so "bring it on". I'd advise against holding your breath,
> though.
Okay, let's stop beating around the bush.
Nobody has told me so explicitly during this discussion, but implicitly
I understand that everyone seems to think doing annotations in the
schema is what we really want to have.
Instead of continuing to argue which option to get around it is the
least ugly one - how bad is the following attempt at just implementing
what we really want?
Only for structs for now, but that's all we need at the moment. Should
be easy to extend during the 4.1 cycle (or whenever we need something
else).
Note that even if there are bugs in it, they are not user-visible and
can just be fixed in 4.1. Currently, a diff between old and new
query-qmp-schema output looks sane - just the added 'features' list
where we want it.
Kevin
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..d74bf4a88a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2852,7 +2852,8 @@
'*aio': 'BlockdevAioOptions',
'*drop-cache': {'type': 'bool',
'if': 'defined(CONFIG_LINUX)'},
- '*x-check-cache-dropped': 'bool' } }
+ '*x-check-cache-dropped': 'bool' },
+ 'features': [ 'dynamic-auto-read-only' ] }
##
# @BlockdevOptionsNull:
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index f07869ec73..668ebb654a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -886,12 +886,22 @@ def check_enum(expr, info):
def check_struct(expr, info):
name = expr['struct']
members = expr['data']
+ features = expr.get('features')
check_type(info, "'data' for struct '%s'" % name, members,
allow_dict=True, allow_optional=True)
check_type(info, "'base' for struct '%s'" % name, expr.get('base'),
allow_metas=['struct'])
+ if features:
+ if not isinstance(features, list):
+ raise QAPISemError(info,
+ "Struct '%s' requires an array for 'features'" %
+ name)
+ for s in features:
+ if not isinstance(s, str):
+ raise QAPISemError(info, "'features' for struct '%s' must "
+ "contain strings" % name)
def check_known_keys(info, source, keys, required, optional):
@@ -986,7 +996,7 @@ def check_exprs(exprs):
normalize_members(expr['data'])
elif 'struct' in expr:
meta = 'struct'
- check_keys(expr_elem, 'struct', ['data'], ['base', 'if'])
+ check_keys(expr_elem, 'struct', ['data'], ['base', 'if', 'features'])
normalize_members(expr['data'])
struct_types[expr[meta]] = expr
elif 'command' in expr:
@@ -1129,7 +1139,8 @@ class QAPISchemaVisitor(object):
def visit_object_type(self, name, info, ifcond, base, members, variants):
pass
- def visit_object_type_flat(self, name, info, ifcond, members, variants):
+ def visit_object_type_flat(self, name, info, ifcond, members, variants,
+ features):
pass
def visit_alternate_type(self, name, info, ifcond, variants):
@@ -1290,7 +1301,7 @@ class QAPISchemaArrayType(QAPISchemaType):
class QAPISchemaObjectType(QAPISchemaType):
def __init__(self, name, info, doc, ifcond,
- base, local_members, variants):
+ base, local_members, variants, features):
# struct has local_members, optional base, and no variants
# flat union has base, variants, and no local_members
# simple union has local_members, variants, and no base
@@ -1307,6 +1318,7 @@ class QAPISchemaObjectType(QAPISchemaType):
self.local_members = local_members
self.variants = variants
self.members = None
+ self.features = features
def check(self, schema):
QAPISchemaType.check(self, schema)
@@ -1370,7 +1382,8 @@ class QAPISchemaObjectType(QAPISchemaType):
visitor.visit_object_type(self.name, self.info, self.ifcond,
self.base, self.local_members, self.variants)
visitor.visit_object_type_flat(self.name, self.info, self.ifcond,
- self.members, self.variants)
+ self.members, self.variants,
+ self.features)
class QAPISchemaMember(object):
@@ -1675,7 +1688,7 @@ class QAPISchema(object):
('null', 'null', 'QNull' + pointer_suffix)]:
self._def_builtin_type(*t)
self.the_empty_object_type = QAPISchemaObjectType(
- 'q_empty', None, None, None, None, [], None)
+ 'q_empty', None, None, None, None, [], None, [])
self._def_entity(self.the_empty_object_type)
qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
@@ -1721,7 +1734,7 @@ class QAPISchema(object):
assert ifcond == typ._ifcond # pylint: disable=protected-access
else:
self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
- None, members, None))
+ None, members, None, []))
return name
def _def_enum_type(self, expr, info, doc):
@@ -1752,9 +1765,10 @@ class QAPISchema(object):
base = expr.get('base')
data = expr['data']
ifcond = expr.get('if')
+ features = expr.get('features')
self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base,
self._make_members(data, info),
- None))
+ None, features))
def _make_variant(self, case, typ, ifcond):
return QAPISchemaObjectTypeVariant(case, typ, ifcond)
@@ -1795,7 +1809,7 @@ class QAPISchema(object):
QAPISchemaObjectType(name, info, doc, ifcond, base, members,
QAPISchemaObjectTypeVariants(tag_name,
tag_member,
- variants)))
+ variants), []))
def _def_alternate_type(self, expr, info, doc):
name = expr['alternate']
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f7f2ca07e4..f93a31aaa8 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s;
self._gen_qlit('[' + element + ']', 'array', {'element-type': element},
ifcond)
- def visit_object_type_flat(self, name, info, ifcond, members, variants):
+ def visit_object_type_flat(self, name, info, ifcond, members, variants,
+ features):
obj = {'members': [self._gen_member(m) for m in members]}
if variants:
obj.update(self._gen_variants(variants.tag_member.name,
variants.variants))
+ if features:
+ obj['features'] = features
+
self._gen_qlit(name, 'object', obj, ifcond)
def visit_alternate_type(self, name, info, ifcond, variants):
Kevin Wolf <kwolf@redhat.com> writes:
> Am 01.04.2019 um 15:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > Option 4:
>> >
>> > Add a dummy option to BlockdevOptionsFile:
>> >
>> > '*x-auto-read-only-is-dynamic': { 'type': 'null',
>> > 'if': 'defined(CONFIG_POSIX)' }
>> >
>> > Specifying it has no effect, so the ridiculous complexity of three bools
>> > to select from three options is avoided. Its presence in the schema
>> > indicates that file-posix implements dynamic auto-read-only.
>> >
>> > Basically this is features flags in the schema without having proper
>> > feature flags yet.
>> >
>> > Once we add real annotations (hopefully 4.1), this dummy field can be
>> > removed again.
>>
>> Exact same patch as for option 3, with the parameter renamed and the
>> sanity check for non-sensical use dropped. *Shrug*
>>
>> Puts more pressure on me to deliver QAPI feature flags sooner rather
>> than later. My QAPI pressure control valve has been shedding pressure
>> for a while, so "bring it on". I'd advise against holding your breath,
>> though.
>
> Okay, let's stop beating around the bush.
>
> Nobody has told me so explicitly during this discussion, but implicitly
> I understand that everyone seems to think doing annotations in the
> schema is what we really want to have.
I think it would make a useful addition to QAPI.
I don't think it's the ideal solution to the problem at hand. But the
problem at hand may not have ideal solutions.
We put @auto-read-only in 3.1 unproven. As specified, it gives QEMU
wide latitude to open read-only instead of read-write, and to reopen.
It doesn't *require* QEMU to do anything in particular.
3.1's implementation conforms to this specification (it would be hard
not to).
Libvirt needs more than is specified (it would be hard not to), and more
than is implemented in 3.1.
We changed the implementation in 4.0 to give libvirt what it needs (to
the best of our knowledge). This implementation still conforms to the
specification (again, hard not to).
Libvirt needs to know whether it's dealing with a 3.1 implementation or
a 4.0 implementation. A feature bit in the schema can expose this
cleanly in introspection.
But it's still an awful interface, to be frank. Let's try to write a
doc comment for it.
# @read-only: whether the block device should be read-only (default: false).
# Note that some block drivers support only read-only access,
# either generally or in certain configurations. In this case,
# the default value does not work and the option must be
# specified explicitly.
# @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.
# If the variant part corresponding to @driver carries
# feature flag dynamic-auto-read-only, then QEMU will
# always open the image read-only, reopen it read/write
# when the first writing user attaches to the node, and
# reopen it read-only when the last writing user
# detaches from the node.
# (default: false, since 3.1)
This still underspecifies fallback read-only. If libvirt wants to rely
on it, we'll get to tighten up that part.
Behavior is governed by three separate entities: feature flag
@dynamic-read-only, members @read-only and @auto-read-only. Bonus: the
feature flag lives somewhere else. Where it'll also need documentation.
Now compare to this hypothetical interface: @read-only can be true,
false, "dynamic" (some drivers only) or "fallback (some drivers only,
assuming we even need it).
To make introspection show the values the driver accepts, @read-only has
to move from BlockdevOptions into the BlockdevOptionsFoo. In some of
them, it remains bool, in BlockdevOptionsFile it becomes an alternate of
bool and an enum with the single value "dynamic", and so forth.
Perhaps this implementing this would be too onerous.
> Instead of continuing to argue which option to get around it is the
> least ugly one - how bad is the following attempt at just implementing
> what we really want?
>
> Only for structs for now, but that's all we need at the moment. Should
> be easy to extend during the 4.1 cycle (or whenever we need something
> else).
>
> Note that even if there are bugs in it, they are not user-visible and
> can just be fixed in 4.1. Currently, a diff between old and new
> query-qmp-schema output looks sane - just the added 'features' list
> where we want it.
I'll try to review the patch later today.
Kevin Wolf <kwolf@redhat.com> writes:
> Am 01.04.2019 um 15:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > Option 4:
>> >
>> > Add a dummy option to BlockdevOptionsFile:
>> >
>> > '*x-auto-read-only-is-dynamic': { 'type': 'null',
>> > 'if': 'defined(CONFIG_POSIX)' }
>> >
>> > Specifying it has no effect, so the ridiculous complexity of three bools
>> > to select from three options is avoided. Its presence in the schema
>> > indicates that file-posix implements dynamic auto-read-only.
>> >
>> > Basically this is features flags in the schema without having proper
>> > feature flags yet.
>> >
>> > Once we add real annotations (hopefully 4.1), this dummy field can be
>> > removed again.
>>
>> Exact same patch as for option 3, with the parameter renamed and the
>> sanity check for non-sensical use dropped. *Shrug*
>>
>> Puts more pressure on me to deliver QAPI feature flags sooner rather
>> than later. My QAPI pressure control valve has been shedding pressure
>> for a while, so "bring it on". I'd advise against holding your breath,
>> though.
>
> Okay, let's stop beating around the bush.
>
> Nobody has told me so explicitly during this discussion, but implicitly
> I understand that everyone seems to think doing annotations in the
> schema is what we really want to have.
>
> Instead of continuing to argue which option to get around it is the
> least ugly one - how bad is the following attempt at just implementing
> what we really want?
>
> Only for structs for now, but that's all we need at the moment. Should
> be easy to extend during the 4.1 cycle (or whenever we need something
> else).
>
> Note that even if there are bugs in it, they are not user-visible and
> can just be fixed in 4.1. Currently, a diff between old and new
> query-qmp-schema output looks sane - just the added 'features' list
> where we want it.
[Patch snipped...]
Caveat emptor: at this time of day, I'm perfectly capable of missing the
obvious. Assuming I don't: for once, something doesn't turn out six
times as hard as anticipated.
Missing:
* Test coverage: two negative tests for the two errors, positive
coverage in qapi-schema-test.json
* Update to introspect.json (without this, qmp-cmd-test fails, and with
qapi-schema-test.json updated, test /visitor/input/qapi-introspect
will fail, too),
* Update a few tests/qapi-schema/*.err
* Update docs/devel/qapi-code-gen.txt
* A general idea how to document feature flags
* Depending on that, perhaps an update to the documentation generator
scripts/qapi/doc.py
* A rough idea on where else feature flags need to go
On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote: > Let's review our options for 4.0. > > Please note my analysis is handicapped by incomplete information, in > particular on libvirt's needs. Libvirt's needs are "simple" we need to do block-commit. It has a few caveats though. The selinux labels for backing images don't allow write operation when starting qemu. When doing block-commit libvirt relabels the backing chain prior to using block-commit. That works with -drive as qemu reopens the files internally, but does not work when using -blockdev. Since we need to keep the images RO until doing the blockjob there needs to be a way when either qemu automagically does what libvirt needs even if the image did not have the write permission originally. auto-read-only was a naive impl when qemu attempted to keep the storage access nodes RW. At the point when I was testing it I didn't enable s-virt as it's a hassle when you want to run git versions of libvirt and qemu and thus didn't see the problem with missing permissions. The dynamic version attempts to fix that. That is still the automagic approach. I don't really mind also doing a hands-on approach where we'd need to tell qemu to reopen given nodes. I'm not sure what ends up being less work. > Terminology: > > * "Hard read-write" semantics: open read/write. > > * "Hard read-only" semantics: open read-only. > > * "Dynamic read-only" semantics: open read-only, reopen read/write when > a writer attaches, reopen read-only when the last writer detaches. > > * "Fallback read-only" semantics:. try to open read/write, fall back to > read-only. > > We have a use case for dynamic read-only: libvirt. I'm not aware of a > use case for fallback read-only. > > Behavior before 3.1: > > * read-only=on: hard read-only > > * read-only=off: hard read-write > > Behavior in 3.1: new parameter auto-read-only, default off. > > * read-only=on: hard read-only (no change) > > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on > silently ignored) > > * read-only=off: hard read-write > > * read-only=off,auto-read-only=on: depends on the driver > > - file-posix.c's drivers: fallback read-only > - some other drivers: fallback read-only > - remaining drivers: hard read/write > > Behavior in 4.0 so far: > > * read-only=on: hard read-only (no change) > > * read-only=on,auto-read-only=on: hard read-only (no change) > > * read-only=off: hard read-write (no change) > > * read-only=off,auto-read-only=on: depends on the driver > > - file-posix.c's drivers: dynamic read-only > - some other drivers: fallback read-only (no change) > - remaining drivers: hard read/write (no change) > > > Option 1: Rename @auto-read-only. > > Rationale: we released auto-read-only in v3.1 with unproven fallback > read-only semantics. It turned out not to be useful. Admit the > mistake, retract it. Release our next attempt in 4.0 under a suitable > new name with fallback read-only semantics. I concur. Nobody probably uses this. Setting up selinux and blockdev is not a trivial excercise. > CON: > > * Compatibility break. No-go if there are users. Users seem quite > unlikely, though. > > * Still unproven, albeit less so: this time we have some unreleased > (unfinished?) libvirt code. unfinished and thus also unreleased. > > * Semantics are still largely left to drivers, and the schema can't tell > which one does what. Awkward. > > * Unless we're fairly confident we won't upgrade drivers from "hard" to > "fallback" to "dynamic", this merely kicks the can down the road: > we'll face the exact same "how can libvirt find out" problem on every > upgrade. > > PRO: > > * When libvirt sees the new name, it can rely on file-posix.c's drivers > providing dynamic read-only semantics. > > > Option 2: Add query-qemu-features command, return > file-dynamic-auto-read-only #ifdef CONFIG_POSIX. > > Rationale: we released auto-read-only in v3.1 with unproven fallback > read-only semantics. It turned out not to be useful. Admit the > mistake, and patch it up in 4.0. Libirt needs to know whether it's > patche up, and this is a simple way to tell it. > > CON: > > * All of option 1's, except for the compatibility break > > * Uses query-qemu-features to expose a property of the build. I > consider that a mistake. > > PRO > > * Libvirt can use either query-qmp-schema or query-qemu-features to find > out whether it can can rely on file-posix.c's drivers providing > dynamic read-only semantics. To make query-qemu-features usable, we > need to promise query-qemu-features never returns false for it. To > make query-qemu-features usable, we need to promise the value will > remain valid for the remainder of the QEMU run (defeats caching) or > for any future run of the same QEMU binary (enables caching). > > > Option 3: Add @dynamic-read-only to the drivers that provide dynamic > read-only, default to value of auto-read-only, promise we'll never add > it to drivers that don't. > > Rationale: give users explicit control over dynamic vs. fallback for all > drivers that can provide dynamic. This makes some sense as an > interface, as long as you ignore the fact that no driver implements both > dynamic and fallback. I can't see why a driver couldn't implement both. > It also makes dynamic support visible in the schema. > > Behavior (three bools -> eight cases): > > * read-only=on: hard read-only (no change) > > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off > > * read-only=on,auto-read-only=on: hard read-only (no change) > > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on > > * read-only=off: hard read-write (no change) > > Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off > > * read-only=off,auto-read-only=on: > > Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on > > - file-posix.c's drivers: dynamic read-only (no change, > dynamic-read-only=on is the default) > - some other drivers: fallback read-only (no change) > - remaining drivers: hard read/write (no change) > > * read-only=off,auto-read-only=on,dynamic-read-only=off > > - file-posix.c's drivers: error > - all other drivers: N/A > > * read-only=off,auto-read-only=off,dynamic-read-only=on > > - file-posix.c's drivers: error > - all other drivers: N/A > > * read-only=on,dynamic-read-only=on > > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on > > - file-posix.c's drivers: error > - all other drivers: N/A > > * read-only=on,auto-read-only=on,dynamic-read-only=off > > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off > > - file-posix.c's drivers: error > - all other drivers: N/A > > CON: > > * Two bools (read-only and auto-read-only) to select from three choices > was already ugly. Three bools (the same plus dynamic-read-only) to > select from four choices is even uglier. > > * The explicit control is just a facade so far: since only the default > setting is implemented, it doesn't actually control anything. > > PRO: > > * When libvirt sees a driver providing a dynamic-read-only parameter, it > knows it can rely on the driver providing dynamic read-only semantics. > > * Adding dynamic read-only capability to drivers creates no > introspection problems: we simply add dynamic-read-only to their > parameters. In the end libvirt does not care that much if the storage is open read only or read write. We need the format node or guest frontend to deny writes if the disk is declared as read-only. We also need to be able to do blockjobs. The requirement is that images may not be accessible in R/W mode when qemu is starting but later may become. Then it's expected that block-commit will succeed.
Peter Krempa <pkrempa@redhat.com> writes: > On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote: >> Let's review our options for 4.0. >> >> Please note my analysis is handicapped by incomplete information, in >> particular on libvirt's needs. > > Libvirt's needs are "simple" we need to do block-commit. It has a few > caveats though. The selinux labels for backing images don't allow write > operation when starting qemu. When doing block-commit libvirt relabels > the backing chain prior to using block-commit. Paraphrasing, to make sure I got you. Consider the common read/write COW image with a backing image. Libvirt wants to grant QEMU read/write access to the COW and read-only access to the backing image. It does that with SELinux. QEMU has to open the backing image read-only (read/write would fail). But libvirt also wants to do block-commit. Libvirt first relabels to grant QEMU read/write access to the backing image, then $something makes QEMU reopen the backing image read/write, block-commit does its job, $something makes QEMU reopen the backing image read-only, libvirt relabels to revoke read/write access. Correct? With dynamic read-only, $something is the block commit job (a writer) attaching and detaching to the backing image node. > That works with -drive as qemu reopens the files internally, but does > not work when using -blockdev. Happy to take your word for it. > Since we need to keep the images RO until doing the blockjob there > needs to be a way when either qemu automagically does what libvirt needs > even if the image did not have the write permission originally. > > auto-read-only was a naive impl when qemu attempted to keep the storage > access nodes RW. At the point when I was testing it I didn't enable > s-virt as it's a hassle when you want to run git versions of libvirt and > qemu and thus didn't see the problem with missing permissions. Yes, fallback read-only is useless for the scenario above. Kevin wrote "libvirt requires dynamic read-only for file-posix. It prefers dynamic read-only for other drivers, but can live with fallback read-only there." I'm not sure how it can live with fallback read-only. Is it because only *files* can labelled with SELinux? If yes, then my next question is how libvirt makes use of fallback read-only for non-files. Can you give me an example? > The dynamic version attempts to fix that. That is still the automagic > approach. I don't really mind also doing a hands-on approach where we'd > need to tell qemu to reopen given nodes. Aha! You just corrected my overly narrow idea of the solution space. > I'm not sure what ends up being less work. "Less work" is an important consideration. We may be able to accept some extra work to get a saner interface. Depends on how much saner and how much extra exactly, and on who needs to do the work. >> Terminology: >> >> * "Hard read-write" semantics: open read/write. >> >> * "Hard read-only" semantics: open read-only. >> >> * "Dynamic read-only" semantics: open read-only, reopen read/write when >> a writer attaches, reopen read-only when the last writer detaches. >> >> * "Fallback read-only" semantics:. try to open read/write, fall back to >> read-only. >> >> We have a use case for dynamic read-only: libvirt. I'm not aware of a >> use case for fallback read-only. >> >> Behavior before 3.1: >> >> * read-only=on: hard read-only >> >> * read-only=off: hard read-write >> >> Behavior in 3.1: new parameter auto-read-only, default off. >> >> * read-only=on: hard read-only (no change) >> >> * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on >> silently ignored) >> >> * read-only=off: hard read-write >> >> * read-only=off,auto-read-only=on: depends on the driver >> >> - file-posix.c's drivers: fallback read-only >> - some other drivers: fallback read-only >> - remaining drivers: hard read/write >> >> Behavior in 4.0 so far: >> >> * read-only=on: hard read-only (no change) >> >> * read-only=on,auto-read-only=on: hard read-only (no change) >> >> * read-only=off: hard read-write (no change) >> >> * read-only=off,auto-read-only=on: depends on the driver >> >> - file-posix.c's drivers: dynamic read-only >> - some other drivers: fallback read-only (no change) >> - remaining drivers: hard read/write (no change) >> >> >> Option 1: Rename @auto-read-only. >> >> Rationale: we released auto-read-only in v3.1 with unproven fallback >> read-only semantics. It turned out not to be useful. Admit the >> mistake, retract it. Release our next attempt in 4.0 under a suitable >> new name with fallback read-only semantics. > > I concur. Nobody probably uses this. Setting up selinux and blockdev is > not a trivial excercise. Right. >> CON: >> >> * Compatibility break. No-go if there are users. Users seem quite >> unlikely, though. >> >> * Still unproven, albeit less so: this time we have some unreleased >> (unfinished?) libvirt code. > > unfinished and thus also unreleased. Noted. >> >> * Semantics are still largely left to drivers, and the schema can't tell >> which one does what. Awkward. >> >> * Unless we're fairly confident we won't upgrade drivers from "hard" to >> "fallback" to "dynamic", this merely kicks the can down the road: >> we'll face the exact same "how can libvirt find out" problem on every >> upgrade. >> >> PRO: >> >> * When libvirt sees the new name, it can rely on file-posix.c's drivers >> providing dynamic read-only semantics. >> >> >> Option 2: Add query-qemu-features command, return >> file-dynamic-auto-read-only #ifdef CONFIG_POSIX. >> >> Rationale: we released auto-read-only in v3.1 with unproven fallback >> read-only semantics. It turned out not to be useful. Admit the >> mistake, and patch it up in 4.0. Libirt needs to know whether it's >> patche up, and this is a simple way to tell it. >> >> CON: >> >> * All of option 1's, except for the compatibility break >> >> * Uses query-qemu-features to expose a property of the build. I >> consider that a mistake. >> >> PRO >> >> * Libvirt can use either query-qmp-schema or query-qemu-features to find >> out whether it can can rely on file-posix.c's drivers providing >> dynamic read-only semantics. To make query-qemu-features usable, we >> need to promise query-qemu-features never returns false for it. To >> make query-qemu-features usable, we need to promise the value will >> remain valid for the remainder of the QEMU run (defeats caching) or >> for any future run of the same QEMU binary (enables caching). >> >> >> Option 3: Add @dynamic-read-only to the drivers that provide dynamic >> read-only, default to value of auto-read-only, promise we'll never add >> it to drivers that don't. >> >> Rationale: give users explicit control over dynamic vs. fallback for all >> drivers that can provide dynamic. This makes some sense as an >> interface, as long as you ignore the fact that no driver implements both >> dynamic and fallback. I can't see why a driver couldn't implement both. >> It also makes dynamic support visible in the schema. >> >> Behavior (three bools -> eight cases): >> >> * read-only=on: hard read-only (no change) >> >> Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off >> >> * read-only=on,auto-read-only=on: hard read-only (no change) >> >> Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on >> >> * read-only=off: hard read-write (no change) >> >> Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off >> >> * read-only=off,auto-read-only=on: >> >> Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on >> >> - file-posix.c's drivers: dynamic read-only (no change, >> dynamic-read-only=on is the default) >> - some other drivers: fallback read-only (no change) >> - remaining drivers: hard read/write (no change) >> >> * read-only=off,auto-read-only=on,dynamic-read-only=off >> >> - file-posix.c's drivers: error >> - all other drivers: N/A >> >> * read-only=off,auto-read-only=off,dynamic-read-only=on >> >> - file-posix.c's drivers: error >> - all other drivers: N/A >> >> * read-only=on,dynamic-read-only=on >> >> Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on >> >> - file-posix.c's drivers: error >> - all other drivers: N/A >> >> * read-only=on,auto-read-only=on,dynamic-read-only=off >> >> Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off >> >> - file-posix.c's drivers: error >> - all other drivers: N/A >> >> CON: >> >> * Two bools (read-only and auto-read-only) to select from three choices >> was already ugly. Three bools (the same plus dynamic-read-only) to >> select from four choices is even uglier. >> >> * The explicit control is just a facade so far: since only the default >> setting is implemented, it doesn't actually control anything. >> >> PRO: >> >> * When libvirt sees a driver providing a dynamic-read-only parameter, it >> knows it can rely on the driver providing dynamic read-only semantics. >> >> * Adding dynamic read-only capability to drivers creates no >> introspection problems: we simply add dynamic-read-only to their >> parameters. > > In the end libvirt does not care that much if the storage is open read > only or read write. We need the format node or guest frontend to deny > writes if the disk is declared as read-only. We also need to be able to > do blockjobs. The requirement is that images may not be accessible in > R/W mode when qemu is starting but later may become. Then it's expected > that block-commit will succeed. Fallback read-only can't help there. Dynamic read-only should work. Explicit reopen triggers could also work.
On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote: > Peter Krempa <pkrempa@redhat.com> writes: > > > On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote: > >> Let's review our options for 4.0. > >> > >> Please note my analysis is handicapped by incomplete information, in > >> particular on libvirt's needs. > > > > Libvirt's needs are "simple" we need to do block-commit. It has a few > > caveats though. The selinux labels for backing images don't allow write > > operation when starting qemu. When doing block-commit libvirt relabels > > the backing chain prior to using block-commit. > > Paraphrasing, to make sure I got you. > > Consider the common read/write COW image with a backing image. > > Libvirt wants to grant QEMU read/write access to the COW and read-only > access to the backing image. It does that with SELinux. QEMU has to > open the backing image read-only (read/write would fail). > > But libvirt also wants to do block-commit. Libvirt first relabels to > grant QEMU read/write access to the backing image, then $something makes > QEMU reopen the backing image read/write, block-commit does its job, > $something makes QEMU reopen the backing image read-only, libvirt > relabels to revoke read/write access. > > Correct? Yes. > > With dynamic read-only, $something is the block commit job (a writer) > attaching and detaching to the backing image node. > > > That works with -drive as qemu reopens the files internally, but does > > not work when using -blockdev. > > Happy to take your word for it. > > > Since we need to keep the images RO until doing the blockjob there > > needs to be a way when either qemu automagically does what libvirt needs > > even if the image did not have the write permission originally. > > > > auto-read-only was a naive impl when qemu attempted to keep the storage > > access nodes RW. At the point when I was testing it I didn't enable > > s-virt as it's a hassle when you want to run git versions of libvirt and > > qemu and thus didn't see the problem with missing permissions. > > Yes, fallback read-only is useless for the scenario above. > > Kevin wrote "libvirt requires dynamic read-only for file-posix. It > prefers dynamic read-only for other drivers, but can live with fallback > read-only there." I'm not sure how it can live with fallback read-only. > Is it because only *files* can labelled with SELinux? > > If yes, then my next question is how libvirt makes use of fallback > read-only for non-files. Can you give me an example? For non-files libvirt usually does not have means to modify the access permissions. A simple example may be a NBD export. If the NBD server exports the image readonly libvirt can't do much about it. Arguably you can consider a situation where the user modifies the access to read-write manually and then invokes the libvirt virDomainBlockCommit API and thus still expects the automagic reopen to happen correctly. At this point I'm not really certain to which extend this would work currently with -drive as there are known issues when attempting to point to a non-file image by path rather than node-name. > > The dynamic version attempts to fix that. That is still the automagic > > approach. I don't really mind also doing a hands-on approach where we'd > > need to tell qemu to reopen given nodes. > > Aha! You just corrected my overly narrow idea of the solution space. > > > I'm not sure what ends up being less work. > > "Less work" is an important consideration. > > We may be able to accept some extra work to get a saner interface. > Depends on how much saner and how much extra exactly, and on who needs > to do the work. I agree. While the 'explicit reopen' approach may be more work for libvirt I'll happily accept that solution. > > >> Terminology: > >> > >> * "Hard read-write" semantics: open read/write. > >> > >> * "Hard read-only" semantics: open read-only. > >> > >> * "Dynamic read-only" semantics: open read-only, reopen read/write when > >> a writer attaches, reopen read-only when the last writer detaches. [...] > >> * When libvirt sees a driver providing a dynamic-read-only parameter, it > >> knows it can rely on the driver providing dynamic read-only semantics. > >> > >> * Adding dynamic read-only capability to drivers creates no > >> introspection problems: we simply add dynamic-read-only to their > >> parameters. > > > > In the end libvirt does not care that much if the storage is open read > > only or read write. We need the format node or guest frontend to deny > > writes if the disk is declared as read-only. We also need to be able to > > do blockjobs. The requirement is that images may not be accessible in > > R/W mode when qemu is starting but later may become. Then it's expected > > that block-commit will succeed. > > Fallback read-only can't help there. > > Dynamic read-only should work. Explicit reopen triggers could also > work. The explicit trigger would work. With the tighter control of blockjobs we should be able to control the lifecycle properly.
Peter Krempa <pkrempa@redhat.com> writes: > On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote: >> Peter Krempa <pkrempa@redhat.com> writes: >> >> > On Sun, Mar 31, 2019 at 14:56:19 +0200, Markus Armbruster wrote: >> >> Let's review our options for 4.0. >> >> >> >> Please note my analysis is handicapped by incomplete information, in >> >> particular on libvirt's needs. >> > >> > Libvirt's needs are "simple" we need to do block-commit. It has a few >> > caveats though. The selinux labels for backing images don't allow write >> > operation when starting qemu. When doing block-commit libvirt relabels >> > the backing chain prior to using block-commit. >> >> Paraphrasing, to make sure I got you. >> >> Consider the common read/write COW image with a backing image. >> >> Libvirt wants to grant QEMU read/write access to the COW and read-only >> access to the backing image. It does that with SELinux. QEMU has to >> open the backing image read-only (read/write would fail). >> >> But libvirt also wants to do block-commit. Libvirt first relabels to >> grant QEMU read/write access to the backing image, then $something makes >> QEMU reopen the backing image read/write, block-commit does its job, >> $something makes QEMU reopen the backing image read-only, libvirt >> relabels to revoke read/write access. >> >> Correct? > > Yes. Thank you. >> With dynamic read-only, $something is the block commit job (a writer) >> attaching and detaching to the backing image node. >> >> > That works with -drive as qemu reopens the files internally, but does >> > not work when using -blockdev. >> >> Happy to take your word for it. >> >> > Since we need to keep the images RO until doing the blockjob there >> > needs to be a way when either qemu automagically does what libvirt needs >> > even if the image did not have the write permission originally. >> > >> > auto-read-only was a naive impl when qemu attempted to keep the storage >> > access nodes RW. At the point when I was testing it I didn't enable >> > s-virt as it's a hassle when you want to run git versions of libvirt and >> > qemu and thus didn't see the problem with missing permissions. >> >> Yes, fallback read-only is useless for the scenario above. >> >> Kevin wrote "libvirt requires dynamic read-only for file-posix. It >> prefers dynamic read-only for other drivers, but can live with fallback >> read-only there." I'm not sure how it can live with fallback read-only. >> Is it because only *files* can labelled with SELinux? >> >> If yes, then my next question is how libvirt makes use of fallback >> read-only for non-files. Can you give me an example? > > For non-files libvirt usually does not have means to modify the access > permissions. > > A simple example may be a NBD export. If the NBD server exports the > image readonly libvirt can't do much about it. Arguably you can consider > a situation where the user modifies the access to read-write manually > and then invokes the libvirt virDomainBlockCommit API and thus still > expects the automagic reopen to happen correctly. At this point I'm not > really certain to which extend this would work currently with -drive as > there are known issues when attempting to point to a non-file image by > path rather than node-name. Both automagic reopen and explicit reopen work here. Both succeed when the user has granted access manually. Libvirt would have to try the explicit reopen even in cases where it can't grant access itself. Sounds like you don't have a use case for fallback read-only. Correct? >> > The dynamic version attempts to fix that. That is still the automagic >> > approach. I don't really mind also doing a hands-on approach where we'd >> > need to tell qemu to reopen given nodes. >> >> Aha! You just corrected my overly narrow idea of the solution space. >> >> > I'm not sure what ends up being less work. >> >> "Less work" is an important consideration. >> >> We may be able to accept some extra work to get a saner interface. >> Depends on how much saner and how much extra exactly, and on who needs >> to do the work. > > I agree. While the 'explicit reopen' approach may be more work for > libvirt I'll happily accept that solution. Would x-blockdev-reopen do? What exactly is hold it in x-space? I guess a specialized blockdev-set-read-only would do as well. >> >> Terminology: >> >> >> >> * "Hard read-write" semantics: open read/write. >> >> >> >> * "Hard read-only" semantics: open read-only. >> >> >> >> * "Dynamic read-only" semantics: open read-only, reopen read/write when >> >> a writer attaches, reopen read-only when the last writer detaches. > > [...] > >> >> * When libvirt sees a driver providing a dynamic-read-only parameter, it >> >> knows it can rely on the driver providing dynamic read-only semantics. >> >> >> >> * Adding dynamic read-only capability to drivers creates no >> >> introspection problems: we simply add dynamic-read-only to their >> >> parameters. >> > >> > In the end libvirt does not care that much if the storage is open read >> > only or read write. We need the format node or guest frontend to deny >> > writes if the disk is declared as read-only. We also need to be able to >> > do blockjobs. The requirement is that images may not be accessible in >> > R/W mode when qemu is starting but later may become. Then it's expected >> > that block-commit will succeed. >> >> Fallback read-only can't help there. >> >> Dynamic read-only should work. Explicit reopen triggers could also >> work. > > The explicit trigger would work. With the tighter control of blockjobs > we should be able to control the lifecycle properly. Good to know, thanks!
Am 01.04.2019 um 18:22 hat Markus Armbruster geschrieben: > Peter Krempa <pkrempa@redhat.com> writes: > > On Mon, Apr 01, 2019 at 16:55:50 +0200, Markus Armbruster wrote: > >> Peter Krempa <pkrempa@redhat.com> writes: > >> > That works with -drive as qemu reopens the files internally, but does > >> > not work when using -blockdev. > >> > >> Happy to take your word for it. The difference is actually not -blockdev vs. -drive, but whether you define the 'file' node separately in its own -blockdev option or whether you create it in the context of the format node (using file.* dotted syntax for its options). Effectively this doesn't change what Peter says because -drive only supports the latter, and for -blockdev we only want to use the former. > >> > Since we need to keep the images RO until doing the blockjob there > >> > needs to be a way when either qemu automagically does what libvirt needs > >> > even if the image did not have the write permission originally. > >> > > >> > auto-read-only was a naive impl when qemu attempted to keep the storage > >> > access nodes RW. At the point when I was testing it I didn't enable > >> > s-virt as it's a hassle when you want to run git versions of libvirt and > >> > qemu and thus didn't see the problem with missing permissions. > >> > >> Yes, fallback read-only is useless for the scenario above. > >> > >> Kevin wrote "libvirt requires dynamic read-only for file-posix. It > >> prefers dynamic read-only for other drivers, but can live with fallback > >> read-only there." I'm not sure how it can live with fallback read-only. > >> Is it because only *files* can labelled with SELinux? > >> > >> If yes, then my next question is how libvirt makes use of fallback > >> read-only for non-files. Can you give me an example? > > > > For non-files libvirt usually does not have means to modify the access > > permissions. > > > > A simple example may be a NBD export. If the NBD server exports the > > image readonly libvirt can't do much about it. Arguably you can consider > > a situation where the user modifies the access to read-write manually > > and then invokes the libvirt virDomainBlockCommit API and thus still > > expects the automagic reopen to happen correctly. At this point I'm not > > really certain to which extend this would work currently with -drive as > > there are known issues when attempting to point to a non-file image by > > path rather than node-name. > > Both automagic reopen and explicit reopen work here. Both succeed when > the user has granted access manually. Libvirt would have to try the > explicit reopen even in cases where it can't grant access itself. > > Sounds like you don't have a use case for fallback read-only. Correct? If everything supports dynamic read-only, I don't think anyone would have a use for fallback read-only. The long-term vision was that we would remove the read-only option and add a new force-read-only option that would allow selecting between "force read-only" and "do whatever is needed to support what the attached parents requested". This should be all we really need. I don't think there is even a use case for a hard read-write option. It's just that dynamic read-only is a bit harder to implement, so we started with fallback read-only as "good enough for libvirt" (seemingly, turned out to be wrong for file-posix). The definition of the option was kept intentionally vague so that it would allow switching to dynamic read-only. The idea was that it only turns an error into working cases, so we can safely do the switch and libvirt would automatically be upgraded from an error case to a working case, too. Of course, in this reasoning we forgot that libvirt might want to just switch back to -drive in the cases that would error out instead of actually receiving that error. > >> > The dynamic version attempts to fix that. That is still the automagic > >> > approach. I don't really mind also doing a hands-on approach where we'd > >> > need to tell qemu to reopen given nodes. > >> > >> Aha! You just corrected my overly narrow idea of the solution space. > >> > >> > I'm not sure what ends up being less work. > >> > >> "Less work" is an important consideration. > >> > >> We may be able to accept some extra work to get a saner interface. > >> Depends on how much saner and how much extra exactly, and on who needs > >> to do the work. > > > > I agree. While the 'explicit reopen' approach may be more work for > > libvirt I'll happily accept that solution. > > Would x-blockdev-reopen do? > > What exactly is hold it in x-space? > > I guess a specialized blockdev-set-read-only would do as well. x-blockdev-reopen was only just added. It provides the same options as blockdev-add, so a first estimation would be that it's roughly the same complexity. Experience from blockdev-add tells us that getting it stable the next release was completely unrealistic because we didn't even fully understand the problems yet. I expect the same for x-blockdev-reopen. Maybe what impresses you more is that x-blockdev-reopen doesn't only change options like read-only, but also references to other nodes. So it's the dynamic graph reconfiguration thing we've talking about for years. This seems hard to get right in the first attempt. But we also know a few more concrete shortcomings. For example, changing child nodes doesn't work if iothreads are involved. Also: Not all option that could theoretically be changed in x-blockdev-reopen are actually supported yet. If we mark it stable before everything is covered, we'll need some kind of schema annotations again that describe in which version changing which option was added. Maybe this time we'd better figure out how to do that before creating the problem. For blockdev-add, we decided to only mark it stable as soon as full feature parity with -drive was achieved (except for options that we _really_ don't want to have in QAPI). So I'm very doubtful of marking x-blockdev-reopen stable in 4.1. A specialised blockdev-set-read-only seems more realistic for 4.1, but it still wouldn't solve the problem now in 4.0 and once we have a stable blockdev-reopen, it's duplicated functionality. Kevin
Markus Armbruster <armbru@redhat.com> writes: > Let's review our options for 4.0. > > Please note my analysis is handicapped by incomplete information, in > particular on libvirt's needs. > > Terminology: > > * "Hard read-write" semantics: open read/write. > > * "Hard read-only" semantics: open read-only. > > * "Dynamic read-only" semantics: open read-only, reopen read/write when > a writer attaches, reopen read-only when the last writer detaches. > > * "Fallback read-only" semantics:. try to open read/write, fall back to > read-only. > > We have a use case for dynamic read-only: libvirt. I'm not aware of a > use case for fallback read-only. > > Behavior before 3.1: > > * read-only=on: hard read-only > > * read-only=off: hard read-write > > Behavior in 3.1: new parameter auto-read-only, default off. > > * read-only=on: hard read-only (no change) > > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on > silently ignored) > > * read-only=off: hard read-write > > * read-only=off,auto-read-only=on: depends on the driver > > - file-posix.c's drivers: fallback read-only > - some other drivers: fallback read-only > - remaining drivers: hard read/write > > Behavior in 4.0 so far: > > * read-only=on: hard read-only (no change) > > * read-only=on,auto-read-only=on: hard read-only (no change) > > * read-only=off: hard read-write (no change) > > * read-only=off,auto-read-only=on: depends on the driver > > - file-posix.c's drivers: dynamic read-only > - some other drivers: fallback read-only (no change) > - remaining drivers: hard read/write (no change) > > > Option 1: Rename @auto-read-only. > > Rationale: we released auto-read-only in v3.1 with unproven fallback > read-only semantics. It turned out not to be useful. Admit the > mistake, retract it. Release our next attempt in 4.0 under a suitable > new name with fallback read-only semantics. > > CON: > > * Compatibility break. No-go if there are users. Users seem quite > unlikely, though. > > * Still unproven, albeit less so: this time we have some unreleased > (unfinished?) libvirt code. > > * Semantics are still largely left to drivers, and the schema can't tell > which one does what. Awkward. > > * Unless we're fairly confident we won't upgrade drivers from "hard" to > "fallback" to "dynamic", this merely kicks the can down the road: > we'll face the exact same "how can libvirt find out" problem on every > upgrade. > > PRO: > > * When libvirt sees the new name, it can rely on file-posix.c's drivers > providing dynamic read-only semantics. > > > Option 2: Add query-qemu-features command, return > file-dynamic-auto-read-only #ifdef CONFIG_POSIX. > > Rationale: we released auto-read-only in v3.1 with unproven fallback > read-only semantics. It turned out not to be useful. Admit the > mistake, and patch it up in 4.0. Libirt needs to know whether it's > patche up, and this is a simple way to tell it. > > CON: > > * All of option 1's, except for the compatibility break > > * Uses query-qemu-features to expose a property of the build. I > consider that a mistake. > > PRO > > * Libvirt can use either query-qmp-schema or query-qemu-features to find > out whether it can can rely on file-posix.c's drivers providing > dynamic read-only semantics. To make query-qemu-features usable, we > need to promise query-qemu-features never returns false for it. To > make query-qemu-features usable, we need to promise the value will > remain valid for the remainder of the QEMU run (defeats caching) or > for any future run of the same QEMU binary (enables caching). > > > Option 3: Add @dynamic-read-only to the drivers that provide dynamic > read-only, default to value of auto-read-only, promise we'll never add > it to drivers that don't. > > Rationale: give users explicit control over dynamic vs. fallback for all > drivers that can provide dynamic. This makes some sense as an > interface, as long as you ignore the fact that no driver implements both > dynamic and fallback. I can't see why a driver couldn't implement both. > It also makes dynamic support visible in the schema. > > Behavior (three bools -> eight cases): > > * read-only=on: hard read-only (no change) > > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off > > * read-only=on,auto-read-only=on: hard read-only (no change) > > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on > > * read-only=off: hard read-write (no change) > > Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off > > * read-only=off,auto-read-only=on: > > Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on > > - file-posix.c's drivers: dynamic read-only (no change, > dynamic-read-only=on is the default) > - some other drivers: fallback read-only (no change) > - remaining drivers: hard read/write (no change) > > * read-only=off,auto-read-only=on,dynamic-read-only=off > > - file-posix.c's drivers: error > - all other drivers: N/A > > * read-only=off,auto-read-only=off,dynamic-read-only=on > > - file-posix.c's drivers: error > - all other drivers: N/A > > * read-only=on,dynamic-read-only=on > > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on > > - file-posix.c's drivers: error > - all other drivers: N/A > > * read-only=on,auto-read-only=on,dynamic-read-only=off > > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off > > - file-posix.c's drivers: error > - all other drivers: N/A > > CON: > > * Two bools (read-only and auto-read-only) to select from three choices > was already ugly. Three bools (the same plus dynamic-read-only) to > select from four choices is even uglier. > > * The explicit control is just a facade so far: since only the default > setting is implemented, it doesn't actually control anything. > > PRO: > > * When libvirt sees a driver providing a dynamic-read-only parameter, it > knows it can rely on the driver providing dynamic read-only semantics. > > * Adding dynamic read-only capability to drivers creates no > introspection problems: we simply add dynamic-read-only to their > parameters. > > Code sketch appended Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile. Message-ID: <20190401115410.GB4935@localhost.localdomain> A variation of option 3, code is almost identical. Where option 3 tries to evolve what we have into the least bad permanent interface, option 4 only wants to serve as a stop gap until we get a better solution, such as option 5. Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say "this block driver provides dynamic read-only". Message-ID: <20190401140842.GD4935@localhost.localdomain> Option 6 [Peter]: Trigger reopen explicitly via QMP. Message-ID: <20190401122054.GA2686@andariel.pipo.sk> [Me] This leaves auto-read-only without a use case; deprecate? Given that the libvirt code isn't ready, why do we need this done and declared stable in 4.0? We're going to tag -rc2 tomorrow...
Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben: > Markus Armbruster <armbru@redhat.com> writes: > > > Let's review our options for 4.0. > > > > Please note my analysis is handicapped by incomplete information, in > > particular on libvirt's needs. > > > > Terminology: > > > > * "Hard read-write" semantics: open read/write. > > > > * "Hard read-only" semantics: open read-only. > > > > * "Dynamic read-only" semantics: open read-only, reopen read/write when > > a writer attaches, reopen read-only when the last writer detaches. > > > > * "Fallback read-only" semantics:. try to open read/write, fall back to > > read-only. > > > > We have a use case for dynamic read-only: libvirt. I'm not aware of a > > use case for fallback read-only. > > > > Behavior before 3.1: > > > > * read-only=on: hard read-only > > > > * read-only=off: hard read-write > > > > Behavior in 3.1: new parameter auto-read-only, default off. > > > > * read-only=on: hard read-only (no change) > > > > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on > > silently ignored) > > > > * read-only=off: hard read-write > > > > * read-only=off,auto-read-only=on: depends on the driver > > > > - file-posix.c's drivers: fallback read-only > > - some other drivers: fallback read-only > > - remaining drivers: hard read/write > > > > Behavior in 4.0 so far: > > > > * read-only=on: hard read-only (no change) > > > > * read-only=on,auto-read-only=on: hard read-only (no change) > > > > * read-only=off: hard read-write (no change) > > > > * read-only=off,auto-read-only=on: depends on the driver > > > > - file-posix.c's drivers: dynamic read-only > > - some other drivers: fallback read-only (no change) > > - remaining drivers: hard read/write (no change) > > > > > > Option 1: Rename @auto-read-only. > > > > Rationale: we released auto-read-only in v3.1 with unproven fallback > > read-only semantics. It turned out not to be useful. Admit the > > mistake, retract it. Release our next attempt in 4.0 under a suitable > > new name with fallback read-only semantics. > > > > CON: > > > > * Compatibility break. No-go if there are users. Users seem quite > > unlikely, though. > > > > * Still unproven, albeit less so: this time we have some unreleased > > (unfinished?) libvirt code. > > > > * Semantics are still largely left to drivers, and the schema can't tell > > which one does what. Awkward. > > > > * Unless we're fairly confident we won't upgrade drivers from "hard" to > > "fallback" to "dynamic", this merely kicks the can down the road: > > we'll face the exact same "how can libvirt find out" problem on every > > upgrade. > > > > PRO: > > > > * When libvirt sees the new name, it can rely on file-posix.c's drivers > > providing dynamic read-only semantics. > > > > > > Option 2: Add query-qemu-features command, return > > file-dynamic-auto-read-only #ifdef CONFIG_POSIX. > > > > Rationale: we released auto-read-only in v3.1 with unproven fallback > > read-only semantics. It turned out not to be useful. Admit the > > mistake, and patch it up in 4.0. Libirt needs to know whether it's > > patche up, and this is a simple way to tell it. > > > > CON: > > > > * All of option 1's, except for the compatibility break > > > > * Uses query-qemu-features to expose a property of the build. I > > consider that a mistake. > > > > PRO > > > > * Libvirt can use either query-qmp-schema or query-qemu-features to find > > out whether it can can rely on file-posix.c's drivers providing > > dynamic read-only semantics. To make query-qemu-features usable, we > > need to promise query-qemu-features never returns false for it. To > > make query-qemu-features usable, we need to promise the value will > > remain valid for the remainder of the QEMU run (defeats caching) or > > for any future run of the same QEMU binary (enables caching). > > > > > > Option 3: Add @dynamic-read-only to the drivers that provide dynamic > > read-only, default to value of auto-read-only, promise we'll never add > > it to drivers that don't. > > > > Rationale: give users explicit control over dynamic vs. fallback for all > > drivers that can provide dynamic. This makes some sense as an > > interface, as long as you ignore the fact that no driver implements both > > dynamic and fallback. I can't see why a driver couldn't implement both. > > It also makes dynamic support visible in the schema. > > > > Behavior (three bools -> eight cases): > > > > * read-only=on: hard read-only (no change) > > > > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off > > > > * read-only=on,auto-read-only=on: hard read-only (no change) > > > > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on > > > > * read-only=off: hard read-write (no change) > > > > Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off > > > > * read-only=off,auto-read-only=on: > > > > Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on > > > > - file-posix.c's drivers: dynamic read-only (no change, > > dynamic-read-only=on is the default) > > - some other drivers: fallback read-only (no change) > > - remaining drivers: hard read/write (no change) > > > > * read-only=off,auto-read-only=on,dynamic-read-only=off > > > > - file-posix.c's drivers: error > > - all other drivers: N/A > > > > * read-only=off,auto-read-only=off,dynamic-read-only=on > > > > - file-posix.c's drivers: error > > - all other drivers: N/A > > > > * read-only=on,dynamic-read-only=on > > > > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on > > > > - file-posix.c's drivers: error > > - all other drivers: N/A > > > > * read-only=on,auto-read-only=on,dynamic-read-only=off > > > > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off > > > > - file-posix.c's drivers: error > > - all other drivers: N/A > > > > CON: > > > > * Two bools (read-only and auto-read-only) to select from three choices > > was already ugly. Three bools (the same plus dynamic-read-only) to > > select from four choices is even uglier. > > > > * The explicit control is just a facade so far: since only the default > > setting is implemented, it doesn't actually control anything. > > > > PRO: > > > > * When libvirt sees a driver providing a dynamic-read-only parameter, it > > knows it can rely on the driver providing dynamic read-only semantics. > > > > * Adding dynamic read-only capability to drivers creates no > > introspection problems: we simply add dynamic-read-only to their > > parameters. > > > > Code sketch appended > > Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile. > Message-ID: <20190401115410.GB4935@localhost.localdomain> > > A variation of option 3, code is almost identical. Where option 3 tries > to evolve what we have into the least bad permanent interface, option 4 > only wants to serve as a stop gap until we get a better solution, such > as option 5. I think code-wise, this is actually very different from option 3. We need to actually process the dynamic-read-only option with option 3, and I can see things go wrong easily, which is not something to do for -rc2. Option 4, in contrast, is purely a minimal schema change without touching any C source file. > Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say > "this block driver provides dynamic read-only". > Message-ID: <20190401140842.GD4935@localhost.localdomain> More complex than option 4, but the advantage here is that none of the new code is actually externally visible, so if the implementation turns out buggy, we can fix it without having to maintain compatibility with the broken state. The actually visible change, the query-qmp-schema output, is almost as minimal as option 4 and can manually be verified. A bit more invasive than option 4, but I'd consider it still doable for -rc2 if we decide quickly. > Option 6 [Peter]: Trigger reopen explicitly via QMP. > Message-ID: <20190401122054.GA2686@andariel.pipo.sk> > [Me] This leaves auto-read-only without a use case; deprecate? We have made some progress there with the new x-blockdev-reopen command. There are a few things that we know must be addressed before we can declare it stable, and probably there are more such things that we don't know of. x-blockdev-reopen takes the same options as blockdev-add and as such has a similar complexity. The experience with blockdev-add makes me doubt that we can mark it stable even for 4.1. So this might be a long-term solution, but we'd have to do something else for the meantime. > Given that the libvirt code isn't ready, why do we need this done and > declared stable in 4.0? We're going to tag -rc2 tomorrow... The next libvirt release will be long before the next QEMU release. If we don't have it in 4.0, enabling -blockdev will have to wait until the first libvirt release after the QEMU 4.1 release (so maybe September) and we'll still be stuck with -drive and unable to start deprecating it until then. Also, we don't want to give Peter the excuse that the qemu code isn't ready. ;-) Kevin
On Tue, Apr 02, 2019 at 10:19:29 +0200, Kevin Wolf wrote: > Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben: > > Markus Armbruster <armbru@redhat.com> writes: [...] > > Given that the libvirt code isn't ready, why do we need this done and > > declared stable in 4.0? We're going to tag -rc2 tomorrow... > > The next libvirt release will be long before the next QEMU release. If > we don't have it in 4.0, enabling -blockdev will have to wait until the > first libvirt release after the QEMU 4.1 release (so maybe September) > and we'll still be stuck with -drive and unable to start deprecating it > until then. > > Also, we don't want to give Peter the excuse that the qemu code isn't > ready. ;-) It will not be an excuse. That's because we have plenty of 'prior art' in adding version-based capabilities if everything else fails. I'm just always unhappy when I see it. In this instance I failed to speak up soon enough to get a better fix ready. I could try to excuse myself, but I will not. Just please don't drop the work that can be used to detect further changes like this. It might come in handy sooner or later. At this point I'd not change anything. I agree that last second fixes have a big probability to be regretted later. In the end it may end up being necessary to fix yet another thing for blockdev or something else and we can at least prevent that from having a version-based check.
If in a hurry, skip ahead to "bottom line".
Kevin Wolf <kwolf@redhat.com> writes:
> Am 01.04.2019 um 22:02 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>> > Let's review our options for 4.0.
>> >
>> > Please note my analysis is handicapped by incomplete information, in
>> > particular on libvirt's needs.
>> >
>> > Terminology:
>> >
>> > * "Hard read-write" semantics: open read/write.
>> >
>> > * "Hard read-only" semantics: open read-only.
>> >
>> > * "Dynamic read-only" semantics: open read-only, reopen read/write when
>> > a writer attaches, reopen read-only when the last writer detaches.
>> >
>> > * "Fallback read-only" semantics:. try to open read/write, fall back to
>> > read-only.
>> >
>> > We have a use case for dynamic read-only: libvirt. I'm not aware of a
>> > use case for fallback read-only.
>> >
>> > Behavior before 3.1:
>> >
>> > * read-only=on: hard read-only
>> >
>> > * read-only=off: hard read-write
>> >
>> > Behavior in 3.1: new parameter auto-read-only, default off.
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (auto-read-only=on
>> > silently ignored)
>> >
>> > * read-only=off: hard read-write
>> >
>> > * read-only=off,auto-read-only=on: depends on the driver
>> >
>> > - file-posix.c's drivers: fallback read-only
>> > - some other drivers: fallback read-only
>> > - remaining drivers: hard read/write
>> >
>> > Behavior in 4.0 so far:
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (no change)
>> >
>> > * read-only=off: hard read-write (no change)
>> >
>> > * read-only=off,auto-read-only=on: depends on the driver
>> >
>> > - file-posix.c's drivers: dynamic read-only
>> > - some other drivers: fallback read-only (no change)
>> > - remaining drivers: hard read/write (no change)
>> >
>> >
>> > Option 1: Rename @auto-read-only.
>> >
>> > Rationale: we released auto-read-only in v3.1 with unproven fallback
>> > read-only semantics. It turned out not to be useful. Admit the
>> > mistake, retract it. Release our next attempt in 4.0 under a suitable
>> > new name with fallback read-only semantics.
>> >
>> > CON:
>> >
>> > * Compatibility break. No-go if there are users. Users seem quite
>> > unlikely, though.
>> >
>> > * Still unproven, albeit less so: this time we have some unreleased
>> > (unfinished?) libvirt code.
>> >
>> > * Semantics are still largely left to drivers, and the schema can't tell
>> > which one does what. Awkward.
>> >
>> > * Unless we're fairly confident we won't upgrade drivers from "hard" to
>> > "fallback" to "dynamic", this merely kicks the can down the road:
>> > we'll face the exact same "how can libvirt find out" problem on every
>> > upgrade.
>> >
>> > PRO:
>> >
>> > * When libvirt sees the new name, it can rely on file-posix.c's drivers
>> > providing dynamic read-only semantics.
>> >
>> >
>> > Option 2: Add query-qemu-features command, return
>> > file-dynamic-auto-read-only #ifdef CONFIG_POSIX.
>> >
>> > Rationale: we released auto-read-only in v3.1 with unproven fallback
>> > read-only semantics. It turned out not to be useful. Admit the
>> > mistake, and patch it up in 4.0. Libirt needs to know whether it's
>> > patche up, and this is a simple way to tell it.
>> >
>> > CON:
>> >
>> > * All of option 1's, except for the compatibility break
>> >
>> > * Uses query-qemu-features to expose a property of the build. I
>> > consider that a mistake.
>> >
>> > PRO
>> >
>> > * Libvirt can use either query-qmp-schema or query-qemu-features to find
>> > out whether it can can rely on file-posix.c's drivers providing
>> > dynamic read-only semantics. To make query-qemu-features usable, we
>> > need to promise query-qemu-features never returns false for it. To
>> > make query-qemu-features usable, we need to promise the value will
>> > remain valid for the remainder of the QEMU run (defeats caching) or
>> > for any future run of the same QEMU binary (enables caching).
>> >
>> >
>> > Option 3: Add @dynamic-read-only to the drivers that provide dynamic
>> > read-only, default to value of auto-read-only, promise we'll never add
>> > it to drivers that don't.
>> >
>> > Rationale: give users explicit control over dynamic vs. fallback for all
>> > drivers that can provide dynamic. This makes some sense as an
>> > interface, as long as you ignore the fact that no driver implements both
>> > dynamic and fallback. I can't see why a driver couldn't implement both.
>> > It also makes dynamic support visible in the schema.
>> >
>> > Behavior (three bools -> eight cases):
>> >
>> > * read-only=on: hard read-only (no change)
>> >
>> > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=off
>> >
>> > * read-only=on,auto-read-only=on: hard read-only (no change)
>> >
>> > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=on
>> >
>> > * read-only=off: hard read-write (no change)
>> >
>> > Shorthand for read-only=off,auto-read-only=off,dynamic-read-only=off
>> >
>> > * read-only=off,auto-read-only=on:
>> >
>> > Shorthand for read-only=off,auto-read-only=on,dynamic-read-only=on
>> >
>> > - file-posix.c's drivers: dynamic read-only (no change,
>> > dynamic-read-only=on is the default)
>> > - some other drivers: fallback read-only (no change)
>> > - remaining drivers: hard read/write (no change)
>> >
>> > * read-only=off,auto-read-only=on,dynamic-read-only=off
>> >
>> > - file-posix.c's drivers: error
>> > - all other drivers: N/A
>> >
>> > * read-only=off,auto-read-only=off,dynamic-read-only=on
>> >
>> > - file-posix.c's drivers: error
>> > - all other drivers: N/A
>> >
>> > * read-only=on,dynamic-read-only=on
>> >
>> > Shorthand for read-only=on,auto-read-only=off,dynamic-read-only=on
>> >
>> > - file-posix.c's drivers: error
>> > - all other drivers: N/A
>> >
>> > * read-only=on,auto-read-only=on,dynamic-read-only=off
>> >
>> > Shorthand for read-only=on,auto-read-only=on,dynamic-read-only=off
>> >
>> > - file-posix.c's drivers: error
>> > - all other drivers: N/A
>> >
>> > CON:
>> >
>> > * Two bools (read-only and auto-read-only) to select from three choices
>> > was already ugly. Three bools (the same plus dynamic-read-only) to
>> > select from four choices is even uglier.
>> >
>> > * The explicit control is just a facade so far: since only the default
>> > setting is implemented, it doesn't actually control anything.
>> >
>> > PRO:
>> >
>> > * When libvirt sees a driver providing a dynamic-read-only parameter, it
>> > knows it can rely on the driver providing dynamic read-only semantics.
>> >
>> > * Adding dynamic read-only capability to drivers creates no
>> > introspection problems: we simply add dynamic-read-only to their
>> > parameters.
>> >
>> > Code sketch appended
>>
>> Option 4 [Kevin]: Add a dummy option to BlockdevOptionsFile.
>> Message-ID: <20190401115410.GB4935@localhost.localdomain>
>>
>> A variation of option 3, code is almost identical. Where option 3 tries
>> to evolve what we have into the least bad permanent interface, option 4
>> only wants to serve as a stop gap until we get a better solution, such
>> as option 5.
>
> I think code-wise, this is actually very different from option 3. We
> need to actually process the dynamic-read-only option with option 3, and
> I can see things go wrong easily, which is not something to do for -rc2.
>
> Option 4, in contrast, is purely a minimal schema change without
> touching any C source file.
While I reject your "is very different" claim, I accept your "option 4
is less risky than option 3" claim.
Common part modulo naming:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..0cf15477ce 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2827,6 +2827,7 @@
# Driver specific block device options for the file backend.
#
# @filename: path to the image file
+# @dynamic-read-only: FIXME document (since 4.0)
# @pr-manager: the id for the object that will handle persistent reservations
# for this device (default: none, forward the commands via SG_IO;
# since 2.11)
@@ -2847,6 +2848,8 @@
##
{ 'struct': 'BlockdevOptionsFile',
'data': { 'filename': 'str',
+ '*dynamic-read-only': {'type': 'bool',
+ 'if': 'defined(CONFIG_POSIX)'},
'*pr-manager': 'str',
'*locking': 'OnOffAuto',
'*aio': 'BlockdevAioOptions',
Option 3 only:
diff --git a/block/file-posix.c b/block/file-posix.c
index db4cccbe51..d62681df14 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -420,6 +420,11 @@ static QemuOptsList raw_runtime_opts = {
.type = QEMU_OPT_STRING,
.help = "File name of the image",
},
+ {
+ .name = "dynamic-read-only",
+ .type = QEMU_OPT_BOOL,
+ .help = "FIXME",
+ },
{
.name = "aio",
.type = QEMU_OPT_STRING,
@@ -540,6 +545,13 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
s->open_flags = open_flags;
raw_parse_flags(bdrv_flags, &s->open_flags, false);
+ if (qemu_opt_get_bool(opts, "dynamic-read-only",
+ bdrv_flags & BDRV_O_AUTO_RDONLY)
+ == !(bdrv_flags & BDRV_O_AUTO_RDONLY)) {
+ error_setg(errp, "FIXME");
+ ret = - EINVAL;
+ }
+
s->fd = -1;
fd = qemu_open(filename, s->open_flags, 0644);
ret = fd < 0 ? -errno : 0;
Yes, this is more code, and yes, things could conceivably go wrong here.
>> Option 5 [Kevin]: Add feature flags to the QAPI language, use one to say
>> "this block driver provides dynamic read-only".
>> Message-ID: <20190401140842.GD4935@localhost.localdomain>
>
> More complex than option 4, but the advantage here is that none of the
> new code is actually externally visible, so if the implementation turns
> out buggy, we can fix it without having to maintain compatibility with
> the broken state.
>
> The actually visible change, the query-qmp-schema output, is almost as
> minimal as option 4 and can manually be verified.
>
> A bit more invasive than option 4, but I'd consider it still doable for
> -rc2 if we decide quickly.
It's a good option given the mess we made with auto-read-only in 3.1.
It's just three weeks late.
>> Option 6 [Peter]: Trigger reopen explicitly via QMP.
>> Message-ID: <20190401122054.GA2686@andariel.pipo.sk>
>> [Me] This leaves auto-read-only without a use case; deprecate?
>
> We have made some progress there with the new x-blockdev-reopen command.
> There are a few things that we know must be addressed before we can
> declare it stable, and probably there are more such things that we don't
> know of.
>
> x-blockdev-reopen takes the same options as blockdev-add and as such has
> a similar complexity. The experience with blockdev-add makes me doubt
> that we can mark it stable even for 4.1. So this might be a long-term
> solution, but we'd have to do something else for the meantime.
Understand.
>> Given that the libvirt code isn't ready, why do we need this done and
>> declared stable in 4.0? We're going to tag -rc2 tomorrow...
>
> The next libvirt release will be long before the next QEMU release. If
Libvirt releases roughly monthly.
QEMU releases roughly April, August, December.
> we don't have it in 4.0, enabling -blockdev will have to wait until the
> first libvirt release after the QEMU 4.1 release (so maybe September)
> and we'll still be stuck with -drive and unable to start deprecating it
> until then.
Unless libvirt does yet another version check.
I talked this over with Peter.
libvirt 5.2 is expected today. Peter is trying to get his work merged
in time for the next libvirt release (but he was trying that a month
ago, too). So we're talking about 5.3 (May) or 5.4 (June). Leaves a
gap of 2-3 month to 4.1 (August).
I offered option 4 "Add a dummy option to BlockdevOptionsFile". He said
he'd advise against ugly hacks at this point, but going forward, QEMU
really needs a way to let libvirt detect code fixes, such as QAPI schema
feature flags.
> Also, we don't want to give Peter the excuse that the qemu code isn't
> ready. ;-)
Ah, you'd never do that to us, Peter, would you?
Bottom line:
* I can't bring myself to do QAPI schema language changes (option 5)
this late in the game. Sorry.
* I'm okay with option 4 if Kevin believes it could be useful. Even if
Peter won't actually use it, we're fairly unlikely to regret it.
* A more complete respin of the proposed QAPI schema language changes
would be fair game for 4.1.
* QEMU and libvirt need to get better at co-developing features. QEMU
doesn't want to release stable interfaces without a user, and libvirt
doesn't want to release a user without QEMU releasing first. When we
break this cycle by having QEMU releases stable interfaces completely
unproven, we unsurprisingly get to regret it later.
Does this make sense?
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;
> +}
© 2016 - 2026 Red Hat, Inc.