* [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
@ 2019-01-17 15:33 Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links Alberto Garcia
` (14 more replies)
0 siblings, 15 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Hi,
here's a patch series to implement a QMP command for bdrv_reopen().
This is not too different from the previous iteration (RFC v2, see
changes below), but I'm not tagging it as RFC any longer.
The new command is called x-blockdev-reopen, and it's designed to be
similar to blockdev-add. It also takes BlockdevOptions as a
parameter. The "node-name" field of BlockdevOptions must be set, and
it is used to select the BlockDriverState to reopen.
In this example "hd0" is reopened with the given set of options:
{ "execute": "x-blockdev-reopen",
"arguments": {
"driver": "qcow2",
"node-name": "hd0",
"file": {
"driver": "file",
"filename": "hd0.qcow2",
"node-name": "hd0f"
},
"l2-cache-size": 524288
}
}
Changing options
================
The general idea is quite straightforward, but the devil is in the
details. Since this command tries to mimic blockdev-add, the state of
the block device after being reopened should generally be equivalent
to that of a block device added with the same set of options.
There are two important things with this:
a) Some options cannot be changed (most drivers don't allow that, in
fact).
b) If an option is missing, it should be reset to its default value
(rather than keeping its previous value).
Example: the default value of l2-cache-size is 1MB. If we set a
different value (2MB) on blockdev-add but then omit the option in
x-blockdev-reopen, then it should be reset back to 1MB. The current
bdrv_reopen() code keeps the previous value.
Trying to change an option that doesn't allow it is already being
handled by the code. The loop at the end of bdrv_reopen_prepare()
checks all options that were not handled by the block driver and sees
if any of them has been modified.
However, as I explained earlier in (b), omitting an option is supposed
to reset it to its default value, so it's also a way of changing
it. If the option cannot be changed then we should detect it and
return an error. What I'm doing in this series is making every driver
publish its list of run-time options, and which ones of them can be
modified.
Backing files
=============
This command allows reconfigurations in the node graph. Currently it
only allows changing an image's backing file, but it should be
possible to implement similar functionalities in drivers that have
other children, like blkverify or quorum.
Let's add an image with its backing file (hd1 <- hd0) like this:
{ "execute": "blockdev-add",
"arguments": {
"driver": "qcow2",
"node-name": "hd0",
"file": {
"driver": "file",
"filename": "hd0.qcow2",
"node-name": "hd0f"
},
"backing": {
"driver": "qcow2",
"node-name": "hd1",
"file": {
"driver": "file",
"filename": "hd1.qcow2",
"node-name": "hd1f"
}
}
}
}
Removing the backing file can be done by simply passing the option {
..., "backing": null } to x-blockdev-reopen.
Replacing it is not so straightforward. If we pass something like {
..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen will
assume that we're trying to change the options of the existing backing
file (and it will fail in most cases because it'll likely think that
we're trying to change the node name, among other options).
So I decided to use a reference instead: { ..., "backing": "hd2" },
where "hd2" is of an existing node that has been added previously.
Leaving out the "backing" option can be ambiguous on some cases (what
should it do? keep the current backing file? open the default one
specified in the image file?) so x-blockdev-reopen requires that this
option is present. For convenience, if the BDS doesn't have a backing
file then we allow leaving the option out.
As you can see in the patch series, I wrote a relatively large amount
of tests for many different scenarios, including some tricky ones.
Perhaps the part that I'm still not convinced about is the one about
freezing backing links to prevent them from being removed, but the
implementation was quite simple so I decided to give it a go. As
you'll see in the patches I chose to use a bool instead of a counter
because I couldn't think of a case where it would make sense to have
two users freezing the same backing link.
Thanks,
Berto
v1:
- Patch 10: forbid setting a new backing file with a different
AioContext.
- Patch 13 (new): Remove unused parameter from bdrv_reopen_multiple.
- Patch 14: Acquire the AioContext before calling
bdrv_reopen_multiple().
- Patch 15: More test cases.
- Patches 3, 8, 9, 11, 12: scripts/checkpatch.pl is more picky now
with the format of multi-line comments, so correct them.
RFCv2: https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00901.html
Alberto Garcia (13):
block: Allow freezing BdrvChild links
block: Freeze the backing chain for the duration of the commit job
block: Freeze the backing chain for the duration of the mirror job
block: Freeze the backing chain for the duration of the stream job
block: Add 'keep_old_opts' parameter to bdrv_reopen_queue()
block: Handle child references in bdrv_reopen_queue()
block: Allow omitting the 'backing' option in certain cases
block: Allow changing the backing file on reopen
block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
block: Add bdrv_reset_options_allowed()
block: Remove the AioContext parameter from bdrv_reopen_multiple()
block: Add an 'x-blockdev-reopen' QMP command
qemu-iotests: Test the x-blockdev-reopen QMP command
block.c | 333 +++++++++++++--
block/blkdebug.c | 1 +
block/commit.c | 8 +
block/crypto.c | 1 +
block/file-posix.c | 10 +
block/iscsi.c | 2 +
block/mirror.c | 8 +
block/null.c | 2 +
block/nvme.c | 1 +
block/qcow.c | 1 +
block/rbd.c | 1 +
block/replication.c | 7 +-
block/sheepdog.c | 2 +
block/stream.c | 16 +
block/vpc.c | 1 +
blockdev.c | 47 +++
include/block/block.h | 11 +-
include/block/block_int.h | 12 +
qapi/block-core.json | 42 ++
qemu-io-cmds.c | 4 +-
tests/qemu-iotests/224 | 1001 ++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/224.out | 5 +
tests/qemu-iotests/group | 1 +
23 files changed, 1484 insertions(+), 33 deletions(-)
create mode 100644 tests/qemu-iotests/224
create mode 100644 tests/qemu-iotests/224.out
--
2.11.0
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-02-12 14:47 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job Alberto Garcia
` (13 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Our permission system is useful to define what operations are allowed
on a certain block node and includes things like BLK_PERM_WRITE or
BLK_PERM_RESIZE among others.
One of the permissions is BLK_PERM_GRAPH_MOD which allows "changing
the node that this BdrvChild points to". The exact meaning of this has
never been very clear, but it can be understood as "change any of the
links connected to the node". This can be used to prevent changing a
backing link, but it's too coarse.
This patch adds a new 'frozen' attribute to BdrvChild, which forbids
detaching the link from the node it points to, and new API to freeze
and unfreeze a backing chain.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 5 ++++
include/block/block_int.h | 5 ++++
3 files changed, 76 insertions(+)
diff --git a/block.c b/block.c
index 4f5ff2cc12..51fac086c7 100644
--- a/block.c
+++ b/block.c
@@ -2062,6 +2062,8 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
BlockDriverState *old_bs = child->bs;
int i;
+ assert(!child->frozen);
+
if (old_bs && new_bs) {
assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
}
@@ -2278,6 +2280,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
bdrv_inherits_from_recursive(backing_hd, bs);
+ if (bdrv_is_backing_chain_frozen(bs, backing_bs(bs), errp)) {
+ return;
+ }
+
if (backing_hd) {
bdrv_ref(backing_hd);
}
@@ -3813,6 +3819,62 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
}
/*
+ * Return true if at least one of the backing links between @bs and
+ * @base is frozen. @errp is set if that's the case.
+ */
+bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp)
+{
+ BlockDriverState *i;
+
+ for (i = bs; i != base && i->backing; i = backing_bs(i)) {
+ if (i->backing->frozen) {
+ error_setg(errp, "Cannot remove link from '%s' to '%s'",
+ i->node_name, backing_bs(i)->node_name);
+ return true;
+ }
+ }
+
+ return false;
+}
+
+/*
+ * Freeze all backing links between @bs and @base.
+ * If any of the links is already frozen the operation is aborted and
+ * none of the links are modified.
+ * Returns 0 on success. On failure returns < 0 and sets @errp.
+ */
+int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp)
+{
+ BlockDriverState *i;
+
+ if (bdrv_is_backing_chain_frozen(bs, base, errp)) {
+ return -EPERM;
+ }
+
+ for (i = bs; i != base && i->backing; i = backing_bs(i)) {
+ i->backing->frozen = true;
+ }
+
+ return 0;
+}
+
+/*
+ * Unfreeze all backing links between @bs and @base. The caller must
+ * ensure that all links are frozen before using this function.
+ */
+void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base)
+{
+ BlockDriverState *i;
+
+ for (i = bs; i != base && i->backing; i = backing_bs(i)) {
+ assert(i->backing->frozen);
+ i->backing->frozen = false;
+ }
+}
+
+/*
* Drops images above 'base' up to and including 'top', and sets the image
* above 'top' to have base as its backing file.
*
@@ -3861,6 +3923,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
goto exit;
}
+ if (bdrv_is_backing_chain_frozen(top, base, NULL)) {
+ goto exit;
+ }
+
/* If 'base' recursively inherits from 'top' then we should set
* base->inherits_from to top->inherits_from after 'top' and all
* other intermediate nodes have been dropped.
diff --git a/include/block/block.h b/include/block/block.h
index f70a843b72..6f10a8fcfc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -353,6 +353,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
BlockDriverState *bs);
BlockDriverState *bdrv_find_base(BlockDriverState *bs);
+bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp);
+int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp);
+void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
typedef struct BdrvCheckResult {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622216..fd0e88d17a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -661,6 +661,11 @@ struct BdrvChild {
*/
uint64_t shared_perm;
+ /*
+ * This link is frozen: the child cannot be detached from the parent.
+ */
+ bool frozen;
+
QLIST_ENTRY(BdrvChild) next;
QLIST_ENTRY(BdrvChild) next_parent;
};
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-02-12 14:54 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 03/13] block: Freeze the backing chain for the duration of the mirror job Alberto Garcia
` (12 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/commit.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/block/commit.c b/block/commit.c
index 53148e610b..8824d135e0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -73,6 +73,8 @@ static int commit_prepare(Job *job)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+ bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
+
/* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
* the normal backing chain can be restored. */
blk_unref(s->base);
@@ -89,6 +91,8 @@ static void commit_abort(Job *job)
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
BlockDriverState *top_bs = blk_bs(s->top);
+ bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
+
/* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
bdrv_ref(top_bs);
bdrv_ref(s->commit_top_bs);
@@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs,
}
}
+ if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
+ goto fail;
+ }
+
ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
if (ret < 0) {
goto fail;
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 03/13] block: Freeze the backing chain for the duration of the mirror job
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job Alberto Garcia
` (11 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/mirror.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index 22bef9f7e9..afbc30da61 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -630,6 +630,10 @@ static int mirror_exit_common(Job *job)
}
s->prepared = true;
+ if (bdrv_chain_contains(src, target_bs)) {
+ bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
+ }
+
bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
/* Make sure that the source BDS doesn't go away during bdrv_replace_node,
@@ -1641,6 +1645,10 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
goto fail;
}
}
+
+ if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
+ goto fail;
+ }
}
QTAILQ_INIT(&s->ops_in_flight);
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (2 preceding siblings ...)
2019-01-17 15:33 ` [Qemu-devel] [PATCH 03/13] block: Freeze the backing chain for the duration of the mirror job Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-02-12 15:15 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 05/13] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue() Alberto Garcia
` (10 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/stream.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/block/stream.c b/block/stream.c
index 7a49ac0992..39a2e10892 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,6 +54,14 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
}
+static void stream_abort(Job *job)
+{
+ StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
+ BlockJob *bjob = &s->common;
+
+ bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
+}
+
static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
@@ -63,6 +71,8 @@ static int stream_prepare(Job *job)
Error *local_err = NULL;
int ret = 0;
+ bdrv_unfreeze_backing_chain(bs, base);
+
if (bs->backing) {
const char *base_id = NULL, *base_fmt = NULL;
if (base) {
@@ -213,6 +223,7 @@ static const BlockJobDriver stream_job_driver = {
.free = block_job_free,
.run = stream_run,
.prepare = stream_prepare,
+ .abort = stream_abort,
.clean = stream_clean,
.user_resume = block_job_user_resume,
.drain = block_job_drain,
@@ -259,6 +270,11 @@ void stream_start(const char *job_id, BlockDriverState *bs,
&error_abort);
}
+ if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
+ job_early_fail(&s->common.job);
+ goto fail;
+ }
+
s->base = base;
s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 05/13] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue()
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (3 preceding siblings ...)
2019-01-17 15:33 ` [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue() Alberto Garcia
` (9 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
The bdrv_reopen_queue() function is used to create a queue with
the BDSs that are going to be reopened and their new options. Once
the queue is ready bdrv_reopen_multiple() is called to perform the
operation.
The original options from each one of the BDSs are kept, with the new
options passed to bdrv_reopen_queue() applied on top of them.
For "x-blockdev-reopen" we want a function that behaves much like
"blockdev-add". We want to ignore the previous set of options so that
only the ones actually specified by the user are applied, with the
rest having their default values.
One of the things that we need is a way to tell bdrv_reopen_queue()
whether we want to keep the old set of options or not, and that's what
this patch does. All current callers are setting this new parameter to
true and x-blockdev-reopen will set it to false.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 34 +++++++++++++++++++---------------
block/replication.c | 4 ++--
include/block/block.h | 3 ++-
qemu-io-cmds.c | 2 +-
4 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/block.c b/block.c
index 51fac086c7..056a620eb2 100644
--- a/block.c
+++ b/block.c
@@ -2935,7 +2935,8 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
QDict *options,
const BdrvChildRole *role,
QDict *parent_options,
- int parent_flags)
+ int parent_flags,
+ bool keep_old_opts)
{
assert(bs != NULL);
@@ -2975,13 +2976,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
*/
/* Old explicitly set values (don't overwrite by inherited value) */
- if (bs_entry) {
- old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
- } else {
- old_options = qdict_clone_shallow(bs->explicit_options);
+ if (bs_entry || keep_old_opts) {
+ old_options = qdict_clone_shallow(bs_entry ?
+ bs_entry->state.explicit_options :
+ bs->explicit_options);
+ bdrv_join_options(bs, options, old_options);
+ qobject_unref(old_options);
}
- bdrv_join_options(bs, options, old_options);
- qobject_unref(old_options);
explicit_options = qdict_clone_shallow(options);
@@ -2993,10 +2994,12 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
flags = bdrv_get_flags(bs);
}
- /* Old values are used for options that aren't set yet */
- old_options = qdict_clone_shallow(bs->options);
- bdrv_join_options(bs, options, old_options);
- qobject_unref(old_options);
+ if (keep_old_opts) {
+ /* Old values are used for options that aren't set yet */
+ old_options = qdict_clone_shallow(bs->options);
+ bdrv_join_options(bs, options, old_options);
+ qobject_unref(old_options);
+ }
/* We have the final set of options so let's update the flags */
options_copy = qdict_clone_shallow(options);
@@ -3046,7 +3049,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
g_free(child_key_dot);
bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
- child->role, options, flags);
+ child->role, options, flags, keep_old_opts);
}
return bs_queue;
@@ -3054,9 +3057,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
- QDict *options)
+ QDict *options, bool keep_old_opts)
{
- return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, NULL, 0);
+ return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, NULL, 0,
+ keep_old_opts);
}
/*
@@ -3128,7 +3132,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
bdrv_subtree_drained_begin(bs);
- queue = bdrv_reopen_queue(NULL, bs, opts);
+ queue = bdrv_reopen_queue(NULL, bs, opts, true);
ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
bdrv_subtree_drained_end(bs);
diff --git a/block/replication.c b/block/replication.c
index e70dd95001..d35214370e 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -374,14 +374,14 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
QDict *opts = qdict_new();
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
- opts);
+ opts, true);
}
if (s->orig_secondary_read_only) {
QDict *opts = qdict_new();
qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
- opts);
+ opts, true);
}
if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index 6f10a8fcfc..a061ef3944 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -299,7 +299,8 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
int flags, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
- BlockDriverState *bs, QDict *options);
+ BlockDriverState *bs, QDict *options,
+ bool keep_old_opts);
int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2c39124036..db8887205e 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2075,7 +2075,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
}
bdrv_subtree_drained_begin(bs);
- brq = bdrv_reopen_queue(NULL, bs, opts);
+ brq = bdrv_reopen_queue(NULL, bs, opts, true);
bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
bdrv_subtree_drained_end(bs);
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue()
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (4 preceding siblings ...)
2019-01-17 15:33 ` [Qemu-devel] [PATCH 05/13] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue() Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-02-12 16:28 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 07/13] block: Allow omitting the 'backing' option in certain cases Alberto Garcia
` (8 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Children in QMP are specified with BlockdevRef / BlockdevRefOrNull,
which can contain a set of child options, a child reference, or
NULL. In optional attributes like "backing" it can also be missing.
Only the first case (set of child options) is being handled properly
by bdrv_reopen_queue(). This patch deals with all the others.
Here's how these cases should be handled when bdrv_reopen_queue() is
deciding what to do with each child of a BlockDriverState:
1) Set of child options: the options are removed from the parent's
options QDict and are passed to the child with a recursive
bdrv_reopen_queue() call. This case was already working fine.
2) Child reference: there's two possibilites here.
2a) Reference to the current child: the child is put in the
reopen queue, keeping its current set of options (since this
was a child reference there was no way to specify a
different set of options).
2b) Reference to a different BDS: the current child is not put
in the reopen queue at all. Passing a reference to a
different BDS can be used to replace a child, although at
the moment no driver implements this, so it results in an
error. In any case, the current child is not going to be
reopened (and might in fact disappear if it's replaced)
3) NULL: This is similar to (2b). Although no driver allows this
yet it can be used to detach the current child so it should not
be put in the reopen queue.
4) Missing option: at the moment "backing" is the only case where
this can happen. With "blockdev-add", leaving "backing" out
means that the default backing file is opened. We don't want to
open a new image during reopen, so we require that "backing" is
always present. We'll relax this requirement a bit in the next
patch.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
include/block/block.h | 1 +
2 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 056a620eb2..fd51f1cd35 100644
--- a/block.c
+++ b/block.c
@@ -3032,9 +3032,21 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
bs_entry->state.perm = UINT64_MAX;
bs_entry->state.shared_perm = 0;
+ /*
+ * If keep_old_opts is false then it means that unspecified
+ * options must be reset to its original value. We don't allow
+ * resetting 'backing' but we need to know if the option is
+ * missing in order to decide if we have to return an error.
+ */
+ if (!keep_old_opts) {
+ bs_entry->state.backing_missing =
+ !qdict_haskey(options, "backing") &&
+ !qdict_haskey(options, "backing.driver");
+ }
+
QLIST_FOREACH(child, &bs->children, next) {
- QDict *new_child_options;
- char *child_key_dot;
+ QDict *new_child_options = NULL;
+ bool child_keep_old = keep_old_opts;
/* reopen can only change the options of block devices that were
* implicitly created and inherited options. For other (referenced)
@@ -3043,13 +3055,31 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
continue;
}
- child_key_dot = g_strdup_printf("%s.", child->name);
- qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
- qdict_extract_subqdict(options, &new_child_options, child_key_dot);
- g_free(child_key_dot);
+ /* Check if the options contain a child reference */
+ if (qdict_haskey(options, child->name)) {
+ const char *childref = qdict_get_try_str(options, child->name);
+ /*
+ * The current child must not be reopened if the child
+ * reference does not point to it.
+ */
+ if (g_strcmp0(childref, child->bs->node_name)) {
+ continue;
+ }
+ /*
+ * If the child reference points to the current child then
+ * reopen it with its existing set of options.
+ */
+ child_keep_old = true;
+ } else {
+ /* Extract child options ("child-name.*") */
+ char *child_key_dot = g_strdup_printf("%s.", child->name);
+ qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
+ qdict_extract_subqdict(options, &new_child_options, child_key_dot);
+ g_free(child_key_dot);
+ }
bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
- child->role, options, flags, keep_old_opts);
+ child->role, options, flags, child_keep_old);
}
return bs_queue;
@@ -3306,6 +3336,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
drv_prepared = true;
+ if (reopen_state->backing_missing) {
+ error_setg(errp, "backing is missing for '%s'",
+ reopen_state->bs->node_name);
+ ret = -EINVAL;
+ goto error;
+ }
+
/* Options that are not handled are only okay if they are unchanged
* compared to the old state. It is expected that some options are only
* used for the initial open, but not reopen (e.g. filename) */
diff --git a/include/block/block.h b/include/block/block.h
index a061ef3944..e21616a038 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -187,6 +187,7 @@ typedef struct BDRVReopenState {
BlockDriverState *bs;
int flags;
BlockdevDetectZeroesOptions detect_zeroes;
+ bool backing_missing;
uint64_t perm, shared_perm;
QDict *options;
QDict *explicit_options;
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 07/13] block: Allow omitting the 'backing' option in certain cases
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (5 preceding siblings ...)
2019-01-17 15:33 ` [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue() Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-02-12 16:38 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen Alberto Garcia
` (7 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
Of all options of type BlockdevRef used to specify children in
BlockdevOptions, 'backing' is the only one that is optional.
For "x-blockdev-reopen" we want that if an option is omitted then it
must be reset to its default value. The default value of 'backing'
means that QEMU opens the backing file specified in the image
metadata, but this is not something that we want to support for the
reopen operation.
Because of this the 'backing' option has to be specified during
reopen, pointing to the existing backing file if we want to keep it,
or pointing to a different one (or NULL) if we want to replace it (to
be implemented in a subsequent patch).
In order to simplify things a bit and not to require that the user
passes the 'backing' option to every single block device even when
it's clearly not necessary, this patch allows omitting this option if
the block device being reopened doesn't have a backing file attached
_and_ no default backing file is specified in the image metadata.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index fd51f1cd35..897c8b85cd 100644
--- a/block.c
+++ b/block.c
@@ -3336,7 +3336,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
drv_prepared = true;
- if (reopen_state->backing_missing) {
+ /*
+ * We must provide the 'backing' option if the BDS has a backing
+ * file or if the image file has a backing file name as part of
+ * its metadata. Otherwise the 'backing' option can be omitted.
+ */
+ if (reopen_state->backing_missing &&
+ (backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) {
error_setg(errp, "backing is missing for '%s'",
reopen_state->bs->node_name);
ret = -EINVAL;
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (6 preceding siblings ...)
2019-01-17 15:33 ` [Qemu-devel] [PATCH 07/13] block: Allow omitting the 'backing' option in certain cases Alberto Garcia
@ 2019-01-17 15:33 ` Alberto Garcia
2019-02-12 17:27 ` Kevin Wolf
2019-01-17 15:34 ` [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver Alberto Garcia
` (6 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
This patch allows the user to change the backing file of an image that
is being reopened. Here's what it does:
- In bdrv_reopen_prepare(): check that the value of 'backing' points
to an existing node or is null. If it points to an existing node it
also needs to make sure that replacing the backing file will not
create a cycle in the node graph (i.e. you cannot reach the parent
from the new backing file).
- In bdrv_reopen_commit(): perform the actual node replacement by
calling bdrv_set_backing_hd(). It may happen that there are
temporary implicit nodes between the BDS that we are reopening and
the backing file that we want to replace (e.g. a commit fiter node),
so we must skip them.
Although x-blockdev-reopen is meant to be used like blockdev-add,
there's an important thing that must be taken into account: the only
way to set a new backing file is by using a reference to an existing
node (previously added with e.g. blockdev-add). If 'backing' contains
a dictionary with a new set of options ({"driver": "qcow2", "file": {
... }}) then it is interpreted that the _existing_ backing file must
be reopened with those options.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 122 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 897c8b85cd..10847416b2 100644
--- a/block.c
+++ b/block.c
@@ -2909,6 +2909,27 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
}
/*
+ * Returns true if @child can be reached recursively from @bs
+ */
+static bool bdrv_recurse_has_child(BlockDriverState *bs,
+ BlockDriverState *child)
+{
+ BdrvChild *c;
+
+ if (bs == child) {
+ return true;
+ }
+
+ QLIST_FOREACH(c, &bs->children, next) {
+ if (bdrv_recurse_has_child(c->bs, child)) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+/*
* Adds a BlockDriverState to a simple queue for an atomic, transactional
* reopen of multiple devices.
*
@@ -3217,6 +3238,63 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
}
/*
+ * Return 0 if the 'backing' option of @bs can be replaced with
+ * @value, otherwise return < 0 and set @errp.
+ */
+static int bdrv_reopen_parse_backing(BlockDriverState *bs, QObject *value,
+ Error **errp)
+{
+ BlockDriverState *overlay_bs, *new_backing_bs;
+ const char *str;
+
+ switch (qobject_type(value)) {
+ case QTYPE_QNULL:
+ new_backing_bs = NULL;
+ break;
+ case QTYPE_QSTRING:
+ str = qobject_get_try_str(value);
+ new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
+ if (new_backing_bs == NULL) {
+ return -EINVAL;
+ } else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
+ error_setg(errp, "Making '%s' a backing file of '%s' "
+ "would create a cycle", str, bs->node_name);
+ return -EINVAL;
+ }
+ break;
+ default:
+ /* 'backing' does not allow any other data type */
+ g_assert_not_reached();
+ }
+
+ if (new_backing_bs) {
+ if (bdrv_get_aio_context(new_backing_bs) != bdrv_get_aio_context(bs)) {
+ error_setg(errp, "Cannot use a new backing file "
+ "with a different AioContext");
+ return -EINVAL;
+ }
+ }
+
+ /*
+ * Skip all links that point to an implicit node, if any
+ * (e.g. a commit filter node). We don't want to change those.
+ */
+ overlay_bs = bs;
+ while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
+ overlay_bs = backing_bs(overlay_bs);
+ }
+
+ if (new_backing_bs != backing_bs(overlay_bs)) {
+ if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
+ errp)) {
+ return -EPERM;
+ }
+ }
+
+ return 0;
+}
+
+/*
* Prepares a BlockDriverState for reopen. All changes are staged in the
* 'opaque' field of the BDRVReopenState, which is used and allocated by
* the block driver layer .bdrv_reopen_prepare()
@@ -3359,6 +3437,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
QObject *new = entry->value;
QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+ /*
+ * Allow changing the 'backing' option. The new value can be
+ * either a reference to an existing node (using its node name)
+ * or NULL to simply detach the current backing file.
+ */
+ if (!strcmp(entry->key, "backing")) {
+ ret = bdrv_reopen_parse_backing(reopen_state->bs, new, errp);
+ if (ret < 0) {
+ goto error;
+ }
+ continue; /* 'backing' option processed, go to the next one */
+ }
+
/* Allow child references (child_name=node_name) as long as they
* point to the current child (i.e. everything stays the same). */
if (qobject_type(new) == QTYPE_QSTRING) {
@@ -3437,9 +3528,10 @@ error:
void bdrv_reopen_commit(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
- BlockDriverState *bs;
+ BlockDriverState *bs, *new_backing_bs;
BdrvChild *child;
- bool old_can_write, new_can_write;
+ bool old_can_write, new_can_write, change_backing_bs;
+ QObject *qobj;
assert(reopen_state != NULL);
bs = reopen_state->bs;
@@ -3464,6 +3556,15 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
bs->detect_zeroes = reopen_state->detect_zeroes;
+ qobj = qdict_get(bs->options, "backing");
+ change_backing_bs = (qobj != NULL);
+ if (change_backing_bs) {
+ const char *str = qobject_get_try_str(qobj);
+ new_backing_bs = str ? bdrv_find_node(str) : NULL;
+ qdict_del(bs->explicit_options, "backing");
+ qdict_del(bs->options, "backing");
+ }
+
/* Remove child references from bs->options and bs->explicit_options.
* Child options were already removed in bdrv_reopen_queue_child() */
QLIST_FOREACH(child, &bs->children, next) {
@@ -3471,6 +3572,25 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
qdict_del(bs->options, child->name);
}
+ /*
+ * Change the backing file if a new one was specified. We do this
+ * after updating bs->options, so bdrv_refresh_filename() (called
+ * from bdrv_set_backing_hd()) has the new values.
+ */
+ if (change_backing_bs) {
+ BlockDriverState *overlay = bs;
+ /*
+ * Skip all links that point to an implicit node, if any
+ * (e.g. a commit filter node). We don't want to change those.
+ */
+ while (backing_bs(overlay) && backing_bs(overlay)->implicit) {
+ overlay = backing_bs(overlay);
+ }
+ if (new_backing_bs != backing_bs(overlay)) {
+ bdrv_set_backing_hd(overlay, new_backing_bs, &error_abort);
+ }
+ }
+
bdrv_refresh_limits(bs, NULL);
bdrv_set_perm(reopen_state->bs, reopen_state->perm,
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (7 preceding siblings ...)
2019-01-17 15:33 ` [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen Alberto Garcia
@ 2019-01-17 15:34 ` Alberto Garcia
2019-02-12 18:02 ` Kevin Wolf
2019-01-17 15:34 ` [Qemu-devel] [PATCH 10/13] block: Add bdrv_reset_options_allowed() Alberto Garcia
` (5 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
This patch adds two new fields to BlockDriver:
- runtime_opts: list of runtime options for a particular block
driver. We'll use this list later to detect what options are
missing when we try to reopen a block device.
- mutable_opts: names of the runtime options that can be reset to
their default value after a block device has been added. This way
an option can not be reset by omitting it during reopen unless we
explicitly allow it.
This also sets runtime_opts (and mutable_opts where appropriate) in
all drivers that allow reopening. Most of those drivers don't actually
support changing any of their options. If the user specifies a new
value for an option that can't be changed then we already detect that
and forbid it (in bdrv_reopen_prepare()). But if the user omits an
option in order to try to reset it to its default value we need to
detect that, so we'll use these two new fields for that.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/blkdebug.c | 1 +
block/crypto.c | 1 +
block/file-posix.c | 10 ++++++++++
block/iscsi.c | 2 ++
block/null.c | 2 ++
block/nvme.c | 1 +
block/qcow.c | 1 +
block/rbd.c | 1 +
block/sheepdog.c | 2 ++
block/vpc.c | 1 +
include/block/block_int.h | 7 +++++++
11 files changed, 29 insertions(+)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 0759452925..bba7645e09 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -893,6 +893,7 @@ static BlockDriver bdrv_blkdebug = {
.protocol_name = "blkdebug",
.instance_size = sizeof(BDRVBlkdebugState),
.is_filter = true,
+ .runtime_opts = &runtime_opts,
.bdrv_parse_filename = blkdebug_parse_filename,
.bdrv_file_open = blkdebug_open,
diff --git a/block/crypto.c b/block/crypto.c
index f0a5f6b987..ba4da53191 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -635,6 +635,7 @@ BlockDriver bdrv_crypto_luks = {
.bdrv_co_create_opts = block_crypto_co_create_opts_luks,
.bdrv_co_truncate = block_crypto_co_truncate,
.create_opts = &block_crypto_create_opts_luks,
+ .runtime_opts = &block_crypto_runtime_opts_luks,
.bdrv_reopen_prepare = block_crypto_reopen_prepare,
.bdrv_refresh_limits = block_crypto_refresh_limits,
diff --git a/block/file-posix.c b/block/file-posix.c
index 8aee7a3fb8..d08233ece3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -443,6 +443,8 @@ static QemuOptsList raw_runtime_opts = {
},
};
+static const char *const mutable_opts[] = { "x-check-cache-dropped", NULL };
+
static int raw_open_common(BlockDriverState *bs, QDict *options,
int bdrv_flags, int open_flags,
bool device, Error **errp)
@@ -2743,6 +2745,8 @@ BlockDriver bdrv_file = {
.format_name = "file",
.protocol_name = "file",
.instance_size = sizeof(BDRVRawState),
+ .runtime_opts = &raw_runtime_opts,
+ .mutable_opts = mutable_opts,
.bdrv_needs_filename = true,
.bdrv_probe = NULL, /* no probe for protocols */
.bdrv_parse_filename = raw_parse_filename,
@@ -3220,6 +3224,8 @@ static BlockDriver bdrv_host_device = {
.format_name = "host_device",
.protocol_name = "host_device",
.instance_size = sizeof(BDRVRawState),
+ .runtime_opts = &raw_runtime_opts,
+ .mutable_opts = mutable_opts,
.bdrv_needs_filename = true,
.bdrv_probe_device = hdev_probe_device,
.bdrv_parse_filename = hdev_parse_filename,
@@ -3346,6 +3352,8 @@ static BlockDriver bdrv_host_cdrom = {
.format_name = "host_cdrom",
.protocol_name = "host_cdrom",
.instance_size = sizeof(BDRVRawState),
+ .runtime_opts = &raw_runtime_opts,
+ .mutable_opts = mutable_opts,
.bdrv_needs_filename = true,
.bdrv_probe_device = cdrom_probe_device,
.bdrv_parse_filename = cdrom_parse_filename,
@@ -3479,6 +3487,8 @@ static BlockDriver bdrv_host_cdrom = {
.format_name = "host_cdrom",
.protocol_name = "host_cdrom",
.instance_size = sizeof(BDRVRawState),
+ .runtime_opts = &raw_runtime_opts,
+ .mutable_opts = mutable_opts,
.bdrv_needs_filename = true,
.bdrv_probe_device = cdrom_probe_device,
.bdrv_parse_filename = cdrom_parse_filename,
diff --git a/block/iscsi.c b/block/iscsi.c
index a7e8c1ffaf..70f07b2105 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2456,6 +2456,7 @@ static BlockDriver bdrv_iscsi = {
.bdrv_close = iscsi_close,
.bdrv_co_create_opts = iscsi_co_create_opts,
.create_opts = &iscsi_create_opts,
+ .runtime_opts = &runtime_opts,
.bdrv_reopen_prepare = iscsi_reopen_prepare,
.bdrv_reopen_commit = iscsi_reopen_commit,
.bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
@@ -2493,6 +2494,7 @@ static BlockDriver bdrv_iser = {
.bdrv_close = iscsi_close,
.bdrv_co_create_opts = iscsi_co_create_opts,
.create_opts = &iscsi_create_opts,
+ .runtime_opts = &runtime_opts,
.bdrv_reopen_prepare = iscsi_reopen_prepare,
.bdrv_reopen_commit = iscsi_reopen_commit,
.bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
diff --git a/block/null.c b/block/null.c
index d442d3e901..5df4b46723 100644
--- a/block/null.c
+++ b/block/null.c
@@ -256,6 +256,7 @@ static BlockDriver bdrv_null_co = {
.format_name = "null-co",
.protocol_name = "null-co",
.instance_size = sizeof(BDRVNullState),
+ .runtime_opts = &runtime_opts,
.bdrv_file_open = null_file_open,
.bdrv_parse_filename = null_co_parse_filename,
@@ -275,6 +276,7 @@ static BlockDriver bdrv_null_aio = {
.format_name = "null-aio",
.protocol_name = "null-aio",
.instance_size = sizeof(BDRVNullState),
+ .runtime_opts = &runtime_opts,
.bdrv_file_open = null_file_open,
.bdrv_parse_filename = null_aio_parse_filename,
diff --git a/block/nvme.c b/block/nvme.c
index 982097b5b1..7c71fba057 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1139,6 +1139,7 @@ static BlockDriver bdrv_nvme = {
.format_name = "nvme",
.protocol_name = "nvme",
.instance_size = sizeof(BDRVNVMeState),
+ .runtime_opts = &runtime_opts,
.bdrv_parse_filename = nvme_parse_filename,
.bdrv_file_open = nvme_file_open,
diff --git a/block/qcow.c b/block/qcow.c
index 0a235bf393..bffece818a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -1186,6 +1186,7 @@ static QemuOptsList qcow_create_opts = {
static BlockDriver bdrv_qcow = {
.format_name = "qcow",
.instance_size = sizeof(BDRVQcowState),
+ .runtime_opts = &qcow_runtime_opts,
.bdrv_probe = qcow_probe,
.bdrv_open = qcow_open,
.bdrv_close = qcow_close,
diff --git a/block/rbd.c b/block/rbd.c
index 8a1a9f4b6e..6de6112ce8 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1231,6 +1231,7 @@ static QemuOptsList qemu_rbd_create_opts = {
static BlockDriver bdrv_rbd = {
.format_name = "rbd",
.instance_size = sizeof(BDRVRBDState),
+ .runtime_opts = &runtime_opts,
.bdrv_parse_filename = qemu_rbd_parse_filename,
.bdrv_refresh_limits = qemu_rbd_refresh_limits,
.bdrv_file_open = qemu_rbd_open,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 90ab43baa4..6dd66d0b99 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3288,6 +3288,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .runtime_opts = &runtime_opts,
};
static BlockDriver bdrv_sheepdog_unix = {
@@ -3325,6 +3326,7 @@ static BlockDriver bdrv_sheepdog_unix = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .runtime_opts = &runtime_opts,
};
static void bdrv_sheepdog_init(void)
diff --git a/block/vpc.c b/block/vpc.c
index d886465b7e..640c956d0d 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1235,6 +1235,7 @@ static BlockDriver bdrv_vpc = {
.bdrv_get_info = vpc_get_info,
.create_opts = &vpc_create_opts,
+ .runtime_opts = &vpc_runtime_opts,
.bdrv_has_zero_init = vpc_has_zero_init,
};
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fd0e88d17a..e680dda86b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -345,6 +345,13 @@ struct BlockDriver {
/* List of options for creating images, terminated by name == NULL */
QemuOptsList *create_opts;
+ /* Runtime options for a block device, terminated by name == NULL */
+ QemuOptsList *runtime_opts;
+ /*
+ * Names of the runtime options that can be reset by omitting
+ * their value on reopen, NULL-terminated.
+ */
+ const char *const *mutable_opts;
/*
* Returns 0 for completed check, -errno for internal errors.
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 10/13] block: Add bdrv_reset_options_allowed()
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (8 preceding siblings ...)
2019-01-17 15:34 ` [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver Alberto Garcia
@ 2019-01-17 15:34 ` Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 11/13] block: Remove the AioContext parameter from bdrv_reopen_multiple() Alberto Garcia
` (4 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
bdrv_reopen_prepare() receives a BDRVReopenState with (among other
things) a new set of options to be applied to that BlockDriverState.
If an option is missing then it means that we want to reset it to its
default value rather than keep the previous one. This way the state
of the block device after being reopened is comparable to that of a
device added with "blockdev-add" using the same set of options.
Not all options from all drivers can be changed this way, however.
If the user attempts to reset an immutable option to its default value
using this method then we must forbid it.
This new function takes a QemuOptsList with the options of a block
driver and checks if there's any that was previously set but is
missing from the new set of options in the BDRVReopenState.
If the option is present in both sets we don't need to check that they
have the same value. The loop at the end of bdrv_reopen_prepare()
already takes care of that.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/block.c b/block.c
index 10847416b2..eea7aefa99 100644
--- a/block.c
+++ b/block.c
@@ -2909,6 +2909,43 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
}
/*
+ * For every option in @list, check that if it is set in the current
+ * set of options (@state->bs->options) then it is also set in the new
+ * set (@state->options). Options listed in @mutable_opts are skipped.
+ *
+ * @mutable_opts is either NULL or a NULL-terminated array of option
+ * names.
+ *
+ * Return 0 on success, -EINVAL otherwise.
+ */
+static int bdrv_reset_options_allowed(BDRVReopenState *state,
+ QemuOptsList *list,
+ const char *const mutable_opts[],
+ Error **errp)
+{
+ QemuOptDesc *desc = list->desc;
+ while (desc->name) {
+ unsigned i;
+ for (i = 0; mutable_opts != NULL && mutable_opts[i] != NULL; i++) {
+ if (!strcmp(desc->name, mutable_opts[i])) {
+ goto next;
+ }
+ }
+
+ if (!qdict_haskey(state->options, desc->name) &&
+ qdict_haskey(state->bs->options, desc->name)) {
+ error_setg(errp, "Option '%s' can't be reset to its default value",
+ desc->name);
+ return -EINVAL;
+ }
+ next:
+ desc++;
+ }
+
+ return 0;
+}
+
+/*
* Returns true if @child can be reached recursively from @bs
*/
static bool bdrv_recurse_has_child(BlockDriverState *bs,
@@ -3392,6 +3429,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
}
if (drv->bdrv_reopen_prepare) {
+ /*
+ * If a driver-specific option is missing, it means that we
+ * should reset it to its default value.
+ * But not all options allow that, so we need to check it first.
+ */
+ if (drv->runtime_opts) {
+ ret = bdrv_reset_options_allowed(reopen_state, drv->runtime_opts,
+ drv->mutable_opts, errp);
+ if (ret) {
+ goto error;
+ }
+ }
+
ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
if (ret) {
if (local_err != NULL) {
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 11/13] block: Remove the AioContext parameter from bdrv_reopen_multiple()
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (9 preceding siblings ...)
2019-01-17 15:34 ` [Qemu-devel] [PATCH 10/13] block: Add bdrv_reset_options_allowed() Alberto Garcia
@ 2019-01-17 15:34 ` Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 12/13] block: Add an 'x-blockdev-reopen' QMP command Alberto Garcia
` (3 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
This parameter has been unused since 1a63a907507fbbcfaee3f622907ec244b
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 4 ++--
block/replication.c | 3 +--
include/block/block.h | 2 +-
qemu-io-cmds.c | 2 +-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index eea7aefa99..1eaca35dfc 100644
--- a/block.c
+++ b/block.c
@@ -3168,7 +3168,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
* All affected nodes must be drained between bdrv_reopen_queue() and
* bdrv_reopen_multiple().
*/
-int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp)
+int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
{
int ret = -1;
BlockReopenQueueEntry *bs_entry, *next;
@@ -3221,7 +3221,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
bdrv_subtree_drained_begin(bs);
queue = bdrv_reopen_queue(NULL, bs, opts, true);
- ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
+ ret = bdrv_reopen_multiple(queue, errp);
bdrv_subtree_drained_end(bs);
return ret;
diff --git a/block/replication.c b/block/replication.c
index d35214370e..1c167e0d90 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -385,8 +385,7 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
}
if (reopen_queue) {
- bdrv_reopen_multiple(bdrv_get_aio_context(bs),
- reopen_queue, &local_err);
+ bdrv_reopen_multiple(reopen_queue, &local_err);
error_propagate(errp, local_err);
}
diff --git a/include/block/block.h b/include/block/block.h
index e21616a038..0fa01c3139 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -302,7 +302,7 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs, QDict *options,
bool keep_old_opts);
-int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp);
+int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index db8887205e..416e74b53d 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2076,7 +2076,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
bdrv_subtree_drained_begin(bs);
brq = bdrv_reopen_queue(NULL, bs, opts, true);
- bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
+ bdrv_reopen_multiple(brq, &local_err);
bdrv_subtree_drained_end(bs);
if (local_err) {
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 12/13] block: Add an 'x-blockdev-reopen' QMP command
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (10 preceding siblings ...)
2019-01-17 15:34 ` [Qemu-devel] [PATCH 11/13] block: Remove the AioContext parameter from bdrv_reopen_multiple() Alberto Garcia
@ 2019-01-17 15:34 ` Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 13/13] qemu-iotests: Test the x-blockdev-reopen " Alberto Garcia
` (2 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
This command allows reopening an arbitrary BlockDriverState with a
new set of options. Some options (e.g node-name) cannot be changed
and some block drivers don't allow reopening, but otherwise this
command is modelled after 'blockdev-add' and the state of the reopened
BlockDriverState should generally be the same as if it had just been
added by 'blockdev-add' with the same set of options.
One notable exception is the 'backing' option: 'x-blockdev-reopen'
requires that it is always present unless the BlockDriverState in
question doesn't have a current or default backing file.
This command allows reconfiguring the graph by using the appropriate
options to change the children of a node. At the moment it's possible
to change a backing file by setting the 'backing' option to the name
of the new node, but it should also be possible to add a similar
functionality to other block drivers (e.g. Quorum, blkverify).
Although the API is unlikely to change, this command is marked
experimental for the time being so there's room to see if the
semantics need changes.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index a8fa8748a9..d8831db7e1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4258,6 +4258,53 @@ fail:
visit_free(v);
}
+void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
+{
+ BlockDriverState *bs;
+ AioContext *ctx;
+ QObject *obj;
+ Visitor *v = qobject_output_visitor_new(&obj);
+ Error *local_err = NULL;
+ BlockReopenQueue *queue;
+ QDict *qdict;
+
+ /* Check for the selected node name */
+ if (!options->has_node_name) {
+ error_setg(errp, "Node name not specified");
+ goto fail;
+ }
+
+ bs = bdrv_find_node(options->node_name);
+ if (!bs) {
+ error_setg(errp, "Cannot find node named '%s'", options->node_name);
+ goto fail;
+ }
+
+ /* Put all options in a QDict and flatten it */
+ visit_type_BlockdevOptions(v, NULL, &options, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ visit_complete(v, &obj);
+ qdict = qobject_to(QDict, obj);
+
+ qdict_flatten(qdict);
+
+ /* Perform the reopen operation */
+ ctx = bdrv_get_aio_context(bs);
+ aio_context_acquire(ctx);
+ bdrv_subtree_drained_begin(bs);
+ queue = bdrv_reopen_queue(NULL, bs, qdict, false);
+ bdrv_reopen_multiple(queue, errp);
+ bdrv_subtree_drained_end(bs);
+ aio_context_release(ctx);
+
+fail:
+ visit_free(v);
+}
+
void qmp_blockdev_del(const char *node_name, Error **errp)
{
AioContext *aio_context;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 91685be6c2..1cfaccd500 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3816,6 +3816,48 @@
{ 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
##
+# @x-blockdev-reopen:
+#
+# Reopens a block device using the given set of options. Any option
+# not specified will be reset to its default value regardless of its
+# previous status. If an option cannot be changed or a particular
+# driver does not support reopening then the command will return an
+# error.
+#
+# The top-level @node-name option (from BlockdevOptions) must be
+# specified and is used to select the block device to be reopened.
+# Other @node-name options must be either omitted or set to the
+# current name of the appropriate node. This command won't change any
+# node name and any attempt to do it will result in an error.
+#
+# In the case of options that refer to child nodes, the behavior of
+# this command depends on the value:
+#
+# 1) A set of options (BlockdevOptions): the child is reopened with
+# the specified set of options.
+#
+# 2) A reference to the current child: the child is reopened using
+# its existing set of options.
+#
+# 3) A reference to a different node: the current child is replaced
+# with the specified one.
+#
+# 4) NULL: the current child (if any) is detached.
+#
+# Options (1) and (2) are supported in all cases, but at the moment
+# only @backing allows replacing or detaching an existing child.
+#
+# Unlike with blockdev-add, the @backing option must always be present
+# unless the node being reopened does not have a backing file and its
+# image does not have a default backing file name as part of its
+# metadata.
+#
+# Since: 4.0
+##
+{ 'command': 'x-blockdev-reopen',
+ 'data': 'BlockdevOptions', 'boxed': true }
+
+##
# @blockdev-del:
#
# Deletes a block device that has been added using blockdev-add.
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (11 preceding siblings ...)
2019-01-17 15:34 ` [Qemu-devel] [PATCH 12/13] block: Add an 'x-blockdev-reopen' QMP command Alberto Garcia
@ 2019-01-17 15:34 ` Alberto Garcia
2019-01-17 15:50 ` [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' " Eric Blake
2019-01-31 15:11 ` Alberto Garcia
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 15:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz
This patch adds several tests for the x-blockdev-reopen QMP command.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
tests/qemu-iotests/224 | 1001 ++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/224.out | 5 +
tests/qemu-iotests/group | 1 +
3 files changed, 1007 insertions(+)
create mode 100644 tests/qemu-iotests/224
create mode 100644 tests/qemu-iotests/224.out
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
new file mode 100644
index 0000000000..3a0394dd3d
--- /dev/null
+++ b/tests/qemu-iotests/224
@@ -0,0 +1,1001 @@
+#!/usr/bin/env python
+#
+# Test cases for the QMP 'x-blockdev-reopen' command
+#
+# Copyright (C) 2018 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import re
+import iotests
+import copy
+import json
+from iotests import qemu_img, qemu_io
+
+hd_path = [
+ os.path.join(iotests.test_dir, 'hd0.img'),
+ os.path.join(iotests.test_dir, 'hd1.img'),
+ os.path.join(iotests.test_dir, 'hd2.img')
+]
+
+def hd_opts(idx):
+ return {'driver': iotests.imgfmt,
+ 'node-name': 'hd%d' % idx,
+ 'file': {'driver': 'file',
+ 'node-name': 'hd%d-file' % idx,
+ 'filename': hd_path[idx] } }
+
+class TestBlockdevReopen(iotests.QMPTestCase):
+ total_io_cmds = 0
+
+ def setUp(self):
+ qemu_img('create', '-f', iotests.imgfmt, hd_path[0], '3M')
+ qemu_img('create', '-f', iotests.imgfmt, '-b', hd_path[0], hd_path[1])
+ qemu_img('create', '-f', iotests.imgfmt, hd_path[2], '3M')
+ qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xa0 0 1M', hd_path[0])
+ qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xa1 1M 1M', hd_path[1])
+ qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xa2 2M 1M', hd_path[2])
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ self.check_qemu_io_errors()
+ os.remove(hd_path[0])
+ os.remove(hd_path[1])
+ os.remove(hd_path[2])
+
+ # The output of qemu-io is not returned by vm.hmp_qemu_io() but
+ # it's stored in the log and can only be read when the VM has been
+ # shut down. This function runs qemu-io and keeps track of the
+ # number of times it's been called.
+ def run_qemu_io(self, img, cmd):
+ result = self.vm.hmp_qemu_io(img, cmd)
+ self.assert_qmp(result, 'return', '')
+ self.total_io_cmds += 1
+
+ # Once the VM is shut down we can parse the log and see if qemu-io
+ # ran without errors.
+ def check_qemu_io_errors(self):
+ self.assertFalse(self.vm.is_running())
+ found = 0
+ log = self.vm.get_log()
+ for line in log.split("\n"):
+ if line.startswith("Pattern verification failed"):
+ raise Exception("%s (command #%d)" % (line, found))
+ if re.match("read .*/.* bytes at offset", line):
+ found += 1
+ self.assertEqual(found, self.total_io_cmds,
+ "Expected output of %d qemu-io commands, found %d" %
+ (found, self.total_io_cmds))
+
+ # Run x-blockdev-reopen with 'opts' but applying 'newopts'
+ # on top of it. The original 'opts' dict is unmodified
+ def reopen(self, opts, newopts = {}, errmsg = None):
+ opts = copy.deepcopy(opts)
+
+ # Apply the changes from 'newopts' on top of 'opts'
+ for key in newopts:
+ value = newopts[key]
+ # If key has the form "foo.bar" then we need to do
+ # opts["foo"]["bar"] = value, not opts["foo.bar"] = value
+ subdict = opts
+ while key.find('.') != -1:
+ [prefix, key] = key.split('.', 1)
+ subdict = opts[prefix]
+ subdict[key] = value
+
+ result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, **opts)
+ if errmsg:
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', errmsg)
+ else:
+ self.assert_qmp(result, 'return', {})
+
+
+ # Run query-named-block-nodes and return the specified entry
+ def get_node(self, node_name):
+ result = self.vm.qmp('query-named-block-nodes')
+ for node in result['return']:
+ if node['node-name'] == node_name:
+ return node
+ return None
+
+ # Run 'query-named-block-nodes' and compare its output with the
+ # value passed by the user in 'graph'
+ def check_node_graph(self, graph):
+ result = self.vm.qmp('query-named-block-nodes')
+ self.assertEqual(json.dumps(graph, sort_keys=True),
+ json.dumps(result, sort_keys=True))
+
+ # This test opens one single disk image (without backing files)
+ # and tries to reopen it with illegal / incorrect parameters.
+ def test_incorrect_parameters_single_file(self):
+ # Open 'hd0' only (no backing files)
+ opts = hd_opts(0)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+ original_graph = self.vm.qmp('query-named-block-nodes')
+
+ # We can reopen the image passing the same options
+ self.reopen(opts)
+
+ # We can also reopen passing a child reference in 'file'
+ self.reopen(opts, {'file': 'hd0-file'})
+
+ # We cannot change any of these
+ self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 'not-found'")
+ self.reopen(opts, {'node-name': ''}, "Cannot find node named ''")
+ self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string")
+ self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'")
+ self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
+ self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string")
+ self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'")
+ self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
+ self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
+ self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'")
+ self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'")
+ self.reopen(opts, {'file.filename': hd_path[1]}, "Cannot change the option 'filename'")
+ self.reopen(opts, {'file.aio': 'native'}, "Cannot change the option 'aio'")
+ self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
+ self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'file.filename', expected: string")
+
+ # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
+ del opts['node-name']
+ self.reopen(opts, {}, "Node name not specified")
+
+ # Check that nothing has changed
+ self.check_node_graph(original_graph)
+
+ # Remove the node
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ # This test opens an image with a backing file and tries to reopen
+ # it with illegal / incorrect parameters.
+ def test_incorrect_parameters_backing_file(self):
+ # Open hd1 omitting the backing options (hd0 will be opened
+ # with the default options)
+ opts = hd_opts(1)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+ original_graph = self.vm.qmp('query-named-block-nodes')
+
+ # We can't reopen the image passing the same options, 'backing' is mandatory
+ self.reopen(opts, {}, "backing is missing for 'hd1'")
+
+ # Everything works if we pass 'backing' using the existing node name
+ for node in original_graph['return']:
+ if node['drv'] == iotests.imgfmt and node['file'] == hd_path[0]:
+ backing_node_name = node['node-name']
+ self.reopen(opts, {'backing': backing_node_name})
+
+ # We can't use a non-existing or empty (non-NULL) node as the backing image
+ self.reopen(opts, {'backing': 'not-found'}, "Cannot find device= nor node_name=not-found")
+ self.reopen(opts, {'backing': ''}, "Cannot find device= nor node_name=")
+
+ # We can reopen the image just fine if we specify the backing options
+ opts['backing'] = {'driver': iotests.imgfmt,
+ 'file': {'driver': 'file',
+ 'filename': hd_path[0]}}
+ self.reopen(opts)
+
+ # We cannot change any of these options
+ self.reopen(opts, {'backing.node-name': 'newname'}, "Cannot change the option 'node-name'")
+ self.reopen(opts, {'backing.driver': 'raw'}, "Cannot change the option 'driver'")
+ self.reopen(opts, {'backing.file.node-name': 'newname'}, "Cannot change the option 'node-name'")
+ self.reopen(opts, {'backing.file.driver': 'host_device'}, "Cannot change the option 'driver'")
+
+ # Check that nothing has changed since the beginning
+ self.check_node_graph(original_graph)
+
+ # Remove the node
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd1')
+ self.assert_qmp(result, 'return', {})
+
+ # Reopen an image several times changing some of its options
+ def test_reopen(self):
+ # Open the hd1 image passing all backing options
+ opts = hd_opts(1)
+ opts['backing'] = hd_opts(0)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+ original_graph = self.vm.qmp('query-named-block-nodes')
+
+ # We can reopen the image passing the same options
+ self.reopen(opts)
+
+ # Reopen in read-only mode
+ self.assert_qmp(self.get_node('hd1'), 'ro', False)
+
+ self.reopen(opts, {'read-only': True})
+ self.assert_qmp(self.get_node('hd1'), 'ro', True)
+ self.reopen(opts)
+ self.assert_qmp(self.get_node('hd1'), 'ro', False)
+
+ # Change the cache options
+ self.assert_qmp(self.get_node('hd1'), 'cache/writeback', True)
+ self.assert_qmp(self.get_node('hd1'), 'cache/direct', False)
+ self.assert_qmp(self.get_node('hd1'), 'cache/no-flush', False)
+ self.reopen(opts, {'cache': { 'direct': True, 'no-flush': True }})
+ self.assert_qmp(self.get_node('hd1'), 'cache/writeback', True)
+ self.assert_qmp(self.get_node('hd1'), 'cache/direct', True)
+ self.assert_qmp(self.get_node('hd1'), 'cache/no-flush', True)
+
+ # Reopen again with the original options
+ self.reopen(opts)
+ self.assert_qmp(self.get_node('hd1'), 'cache/writeback', True)
+ self.assert_qmp(self.get_node('hd1'), 'cache/direct', False)
+ self.assert_qmp(self.get_node('hd1'), 'cache/no-flush', False)
+
+ # Change 'detect-zeroes' and 'discard'
+ self.assert_qmp(self.get_node('hd1'), 'detect_zeroes', 'off')
+ self.reopen(opts, {'detect-zeroes': 'on'})
+ self.assert_qmp(self.get_node('hd1'), 'detect_zeroes', 'on')
+ self.reopen(opts, {'detect-zeroes': 'unmap'},
+ "setting detect-zeroes to unmap is not allowed " +
+ "without setting discard operation to unmap")
+ self.assert_qmp(self.get_node('hd1'), 'detect_zeroes', 'on')
+ self.reopen(opts, {'detect-zeroes': 'unmap', 'discard': 'unmap'})
+ self.assert_qmp(self.get_node('hd1'), 'detect_zeroes', 'unmap')
+ self.reopen(opts)
+ self.assert_qmp(self.get_node('hd1'), 'detect_zeroes', 'off')
+
+ # Changing 'force-share' is currently not supported
+ self.reopen(opts, {'force-share': True}, "Cannot change the option 'force-share'")
+
+ # Change some qcow2-specific options
+ # No way to test for success other than checking the return message
+ if iotests.imgfmt == 'qcow2':
+ self.reopen(opts, {'l2-cache-entry-size': 128 * 1024},
+ "L2 cache entry size must be a power of two "+
+ "between 512 and the cluster size (65536)")
+ self.reopen(opts, {'l2-cache-size': 1024 * 1024,
+ 'cache-size': 512 * 1024},
+ "l2-cache-size may not exceed cache-size")
+ self.reopen(opts, {'l2-cache-size': 4 * 1024 * 1024,
+ 'refcount-cache-size': 4 * 1024 * 1024,
+ 'l2-cache-entry-size': 32 * 1024})
+ self.reopen(opts, {'pass-discard-request': True})
+
+ # Check that nothing has changed since the beginning
+ # (from the parameters that we can check)
+ self.check_node_graph(original_graph)
+
+ # Check that the node names (other than the top-level one) are optional
+ del opts['file']['node-name']
+ del opts['backing']['node-name']
+ del opts['backing']['file']['node-name']
+ self.reopen(opts)
+ self.check_node_graph(original_graph)
+
+ # Reopen setting backing = null, this removes the backing image from the chain
+ self.reopen(opts, {'backing': None})
+ self.assert_qmp_absent(self.get_node('hd1'), 'image/backing-image')
+
+ # Open the 'hd0' image
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **hd_opts(0))
+ self.assert_qmp(result, 'return', {})
+
+ # Reopen the hd1 image setting 'hd0' as its backing image
+ self.reopen(opts, {'backing': 'hd0'})
+ self.assert_qmp(self.get_node('hd1'), 'image/backing-image/filename', hd_path[0])
+
+ # Check that nothing has changed since the beginning
+ self.reopen(hd_opts(0), {'read-only': True})
+ self.check_node_graph(original_graph)
+
+ # The backing file (hd0) is now a reference, we cannot change backing.* anymore
+ self.reopen(opts, {}, "Cannot change the option 'backing.driver'")
+
+ # We can't remove 'hd0' while it's a backing image of 'hd1'
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', "Node 'hd0' is busy: node is used as backing hd of 'hd1'")
+
+ # But we can remove both nodes if done in the proper order
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd1')
+ self.assert_qmp(result, 'return', {})
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ # Reopen a raw image and see the effect of changing the 'offset' option
+ def test_reopen_raw(self):
+ opts = {'driver': 'raw', 'node-name': 'hd0',
+ 'file': { 'driver': 'file',
+ 'filename': hd_path[0],
+ 'node-name': 'hd0-file' } }
+
+ # First we create a 2MB raw file, and fill each half with a
+ # different value
+ qemu_img('create', '-f', 'raw', hd_path[0], '2M')
+ qemu_io('-f', 'raw', '-c', 'write -P 0xa0 0 1M', hd_path[0])
+ qemu_io('-f', 'raw', '-c', 'write -P 0xa1 1M 1M', hd_path[0])
+
+ # Open the raw file with QEMU
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # Read 1MB from offset 0
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+
+ # Reopen the image with a 1MB offset.
+ # Now the results are different
+ self.reopen(opts, {'offset': 1024*1024})
+ self.run_qemu_io("hd0", "read -P 0xa1 0 1M")
+
+ # Reopen again with the original options.
+ # We get the original results again
+ self.reopen(opts)
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+
+ # Remove the block device
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ # Omitting an option should reset it to the default value, but if
+ # an option cannot be changed it shouldn't be possible to reset it
+ # to its default value either
+ def test_reset_default_values(self):
+ opts = {'driver': 'qcow2', 'node-name': 'hd0',
+ 'file': { 'driver': 'file',
+ 'filename': hd_path[0],
+ 'x-check-cache-dropped': True, # This one can be changed
+ 'locking': 'off', # This one can NOT be changed
+ 'node-name': 'hd0-file' } }
+
+ # Open the file with QEMU
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # file.x-check-cache-dropped can be changed...
+ self.reopen(opts, { 'file.x-check-cache-dropped': False })
+ # ...and dropped completely (resetting to the default value)
+ del opts['file']['x-check-cache-dropped']
+ self.reopen(opts)
+
+ # file.locking cannot be changed nor reset to the default value
+ self.reopen(opts, { 'file.locking': 'on' }, "Cannot change the option 'locking'")
+ del opts['file']['locking']
+ self.reopen(opts, {}, "Option 'locking' can't be reset to its default value")
+ # But we can reopen it if we maintain its previous value
+ self.reopen(opts, { 'file.locking': 'off' })
+
+ # Remove the block device
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ # This test modifies the node graph a few times by changing the
+ # 'backing' option on reopen and verifies that the guest data that
+ # is read afterwards is consistent with the graph changes.
+ def test_io_with_graph_changes(self):
+ opts = []
+
+ # Open hd0, hd1 and hd2 without any backing image
+ for i in range(3):
+ opts.append(hd_opts(i))
+ opts[i]['backing'] = None
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts[i])
+ self.assert_qmp(result, 'return', {})
+
+ # hd0
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+ self.run_qemu_io("hd0", "read -P 0 1M 1M")
+ self.run_qemu_io("hd0", "read -P 0 2M 1M")
+
+ # hd1 <- hd0
+ self.reopen(opts[0], {'backing': 'hd1'})
+
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+ self.run_qemu_io("hd0", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd0", "read -P 0 2M 1M")
+
+ # hd1 <- hd0 , hd1 <- hd2
+ self.reopen(opts[2], {'backing': 'hd1'})
+
+ self.run_qemu_io("hd2", "read -P 0 0 1M")
+ self.run_qemu_io("hd2", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd2", "read -P 0xa2 2M 1M")
+
+ # hd1 <- hd2 <- hd0
+ self.reopen(opts[0], {'backing': 'hd2'})
+
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+ self.run_qemu_io("hd0", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd0", "read -P 0xa2 2M 1M")
+
+ # hd2 <- hd0
+ self.reopen(opts[2], {'backing': None})
+
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+ self.run_qemu_io("hd0", "read -P 0 1M 1M")
+ self.run_qemu_io("hd0", "read -P 0xa2 2M 1M")
+
+ # hd2 <- hd1 <- hd0
+ self.reopen(opts[1], {'backing': 'hd2'})
+ self.reopen(opts[0], {'backing': 'hd1'})
+
+ self.run_qemu_io("hd0", "read -P 0xa0 0 1M")
+ self.run_qemu_io("hd0", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd0", "read -P 0xa2 2M 1M")
+
+ # Illegal operation: hd2 is a child of hd1
+ self.reopen(opts[2], {'backing': 'hd1'},
+ "Making 'hd1' a backing file of 'hd2' would create a cycle")
+
+ # hd2 <- hd0, hd2 <- hd1
+ self.reopen(opts[0], {'backing': 'hd2'})
+
+ self.run_qemu_io("hd1", "read -P 0 0 1M")
+ self.run_qemu_io("hd1", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd1", "read -P 0xa2 2M 1M")
+
+ # More illegal operations
+ self.reopen(opts[2], {'backing': 'hd1'},
+ "Making 'hd1' a backing file of 'hd2' would create a cycle")
+ self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'")
+
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', "Node 'hd2' is busy: node is used as backing hd of 'hd0'")
+
+ # hd1 doesn't have a backing file now
+ self.reopen(opts[1], {'backing': None})
+
+ self.run_qemu_io("hd1", "read -P 0 0 1M")
+ self.run_qemu_io("hd1", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd1", "read -P 0 2M 1M")
+
+ # We can't remove the 'backing' option if the image has a
+ # default backing file
+ del opts[1]['backing']
+ self.reopen(opts[1], {}, "backing is missing for 'hd1'")
+
+ self.run_qemu_io("hd1", "read -P 0 0 1M")
+ self.run_qemu_io("hd1", "read -P 0xa1 1M 1M")
+ self.run_qemu_io("hd1", "read -P 0 2M 1M")
+
+ # This test verifies that we can't change the children of a block
+ # device during a reopen operation in a way that would create
+ # cycles in the node graph
+ def test_graph_cycles(self):
+ opts = []
+
+ # Open all three images without backing file
+ for i in range(3):
+ opts.append(hd_opts(i))
+ opts[i]['backing'] = None
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts[i])
+ self.assert_qmp(result, 'return', {})
+
+ # hd1 <- hd0, hd1 <- hd2
+ self.reopen(opts[0], {'backing': 'hd1'})
+ self.reopen(opts[2], {'backing': 'hd1'})
+
+ # Illegal: hd2 is backed by hd1
+ self.reopen(opts[1], {'backing': 'hd2'},
+ "Making 'hd2' a backing file of 'hd1' would create a cycle")
+
+ # hd1 <- hd0 <- hd2
+ self.reopen(opts[2], {'backing': 'hd0'})
+
+ # Illegal: hd2 is backed by hd0, which is backed by hd1
+ self.reopen(opts[1], {'backing': 'hd2'},
+ "Making 'hd2' a backing file of 'hd1' would create a cycle")
+
+ # Illegal: hd1 cannot point to itself
+ self.reopen(opts[1], {'backing': 'hd1'},
+ "Making 'hd1' a backing file of 'hd1' would create a cycle")
+
+ # Remove all backing files
+ self.reopen(opts[0])
+ self.reopen(opts[2])
+
+ ##########################################
+ # Add a blkverify node using hd0 and hd1 #
+ ##########################################
+ bvopts = {'driver': 'blkverify',
+ 'node-name': 'bv',
+ 'test': 'hd0',
+ 'raw': 'hd1'}
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **bvopts)
+ self.assert_qmp(result, 'return', {})
+
+ # blkverify doesn't currently allow reopening. TODO: implement this
+ self.reopen(bvopts, {}, "Block format 'blkverify' used by node 'bv'" +
+ " does not support reopening files")
+
+ # Illegal: hd0 is a child of the blkverify node
+ self.reopen(opts[0], {'backing': 'bv'},
+ "Making 'bv' a backing file of 'hd0' would create a cycle")
+
+ # Delete the blkverify node
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv')
+ self.assert_qmp(result, 'return', {})
+
+ # Misc reopen tests with different block drivers
+ def test_misc_drivers(self):
+ ####################
+ ###### quorum ######
+ ####################
+ for i in range(3):
+ opts = hd_opts(i)
+ # Open all three images without backing file
+ opts['backing'] = None
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ opts = {'driver': 'quorum',
+ 'node-name': 'quorum0',
+ 'children': ['hd0', 'hd1', 'hd2'],
+ 'vote-threshold': 2}
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # Quorum doesn't currently allow reopening. TODO: implement this
+ self.reopen(opts, {}, "Block format 'quorum' used by node 'quorum0'" +
+ " does not support reopening files")
+
+ # You can't make quorum0 a backing file of hd0:
+ # hd0 is already a child of quorum0.
+ self.reopen(hd_opts(0), {'backing': 'quorum0'},
+ "Making 'quorum0' a backing file of 'hd0' would create a cycle")
+
+ # Delete quorum0
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'quorum0')
+ self.assert_qmp(result, 'return', {})
+
+ # Delete hd0, hd1 and hd2
+ for i in range(3):
+ result = self.vm.qmp('blockdev-del', conv_keys = True,
+ node_name = 'hd%d' % i)
+ self.assert_qmp(result, 'return', {})
+
+ ######################
+ ###### blkdebug ######
+ ######################
+ opts = {'driver': 'blkdebug',
+ 'node-name': 'bd',
+ 'config': '/dev/null',
+ 'image': hd_opts(0)}
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # blkdebug allows reopening if we keep the same options
+ self.reopen(opts)
+
+ # but it currently does not allow changes
+ self.reopen(opts, {'image': 'hd1'}, "Cannot change the option 'image'")
+ self.reopen(opts, {'align': 33554432}, "Cannot change the option 'align'")
+ self.reopen(opts, {'config': '/non/existent'}, "Cannot change the option 'config'")
+ del opts['config']
+ self.reopen(opts, {}, "Option 'config' can't be reset to its default value")
+
+ # Delete the blkdebug node
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bd')
+ self.assert_qmp(result, 'return', {})
+
+ ##################
+ ###### null ######
+ ##################
+ opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
+
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # 1 << 30 is the default value, but we cannot change it explicitly
+ self.reopen(opts, {'size': (1 << 30)}, "Cannot change the option 'size'")
+
+ # We cannot change 'size' back to its default value either
+ del opts['size']
+ self.reopen(opts, {}, "Option 'size' can't be reset to its default value")
+
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'root')
+ self.assert_qmp(result, 'return', {})
+
+ ##################
+ ###### file ######
+ ##################
+ opts = hd_opts(0)
+ opts['file']['locking'] = 'on'
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # 'locking' cannot be changed
+ del opts['file']['locking']
+ self.reopen(opts, {'file.locking': 'on'})
+ self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
+ self.reopen(opts, {}, "Option 'locking' can't be reset to its default value")
+
+ # Trying to reopen the 'file' node directly does not make a difference
+ opts = opts['file']
+ self.reopen(opts, {'locking': 'on'})
+ self.reopen(opts, {'locking': 'off'}, "Cannot change the option 'locking'")
+ self.reopen(opts, {}, "Option 'locking' can't be reset to its default value")
+
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ ######################
+ ###### throttle ######
+ ######################
+ opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+ 'props': { 'limits': { 'iops-total': 1000 } } }
+ result = self.vm.qmp('object-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ opts = { 'qom-type': 'throttle-group', 'id': 'group1',
+ 'props': { 'limits': { 'iops-total': 2000 } } }
+ result = self.vm.qmp('object-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # Add a throttle filter with group = group0
+ opts = { 'driver': 'throttle', 'node-name': 'throttle0',
+ 'throttle-group': 'group0', 'file': hd_opts(0) }
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # We can reopen it if we keep the same options
+ self.reopen(opts)
+
+ # We can also reopen if 'file' is a reference to the child
+ self.reopen(opts, {'file': 'hd0'})
+
+ # This is illegal
+ self.reopen(opts, {'throttle-group': 'notfound'}, "Throttle group 'notfound' does not exist")
+
+ # But it's possible to change the group to group1
+ self.reopen(opts, {'throttle-group': 'group1'})
+
+ # Now group1 is in use, it cannot be deleted
+ result = self.vm.qmp('object-del', id = 'group1')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', "object 'group1' is in use, can not be deleted")
+
+ # Default options, this switches the group back to group0
+ self.reopen(opts)
+
+ # So now we cannot delete group0
+ result = self.vm.qmp('object-del', id = 'group0')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', "object 'group0' is in use, can not be deleted")
+
+ # But group1 is free this time, and it can be deleted
+ result = self.vm.qmp('object-del', id = 'group1')
+ self.assert_qmp(result, 'return', {})
+
+ # Let's delete the filter node
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'throttle0')
+ self.assert_qmp(result, 'return', {})
+
+ # And we can finally get rid of group0
+ result = self.vm.qmp('object-del', id = 'group0')
+ self.assert_qmp(result, 'return', {})
+
+ # If an image has a backing file then the 'backing' option must be
+ # passed on reopen. We don't allow leaving the option out in this
+ # case because it's unclear what the correct semantics would be.
+ def test_missing_backing_options_1(self):
+ # hd2
+ opts = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # hd0
+ opts = hd_opts(0)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # hd0 has no backing file: we can omit the 'backing' option
+ self.reopen(opts)
+
+ # hd2 <- hd0
+ self.reopen(opts, {'backing': 'hd2'})
+
+ # hd0 has a backing file: we must set the 'backing' option
+ self.reopen(opts, {}, "backing is missing for 'hd0'")
+
+ # hd2 can't be removed because it's the backing file of hd0
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_qmp(result, 'error/desc', "Node 'hd2' is busy: node is used as backing hd of 'hd0'")
+
+ # Detach hd2 from hd0.
+ self.reopen(opts, {'backing': None})
+ self.reopen(opts, {}, "backing is missing for 'hd0'")
+
+ # Remove both hd0 and hd2
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2')
+ self.assert_qmp(result, 'return', {})
+
+ # If an image has default backing file (as part of its metadata)
+ # then the 'backing' option must be passed on reopen. We don't
+ # allow leaving the option out in this case because it's unclear
+ # what the correct semantics would be.
+ def test_missing_backing_options_2(self):
+ # hd0 <- hd1
+ # (hd0 is hd1's default backing file)
+ opts = hd_opts(1)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # hd1 has a backing file: we can't omit the 'backing' option
+ self.reopen(opts, {}, "backing is missing for 'hd1'")
+
+ # Let's detach the backing file
+ self.reopen(opts, {'backing': None})
+
+ # No backing file attached to hd1 now, but we still can't omit the 'backing' option
+ self.reopen(opts, {}, "backing is missing for 'hd1'")
+
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd1')
+ self.assert_qmp(result, 'return', {})
+
+ # Test that making 'backing' a reference to an existing child
+ # keeps its current options
+ def test_backing_reference(self):
+ # hd2 <- hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = hd_opts(2)
+ # Enable 'detect-zeroes' on all three nodes
+ opts['detect-zeroes'] = 'on'
+ opts['backing']['detect-zeroes'] = 'on'
+ opts['backing']['backing']['detect-zeroes'] = 'on'
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # Reopen the chain passing the minimum amount of required options.
+ # By making 'backing' a reference to hd1 (instead of a sub-dict)
+ # we tell QEMU to keep its current set of options.
+ opts = {'driver': iotests.imgfmt,
+ 'node-name': 'hd0',
+ 'file': 'hd0-file',
+ 'backing': 'hd1' }
+ self.reopen(opts)
+
+ # This has reset 'detect-zeroes' on hd0, but not on hd1 and hd2.
+ self.assert_qmp(self.get_node('hd0'), 'detect_zeroes', 'off')
+ self.assert_qmp(self.get_node('hd1'), 'detect_zeroes', 'on')
+ self.assert_qmp(self.get_node('hd2'), 'detect_zeroes', 'on')
+
+ # Test what happens if the graph changes due to other operations
+ # such as block-stream
+ def test_block_stream_1(self):
+ # hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = None
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # Stream hd1 into hd0 and wait until it's done
+ result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0', device = 'hd0')
+ self.assert_qmp(result, 'return', {})
+ self.wait_until_completed(drive = 'stream0')
+
+ # Now we have only hd0
+ self.assertEqual(self.get_node('hd1'), None)
+
+ # We have backing.* options but there's no backing file anymore
+ self.reopen(opts, {}, "Cannot change the option 'backing.driver'")
+
+ # If we remove the 'backing' option then we can reopen hd0 just fine
+ del opts['backing']
+ self.reopen(opts)
+
+ # We can also reopen hd0 if we set 'backing' to null
+ self.reopen(opts, {'backing': None})
+
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+
+ # Another block_stream test
+ def test_block_stream_2(self):
+ # hd2 <- hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # Stream hd1 into hd0 and wait until it's done
+ result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0',
+ device = 'hd0', base_node = 'hd2')
+ self.assert_qmp(result, 'return', {})
+ self.wait_until_completed(drive = 'stream0')
+
+ # The chain is hd2 <- hd0 now. hd1 is missing
+ self.assertEqual(self.get_node('hd1'), None)
+
+ # The backing options in the dict were meant for hd1, but we cannot
+ # use them with hd2 because hd1 had a backing file while hd2 does not.
+ self.reopen(opts, {}, "Cannot change the option 'backing.driver'")
+
+ # If we remove hd1's options from the dict then things work fine
+ opts['backing'] = opts['backing']['backing']
+ self.reopen(opts)
+
+ # We can also reopen hd0 if we use a reference to the backing file
+ self.reopen(opts, {'backing': 'hd2'})
+
+ # But we cannot leave the option out
+ del opts['backing']
+ self.reopen(opts, {}, "backing is missing for 'hd0'")
+
+ # Now we can delete hd0 (and hd2)
+ result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd0')
+ self.assert_qmp(result, 'return', {})
+ self.assertEqual(self.get_node('hd2'), None)
+
+ # Reopen the chain during a block-stream job (from hd1 to hd0)
+ def test_block_stream_3(self):
+ # hd2 <- hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # hd2 <- hd0
+ result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0',
+ device = 'hd0', base_node = 'hd2', speed = 512 * 1024)
+ self.assert_qmp(result, 'return', {})
+
+ # We can't remove hd2 while the stream job is ongoing
+ opts['backing']['backing'] = None
+ self.reopen(opts, {}, "Cannot remove link from 'hd1' to 'hd2'")
+
+ # We can't remove hd1 while the stream job is ongoing
+ opts['backing'] = None
+ self.reopen(opts, {}, "Cannot remove link from 'hd0' to 'hd1'")
+
+ self.wait_until_completed(drive = 'stream0')
+
+ # Reopen the chain during a block-stream job (from hd2 to hd1)
+ def test_block_stream_4(self):
+ # hd2 <- hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ # hd1 <- hd0
+ result = self.vm.qmp('block-stream', conv_keys = True, job_id = 'stream0',
+ device = 'hd1', speed = 512 * 1024)
+ self.assert_qmp(result, 'return', {})
+
+ # We can't reopen with the original options because that would
+ # make hd1 read-only and block-stream requires it to be read-write
+ self.reopen(opts, {}, "Block node is read-only")
+
+ # We can't remove hd2 while the stream job is ongoing
+ opts['backing']['backing'] = None
+ self.reopen(opts, {'backing.read-only': False}, "Cannot remove link from 'hd1' to 'hd2'")
+
+ # We can detach hd1 from hd0 because it doesn't affect the stream job
+ opts['backing'] = None
+ self.reopen(opts)
+
+ self.wait_until_completed(drive = 'stream0')
+
+ # Reopen the chain during a block-commit job (from hd0 to hd2)
+ def test_block_commit_1(self):
+ # hd2 <- hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('block-commit', conv_keys = True, job_id = 'commit0',
+ device = 'hd0', speed = 1024 * 1024)
+ self.assert_qmp(result, 'return', {})
+
+ # We can't remove hd2 while the commit job is ongoing
+ opts['backing']['backing'] = None
+ self.reopen(opts, {}, "Cannot remove link from 'hd1' to 'hd2'")
+
+ # We can't remove hd1 while the commit job is ongoing
+ opts['backing'] = None
+ self.reopen(opts, {}, "Cannot remove link from 'hd0' to 'hd1'")
+
+ event = self.vm.event_wait(name='BLOCK_JOB_READY')
+ self.assert_qmp(event, 'data/device', 'commit0')
+ self.assert_qmp(event, 'data/type', 'commit')
+ self.assert_qmp_absent(event, 'data/error')
+
+ result = self.vm.qmp('block-job-complete', device='commit0')
+ self.assert_qmp(result, 'return', {})
+
+ self.wait_until_completed(drive = 'commit0')
+
+ # Reopen the chain during a block-commit job (from hd1 to hd2)
+ def test_block_commit_2(self):
+ # hd2 <- hd1 <- hd0
+ opts = hd_opts(0)
+ opts['backing'] = hd_opts(1)
+ opts['backing']['backing'] = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('block-commit', conv_keys = True, job_id = 'commit0',
+ device = 'hd0', top_node = 'hd1', speed = 1024 * 1024)
+ self.assert_qmp(result, 'return', {})
+
+ # The commit job inserts a temporary node between hd0 and hd1.
+ # We need its node name in order to check the error messages.
+ commit_top_name = None
+ result = self.vm.qmp('query-named-block-nodes')
+ for node in result['return']:
+ if node['drv'] == 'commit_top':
+ commit_top_name = node['node-name']
+ self.assertIsNotNone(commit_top_name)
+
+ # We can't remove hd2 while the commit job is ongoing
+ opts['backing']['backing'] = None
+ self.reopen(opts, {}, "Cannot change the option 'backing.driver'")
+
+ # We can't remove hd1 while the commit job is ongoing
+ opts['backing'] = None
+ self.reopen(opts, {}, "Cannot remove link from '%s' to 'hd1'" % commit_top_name)
+
+ # hd2 <- hd0
+ self.wait_until_completed(drive = 'commit0')
+
+ self.assert_qmp(self.get_node('hd0'), 'ro', False)
+ self.assertEqual(self.get_node(commit_top_name), None)
+ self.assertEqual(self.get_node('hd1'), None)
+ self.assert_qmp(self.get_node('hd2'), 'ro', True)
+
+ # We don't allow setting a backing file that uses a different AioContext
+ def test_iothreads(self):
+ opts = hd_opts(0)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+ self.assert_qmp(result, 'return', {})
+
+ opts2 = hd_opts(2)
+ result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('object-add', qom_type='iothread', id='iothread0')
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('object-add', qom_type='iothread', id='iothread1')
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('x-blockdev-set-iothread', node_name='hd0', iothread='iothread0')
+ self.assert_qmp(result, 'return', {})
+
+ self.reopen(opts, {'backing': 'hd2'}, "Cannot use a new backing file with a different AioContext")
+
+ result = self.vm.qmp('x-blockdev-set-iothread', node_name='hd2', iothread='iothread1')
+ self.assert_qmp(result, 'return', {})
+
+ self.reopen(opts, {'backing': 'hd2'}, "Cannot use a new backing file with a different AioContext")
+
+ result = self.vm.qmp('x-blockdev-set-iothread', node_name='hd2', iothread='iothread0')
+ self.assert_qmp(result, 'return', {})
+
+ self.reopen(opts, {'backing': 'hd2'})
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/224.out b/tests/qemu-iotests/224.out
new file mode 100644
index 0000000000..71009c239f
--- /dev/null
+++ b/tests/qemu-iotests/224.out
@@ -0,0 +1,5 @@
+..................
+----------------------------------------------------------------------
+Ran 18 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index f6b245917a..ba75cb9a66 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -224,6 +224,7 @@
221 rw auto quick
222 rw auto quick
223 rw auto quick
+224 rw auto quick
225 rw auto quick
226 auto quick
227 auto quick
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (12 preceding siblings ...)
2019-01-17 15:34 ` [Qemu-devel] [PATCH 13/13] qemu-iotests: Test the x-blockdev-reopen " Alberto Garcia
@ 2019-01-17 15:50 ` Eric Blake
2019-01-17 16:05 ` Alberto Garcia
2019-01-22 8:15 ` Vladimir Sementsov-Ogievskiy
2019-01-31 15:11 ` Alberto Garcia
14 siblings, 2 replies; 43+ messages in thread
From: Eric Blake @ 2019-01-17 15:50 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 4660 bytes --]
On 1/17/19 9:33 AM, Alberto Garcia wrote:
>
> Changing options
> ================
> The general idea is quite straightforward, but the devil is in the
> details. Since this command tries to mimic blockdev-add, the state of
> the block device after being reopened should generally be equivalent
> to that of a block device added with the same set of options.
>
> There are two important things with this:
>
> a) Some options cannot be changed (most drivers don't allow that, in
> fact).
> b) If an option is missing, it should be reset to its default value
> (rather than keeping its previous value).
Could we use QAPI alternate types where we could state that:
"option":"new" - set the option
"option":null - reset the option to its default
omit option - leave the option unchanged
basically, making each of the options an Alternate with JSON null, so
that a request to reset to the default becomes an explicit action?
>
> Example: the default value of l2-cache-size is 1MB. If we set a
> different value (2MB) on blockdev-add but then omit the option in
> x-blockdev-reopen, then it should be reset back to 1MB. The current
> bdrv_reopen() code keeps the previous value.
>
> Trying to change an option that doesn't allow it is already being
> handled by the code. The loop at the end of bdrv_reopen_prepare()
> checks all options that were not handled by the block driver and sees
> if any of them has been modified.
>
> However, as I explained earlier in (b), omitting an option is supposed
> to reset it to its default value, so it's also a way of changing
> it. If the option cannot be changed then we should detect it and
> return an error. What I'm doing in this series is making every driver
> publish its list of run-time options, and which ones of them can be
> modified.
>
> Backing files
> =============
> This command allows reconfigurations in the node graph. Currently it
> only allows changing an image's backing file, but it should be
> possible to implement similar functionalities in drivers that have
> other children, like blkverify or quorum.
>
> Let's add an image with its backing file (hd1 <- hd0) like this:
>
> { "execute": "blockdev-add",
> "arguments": {
> "driver": "qcow2",
> "node-name": "hd0",
> "file": {
> "driver": "file",
> "filename": "hd0.qcow2",
> "node-name": "hd0f"
> },
> "backing": {
> "driver": "qcow2",
> "node-name": "hd1",
> "file": {
> "driver": "file",
> "filename": "hd1.qcow2",
> "node-name": "hd1f"
> }
> }
> }
> }
>
> Removing the backing file can be done by simply passing the option {
> ..., "backing": null } to x-blockdev-reopen.
>
> Replacing it is not so straightforward. If we pass something like {
> ..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen will
> assume that we're trying to change the options of the existing backing
> file (and it will fail in most cases because it'll likely think that
> we're trying to change the node name, among other options).
>
> So I decided to use a reference instead: { ..., "backing": "hd2" },
> where "hd2" is of an existing node that has been added previously.
>
> Leaving out the "backing" option can be ambiguous on some cases (what
> should it do? keep the current backing file? open the default one
> specified in the image file?) so x-blockdev-reopen requires that this
> option is present. For convenience, if the BDS doesn't have a backing
> file then we allow leaving the option out.
Hmm - that makes my proposal of "option":null as an explicit request to
the default a bit trickier, if we are already using null for a different
meaning for backing files.
>
> As you can see in the patch series, I wrote a relatively large amount
> of tests for many different scenarios, including some tricky ones.
>
> Perhaps the part that I'm still not convinced about is the one about
> freezing backing links to prevent them from being removed, but the
> implementation was quite simple so I decided to give it a go. As
> you'll see in the patches I chose to use a bool instead of a counter
> because I couldn't think of a case where it would make sense to have
> two users freezing the same backing link.
>
> Thanks,
>
> Berto
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
2019-01-17 15:50 ` [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' " Eric Blake
@ 2019-01-17 16:05 ` Alberto Garcia
2019-01-22 8:15 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-17 16:05 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On Thu 17 Jan 2019 04:50:20 PM CET, Eric Blake wrote:
>> Removing the backing file can be done by simply passing the option {
>> ..., "backing": null } to x-blockdev-reopen.
>>
> Hmm - that makes my proposal of "option":null as an explicit request
> to the default a bit trickier, if we are already using null for a
> different meaning for backing files.
At the moment 'backing' seems to be the only option that allows a null
value (BlockdevRefOrNull in particular). I suppose there could be more
in the future?
Also, having 'null' meaning 'reset to default' would make this API
differ from blockdev-add, which I'm trying not to do.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
2019-01-17 15:50 ` [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' " Eric Blake
2019-01-17 16:05 ` Alberto Garcia
@ 2019-01-22 8:15 ` Vladimir Sementsov-Ogievskiy
2019-01-23 15:56 ` Alberto Garcia
1 sibling, 1 reply; 43+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-01-22 8:15 UTC (permalink / raw)
To: Eric Blake, Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
17.01.2019 18:50, Eric Blake wrote:
> On 1/17/19 9:33 AM, Alberto Garcia wrote:
>
>>
>> Changing options
>> ================
>> The general idea is quite straightforward, but the devil is in the
>> details. Since this command tries to mimic blockdev-add, the state of
>> the block device after being reopened should generally be equivalent
>> to that of a block device added with the same set of options.
>>
>> There are two important things with this:
>>
>> a) Some options cannot be changed (most drivers don't allow that, in
>> fact).
>> b) If an option is missing, it should be reset to its default value
>> (rather than keeping its previous value).
>
> Could we use QAPI alternate types where we could state that:
>
> "option":"new" - set the option
> "option":null - reset the option to its default
> omit option - leave the option unchanged
>
> basically, making each of the options an Alternate with JSON null, so
> that a request to reset to the default becomes an explicit action?
+1
Sorry, I missed the previous discussion. What is the reason to reset option
to default if it missed? It looks more common to not touch things which are
not asked to.
Also, should we consider an option which may be changed during life-cycle of
a device not by user request to change it? If yes, it should be safer to not
reset it.
>
>>
>> Example: the default value of l2-cache-size is 1MB. If we set a
>> different value (2MB) on blockdev-add but then omit the option in
>> x-blockdev-reopen, then it should be reset back to 1MB. The current
>> bdrv_reopen() code keeps the previous value.
>>
>> Trying to change an option that doesn't allow it is already being
>> handled by the code. The loop at the end of bdrv_reopen_prepare()
>> checks all options that were not handled by the block driver and sees
>> if any of them has been modified.
>>
>> However, as I explained earlier in (b), omitting an option is supposed
>> to reset it to its default value, so it's also a way of changing
>> it. If the option cannot be changed then we should detect it and
>> return an error. What I'm doing in this series is making every driver
>> publish its list of run-time options, and which ones of them can be
>> modified.
>>
>> Backing files
>> =============
>> This command allows reconfigurations in the node graph. Currently it
>> only allows changing an image's backing file, but it should be
>> possible to implement similar functionalities in drivers that have
>> other children, like blkverify or quorum.
>>
>> Let's add an image with its backing file (hd1 <- hd0) like this:
>>
>> { "execute": "blockdev-add",
>> "arguments": {
>> "driver": "qcow2",
>> "node-name": "hd0",
>> "file": {
>> "driver": "file",
>> "filename": "hd0.qcow2",
>> "node-name": "hd0f"
>> },
>> "backing": {
>> "driver": "qcow2",
>> "node-name": "hd1",
>> "file": {
>> "driver": "file",
>> "filename": "hd1.qcow2",
>> "node-name": "hd1f"
>> }
>> }
>> }
>> }
>>
>> Removing the backing file can be done by simply passing the option {
>> ..., "backing": null } to x-blockdev-reopen.
>>
>> Replacing it is not so straightforward. If we pass something like {
>> ..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen will
>> assume that we're trying to change the options of the existing backing
>> file (and it will fail in most cases because it'll likely think that
>> we're trying to change the node name, among other options).
>>
>> So I decided to use a reference instead: { ..., "backing": "hd2" },
>> where "hd2" is of an existing node that has been added previously.
>>
>> Leaving out the "backing" option can be ambiguous on some cases (what
>> should it do? keep the current backing file? open the default one
>> specified in the image file?) so x-blockdev-reopen requires that this
>> option is present. For convenience, if the BDS doesn't have a backing
>> file then we allow leaving the option out.
>
> Hmm - that makes my proposal of "option":null as an explicit request to
> the default a bit trickier, if we are already using null for a different
> meaning for backing files.
And here: if we are going to reset to default for not listed options, than
just not listing "backing" should remove it (as default is null, obviously),
and we don't need this special "null" parameter.
Moreover, backing is an example of an option I mentioned, it definitely may
change after block-stream for example, so resetting to default is unsafe,
and user will have to carefully set backing on reopen (not tha backing, that
was used with first blockdev-add, but backing which we have after block-stream)
>
>>
>> As you can see in the patch series, I wrote a relatively large amount
>> of tests for many different scenarios, including some tricky ones.
>>
>> Perhaps the part that I'm still not convinced about is the one about
>> freezing backing links to prevent them from being removed, but the
>> implementation was quite simple so I decided to give it a go. As
>> you'll see in the patches I chose to use a bool instead of a counter
>> because I couldn't think of a case where it would make sense to have
>> two users freezing the same backing link.
>>
>> Thanks,
>>
>> Berto
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
2019-01-22 8:15 ` Vladimir Sementsov-Ogievskiy
@ 2019-01-23 15:56 ` Alberto Garcia
0 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-23 15:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Eric Blake, qemu-devel
Cc: Kevin Wolf, qemu-block, Max Reitz
On Tue 22 Jan 2019 09:15:25 AM CET, Vladimir Sementsov-Ogievskiy wrote:
>>> a) Some options cannot be changed (most drivers don't allow that, in
>>> fact).
>>> b) If an option is missing, it should be reset to its default value
>>> (rather than keeping its previous value).
>>
>> Could we use QAPI alternate types where we could state that:
>>
>> "option":"new" - set the option
>> "option":null - reset the option to its default
>> omit option - leave the option unchanged
>>
>> basically, making each of the options an Alternate with JSON null, so
>> that a request to reset to the default becomes an explicit action?
>
> +1
>
> Sorry, I missed the previous discussion. What is the reason to reset
> option to default if it missed? It looks more common to not touch
> things which are not asked to.
The idea is to have a command that works like blockdev-add. If you pass
a set of options to x-blockdev-reopen then the end result should be
similar to what you would get if you had used blockdev-add.
> Also, should we consider an option which may be changed during
> life-cycle of a device not by user request to change it? If yes, it
> should be safer to not reset it.
Do you have any example in mind?
> And here: if we are going to reset to default for not listed options,
> than just not listing "backing" should remove it (as default is null,
> obviously), and we don't need this special "null" parameter.
This is a bit trickier: the default for 'backing' is not to omit the
backing file, but to open the one that is specified in the image
metadata.
$ qemu-img create -f qcow2 base.qcow2 1M
$ qemu-img create -f qcow2 -b base.qcow2 active.qcow2
If you open active.qcow2 with blockdev-add then base.qcow2 is opened as
its backing file, and the only way to prevent that is to specify a
different backing file or null if you don't want to open any.
For x-blockdev-reopen we want to keep the same behavior as much as
possible, but instead of opening base.qcow2 if 'backing' is missing we
force the user to specify that option.
> Moreover, backing is an example of an option I mentioned, it
> definitely may change after block-stream for example, so resetting to
> default is unsafe, and user will have to carefully set backing on
> reopen (not tha backing, that was used with first blockdev-add, but
> backing which we have after block-stream)
Exactly. Since opening the default backing file is not something that we
want to do during a reopen operation we don't allow leaving that option
out.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
` (13 preceding siblings ...)
2019-01-17 15:50 ` [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' " Eric Blake
@ 2019-01-31 15:11 ` Alberto Garcia
14 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-01-31 15:11 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, Kevin Wolf, Max Reitz
ping
On Thu 17 Jan 2019 04:33:51 PM CET, Alberto Garcia wrote:
> Hi,
>
> here's a patch series to implement a QMP command for bdrv_reopen().
> This is not too different from the previous iteration (RFC v2, see
> changes below), but I'm not tagging it as RFC any longer.
>
> The new command is called x-blockdev-reopen, and it's designed to be
> similar to blockdev-add. It also takes BlockdevOptions as a
> parameter. The "node-name" field of BlockdevOptions must be set, and
> it is used to select the BlockDriverState to reopen.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links
2019-01-17 15:33 ` [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links Alberto Garcia
@ 2019-02-12 14:47 ` Kevin Wolf
2019-02-14 14:13 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 14:47 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> Our permission system is useful to define what operations are allowed
> on a certain block node and includes things like BLK_PERM_WRITE or
> BLK_PERM_RESIZE among others.
>
> One of the permissions is BLK_PERM_GRAPH_MOD which allows "changing
> the node that this BdrvChild points to". The exact meaning of this has
> never been very clear, but it can be understood as "change any of the
> links connected to the node". This can be used to prevent changing a
> backing link, but it's too coarse.
>
> This patch adds a new 'frozen' attribute to BdrvChild, which forbids
> detaching the link from the node it points to, and new API to freeze
> and unfreeze a backing chain.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 5 ++++
> include/block/block_int.h | 5 ++++
> 3 files changed, 76 insertions(+)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f605622216..fd0e88d17a 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -661,6 +661,11 @@ struct BdrvChild {
> */
> uint64_t shared_perm;
>
> + /*
> + * This link is frozen: the child cannot be detached from the parent.
> + */
> + bool frozen;
> +
> QLIST_ENTRY(BdrvChild) next;
> QLIST_ENTRY(BdrvChild) next_parent;
> };
Is this enough, or should it prevent both detaching from the parent _and_
changing the child node it points to?
> diff --git a/block.c b/block.c
> index 4f5ff2cc12..51fac086c7 100644
> --- a/block.c
> +++ b/block.c
> @@ -2062,6 +2062,8 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
> BlockDriverState *old_bs = child->bs;
> int i;
>
> + assert(!child->frozen);
> +
> if (old_bs && new_bs) {
> assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
> }
Okay, this does forbid both, so I think it's just the comment that needs
to be changed in the hunk above.
In order to avoid running into an assertion failure, we need to detect
the situation in all callers, or audit that it can never happen:
* bdrv_replace_child()
* bdrv_root_attach_child(): Creates a new BdrvChild, never frozen
* bdrv_detach_child()
* bdrv_root_unref_child()
* blk_remove_bs(): Not a backing child, safe
* block_job_remove_all_bdrv(): Not a backing child, safe
* bdrv_unref_child()
* bdrv_set_backing_hd(): Adds a check
* bdrv_close(): Not a backing child, safe
* bdrv_open_driver(): Not a backing child, safe
* bdrv_drop_intermediate(): Adds a check, though see below
* bdrv_replace_node(): Missing check?
* bdrv_append()
For example, mirror_start_job() calls it to insert the
mirror_top_bs node above the source, which could be anywhere in
the graph. This may involve changing the target of backing links.
* Other callers looks questionable, too
bdrv_replace_node() can return an error, so I think we should add a
check there.
For the bdrv_replace_child() call chain, the state looks better, but it
was tedious to verify and future additions could easily forget to add
the check. I wonder if we should allow some function there to return
errors so that we don't have to add the checks in the outermost
functions of the call chain.
> @@ -3813,6 +3819,62 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
> }
>
> /*
> + * Return true if at least one of the backing links between @bs and
> + * @base is frozen. @errp is set if that's the case.
> + */
> +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
> + Error **errp)
> +{
> + BlockDriverState *i;
> +
> + for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> + if (i->backing->frozen) {
> + error_setg(errp, "Cannot remove link from '%s' to '%s'",
Maybe s/remove/change/?
Might also be worth including the BdrvChild name in the error message.
> + i->node_name, backing_bs(i)->node_name);
> + return true;
> + }
> + }
> +
> + return false;
> +}
> @@ -3861,6 +3923,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
> goto exit;
> }
>
> + if (bdrv_is_backing_chain_frozen(top, base, NULL)) {
> + goto exit;
> + }
> +
> /* If 'base' recursively inherits from 'top' then we should set
> * base->inherits_from to top->inherits_from after 'top' and all
> * other intermediate nodes have been dropped.
bdrv_drop_intermediate() doesn't change the links between in the chain
between top and base, but the links between the parents of top and top
are changed to point to base instead.
I think this is checking the wrong thing.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job
2019-01-17 15:33 ` [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job Alberto Garcia
@ 2019-02-12 14:54 ` Kevin Wolf
2019-02-14 15:21 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 14:54 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/commit.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/block/commit.c b/block/commit.c
> index 53148e610b..8824d135e0 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -73,6 +73,8 @@ static int commit_prepare(Job *job)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>
> + bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
> +
> /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> * the normal backing chain can be restored. */
> blk_unref(s->base);
> @@ -89,6 +91,8 @@ static void commit_abort(Job *job)
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> BlockDriverState *top_bs = blk_bs(s->top);
>
> + bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
commit_abort() will run if commit_prepare() returned an error, so we may
end up unfreezing twice. Maybe move it into the 'if (s->base)' block a
few lines down.
> /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
> bdrv_ref(top_bs);
> bdrv_ref(s->commit_top_bs);
> @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> }
> }
>
> + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
> + goto fail;
> + }
Don't error paths need to unfreeze after this?
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job
2019-01-17 15:33 ` [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job Alberto Garcia
@ 2019-02-12 15:15 ` Kevin Wolf
2019-02-12 16:06 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 15:15 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/stream.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/block/stream.c b/block/stream.c
> index 7a49ac0992..39a2e10892 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -54,6 +54,14 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
> return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
> }
>
> +static void stream_abort(Job *job)
> +{
> + StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> + BlockJob *bjob = &s->common;
> +
> + bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
> +}
Like in commit, you can get a double unfreeze here if .abort is called
after .prepare returned an error.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job
2019-02-12 15:15 ` Kevin Wolf
@ 2019-02-12 16:06 ` Alberto Garcia
0 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-02-12 16:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Tue 12 Feb 2019 04:15:58 PM CET, Kevin Wolf wrote:
> Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>> block/stream.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 7a49ac0992..39a2e10892 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -54,6 +54,14 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
>> return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
>> }
>>
>> +static void stream_abort(Job *job)
>> +{
>> + StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> + BlockJob *bjob = &s->common;
>> +
>> + bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
>> +}
>
> Like in commit, you can get a double unfreeze here if .abort is called
> after .prepare returned an error.
You're right, I'll fix it
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue()
2019-01-17 15:33 ` [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue() Alberto Garcia
@ 2019-02-12 16:28 ` Kevin Wolf
2019-02-12 16:55 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
2019-02-19 16:00 ` [Qemu-devel] " Alberto Garcia
0 siblings, 2 replies; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 16:28 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> Children in QMP are specified with BlockdevRef / BlockdevRefOrNull,
> which can contain a set of child options, a child reference, or
> NULL. In optional attributes like "backing" it can also be missing.
>
> Only the first case (set of child options) is being handled properly
> by bdrv_reopen_queue(). This patch deals with all the others.
>
> Here's how these cases should be handled when bdrv_reopen_queue() is
> deciding what to do with each child of a BlockDriverState:
>
> 1) Set of child options: the options are removed from the parent's
> options QDict and are passed to the child with a recursive
> bdrv_reopen_queue() call. This case was already working fine.
Small addition: This is only allowed if the child was implicitly created
(i.e. it inherits from the parent we're coming from).
> 2) Child reference: there's two possibilites here.
>
> 2a) Reference to the current child: the child is put in the
> reopen queue, keeping its current set of options (since this
> was a child reference there was no way to specify a
> different set of options).
Why even put it in the reopen queue when nothing is going to change?
Ah, it's because inherited options might change, right?
But if the child did not inherit from this parent, we don't need to put
it into the queue anyway. The operation should still be allowed.
> 2b) Reference to a different BDS: the current child is not put
> in the reopen queue at all. Passing a reference to a
> different BDS can be used to replace a child, although at
> the moment no driver implements this, so it results in an
> error. In any case, the current child is not going to be
> reopened (and might in fact disappear if it's replaced)
Do we need to break a possible inheritance relationship between the
parent and the old child node in this case?
> 3) NULL: This is similar to (2b). Although no driver allows this
> yet it can be used to detach the current child so it should not
> be put in the reopen queue.
Same comment as for 2b.
> 4) Missing option: at the moment "backing" is the only case where
> this can happen. With "blockdev-add", leaving "backing" out
> means that the default backing file is opened. We don't want to
> open a new image during reopen, so we require that "backing" is
> always present. We'll relax this requirement a bit in the next
> patch.
I think this is only for keep_old_opts == false; otherwise it should be
treated the same as 2a to avoid breaking compatibility.
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
> include/block/block.h | 1 +
> 2 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index 056a620eb2..fd51f1cd35 100644
> --- a/block.c
> +++ b/block.c
> @@ -3032,9 +3032,21 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
> bs_entry->state.perm = UINT64_MAX;
> bs_entry->state.shared_perm = 0;
>
> + /*
> + * If keep_old_opts is false then it means that unspecified
> + * options must be reset to its original value. We don't allow
s/its/their/
> + * resetting 'backing' but we need to know if the option is
> + * missing in order to decide if we have to return an error.
> + */
> + if (!keep_old_opts) {
> + bs_entry->state.backing_missing =
> + !qdict_haskey(options, "backing") &&
> + !qdict_haskey(options, "backing.driver");
> + }
> +
> QLIST_FOREACH(child, &bs->children, next) {
> - QDict *new_child_options;
> - char *child_key_dot;
> + QDict *new_child_options = NULL;
> + bool child_keep_old = keep_old_opts;
>
> /* reopen can only change the options of block devices that were
> * implicitly created and inherited options. For other (referenced)
> @@ -3043,13 +3055,31 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
> continue;
> }
The 'continue' in the context just skipped any children that don't
inherit from this parent. Child options for those will stay unmodified
in the options dict.
For case 1, we'll later get an error because keys like 'child.foo' will
still be present and can't be processed by the driver. Implements
exactly what I commented above.
For case 2, the child isn't put in the reopen queue, and the child
reference can be used later. Matches my comment as well.
Good.
> - child_key_dot = g_strdup_printf("%s.", child->name);
> - qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
> - qdict_extract_subqdict(options, &new_child_options, child_key_dot);
> - g_free(child_key_dot);
> + /* Check if the options contain a child reference */
> + if (qdict_haskey(options, child->name)) {
> + const char *childref = qdict_get_try_str(options, child->name);
> + /*
> + * The current child must not be reopened if the child
> + * reference does not point to it.
> + */
> + if (g_strcmp0(childref, child->bs->node_name)) {
This is where we would break the inheritance relationship.
Is this the code path that a QNull should take as well? (case 3)
> + continue;
> + }
> + /*
> + * If the child reference points to the current child then
> + * reopen it with its existing set of options.
Mention option inheritance here as the reason why to even bother to
reopen a child with unchanged options?
> + */
> + child_keep_old = true;
> + } else {
> + /* Extract child options ("child-name.*") */
> + char *child_key_dot = g_strdup_printf("%s.", child->name);
> + qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
> + qdict_extract_subqdict(options, &new_child_options, child_key_dot);
> + g_free(child_key_dot);
> + }
>
> bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
> - child->role, options, flags, keep_old_opts);
> + child->role, options, flags, child_keep_old);
> }
>
> return bs_queue;
> @@ -3306,6 +3336,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>
> drv_prepared = true;
>
> + if (reopen_state->backing_missing) {
> + error_setg(errp, "backing is missing for '%s'",
> + reopen_state->bs->node_name);
> + ret = -EINVAL;
> + goto error;
> + }
What about drivers that don't even support backing files?
> /* Options that are not handled are only okay if they are unchanged
> * compared to the old state. It is expected that some options are only
> * used for the initial open, but not reopen (e.g. filename) */
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] block: Allow omitting the 'backing' option in certain cases
2019-01-17 15:33 ` [Qemu-devel] [PATCH 07/13] block: Allow omitting the 'backing' option in certain cases Alberto Garcia
@ 2019-02-12 16:38 ` Kevin Wolf
0 siblings, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 16:38 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> Of all options of type BlockdevRef used to specify children in
> BlockdevOptions, 'backing' is the only one that is optional.
>
> For "x-blockdev-reopen" we want that if an option is omitted then it
> must be reset to its default value. The default value of 'backing'
> means that QEMU opens the backing file specified in the image
> metadata, but this is not something that we want to support for the
> reopen operation.
>
> Because of this the 'backing' option has to be specified during
> reopen, pointing to the existing backing file if we want to keep it,
> or pointing to a different one (or NULL) if we want to replace it (to
> be implemented in a subsequent patch).
>
> In order to simplify things a bit and not to require that the user
> passes the 'backing' option to every single block device even when
> it's clearly not necessary, this patch allows omitting this option if
> the block device being reopened doesn't have a backing file attached
> _and_ no default backing file is specified in the image metadata.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index fd51f1cd35..897c8b85cd 100644
> --- a/block.c
> +++ b/block.c
> @@ -3336,7 +3336,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>
> drv_prepared = true;
>
> - if (reopen_state->backing_missing) {
> + /*
> + * We must provide the 'backing' option if the BDS has a backing
> + * file or if the image file has a backing file name as part of
> + * its metadata. Otherwise the 'backing' option can be omitted.
> + */
> + if (reopen_state->backing_missing &&
> + (backing_bs(reopen_state->bs) || reopen_state->bs->backing_file[0])) {
> error_setg(errp, "backing is missing for '%s'",
> reopen_state->bs->node_name);
> ret = -EINVAL;
Okay, this should be enough to fix drivers without support for backing
files again. Might be worth mentioning in the commit message of this and
the previous patch.
Normally, I would suggest merging this into the previous patch to keep
things bisectable, but keep_old_opts == false isn't used yet, so this
isn't a concern in this case.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue()
2019-02-12 16:28 ` Kevin Wolf
@ 2019-02-12 16:55 ` Kevin Wolf
2019-02-19 16:00 ` [Qemu-devel] " Alberto Garcia
1 sibling, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 16:55 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 12.02.2019 um 17:28 hat Kevin Wolf geschrieben:
> > - child_key_dot = g_strdup_printf("%s.", child->name);
> > - qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
> > - qdict_extract_subqdict(options, &new_child_options, child_key_dot);
> > - g_free(child_key_dot);
> > + /* Check if the options contain a child reference */
> > + if (qdict_haskey(options, child->name)) {
> > + const char *childref = qdict_get_try_str(options, child->name);
> > + /*
> > + * The current child must not be reopened if the child
> > + * reference does not point to it.
> > + */
> > + if (g_strcmp0(childref, child->bs->node_name)) {
>
> This is where we would break the inheritance relationship.
Oops, correcting myself: It's not. We may only do that in the commit
stage, of course.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen
2019-01-17 15:33 ` [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen Alberto Garcia
@ 2019-02-12 17:27 ` Kevin Wolf
2019-02-21 14:46 ` Alberto Garcia
2019-02-21 14:53 ` Alberto Garcia
0 siblings, 2 replies; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 17:27 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> This patch allows the user to change the backing file of an image that
> is being reopened. Here's what it does:
>
> - In bdrv_reopen_prepare(): check that the value of 'backing' points
> to an existing node or is null. If it points to an existing node it
> also needs to make sure that replacing the backing file will not
> create a cycle in the node graph (i.e. you cannot reach the parent
> from the new backing file).
>
> - In bdrv_reopen_commit(): perform the actual node replacement by
> calling bdrv_set_backing_hd(). It may happen that there are
> temporary implicit nodes between the BDS that we are reopening and
> the backing file that we want to replace (e.g. a commit fiter node),
> so we must skip them.
Hmm... I really dislike getting into the business of dealing with
implicit nodes here. If you want to manage the block graph at the node
level, you should manage all of it and just avoid getting implicit
nodes in the first place. Otherwise, we'd have to guess where in the
implicit chain you really want to add or remove nodes, and we're bound
to guess wrong for some users.
There is one problem with not skipping implicit nodes, though: Even if
you don't want to manage things at the node level, we already force you
to specify 'backing'. If there are implicit nodes, you don't knwo the
real node name of the first backing child.
So I suggest that we allow skipping implicit nodes for the purpose of
leaving the backing link unchanged; but we return an error if you want
to change the backing link and there are implicit nodes in the way.
> Although x-blockdev-reopen is meant to be used like blockdev-add,
> there's an important thing that must be taken into account: the only
> way to set a new backing file is by using a reference to an existing
> node (previously added with e.g. blockdev-add). If 'backing' contains
> a dictionary with a new set of options ({"driver": "qcow2", "file": {
> ... }}) then it is interpreted that the _existing_ backing file must
> be reopened with those options.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 897c8b85cd..10847416b2 100644
> --- a/block.c
> +++ b/block.c
> @@ -2909,6 +2909,27 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
> }
>
> /*
> + * Returns true if @child can be reached recursively from @bs
> + */
> +static bool bdrv_recurse_has_child(BlockDriverState *bs,
> + BlockDriverState *child)
> +{
> + BdrvChild *c;
> +
> + if (bs == child) {
> + return true;
> + }
> +
> + QLIST_FOREACH(c, &bs->children, next) {
> + if (bdrv_recurse_has_child(c->bs, child)) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> * Adds a BlockDriverState to a simple queue for an atomic, transactional
> * reopen of multiple devices.
> *
> @@ -3217,6 +3238,63 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
> }
>
> /*
> + * Return 0 if the 'backing' option of @bs can be replaced with
> + * @value, otherwise return < 0 and set @errp.
> + */
> +static int bdrv_reopen_parse_backing(BlockDriverState *bs, QObject *value,
> + Error **errp)
> +{
> + BlockDriverState *overlay_bs, *new_backing_bs;
> + const char *str;
> +
> + switch (qobject_type(value)) {
> + case QTYPE_QNULL:
> + new_backing_bs = NULL;
> + break;
> + case QTYPE_QSTRING:
> + str = qobject_get_try_str(value);
> + new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
> + if (new_backing_bs == NULL) {
> + return -EINVAL;
> + } else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
> + error_setg(errp, "Making '%s' a backing file of '%s' "
> + "would create a cycle", str, bs->node_name);
> + return -EINVAL;
> + }
> + break;
> + default:
> + /* 'backing' does not allow any other data type */
> + g_assert_not_reached();
> + }
> +
> + if (new_backing_bs) {
> + if (bdrv_get_aio_context(new_backing_bs) != bdrv_get_aio_context(bs)) {
> + error_setg(errp, "Cannot use a new backing file "
> + "with a different AioContext");
> + return -EINVAL;
> + }
> + }
This is okay for a first version, but in reality, you'd usually want to
just move the backing file into the right AioContext. Of course, this is
only correct if the backing file doesn't have other users in different
AioContexts. To get a good implementation for this, we'd probably need
to store the AioContext in BdrvChild, like we already concluded for
other use cases.
Maybe document this as one of the problems to be solved before we can
remove the x- prefix.
> +
> + /*
> + * Skip all links that point to an implicit node, if any
> + * (e.g. a commit filter node). We don't want to change those.
> + */
> + overlay_bs = bs;
> + while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
> + overlay_bs = backing_bs(overlay_bs);
> + }
> +
> + if (new_backing_bs != backing_bs(overlay_bs)) {
> + if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
> + errp)) {
> + return -EPERM;
> + }
> + }
Should this function check new_backing_bs->drv->supports_backing, too?
> + return 0;
> +}
> +
> +/*
> * Prepares a BlockDriverState for reopen. All changes are staged in the
> * 'opaque' field of the BDRVReopenState, which is used and allocated by
> * the block driver layer .bdrv_reopen_prepare()
> @@ -3359,6 +3437,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> QObject *new = entry->value;
> QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>
> + /*
> + * Allow changing the 'backing' option. The new value can be
> + * either a reference to an existing node (using its node name)
> + * or NULL to simply detach the current backing file.
> + */
> + if (!strcmp(entry->key, "backing")) {
> + ret = bdrv_reopen_parse_backing(reopen_state->bs, new, errp);
> + if (ret < 0) {
> + goto error;
> + }
> + continue; /* 'backing' option processed, go to the next one */
> + }
> +
> /* Allow child references (child_name=node_name) as long as they
> * point to the current child (i.e. everything stays the same). */
> if (qobject_type(new) == QTYPE_QSTRING) {
> @@ -3437,9 +3528,10 @@ error:
> void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> {
> BlockDriver *drv;
> - BlockDriverState *bs;
> + BlockDriverState *bs, *new_backing_bs;
> BdrvChild *child;
> - bool old_can_write, new_can_write;
> + bool old_can_write, new_can_write, change_backing_bs;
> + QObject *qobj;
>
> assert(reopen_state != NULL);
> bs = reopen_state->bs;
> @@ -3464,6 +3556,15 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
> bs->detect_zeroes = reopen_state->detect_zeroes;
>
> + qobj = qdict_get(bs->options, "backing");
> + change_backing_bs = (qobj != NULL);
> + if (change_backing_bs) {
> + const char *str = qobject_get_try_str(qobj);
> + new_backing_bs = str ? bdrv_find_node(str) : NULL;
> + qdict_del(bs->explicit_options, "backing");
> + qdict_del(bs->options, "backing");
> + }
> +
> /* Remove child references from bs->options and bs->explicit_options.
> * Child options were already removed in bdrv_reopen_queue_child() */
> QLIST_FOREACH(child, &bs->children, next) {
> @@ -3471,6 +3572,25 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> qdict_del(bs->options, child->name);
> }
>
> + /*
> + * Change the backing file if a new one was specified. We do this
> + * after updating bs->options, so bdrv_refresh_filename() (called
> + * from bdrv_set_backing_hd()) has the new values.
> + */
> + if (change_backing_bs) {
> + BlockDriverState *overlay = bs;
> + /*
> + * Skip all links that point to an implicit node, if any
> + * (e.g. a commit filter node). We don't want to change those.
> + */
> + while (backing_bs(overlay) && backing_bs(overlay)->implicit) {
> + overlay = backing_bs(overlay);
> + }
> + if (new_backing_bs != backing_bs(overlay)) {
> + bdrv_set_backing_hd(overlay, new_backing_bs, &error_abort);
I'm afraid we can't use &error_abort here because bdrv_attach_child()
could still fail due to permission conflicts.
> + }
> + }
> +
> bdrv_refresh_limits(bs, NULL);
>
> bdrv_set_perm(reopen_state->bs, reopen_state->perm,
Hm... Does bdrv_set_perm() work correctly when between bdrv_check_perm()
and here the graph was changed?
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-01-17 15:34 ` [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver Alberto Garcia
@ 2019-02-12 18:02 ` Kevin Wolf
2019-03-01 12:12 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-02-12 18:02 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 17.01.2019 um 16:34 hat Alberto Garcia geschrieben:
> This patch adds two new fields to BlockDriver:
>
> - runtime_opts: list of runtime options for a particular block
> driver. We'll use this list later to detect what options are
> missing when we try to reopen a block device.
>
> - mutable_opts: names of the runtime options that can be reset to
> their default value after a block device has been added. This way
> an option can not be reset by omitting it during reopen unless we
> explicitly allow it.
>
> This also sets runtime_opts (and mutable_opts where appropriate) in
> all drivers that allow reopening.
qcow2 most certainly does support reopening, and is probably one of the
few drivers that actually allow changing things at runtime. Still, it's
missing from this patch.
> Most of those drivers don't actually
> support changing any of their options. If the user specifies a new
> value for an option that can't be changed then we already detect that
> and forbid it (in bdrv_reopen_prepare()). But if the user omits an
> option in order to try to reset it to its default value we need to
> detect that, so we'll use these two new fields for that.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index fd0e88d17a..e680dda86b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -345,6 +345,13 @@ struct BlockDriver {
>
> /* List of options for creating images, terminated by name == NULL */
> QemuOptsList *create_opts;
> + /* Runtime options for a block device, terminated by name == NULL */
> + QemuOptsList *runtime_opts;
I'm not sure if using a QemuOptsList here is a good idea. Currently, we
use QemuOptsLists for most options, but there are some drivers that use
it only for part of their options, or not at all, using direct QDict
accesses or QAPI objects for the rest.
Especially the part with the QAPI objects is something that I'd hope to
see more in the future, and tying things to QemuOpts might make this
conversion harder.
> + /*
> + * Names of the runtime options that can be reset by omitting
> + * their value on reopen, NULL-terminated.
> + */
> + const char *const *mutable_opts;
>
> /*
> * Returns 0 for completed check, -errno for internal errors.
> diff --git a/block/rbd.c b/block/rbd.c
> index 8a1a9f4b6e..6de6112ce8 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -1231,6 +1231,7 @@ static QemuOptsList qemu_rbd_create_opts = {
> static BlockDriver bdrv_rbd = {
> .format_name = "rbd",
> .instance_size = sizeof(BDRVRBDState),
> + .runtime_opts = &runtime_opts,
> .bdrv_parse_filename = qemu_rbd_parse_filename,
> .bdrv_refresh_limits = qemu_rbd_refresh_limits,
> .bdrv_file_open = qemu_rbd_open,
rbd is one of these drivers. In fact, its runtime_opts seems to be
completely unused before this patch.
It has a comemnt 'server.* extracted manually, see qemu_rbd_mon_host()',
but if you compare it to the schema, more options that have been added
since are not covered in runtime_opts.
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 90ab43baa4..6dd66d0b99 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -3288,6 +3288,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
> .bdrv_attach_aio_context = sd_attach_aio_context,
>
> .create_opts = &sd_create_opts,
> + .runtime_opts = &runtime_opts,
> };
Sheepdog is another driver where runtime_opts is incomplete (the
'server' struct is missing).
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links
2019-02-12 14:47 ` Kevin Wolf
@ 2019-02-14 14:13 ` Alberto Garcia
2019-02-14 15:52 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-02-14 14:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Tue 12 Feb 2019 03:47:47 PM CET, Kevin Wolf wrote:
> Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
>> @@ -3861,6 +3923,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>> goto exit;
>> }
>>
>> + if (bdrv_is_backing_chain_frozen(top, base, NULL)) {
>> + goto exit;
>> + }
>> +
>> /* If 'base' recursively inherits from 'top' then we should set
>> * base->inherits_from to top->inherits_from after 'top' and all
>> * other intermediate nodes have been dropped.
>
> bdrv_drop_intermediate() doesn't change the links between in the chain
> between top and base, but the links between the parents of top and top
> are changed to point to base instead.
>
> I think this is checking the wrong thing.
You're right, those links should be checked.
On the other hand it does remove all references from top's parents to
top, so in the general case it will end up deleting all intermediate
notes, and their backing links with them. So I think we still need that
check.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job
2019-02-12 14:54 ` Kevin Wolf
@ 2019-02-14 15:21 ` Alberto Garcia
2019-02-14 15:45 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-02-14 15:21 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Tue 12 Feb 2019 03:54:15 PM CET, Kevin Wolf wrote:
>> @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>> }
>> }
>>
>> + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
>> + goto fail;
>> + }
>
> Don't error paths need to unfreeze after this?
Yes, and while debugging this I realized that the error path is wrong:
if (commit_top_bs) {
bdrv_replace_node(commit_top_bs, top, &error_abort);
}
job_early_fail(&s->common.job);
Doing bdrv_replace_node() here before job_early_fail() fails with
Unexpected error in bdrv_check_update_perm() at block.c:1920:
Conflicts with use by commit job 'virtio0' as 'intermediate node', which does not allow 'consistent read' on <node-name>
Aborted
I'll write a separate patch for this problem.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job
2019-02-14 15:21 ` Alberto Garcia
@ 2019-02-14 15:45 ` Kevin Wolf
0 siblings, 0 replies; 43+ messages in thread
From: Kevin Wolf @ 2019-02-14 15:45 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 14.02.2019 um 16:21 hat Alberto Garcia geschrieben:
> On Tue 12 Feb 2019 03:54:15 PM CET, Kevin Wolf wrote:
> >> @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> >> }
> >> }
> >>
> >> + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
> >> + goto fail;
> >> + }
> >
> > Don't error paths need to unfreeze after this?
>
> Yes, and while debugging this I realized that the error path is wrong:
>
> if (commit_top_bs) {
> bdrv_replace_node(commit_top_bs, top, &error_abort);
> }
> job_early_fail(&s->common.job);
>
> Doing bdrv_replace_node() here before job_early_fail() fails with
>
> Unexpected error in bdrv_check_update_perm() at block.c:1920:
> Conflicts with use by commit job 'virtio0' as 'intermediate node', which does not allow 'consistent read' on <node-name>
> Aborted
Oh, good point. And it seems to have been wrong even since permissions
were introduced to the commit job in 8dfba279776.
> I'll write a separate patch for this problem.
And a test case! :-)
(That is, if the errors can even be triggered reliably without modifying
the code.)
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links
2019-02-14 14:13 ` Alberto Garcia
@ 2019-02-14 15:52 ` Kevin Wolf
2019-02-18 13:35 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-02-14 15:52 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 14.02.2019 um 15:13 hat Alberto Garcia geschrieben:
> On Tue 12 Feb 2019 03:47:47 PM CET, Kevin Wolf wrote:
> > Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
> >> @@ -3861,6 +3923,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
> >> goto exit;
> >> }
> >>
> >> + if (bdrv_is_backing_chain_frozen(top, base, NULL)) {
> >> + goto exit;
> >> + }
> >> +
> >> /* If 'base' recursively inherits from 'top' then we should set
> >> * base->inherits_from to top->inherits_from after 'top' and all
> >> * other intermediate nodes have been dropped.
> >
> > bdrv_drop_intermediate() doesn't change the links between in the chain
> > between top and base, but the links between the parents of top and top
> > are changed to point to base instead.
> >
> > I think this is checking the wrong thing.
>
> You're right, those links should be checked.
>
> On the other hand it does remove all references from top's parents to
> top, so in the general case it will end up deleting all intermediate
> notes, and their backing links with them. So I think we still need that
> check.
That begs the question if removing a frozen child link should be
forbideen even when it's because the parent goes away. It don't think
there is a good reason to check this - who else than the removed parent
could be interested in keeping it frozen?
In fact, this means that the parent forgot to unfreeze its child before
going away, which shouldn't happen. So I guess bdrv_close() could just
assert that this is not the case for any children?
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links
2019-02-14 15:52 ` Kevin Wolf
@ 2019-02-18 13:35 ` Alberto Garcia
0 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-02-18 13:35 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Thu 14 Feb 2019 04:52:52 PM CET, Kevin Wolf wrote:
> Am 14.02.2019 um 15:13 hat Alberto Garcia geschrieben:
>> On Tue 12 Feb 2019 03:47:47 PM CET, Kevin Wolf wrote:
>> > Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben:
>> >> @@ -3861,6 +3923,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>> >> goto exit;
>> >> }
>> >>
>> >> + if (bdrv_is_backing_chain_frozen(top, base, NULL)) {
>> >> + goto exit;
>> >> + }
>> >> +
>> >> /* If 'base' recursively inherits from 'top' then we should set
>> >> * base->inherits_from to top->inherits_from after 'top' and all
>> >> * other intermediate nodes have been dropped.
>> >
>> > bdrv_drop_intermediate() doesn't change the links between in the chain
>> > between top and base, but the links between the parents of top and top
>> > are changed to point to base instead.
>> >
>> > I think this is checking the wrong thing.
>>
>> You're right, those links should be checked.
>>
>> On the other hand it does remove all references from top's parents to
>> top, so in the general case it will end up deleting all intermediate
>> notes, and their backing links with them. So I think we still need that
>> check.
>
> That begs the question if removing a frozen child link should be
> forbideen even when it's because the parent goes away. It don't think
> there is a good reason to check this - who else than the removed
> parent could be interested in keeping it frozen?
>
> In fact, this means that the parent forgot to unfreeze its child
> before going away, which shouldn't happen. So I guess bdrv_close()
> could just assert that this is not the case for any children?
That actually sounds like a good solution.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue()
2019-02-12 16:28 ` Kevin Wolf
2019-02-12 16:55 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
@ 2019-02-19 16:00 ` Alberto Garcia
1 sibling, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-02-19 16:00 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Tue 12 Feb 2019 05:28:06 PM CET, Kevin Wolf wrote:
>> 1) Set of child options: the options are removed from the parent's
>> options QDict and are passed to the child with a recursive
>> bdrv_reopen_queue() call. This case was already working fine.
>
> Small addition: This is only allowed if the child was implicitly
> created (i.e. it inherits from the parent we're coming from).
Ok, fixed.
>> 2) Child reference: there's two possibilites here.
>>
>> 2a) Reference to the current child: the child is put in the
>> reopen queue, keeping its current set of options (since this
>> was a child reference there was no way to specify a
>> different set of options).
>
> Why even put it in the reopen queue when nothing is going to change?
>
> Ah, it's because inherited options might change, right?
>
> But if the child did not inherit from this parent, we don't need to
> put it into the queue anyway. The operation should still be allowed.
I updated the comment to clarify that.
>> 2b) Reference to a different BDS: the current child is not put
>> in the reopen queue at all. Passing a reference to a
>> different BDS can be used to replace a child, although at
>> the moment no driver implements this, so it results in an
>> error. In any case, the current child is not going to be
>> reopened (and might in fact disappear if it's replaced)
>
> Do we need to break a possible inheritance relationship between the
> parent and the old child node in this case?
Actually I think it makes sense (but not in this patch). I guess that
should be done in bdrv_set_backing_hd().
>> 4) Missing option: at the moment "backing" is the only case where
>> this can happen. With "blockdev-add", leaving "backing" out
>> means that the default backing file is opened. We don't want to
>> open a new image during reopen, so we require that "backing" is
>> always present. We'll relax this requirement a bit in the next
>> patch.
>
> I think this is only for keep_old_opts == false; otherwise it should
> be treated the same as 2a to avoid breaking compatibility.
Ok, I clarified that too.
>> QLIST_FOREACH(child, &bs->children, next) {
>> - QDict *new_child_options;
>> - char *child_key_dot;
>> + QDict *new_child_options = NULL;
>> + bool child_keep_old = keep_old_opts;
>>
>> /* reopen can only change the options of block devices that were
>> * implicitly created and inherited options. For other (referenced)
>> @@ -3043,13 +3055,31 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>> continue;
>> }
>
> The 'continue' in the context just skipped any children that don't
> inherit from this parent. Child options for those will stay unmodified
> in the options dict.
>
> For case 1, we'll later get an error because keys like 'child.foo'
> will still be present and can't be processed by the driver. Implements
> exactly what I commented above.
>
> For case 2, the child isn't put in the reopen queue, and the child
> reference can be used later. Matches my comment as well.
>
> Good.
That's correct.
>> - child_key_dot = g_strdup_printf("%s.", child->name);
>> - qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
>> - qdict_extract_subqdict(options, &new_child_options, child_key_dot);
>> - g_free(child_key_dot);
>> + /* Check if the options contain a child reference */
>> + if (qdict_haskey(options, child->name)) {
>> + const char *childref = qdict_get_try_str(options, child->name);
>> + /*
>> + * The current child must not be reopened if the child
>> + * reference does not point to it.
>> + */
>> + if (g_strcmp0(childref, child->bs->node_name)) {
>
> This is where we would break the inheritance relationship.
>
> Is this the code path that a QNull should take as well? (case 3)
Yes, I updated the comment to mention the null case explicitly.
>> + if (reopen_state->backing_missing) {
>> + error_setg(errp, "backing is missing for '%s'",
>> + reopen_state->bs->node_name);
>> + ret = -EINVAL;
>> + goto error;
>> + }
>
> What about drivers that don't even support backing files?
In practice this doesn't matter much because of the changes in patch 7,
but I think it's a good idea to make it explicit and set backing_missing
only when bs->drv->supports_backing is true (because it doesn't make
sense otherwise).
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen
2019-02-12 17:27 ` Kevin Wolf
@ 2019-02-21 14:46 ` Alberto Garcia
2019-02-21 14:53 ` Alberto Garcia
1 sibling, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-02-21 14:46 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Tue 12 Feb 2019 06:27:56 PM CET, Kevin Wolf wrote:
>> - In bdrv_reopen_commit(): perform the actual node replacement by
>> calling bdrv_set_backing_hd(). It may happen that there are
>> temporary implicit nodes between the BDS that we are reopening and
>> the backing file that we want to replace (e.g. a commit fiter node),
>> so we must skip them.
>
> Hmm... I really dislike getting into the business of dealing with
> implicit nodes here. If you want to manage the block graph at the node
> level, you should manage all of it and just avoid getting implicit
> nodes in the first place. Otherwise, we'd have to guess where in the
> implicit chain you really want to add or remove nodes, and we're bound
> to guess wrong for some users.
>
> There is one problem with not skipping implicit nodes, though: Even if
> you don't want to manage things at the node level, we already force
> you to specify 'backing'. If there are implicit nodes, you don't knwo
> the real node name of the first backing child.
>
> So I suggest that we allow skipping implicit nodes for the purpose of
> leaving the backing link unchanged; but we return an error if you want
> to change the backing link and there are implicit nodes in the way.
, that sounds good to me. It doesn't really affect any of the test
cases that I had
>
>> Although x-blockdev-reopen is meant to be used like blockdev-add,
>> there's an important thing that must be taken into account: the only
>> way to set a new backing file is by using a reference to an existing
>> node (previously added with e.g. blockdev-add). If 'backing' contains
>> a dictionary with a new set of options ({"driver": "qcow2", "file": {
>> ... }}) then it is interpreted that the _existing_ backing file must
>> be reopened with those options.
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>> block.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 122 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 897c8b85cd..10847416b2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2909,6 +2909,27 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
>> }
>>
>> /*
>> + * Returns true if @child can be reached recursively from @bs
>> + */
>> +static bool bdrv_recurse_has_child(BlockDriverState *bs,
>> + BlockDriverState *child)
>> +{
>> + BdrvChild *c;
>> +
>> + if (bs == child) {
>> + return true;
>> + }
>> +
>> + QLIST_FOREACH(c, &bs->children, next) {
>> + if (bdrv_recurse_has_child(c->bs, child)) {
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/*
>> * Adds a BlockDriverState to a simple queue for an atomic, transactional
>> * reopen of multiple devices.
>> *
>> @@ -3217,6 +3238,63 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
>> }
>>
>> /*
>> + * Return 0 if the 'backing' option of @bs can be replaced with
>> + * @value, otherwise return < 0 and set @errp.
>> + */
>> +static int bdrv_reopen_parse_backing(BlockDriverState *bs, QObject *value,
>> + Error **errp)
>> +{
>> + BlockDriverState *overlay_bs, *new_backing_bs;
>> + const char *str;
>> +
>> + switch (qobject_type(value)) {
>> + case QTYPE_QNULL:
>> + new_backing_bs = NULL;
>> + break;
>> + case QTYPE_QSTRING:
>> + str = qobject_get_try_str(value);
>> + new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
>> + if (new_backing_bs == NULL) {
>> + return -EINVAL;
>> + } else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
>> + error_setg(errp, "Making '%s' a backing file of '%s' "
>> + "would create a cycle", str, bs->node_name);
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + /* 'backing' does not allow any other data type */
>> + g_assert_not_reached();
>> + }
>> +
>> + if (new_backing_bs) {
>> + if (bdrv_get_aio_context(new_backing_bs) != bdrv_get_aio_context(bs)) {
>> + error_setg(errp, "Cannot use a new backing file "
>> + "with a different AioContext");
>> + return -EINVAL;
>> + }
>> + }
>
> This is okay for a first version, but in reality, you'd usually want to
> just move the backing file into the right AioContext. Of course, this is
> only correct if the backing file doesn't have other users in different
> AioContexts. To get a good implementation for this, we'd probably need
> to store the AioContext in BdrvChild, like we already concluded for
> other use cases.
>
> Maybe document this as one of the problems to be solved before we can
> remove the x- prefix.
>
>> +
>> + /*
>> + * Skip all links that point to an implicit node, if any
>> + * (e.g. a commit filter node). We don't want to change those.
>> + */
>> + overlay_bs = bs;
>> + while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>> + overlay_bs = backing_bs(overlay_bs);
>> + }
>> +
>> + if (new_backing_bs != backing_bs(overlay_bs)) {
>> + if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
>> + errp)) {
>> + return -EPERM;
>> + }
>> + }
>
> Should this function check new_backing_bs->drv->supports_backing, too?
>
>> + return 0;
>> +}
>> +
>> +/*
>> * Prepares a BlockDriverState for reopen. All changes are staged in the
>> * 'opaque' field of the BDRVReopenState, which is used and allocated by
>> * the block driver layer .bdrv_reopen_prepare()
>> @@ -3359,6 +3437,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>> QObject *new = entry->value;
>> QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>>
>> + /*
>> + * Allow changing the 'backing' option. The new value can be
>> + * either a reference to an existing node (using its node name)
>> + * or NULL to simply detach the current backing file.
>> + */
>> + if (!strcmp(entry->key, "backing")) {
>> + ret = bdrv_reopen_parse_backing(reopen_state->bs, new, errp);
>> + if (ret < 0) {
>> + goto error;
>> + }
>> + continue; /* 'backing' option processed, go to the next one */
>> + }
>> +
>> /* Allow child references (child_name=node_name) as long as they
>> * point to the current child (i.e. everything stays the same). */
>> if (qobject_type(new) == QTYPE_QSTRING) {
>> @@ -3437,9 +3528,10 @@ error:
>> void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> {
>> BlockDriver *drv;
>> - BlockDriverState *bs;
>> + BlockDriverState *bs, *new_backing_bs;
>> BdrvChild *child;
>> - bool old_can_write, new_can_write;
>> + bool old_can_write, new_can_write, change_backing_bs;
>> + QObject *qobj;
>>
>> assert(reopen_state != NULL);
>> bs = reopen_state->bs;
>> @@ -3464,6 +3556,15 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>> bs->detect_zeroes = reopen_state->detect_zeroes;
>>
>> + qobj = qdict_get(bs->options, "backing");
>> + change_backing_bs = (qobj != NULL);
>> + if (change_backing_bs) {
>> + const char *str = qobject_get_try_str(qobj);
>> + new_backing_bs = str ? bdrv_find_node(str) : NULL;
>> + qdict_del(bs->explicit_options, "backing");
>> + qdict_del(bs->options, "backing");
>> + }
>> +
>> /* Remove child references from bs->options and bs->explicit_options.
>> * Child options were already removed in bdrv_reopen_queue_child() */
>> QLIST_FOREACH(child, &bs->children, next) {
>> @@ -3471,6 +3572,25 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> qdict_del(bs->options, child->name);
>> }
>>
>> + /*
>> + * Change the backing file if a new one was specified. We do this
>> + * after updating bs->options, so bdrv_refresh_filename() (called
>> + * from bdrv_set_backing_hd()) has the new values.
>> + */
>> + if (change_backing_bs) {
>> + BlockDriverState *overlay = bs;
>> + /*
>> + * Skip all links that point to an implicit node, if any
>> + * (e.g. a commit filter node). We don't want to change those.
>> + */
>> + while (backing_bs(overlay) && backing_bs(overlay)->implicit) {
>> + overlay = backing_bs(overlay);
>> + }
>> + if (new_backing_bs != backing_bs(overlay)) {
>> + bdrv_set_backing_hd(overlay, new_backing_bs, &error_abort);
>
> I'm afraid we can't use &error_abort here because bdrv_attach_child()
> could still fail due to permission conflicts.
>
>> + }
>> + }
>> +
>> bdrv_refresh_limits(bs, NULL);
>>
>> bdrv_set_perm(reopen_state->bs, reopen_state->perm,
>
> Hm... Does bdrv_set_perm() work correctly when between bdrv_check_perm()
> and here the graph was changed?
>
> Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen
2019-02-12 17:27 ` Kevin Wolf
2019-02-21 14:46 ` Alberto Garcia
@ 2019-02-21 14:53 ` Alberto Garcia
1 sibling, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-02-21 14:53 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
(sorry I accidentally sent an incomplete reply a few minutes ago)
On Tue 12 Feb 2019 06:27:56 PM CET, Kevin Wolf wrote:
>> - In bdrv_reopen_commit(): perform the actual node replacement by
>> calling bdrv_set_backing_hd(). It may happen that there are
>> temporary implicit nodes between the BDS that we are reopening and
>> the backing file that we want to replace (e.g. a commit fiter node),
>> so we must skip them.
>
> Hmm... I really dislike getting into the business of dealing with
> implicit nodes here. If you want to manage the block graph at the node
> level, you should manage all of it and just avoid getting implicit
> nodes in the first place. Otherwise, we'd have to guess where in the
> implicit chain you really want to add or remove nodes, and we're bound
> to guess wrong for some users.
>
> There is one problem with not skipping implicit nodes, though: Even if
> you don't want to manage things at the node level, we already force
> you to specify 'backing'. If there are implicit nodes, you don't knwo
> the real node name of the first backing child.
>
> So I suggest that we allow skipping implicit nodes for the purpose of
> leaving the backing link unchanged; but we return an error if you want
> to change the backing link and there are implicit nodes in the way.
That sounds good to me, and it doesn't really affect any of the test
cases that I had written.
>> + if (new_backing_bs) {
>> + if (bdrv_get_aio_context(new_backing_bs) != bdrv_get_aio_context(bs)) {
>> + error_setg(errp, "Cannot use a new backing file "
>> + "with a different AioContext");
>> + return -EINVAL;
>> + }
>> + }
>
> This is okay for a first version, but in reality, you'd usually want
> to just move the backing file into the right AioContext. Of course,
> this is only correct if the backing file doesn't have other users in
> different AioContexts. To get a good implementation for this, we'd
> probably need to store the AioContext in BdrvChild, like we already
> concluded for other use cases.
>
> Maybe document this as one of the problems to be solved before we can
> remove the x- prefix.
I agree, but I didn't want to mess with that yet. I added a comment
explaining that.
>> + /*
>> + * Skip all links that point to an implicit node, if any
>> + * (e.g. a commit filter node). We don't want to change those.
>> + */
>> + overlay_bs = bs;
>> + while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>> + overlay_bs = backing_bs(overlay_bs);
>> + }
>> +
>> + if (new_backing_bs != backing_bs(overlay_bs)) {
>> + if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
>> + errp)) {
>> + return -EPERM;
>> + }
>> + }
>
> Should this function check new_backing_bs->drv->supports_backing, too?
I don't think the new backing file needs to support backing files
itself, one could e.g. replace a backing file (or even a longer chain)
with a raw file containing the same data.
>> + /*
>> + * Change the backing file if a new one was specified. We do this
>> + * after updating bs->options, so bdrv_refresh_filename() (called
>> + * from bdrv_set_backing_hd()) has the new values.
>> + */
>> + if (change_backing_bs) {
>> + BlockDriverState *overlay = bs;
>> + /*
>> + * Skip all links that point to an implicit node, if any
>> + * (e.g. a commit filter node). We don't want to change those.
>> + */
>> + while (backing_bs(overlay) && backing_bs(overlay)->implicit) {
>> + overlay = backing_bs(overlay);
>> + }
>> + if (new_backing_bs != backing_bs(overlay)) {
>> + bdrv_set_backing_hd(overlay, new_backing_bs, &error_abort);
>
> I'm afraid we can't use &error_abort here because bdrv_attach_child()
> could still fail due to permission conflicts.
But those permissions should have been checked already in
bdrv_reopen_prepare(). This function cannot return an error.
>> bdrv_set_perm(reopen_state->bs, reopen_state->perm,
>
> Hm... Does bdrv_set_perm() work correctly when between
> bdrv_check_perm() and here the graph was changed?
I think you're right, bdrv_check_perm() might have been called on the
old backing file and it's not followed by set or abort if we change it.
I'll have a look at this.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-02-12 18:02 ` Kevin Wolf
@ 2019-03-01 12:12 ` Alberto Garcia
2019-03-01 12:36 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-03-01 12:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote:
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index fd0e88d17a..e680dda86b 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -345,6 +345,13 @@ struct BlockDriver {
>>
>> /* List of options for creating images, terminated by name == NULL */
>> QemuOptsList *create_opts;
>> + /* Runtime options for a block device, terminated by name == NULL */
>> + QemuOptsList *runtime_opts;
>
> I'm not sure if using a QemuOptsList here is a good idea. Currently,
> we use QemuOptsLists for most options, but there are some drivers that
> use it only for part of their options, or not at all, using direct
> QDict accesses or QAPI objects for the rest.
My intention was to avoid having two separate lists with the runtime
options of a driver. For this feature we really need that list to
contain all options, otherwise there's no way to know whether a missing
option is really missing or if it doesn't exist in the first place.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-03-01 12:12 ` Alberto Garcia
@ 2019-03-01 12:36 ` Kevin Wolf
2019-03-01 12:42 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-03-01 12:36 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 01.03.2019 um 13:12 hat Alberto Garcia geschrieben:
> On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote:
> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> index fd0e88d17a..e680dda86b 100644
> >> --- a/include/block/block_int.h
> >> +++ b/include/block/block_int.h
> >> @@ -345,6 +345,13 @@ struct BlockDriver {
> >>
> >> /* List of options for creating images, terminated by name == NULL */
> >> QemuOptsList *create_opts;
> >> + /* Runtime options for a block device, terminated by name == NULL */
> >> + QemuOptsList *runtime_opts;
> >
> > I'm not sure if using a QemuOptsList here is a good idea. Currently,
> > we use QemuOptsLists for most options, but there are some drivers that
> > use it only for part of their options, or not at all, using direct
> > QDict accesses or QAPI objects for the rest.
>
> My intention was to avoid having two separate lists with the runtime
> options of a driver. For this feature we really need that list to
> contain all options, otherwise there's no way to know whether a missing
> option is really missing or if it doesn't exist in the first place.
Yes, I understand your intention and the requirement. My point is just
that the existing QemuOptsLists are often _not_ complete, so they can't
fulfill the requirement.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-03-01 12:36 ` Kevin Wolf
@ 2019-03-01 12:42 ` Alberto Garcia
2019-03-01 12:56 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-03-01 12:42 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Fri 01 Mar 2019 01:36:08 PM CET, Kevin Wolf wrote:
> Am 01.03.2019 um 13:12 hat Alberto Garcia geschrieben:
>> On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote:
>> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> >> index fd0e88d17a..e680dda86b 100644
>> >> --- a/include/block/block_int.h
>> >> +++ b/include/block/block_int.h
>> >> @@ -345,6 +345,13 @@ struct BlockDriver {
>> >>
>> >> /* List of options for creating images, terminated by name == NULL */
>> >> QemuOptsList *create_opts;
>> >> + /* Runtime options for a block device, terminated by name == NULL */
>> >> + QemuOptsList *runtime_opts;
>> >
>> > I'm not sure if using a QemuOptsList here is a good idea. Currently,
>> > we use QemuOptsLists for most options, but there are some drivers that
>> > use it only for part of their options, or not at all, using direct
>> > QDict accesses or QAPI objects for the rest.
>>
>> My intention was to avoid having two separate lists with the runtime
>> options of a driver. For this feature we really need that list to
>> contain all options, otherwise there's no way to know whether a missing
>> option is really missing or if it doesn't exist in the first place.
>
> Yes, I understand your intention and the requirement. My point is just
> that the existing QemuOptsLists are often _not_ complete, so they
> can't fulfill the requirement.
Perhaps a new data structure with a list of runtime options and whether
they can be resetted to their default value or not.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-03-01 12:42 ` Alberto Garcia
@ 2019-03-01 12:56 ` Kevin Wolf
2019-03-01 13:05 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-03-01 12:56 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 01.03.2019 um 13:42 hat Alberto Garcia geschrieben:
> On Fri 01 Mar 2019 01:36:08 PM CET, Kevin Wolf wrote:
> > Am 01.03.2019 um 13:12 hat Alberto Garcia geschrieben:
> >> On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote:
> >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> >> index fd0e88d17a..e680dda86b 100644
> >> >> --- a/include/block/block_int.h
> >> >> +++ b/include/block/block_int.h
> >> >> @@ -345,6 +345,13 @@ struct BlockDriver {
> >> >>
> >> >> /* List of options for creating images, terminated by name == NULL */
> >> >> QemuOptsList *create_opts;
> >> >> + /* Runtime options for a block device, terminated by name == NULL */
> >> >> + QemuOptsList *runtime_opts;
> >> >
> >> > I'm not sure if using a QemuOptsList here is a good idea. Currently,
> >> > we use QemuOptsLists for most options, but there are some drivers that
> >> > use it only for part of their options, or not at all, using direct
> >> > QDict accesses or QAPI objects for the rest.
> >>
> >> My intention was to avoid having two separate lists with the runtime
> >> options of a driver. For this feature we really need that list to
> >> contain all options, otherwise there's no way to know whether a missing
> >> option is really missing or if it doesn't exist in the first place.
> >
> > Yes, I understand your intention and the requirement. My point is just
> > that the existing QemuOptsLists are often _not_ complete, so they
> > can't fulfill the requirement.
>
> Perhaps a new data structure with a list of runtime options and whether
> they can be resetted to their default value or not.
Basically just an array that contains names and bools, right? I think
that would work.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-03-01 12:56 ` Kevin Wolf
@ 2019-03-01 13:05 ` Alberto Garcia
2019-03-01 14:18 ` Kevin Wolf
0 siblings, 1 reply; 43+ messages in thread
From: Alberto Garcia @ 2019-03-01 13:05 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Fri 01 Mar 2019 01:56:42 PM CET, Kevin Wolf wrote:
>> >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> >> >> index fd0e88d17a..e680dda86b 100644
>> >> >> --- a/include/block/block_int.h
>> >> >> +++ b/include/block/block_int.h
>> >> >> @@ -345,6 +345,13 @@ struct BlockDriver {
>> >> >>
>> >> >> /* List of options for creating images, terminated by name == NULL */
>> >> >> QemuOptsList *create_opts;
>> >> >> + /* Runtime options for a block device, terminated by name == NULL */
>> >> >> + QemuOptsList *runtime_opts;
>> >> >
>> >> > I'm not sure if using a QemuOptsList here is a good idea. Currently,
>> >> > we use QemuOptsLists for most options, but there are some drivers that
>> >> > use it only for part of their options, or not at all, using direct
>> >> > QDict accesses or QAPI objects for the rest.
>> >>
>> >> My intention was to avoid having two separate lists with the runtime
>> >> options of a driver. For this feature we really need that list to
>> >> contain all options, otherwise there's no way to know whether a missing
>> >> option is really missing or if it doesn't exist in the first place.
>> >
>> > Yes, I understand your intention and the requirement. My point is just
>> > that the existing QemuOptsLists are often _not_ complete, so they
>> > can't fulfill the requirement.
>>
>> Perhaps a new data structure with a list of runtime options and whether
>> they can be resetted to their default value or not.
>
> Basically just an array that contains names and bools, right? I think
> that would work.
Yes exactly. My concern is that the array has to be updated by hand
every time a new option is added to the driver, so they could easily be
out of sync.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-03-01 13:05 ` Alberto Garcia
@ 2019-03-01 14:18 ` Kevin Wolf
2019-03-01 14:37 ` Alberto Garcia
0 siblings, 1 reply; 43+ messages in thread
From: Kevin Wolf @ 2019-03-01 14:18 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 01.03.2019 um 14:05 hat Alberto Garcia geschrieben:
> On Fri 01 Mar 2019 01:56:42 PM CET, Kevin Wolf wrote:
> >> >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> >> >> index fd0e88d17a..e680dda86b 100644
> >> >> >> --- a/include/block/block_int.h
> >> >> >> +++ b/include/block/block_int.h
> >> >> >> @@ -345,6 +345,13 @@ struct BlockDriver {
> >> >> >>
> >> >> >> /* List of options for creating images, terminated by name == NULL */
> >> >> >> QemuOptsList *create_opts;
> >> >> >> + /* Runtime options for a block device, terminated by name == NULL */
> >> >> >> + QemuOptsList *runtime_opts;
> >> >> >
> >> >> > I'm not sure if using a QemuOptsList here is a good idea. Currently,
> >> >> > we use QemuOptsLists for most options, but there are some drivers that
> >> >> > use it only for part of their options, or not at all, using direct
> >> >> > QDict accesses or QAPI objects for the rest.
> >> >>
> >> >> My intention was to avoid having two separate lists with the runtime
> >> >> options of a driver. For this feature we really need that list to
> >> >> contain all options, otherwise there's no way to know whether a missing
> >> >> option is really missing or if it doesn't exist in the first place.
> >> >
> >> > Yes, I understand your intention and the requirement. My point is just
> >> > that the existing QemuOptsLists are often _not_ complete, so they
> >> > can't fulfill the requirement.
> >>
> >> Perhaps a new data structure with a list of runtime options and whether
> >> they can be resetted to their default value or not.
> >
> > Basically just an array that contains names and bools, right? I think
> > that would work.
>
> Yes exactly. My concern is that the array has to be updated by hand
> every time a new option is added to the driver, so they could easily be
> out of sync.
Hm, actually, do you even need the runtime_opts list?
You use it in bdrv_reset_options_allowed() to iterate through each
option and then check whether it was present in the old set of options,
but not in the new set of options.
In order to achieve this, you can just iterate the old options dict
because you don't do anything with options that are present in the
QemuOptsList, but not the old options dict.
Kevin
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
2019-03-01 14:18 ` Kevin Wolf
@ 2019-03-01 14:37 ` Alberto Garcia
0 siblings, 0 replies; 43+ messages in thread
From: Alberto Garcia @ 2019-03-01 14:37 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Fri 01 Mar 2019 03:18:09 PM CET, Kevin Wolf wrote:
> Hm, actually, do you even need the runtime_opts list?
>
> You use it in bdrv_reset_options_allowed() to iterate through each
> option and then check whether it was present in the old set of
> options, but not in the new set of options.
>
> In order to achieve this, you can just iterate the old options dict
> because you don't do anything with options that are present in the
> QemuOptsList, but not the old options dict.
Hmm, that could work, I'll give it a try.
Berto
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2019-03-01 14:37 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-17 15:33 [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 01/13] block: Allow freezing BdrvChild links Alberto Garcia
2019-02-12 14:47 ` Kevin Wolf
2019-02-14 14:13 ` Alberto Garcia
2019-02-14 15:52 ` Kevin Wolf
2019-02-18 13:35 ` Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 02/13] block: Freeze the backing chain for the duration of the commit job Alberto Garcia
2019-02-12 14:54 ` Kevin Wolf
2019-02-14 15:21 ` Alberto Garcia
2019-02-14 15:45 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 03/13] block: Freeze the backing chain for the duration of the mirror job Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 04/13] block: Freeze the backing chain for the duration of the stream job Alberto Garcia
2019-02-12 15:15 ` Kevin Wolf
2019-02-12 16:06 ` Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 05/13] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue() Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 06/13] block: Handle child references in bdrv_reopen_queue() Alberto Garcia
2019-02-12 16:28 ` Kevin Wolf
2019-02-12 16:55 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
2019-02-19 16:00 ` [Qemu-devel] " Alberto Garcia
2019-01-17 15:33 ` [Qemu-devel] [PATCH 07/13] block: Allow omitting the 'backing' option in certain cases Alberto Garcia
2019-02-12 16:38 ` Kevin Wolf
2019-01-17 15:33 ` [Qemu-devel] [PATCH 08/13] block: Allow changing the backing file on reopen Alberto Garcia
2019-02-12 17:27 ` Kevin Wolf
2019-02-21 14:46 ` Alberto Garcia
2019-02-21 14:53 ` Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 09/13] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver Alberto Garcia
2019-02-12 18:02 ` Kevin Wolf
2019-03-01 12:12 ` Alberto Garcia
2019-03-01 12:36 ` Kevin Wolf
2019-03-01 12:42 ` Alberto Garcia
2019-03-01 12:56 ` Kevin Wolf
2019-03-01 13:05 ` Alberto Garcia
2019-03-01 14:18 ` Kevin Wolf
2019-03-01 14:37 ` Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 10/13] block: Add bdrv_reset_options_allowed() Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 11/13] block: Remove the AioContext parameter from bdrv_reopen_multiple() Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 12/13] block: Add an 'x-blockdev-reopen' QMP command Alberto Garcia
2019-01-17 15:34 ` [Qemu-devel] [PATCH 13/13] qemu-iotests: Test the x-blockdev-reopen " Alberto Garcia
2019-01-17 15:50 ` [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' " Eric Blake
2019-01-17 16:05 ` Alberto Garcia
2019-01-22 8:15 ` Vladimir Sementsov-Ogievskiy
2019-01-23 15:56 ` Alberto Garcia
2019-01-31 15:11 ` Alberto Garcia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.