[Qemu-devel] [PATCH v2 00/13] Add a 'x-blockdev-reopen' QMP command

[Qemu-devel] [PATCH v2 00/13] Add a 'x-blockdev-reopen' QMP command

[Alberto Garcia]

Hi,

here's a new version of the patches to add a new QMP command for
bdrv_reopen().

Refer to the cover letter of v1 for a complete description of the
feature:

   https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00623.html

Regards,

Berto

v2:
- Patch 1: Update documentation, fix check in bdrv_drop_intermediate()
  and add a new one in bdrv_replace_node() [Kevin]
- Patch 2: Add a missing unfreeze call if commit_start() fails, and
  prevent a double unfreeze in commit_abort() [Kevin]
- Patch 4: Prevent double unfreeze in stream_abort() [Kevin]
- Patch 6: Update documentation and don't complain for a missing
  'backing' option if the driver does not support backing files [Kevin]
- Patch 7: Contextual differences due to changes in previous patches.
- Patch 8: Forbid changing the backing file if there are implicit
  nodes on the way. Simplify code and update the permission checks to
  handle a new backing file [Kevin, Berto]
- Patch 9: Remove BlockDriver.runtime_opts from the previous version
  and use only mutable_opts [Kevin]
- Patch 10: Compare bs->options with reopen_state->options to see what
  options the user is trying to reset [Kevin]
- Patch 13: Update the tests to reflect all the new changes.

v1: https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00623.html
- 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

Output of backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/13:[0021] [FC] 'block: Allow freezing BdrvChild links'
002/13:[0010] [FC] 'block: Freeze the backing chain for the duration of the commit job'
003/13:[----] [--] 'block: Freeze the backing chain for the duration of the mirror job'
004/13:[0009] [FC] 'block: Freeze the backing chain for the duration of the stream job'
005/13:[----] [--] 'block: Add 'keep_old_opts' parameter to bdrv_reopen_queue()'
006/13:[0009] [FC] 'block: Handle child references in bdrv_reopen_queue()'
007/13:[0004] [FC] 'block: Allow omitting the 'backing' option in certain cases'
008/13:[0118] [FC] 'block: Allow changing the backing file on reopen'
009/13:[down] 'block: Add a 'mutable_opts' field to BlockDriver'
010/13:[0070] [FC] 'block: Add bdrv_reset_options_allowed()'
011/13:[----] [--] 'block: Remove the AioContext parameter from bdrv_reopen_multiple()'
012/13:[----] [--] 'block: Add an 'x-blockdev-reopen' QMP command'
013/13:[0036] [FC] 'qemu-iotests: Test the x-blockdev-reopen QMP command'

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 a 'mutable_opts' field 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                    | 390 ++++++++++++++++--
 block/commit.c             |  16 +
 block/file-posix.c         |   6 +
 block/mirror.c             |   8 +
 block/qcow2.c              |  25 ++
 block/raw-format.c         |   3 +
 block/replication.c        |   7 +-
 block/stream.c             |  21 +
 blockdev.c                 |  47 +++
 include/block/block.h      |  13 +-
 include/block/block_int.h  |  14 +
 qapi/block-core.json       |  42 ++
 qemu-io-cmds.c             |   4 +-
 tests/qemu-iotests/243     | 991 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/243.out |   5 +
 tests/qemu-iotests/group   |   1 +
 16 files changed, 1562 insertions(+), 31 deletions(-)
 create mode 100644 tests/qemu-iotests/243
 create mode 100644 tests/qemu-iotests/243.out

-- 
2.11.0

[Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links

[Alberto Garcia]

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.

After this change a few functions can fail, so they need additional
checks.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c                   | 76 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  5 ++++
 include/block/block_int.h |  6 ++++
 3 files changed, 87 insertions(+)

diff --git a/block.c b/block.c
index 35e78e2172..6e9c72f0cd 100644
--- a/block.c
+++ b/block.c
@@ -2127,6 +2127,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));
     }
@@ -2343,6 +2345,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);
     }
@@ -3712,6 +3718,11 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
         if (!should_update_child(c, to)) {
             continue;
         }
+        if (c->frozen) {
+            error_setg(errp, "Cannot change '%s' link to '%s'",
+                       c->name, from->node_name);
+            goto out;
+        }
         list = g_slist_prepend(list, c);
         perm |= c->perm;
         shared &= c->shared_perm;
@@ -3924,6 +3935,63 @@ 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 change '%s' link from '%s' to '%s'",
+                       i->backing->name, 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.
  *
@@ -3972,6 +4040,14 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         goto exit;
     }
 
