block/file-posix.c | 12 ++++++++++++ qapi/block-core.json | 3 +++ 2 files changed, 15 insertions(+)
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?
© 2016 - 2024 Red Hat, Inc.