+    /* This function changes all links that point to top and makes
+     * them point to base. Check that none of them is frozen. */
+    QLIST_FOREACH(c, &top->parents, next_parent) {
+        if (c->frozen) {
+            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 5b5cf868df..e4a25674b1 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 836d67c1ae..c8a83c3eea 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -709,6 +709,12 @@ struct BdrvChild {
     uint64_t backup_perm;
     uint64_t backup_shared_perm;
 
+    /*
+     * This link is frozen: the child can neither be replaced nor
+     * detached from the parent.
+     */
+    bool frozen;
+
     QLIST_ENTRY(BdrvChild) next;
     QLIST_ENTRY(BdrvChild) next_parent;
 };
-- 
2.11.0

[Qemu-devel] [PATCH v2 02/13] block: Freeze the backing chain for the duration of the commit job

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 3b46ca7f97..ba60fef58a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -39,6 +39,7 @@ typedef struct CommitBlockJob {
     BlockDriverState *base_bs;
     BlockdevOnError on_error;
     bool base_read_only;
+    bool chain_frozen;
     char *backing_file_str;
 } CommitBlockJob;
 
@@ -68,6 +69,9 @@ 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);
+    s->chain_frozen = false;
+
     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
      * the normal backing chain can be restored. */
     blk_unref(s->base);
@@ -84,6 +88,10 @@ static void commit_abort(Job *job)
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
     BlockDriverState *top_bs = blk_bs(s->top);
 
+    if (s->chain_frozen) {
+        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);
@@ -330,6 +338,11 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         }
     }
 
+    if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
+        goto fail;
+    }
+    s->chain_frozen = true;
+
     ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
     if (ret < 0) {
         goto fail;
@@ -362,6 +375,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     return;
 
 fail:
+    if (s->chain_frozen) {
+        bdrv_unfreeze_backing_chain(commit_top_bs, base);
+    }
     if (s->base) {
         blk_unref(s->base);
     }
-- 
2.11.0

[Qemu-devel] [PATCH v2 03/13] block: Freeze the backing chain for the duration of the mirror job

[Alberto Garcia]

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 726d3c27fb..010fdafd79 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,
@@ -1639,6 +1643,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

[Qemu-devel] [PATCH v2 04/13] block: Freeze the backing chain for the duration of the stream job

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/stream.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/stream.c b/block/stream.c
index e14579ff80..6253c86fae 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -35,6 +35,7 @@ typedef struct StreamBlockJob {
     BlockdevOnError on_error;
     char *backing_file_str;
     bool bs_read_only;
+    bool chain_frozen;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -49,6 +50,16 @@ 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);
+
+    if (s->chain_frozen) {
+        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);
@@ -58,6 +69,9 @@ static int stream_prepare(Job *job)
     Error *local_err = NULL;
     int ret = 0;
 
+    bdrv_unfreeze_backing_chain(bs, base);
+    s->chain_frozen = false;
+
     if (bs->backing) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
@@ -208,6 +222,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,
@@ -254,9 +269,15 @@ 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;
+    s->chain_frozen = true;
 
     s->on_error = on_error;
     trace_stream_start(bs, base, s);
-- 
2.11.0

[Qemu-devel] [PATCH v2 05/13] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue()

[Alberto Garcia]

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 6e9c72f0cd..e9ca49fb8e 100644
--- a/block.c
+++ b/block.c
@@ -3011,7 +3011,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);
 
@@ -3051,13 +3052,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);
 
@@ -3069,10 +3070,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);
@@ -3122,7 +3125,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;
@@ -3130,9 +3133,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);
 }
 
 /*
@@ -3204,7 +3208,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 4c80b54daf..a2f3590310 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 e4a25674b1..d36a3a3977 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 b9f189f09b..ff9a5cd80f 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2080,7 +2080,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

[Qemu-devel] [PATCH v2 06/13] block: Handle child references in bdrv_reopen_queue()

[Alberto Garcia]

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: if the child was implicitly created (i.e
      inherits_from points to the parent) then 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: if the child was implicitly
          created then it 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).
          If the child is not implicit then it keeps its current set
          of options but it is not reopened (and therefore does not
          inherit any new option from the parent).

      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. If keep_old_opts is true and "backing" is missing then
      this behaves like 2a (the current child is reopened).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 52 ++++++++++++++++++++++++++++++++++++++++++++-------
 include/block/block.h |  1 +
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index e9ca49fb8e..d7fa577c18 100644
--- a/block.c
+++ b/block.c
@@ -3108,9 +3108,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 their 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)
@@ -3119,13 +3131,32 @@ 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 is null or points to a different node.
+             */
+            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 (note that
+             * it can still inherit new options from the parent).
+             */
+            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;
@@ -3383,6 +3414,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     drv_prepared = true;
 
+    if (drv->supports_backing && 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 d36a3a3977..ee02b69a81 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

[Qemu-devel] [PATCH v2 07/13] block: Allow omitting the 'backing' option in certain cases

[Alberto Garcia]

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 d7fa577c18..6db8a65e61 100644
--- a/block.c
+++ b/block.c
@@ -3414,7 +3414,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     drv_prepared = true;
 
-    if (drv->supports_backing && 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 (drv->supports_backing && 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

[Qemu-devel] [PATCH v2 08/13] block: Allow changing the backing file on reopen

[Alberto Garcia]

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

There may be temporary implicit nodes between a BDS and its backing
file (e.g. a commit filter node). In these cases bdrv_reopen_prepare()
looks for the real (non-implicit) backing file and requires that the
'backing' option points to it. Replacing or detaching a backing file
is forbidden if there are implicit nodes in the middle.

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               | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |   2 +
 2 files changed, 164 insertions(+)

diff --git a/block.c b/block.c
index 6db8a65e61..9c83adab7a 100644
--- a/block.c
+++ b/block.c
@@ -2985,6 +2985,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.
  *
@@ -3294,6 +3315,98 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
 }
 
 /*
+ * Take a BDRVReopenState and check if the value of 'backing' in the
+ * reopen_state->options QDict is valid or not.
+ *
+ * If 'backing' is missing from the QDict then return 0.
+ *
+ * If 'backing' contains the node name of the backing file of
+ * reopen_state->bs then return 0.
+ *
+ * If 'backing' contains a different node name (or is null) then check
+ * whether the current backing file can be replaced with the new one.
+ * If that's the case then reopen_state->replace_backing_bs is set to
+ * true and reopen_state->new_backing_bs contains a pointer to the new
+ * backing BlockDriverState (or NULL).
+ *
+ * Return 0 on success, otherwise return < 0 and set @errp.
+ */
+static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
+                                     Error **errp)
+{
+    BlockDriverState *bs = reopen_state->bs;
+    BlockDriverState *overlay_bs, *new_backing_bs;
+    QObject *value;
+    const char *str;
+
+    value = qdict_get(reopen_state->options, "backing");
+    if (value == NULL) {
+        return 0;
+    }
+
+    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();
+    }
+
+    /*
+     * TODO: before removing the x- prefix from x-blockdev-reopen we
+     * should move the new backing file into the right AioContext
+     * instead of returning an error.
+     */
+    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;
+        }
+    }
+
+    /*
+     * Find the "actual" backing file by skipping all links that point
+     * to an implicit node, if any (e.g. a commit filter node).
+     */
+    overlay_bs = bs;
+    while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
+        overlay_bs = backing_bs(overlay_bs);
+    }
+
+    /* If we want to replace the backing file we need some extra checks */
+    if (new_backing_bs != backing_bs(overlay_bs)) {
+        /* Check for implicit nodes between bs and its backing file */
+        if (bs != overlay_bs) {
+            error_setg(errp, "Cannot change backing link if '%s' has "
+                       "an implicit backing file", bs->node_name);
+            return -EPERM;
+        }
+        /* Check if the backing link that we want to replace is frozen */
+        if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
+                                         errp)) {
+            return -EPERM;
+        }
+        reopen_state->replace_backing_bs = true;
+        reopen_state->new_backing_bs = new_backing_bs;
+    }
+
+    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()
@@ -3427,6 +3540,17 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    /*
+     * 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.
+     */
+    ret = bdrv_reopen_parse_backing(reopen_state, errp);
+    if (ret < 0) {
+        goto error;
+    }
+    qdict_del(reopen_state->options, "backing");
+
     /* 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) */
@@ -3485,6 +3609,20 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    /* Check if new_backing_bs would accept the new permissions */
+    if (reopen_state->replace_backing_bs && reopen_state->new_backing_bs) {
+        uint64_t cur_perm, cur_shared;
+        bdrv_child_perm(reopen_state->bs, reopen_state->new_backing_bs,
+                        NULL, &child_backing, NULL,
+                        reopen_state->perm, reopen_state->shared_perm,
+                        &cur_perm, &cur_shared);
+        ret = bdrv_check_update_perm(reopen_state->new_backing_bs, NULL,
+                                     cur_perm, cur_shared, NULL, errp);
+        if (ret < 0) {
+            goto error;
+        }
+    }
+
     ret = 0;
 
     /* Restore the original reopen_state->options QDict */
@@ -3542,6 +3680,11 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
     bs->detect_zeroes      = reopen_state->detect_zeroes;
 
+    if (reopen_state->replace_backing_bs) {
+        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) {
@@ -3549,6 +3692,21 @@ 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 (reopen_state->replace_backing_bs) {
+        BlockDriverState *old_backing_bs = backing_bs(bs);
+        assert(!old_backing_bs || !old_backing_bs->implicit);
+        /* Abort the permission update on the backing bs we're detaching */
+        if (old_backing_bs) {
+            bdrv_abort_perm_update(old_backing_bs);
+        }
+        bdrv_set_backing_hd(bs, reopen_state->new_backing_bs, &error_abort);
+    }
+
     bdrv_refresh_limits(bs, NULL);
 
     bdrv_set_perm(reopen_state->bs, reopen_state->perm,
@@ -3587,6 +3745,10 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
     }
 
     bdrv_abort_perm_update(reopen_state->bs);
+
+    if (reopen_state->replace_backing_bs && reopen_state->new_backing_bs) {
+        bdrv_abort_perm_update(reopen_state->new_backing_bs);
+    }
 }
 
 
diff --git a/include/block/block.h b/include/block/block.h
index ee02b69a81..3bf83d458a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -188,6 +188,8 @@ typedef struct BDRVReopenState {
     int flags;
     BlockdevDetectZeroesOptions detect_zeroes;
     bool backing_missing;
+    bool replace_backing_bs;  /* new_backing_bs is ignored if this is false */
+    BlockDriverState *new_backing_bs; /* If NULL then detach the current bs */
     uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
-- 
2.11.0

[Qemu-devel] [PATCH v2 09/13] block: Add a 'mutable_opts' field to BlockDriver

[Alberto Garcia]

If we reopen a BlockDriverState and there is an option that is present
in bs->options but missing from the new set of options then we have to
return an error unless the driver is able to reset it to its default
value.

This patch adds a new 'mutable_opts' field to BlockDriver. This is
a list of runtime options that can be modified during reopen. If an
option in this list is unspecified on reopen then it must be reset (or
return an error).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/file-posix.c        |  6 ++++++
 block/qcow2.c             | 25 +++++++++++++++++++++++++
 block/raw-format.c        |  3 +++
 include/block/block_int.h |  8 ++++++++
 4 files changed, 42 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index ba6ab62a38..07c91a985f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -431,6 +431,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)
@@ -2766,6 +2768,7 @@ BlockDriver bdrv_file = {
     .bdrv_set_perm   = raw_set_perm,
     .bdrv_abort_perm_update = raw_abort_perm_update,
     .create_opts = &raw_create_opts,
+    .mutable_opts = mutable_opts,
 };
 
 /***********************************************/
@@ -3217,6 +3220,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_co_create_opts = hdev_co_create_opts,
     .create_opts         = &raw_create_opts,
+    .mutable_opts        = mutable_opts,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
     .bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
 
@@ -3343,6 +3347,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_co_create_opts = hdev_co_create_opts,
     .create_opts         = &raw_create_opts,
+    .mutable_opts        = mutable_opts,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
 
 
@@ -3476,6 +3481,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_co_create_opts = hdev_co_create_opts,
     .create_opts        = &raw_create_opts,
+    .mutable_opts       = mutable_opts,
 
     .bdrv_co_preadv         = raw_co_preadv,
     .bdrv_co_pwritev        = raw_co_pwritev,
diff --git a/block/qcow2.c b/block/qcow2.c
index 7fb2730f09..9421f0d847 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -611,6 +611,30 @@ int qcow2_validate_table(BlockDriverState *bs, uint64_t offset,
     return 0;
 }
 
+static const char *const mutable_opts[] = {
+    QCOW2_OPT_LAZY_REFCOUNTS,
+    QCOW2_OPT_DISCARD_REQUEST,
+    QCOW2_OPT_DISCARD_SNAPSHOT,
+    QCOW2_OPT_DISCARD_OTHER,
+    QCOW2_OPT_OVERLAP,
+    QCOW2_OPT_OVERLAP_TEMPLATE,
+    QCOW2_OPT_OVERLAP_MAIN_HEADER,
+    QCOW2_OPT_OVERLAP_ACTIVE_L1,
+    QCOW2_OPT_OVERLAP_ACTIVE_L2,
+    QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
+    QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
+    QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
+    QCOW2_OPT_OVERLAP_INACTIVE_L1,
+    QCOW2_OPT_OVERLAP_INACTIVE_L2,
+    QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
+    QCOW2_OPT_CACHE_SIZE,
+    QCOW2_OPT_L2_CACHE_SIZE,
+    QCOW2_OPT_L2_CACHE_ENTRY_SIZE,
+    QCOW2_OPT_REFCOUNT_CACHE_SIZE,
+    QCOW2_OPT_CACHE_CLEAN_INTERVAL,
+    NULL
+};
+
 static QemuOptsList qcow2_runtime_opts = {
     .name = "qcow2",
     .head = QTAILQ_HEAD_INITIALIZER(qcow2_runtime_opts.head),
@@ -5053,6 +5077,7 @@ BlockDriver bdrv_qcow2 = {
 
     .create_opts         = &qcow2_create_opts,
     .strong_runtime_opts = qcow2_strong_runtime_opts,
+    .mutable_opts        = mutable_opts,
     .bdrv_co_check       = qcow2_co_check,
     .bdrv_amend_options  = qcow2_amend_options,
 
diff --git a/block/raw-format.c b/block/raw-format.c
index e3e5ba2c8a..cec29986cc 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -37,6 +37,8 @@ typedef struct BDRVRawState {
     bool has_size;
 } BDRVRawState;
 
+static const char *const mutable_opts[] = { "offset", "size", NULL };
+
 static QemuOptsList raw_runtime_opts = {
     .name = "raw",
     .head = QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head),
@@ -570,6 +572,7 @@ BlockDriver bdrv_raw = {
     .create_opts          = &raw_create_opts,
     .bdrv_has_zero_init   = &raw_has_zero_init,
     .strong_runtime_opts  = raw_strong_runtime_opts,
+    .mutable_opts         = mutable_opts,
 };
 
 static void bdrv_raw_init(void)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c8a83c3eea..8e3f3760a0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -381,6 +381,14 @@ struct BlockDriver {
 
     /* List of options for creating images, terminated by name == NULL */
     QemuOptsList *create_opts;
+    /*
+     * If this driver supports reopening images this contains a
+     * NULL-terminated list of the runtime options that can be
+     * modified. If an option in this list is unspecified during
+     * reopen then it _must_ be reset to its default value or return
+     * an error.
+     */
+    const char *const *mutable_opts;
 
     /*
      * Returns 0 for completed check, -errno for internal errors.
-- 
2.11.0

[Qemu-devel] [PATCH v2 10/13] block: Add bdrv_reset_options_allowed()

[Alberto Garcia]

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 BlockDriverState and a new set of options
and checks if there's any option that was previously set but is
missing from the new set of options.

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 | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/block.c b/block.c
index 9c83adab7a..83788457e1 100644
--- a/block.c
+++ b/block.c
@@ -2984,6 +2984,53 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
                              NULL, errp);
 }
 
+/* Return true if the NULL-terminated @list contains @str */
+static bool is_str_in_list(const char *str, const char *const *list)
+{
+    if (str && list) {
+        int i;
+        for (i = 0; list[i] != NULL; i++) {
+            if (!strcmp(str, list[i])) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+/*
+ * Check that every option set in @bs->options is also set in
+ * @new_opts.
+ *
+ * Options listed in the common_options list and in
+ * @bs->drv->mutable_opts are skipped.
+ *
+ * Return 0 on success, otherwise return -EINVAL and set @errp.
+ */
+static int bdrv_reset_options_allowed(BlockDriverState *bs,
+                                      const QDict *new_opts, Error **errp)
+{
+    const QDictEntry *e;
+    /* These options are common to all block drivers and are handled
+     * in bdrv_reopen_prepare() so they can be left out of @new_opts */
+    const char *const common_options[] = {
+        "node-name", "discard", "cache.direct", "cache.no-flush",
+        "read-only", "auto-read-only", "detect-zeroes", NULL
+    };
+
+    for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
+        if (!qdict_haskey(new_opts, e->key) &&
+            !is_str_in_list(e->key, common_options) &&
+            !is_str_in_list(e->key, bs->drv->mutable_opts)) {
+            error_setg(errp, "Option '%s' cannot be reset "
+                       "to its default value", e->key);
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 /*
  * Returns true if @child can be reached recursively from @bs
  */
@@ -3504,6 +3551,17 @@ 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.
+         */
+        ret = bdrv_reset_options_allowed(reopen_state->bs,
+                                         reopen_state->options, errp);
+        if (ret) {
+            goto error;
+        }
+
         ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
         if (ret) {
             if (local_err != NULL) {
-- 
2.11.0

[Qemu-devel] [PATCH v2 11/13] block: Remove the AioContext parameter from bdrv_reopen_multiple()

[Alberto Garcia]

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 83788457e1..56a90ed5c0 100644
--- a/block.c
+++ b/block.c
@@ -3255,7 +3255,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;
@@ -3308,7 +3308,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 a2f3590310..b95bd28802 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 3bf83d458a..fd306ee5d0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -304,7 +304,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 ff9a5cd80f..35dcdcf413 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2081,7 +2081,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

[Qemu-devel] [PATCH v2 12/13] block: Add an 'x-blockdev-reopen' QMP command

[Alberto Garcia]

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 7e6bf9955c..42510a959a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4285,6 +4285,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 2b8afbb924..05577a22f6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3982,6 +3982,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

[Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Alberto Garcia]

This patch adds several tests for the x-blockdev-reopen QMP command.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/243     | 991 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/243.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 997 insertions(+)
 create mode 100644 tests/qemu-iotests/243
 create mode 100644 tests/qemu-iotests/243.out

diff --git a/tests/qemu-iotests/243 b/tests/qemu-iotests/243
new file mode 100644
index 0000000000..35b30092ca
--- /dev/null
+++ b/tests/qemu-iotests/243
@@ -0,0 +1,991 @@
+#!/usr/bin/env python
+#
+# Test cases for the QMP 'x-blockdev-reopen' command
+#
+# Copyright (C) 2018-2019 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' cannot 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' cannot 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' cannot 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' cannot 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' cannot 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 change 'backing' link from 'hd1' to 'hd2'")
+
+        # We can't remove hd1 while the stream job is ongoing
+        opts['backing'] = None
+        self.reopen(opts, {}, "Cannot change 'backing' 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 change 'backing' 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 change 'backing' link from 'hd1' to 'hd2'")
+
+        # We can't remove hd1 while the commit job is ongoing
+        opts['backing'] = None
+        self.reopen(opts, {}, "Cannot change 'backing' 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', {})
+
+        # 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 change backing link if 'hd0' has an implicit backing file")
+
+        # hd2 <- hd0
+        self.wait_until_completed(drive = 'commit0')
+
+        self.assert_qmp(self.get_node('hd0'), 'ro', False)
+        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/243.out b/tests/qemu-iotests/243.out
new file mode 100644
index 0000000000..71009c239f
--- /dev/null
+++ b/tests/qemu-iotests/243.out
@@ -0,0 +1,5 @@
+..................
+----------------------------------------------------------------------
+Ran 18 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b5ca63cf72..a0cceb500b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -241,3 +241,4 @@
 239 rw auto quick
 240 auto quick
 242 rw auto quick
+243 rw auto
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On 06.03.19 19:11, Alberto Garcia wrote:
> This patch adds several tests for the x-blockdev-reopen QMP command.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/243     | 991 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/243.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 997 insertions(+)
>  create mode 100644 tests/qemu-iotests/243
>  create mode 100644 tests/qemu-iotests/243.out
> 
> diff --git a/tests/qemu-iotests/243 b/tests/qemu-iotests/243
> new file mode 100644
> index 0000000000..35b30092ca
> --- /dev/null
> +++ b/tests/qemu-iotests/243
> @@ -0,0 +1,991 @@

[...]

> +    # 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):

[...]

> +        # hd0 has no backing file: we can omit the 'backing' option
> +        self.reopen(opts)

[...]

> +        # Detach hd2 from hd0.
> +        self.reopen(opts, {'backing': None})
> +        self.reopen(opts, {}, "backing is missing for 'hd0'")

I don’t understand the second test.  hd0 has no default backing file and
it currently has no backing child attached to it.  Why would this call
fail now?

I’m asking because this no longer throws an error after “block: Leave
BDS.backing_file constant” of my “block: Deal with filters” series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On 06.03.19 19:11, Alberto Garcia wrote:
> This patch adds several tests for the x-blockdev-reopen QMP command.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/243     | 991 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/243.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 997 insertions(+)
>  create mode 100644 tests/qemu-iotests/243
>  create mode 100644 tests/qemu-iotests/243.out
> 
> diff --git a/tests/qemu-iotests/243 b/tests/qemu-iotests/243
> new file mode 100644
> index 0000000000..35b30092ca
> --- /dev/null
> +++ b/tests/qemu-iotests/243
> @@ -0,0 +1,991 @@

[...]

> +    # 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):

[...]

> +        # hd0 has no backing file: we can omit the 'backing' option
> +        self.reopen(opts)

[...]

> +        # Detach hd2 from hd0.
> +        self.reopen(opts, {'backing': None})
> +        self.reopen(opts, {}, "backing is missing for 'hd0'")

I don’t understand the second test.  hd0 has no default backing file and
it currently has no backing child attached to it.  Why would this call
fail now?

I’m asking because this no longer throws an error after “block: Leave
BDS.backing_file constant” of my “block: Deal with filters” series.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Alberto Garcia]

On Wed 10 Apr 2019 07:03:50 PM CEST, Max Reitz wrote:
>> +        # hd0 has no backing file: we can omit the 'backing' option
>> +        self.reopen(opts)
>
> [...]
>
>> +        # Detach hd2 from hd0.
>> +        self.reopen(opts, {'backing': None})
>> +        self.reopen(opts, {}, "backing is missing for 'hd0'")
>
> I don’t understand the second test.  hd0 has no default backing file
> and it currently has no backing child attached to it.  Why would this
> call fail now?

I think there's a bug.

Calling x-blockdev-reopen without 'backing' should only fail if

 a) the image has a backing file attached to it.
    In this case it doesn't: we just detached it in the previous line.

 b) there's a default backing file written on the image header.
    In this case there isn't (hd0 is created without one in setUp()).

So it should not fail. I think the bug is that the test for condition
(b) in bdrv_reopen_prepare() that returns "backing is missing..." is
using backing_file but it should use (correct me if I'm wrong)
auto_backing_file.

Changing that and replacing the test line with self.reopen(opts) fixes
it for me.

Not directly related to this, but should bdrv_backing_detach() also
clear backing_file ?

Berto

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Alberto Garcia]

On Wed 10 Apr 2019 07:03:50 PM CEST, Max Reitz wrote:
>> +        # hd0 has no backing file: we can omit the 'backing' option
>> +        self.reopen(opts)
>
> [...]
>
>> +        # Detach hd2 from hd0.
>> +        self.reopen(opts, {'backing': None})
>> +        self.reopen(opts, {}, "backing is missing for 'hd0'")
>
> I don’t understand the second test.  hd0 has no default backing file
> and it currently has no backing child attached to it.  Why would this
> call fail now?

I think there's a bug.

Calling x-blockdev-reopen without 'backing' should only fail if

 a) the image has a backing file attached to it.
    In this case it doesn't: we just detached it in the previous line.

 b) there's a default backing file written on the image header.
    In this case there isn't (hd0 is created without one in setUp()).

So it should not fail. I think the bug is that the test for condition
(b) in bdrv_reopen_prepare() that returns "backing is missing..." is
using backing_file but it should use (correct me if I'm wrong)
auto_backing_file.

Changing that and replacing the test line with self.reopen(opts) fixes
it for me.

Not directly related to this, but should bdrv_backing_detach() also
clear backing_file ?

Berto

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On 11.04.19 15:41, Alberto Garcia wrote:
> On Wed 10 Apr 2019 07:03:50 PM CEST, Max Reitz wrote:
>>> +        # hd0 has no backing file: we can omit the 'backing' option
>>> +        self.reopen(opts)
>>
>> [...]
>>
>>> +        # Detach hd2 from hd0.
>>> +        self.reopen(opts, {'backing': None})
>>> +        self.reopen(opts, {}, "backing is missing for 'hd0'")
>>
>> I don’t understand the second test.  hd0 has no default backing file
>> and it currently has no backing child attached to it.  Why would this
>> call fail now?
> 
> I think there's a bug.
> 
> Calling x-blockdev-reopen without 'backing' should only fail if
> 
>  a) the image has a backing file attached to it.
>     In this case it doesn't: we just detached it in the previous line.
> 
>  b) there's a default backing file written on the image header.
>     In this case there isn't (hd0 is created without one in setUp()).

That’s what I thought, too, hence me applying effectively the same
change to the test in v4 of my series as you in your "Fix check for
default backing files" series:

http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

> So it should not fail. I think the bug is that the test for condition
> (b) in bdrv_reopen_prepare() that returns "backing is missing..." is
> using backing_file but it should use (correct me if I'm wrong)
> auto_backing_file.

Well, I think both should be fine, because...

> Changing that and replacing the test line with self.reopen(opts) fixes
> it for me.
> 
> Not directly related to this, but should bdrv_backing_detach() also
> clear backing_file ?

...I don’t think it should.  That’s what that my patch addresses.  The
real problem is that bs->backing_file is not a cache for
bs->backing->bs->filename, so it shouldn’t be treated as such.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On 11.04.19 15:41, Alberto Garcia wrote:
> On Wed 10 Apr 2019 07:03:50 PM CEST, Max Reitz wrote:
>>> +        # hd0 has no backing file: we can omit the 'backing' option
>>> +        self.reopen(opts)
>>
>> [...]
>>
>>> +        # Detach hd2 from hd0.
>>> +        self.reopen(opts, {'backing': None})
>>> +        self.reopen(opts, {}, "backing is missing for 'hd0'")
>>
>> I don’t understand the second test.  hd0 has no default backing file
>> and it currently has no backing child attached to it.  Why would this
>> call fail now?
> 
> I think there's a bug.
> 
> Calling x-blockdev-reopen without 'backing' should only fail if
> 
>  a) the image has a backing file attached to it.
>     In this case it doesn't: we just detached it in the previous line.
> 
>  b) there's a default backing file written on the image header.
>     In this case there isn't (hd0 is created without one in setUp()).

That’s what I thought, too, hence me applying effectively the same
change to the test in v4 of my series as you in your "Fix check for
default backing files" series:

http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html

> So it should not fail. I think the bug is that the test for condition
> (b) in bdrv_reopen_prepare() that returns "backing is missing..." is
> using backing_file but it should use (correct me if I'm wrong)
> auto_backing_file.

Well, I think both should be fine, because...

> Changing that and replacing the test line with self.reopen(opts) fixes
> it for me.
> 
> Not directly related to this, but should bdrv_backing_detach() also
> clear backing_file ?

...I don’t think it should.  That’s what that my patch addresses.  The
real problem is that bs->backing_file is not a cache for
bs->backing->bs->filename, so it shouldn’t be treated as such.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Alberto Garcia]

On Sat 13 Apr 2019 02:53:42 AM CEST, Max Reitz wrote:
>> Calling x-blockdev-reopen without 'backing' should only fail if
>> 
>>  a) the image has a backing file attached to it.
>>     In this case it doesn't: we just detached it in the previous line.
>> 
>>  b) there's a default backing file written on the image header.
>>     In this case there isn't (hd0 is created without one in setUp()).
>
> That’s what I thought, too, hence me applying effectively the same
> change to the test in v4 of my series as you in your "Fix check for
> default backing files" series:
>
> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html
>
>> So it should not fail. I think the bug is that the test for condition
>> (b) in bdrv_reopen_prepare() that returns "backing is missing..." is
>> using backing_file but it should use (correct me if I'm wrong)
>> auto_backing_file.
>
> Well, I think both should be fine, because...

Why would both be fine? backing_file refers to the backing file
currently attached, and auto_backing_file refers to the one written on
the image metadata, or am I wrong?

>> Not directly related to this, but should bdrv_backing_detach() also
>> clear backing_file ?
>
> ...I don’t think it should.  That’s what that my patch addresses. The
> real problem is that bs->backing_file is not a cache for
> bs->backing->bs->filename, so it shouldn’t be treated as such.

But what's the point of having backing_file set if no backing file is
attached?

Berto

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

On 14.05.19 13:30, Alberto Garcia wrote:
> On Sat 13 Apr 2019 02:53:42 AM CEST, Max Reitz wrote:
>>> Calling x-blockdev-reopen without 'backing' should only fail if
>>>
>>>  a) the image has a backing file attached to it.
>>>     In this case it doesn't: we just detached it in the previous line.
>>>
>>>  b) there's a default backing file written on the image header.
>>>     In this case there isn't (hd0 is created without one in setUp()).
>>
>> That’s what I thought, too, hence me applying effectively the same
>> change to the test in v4 of my series as you in your "Fix check for
>> default backing files" series:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html
>>
>>> So it should not fail. I think the bug is that the test for condition
>>> (b) in bdrv_reopen_prepare() that returns "backing is missing..." is
>>> using backing_file but it should use (correct me if I'm wrong)
>>> auto_backing_file.
>>
>> Well, I think both should be fine, because...
> 
> Why would both be fine? backing_file refers to the backing file
> currently attached, and auto_backing_file refers to the one written on
> the image metadata, or am I wrong?

After my series, backing_file refers to the image metadata.

>>> Not directly related to this, but should bdrv_backing_detach() also
>>> clear backing_file ?
>>
>> ...I don’t think it should.  That’s what that my patch addresses. The
>> real problem is that bs->backing_file is not a cache for
>> bs->backing->bs->filename, so it shouldn’t be treated as such.
> 
> But what's the point of having backing_file set if no backing file is
> attached?

What’s the point of having backing_file set to the same value as
bs->backing->bs->filename?

We need some storage for “What does the image header say”.  Currently,
that sometimes is BDS.backing_file.  Sometimes it isn’t.  That’s broken.
 See
http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html.
BDS.auto_backing_file does not refer to the image metadata.  It refers
to something like bdrv_refresh_filename(bdrv_open(bs->backing_file)).
We need this just to calculate @backing_overridden in
bdrv_refresh_filename().

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Alberto Garcia]

On Tue 14 May 2019 03:02:33 PM CEST, Max Reitz wrote:
>> Why would both be fine? backing_file refers to the backing file
>> currently attached, and auto_backing_file refers to the one written
>> on the image metadata, or am I wrong?
>
> After my series, backing_file refers to the image metadata.

I see, clarified!

Berto