[PATCH v5 0/6] Make blockdev-reopen stable

[PATCH v5 0/6] Make blockdev-reopen stable

[Kevin Wolf]

This series picks up the remaining patches from Berto's series "[PATCH
v4 0/6] Allow changing bs->file on reopen", which are not merged into
master yet.

Apart from renaming 'x-blockdev-reopen' into 'blockdev-reopen', the
remaining functional change in this series is taking a list of nodes to
reopen as an argument so that multiple changes to the graph can be made
atomically that would be invalid separately (e.g. due to permission
checks on the intermediate state).

It also contains a qcow2 fix for a bug introduced by the part of the
series that was already picked up in Vladimir's "[PATCH v6 0/9] Allow
changing bs->file on reopen".

Alberto Garcia (4):
  block: Add bdrv_reopen_queue_free()
  block: Support multiple reopening with x-blockdev-reopen
  iotests: Test reopening multiple devices at the same time
  block: Make blockdev-reopen stable API

Kevin Wolf (2):
  qcow2: Fix dangling pointer after reopen for 'file'
  block: Acquire AioContexts during bdrv_reopen_multiple()

 qapi/block-core.json                          | 24 +++---
 include/block/block.h                         |  3 +
 block.c                                       | 71 +++++++++++++----
 block/qcow2.c                                 | 14 ++++
 block/replication.c                           |  7 ++
 blockdev.c                                    | 76 ++++++++++--------
 qemu-io-cmds.c                                |  7 +-
 tests/qemu-iotests/155                        |  9 ++-
 tests/qemu-iotests/165                        |  4 +-
 tests/qemu-iotests/245                        | 78 +++++++++++++++----
 tests/qemu-iotests/245.out                    |  4 +-
 tests/qemu-iotests/248                        |  4 +-
 tests/qemu-iotests/248.out                    |  2 +-
 tests/qemu-iotests/296                        | 11 ++-
 tests/qemu-iotests/298                        |  4 +-
 .../tests/remove-bitmap-from-backing          | 22 +++---
 16 files changed, 240 insertions(+), 100 deletions(-)

-- 
2.31.1

[PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'

[Kevin Wolf]

Without an external data file, s->data_file is a second pointer with the
same value as bs->file. When changing bs->file to a different BdrvChild
and freeing the old BdrvChild, s->data_file must also be updated,
otherwise it points to freed memory and causes crashes.

This problem was caught by iotests case 245.

Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index ee4530cdbd..cb459ef6a6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
 }
 
 typedef struct Qcow2ReopenState {
+    bool had_data_file;
     Qcow2Cache *l2_table_cache;
     Qcow2Cache *refcount_block_cache;
     int l2_slice_size; /* Number of entries in a slice of the L2 table */
@@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
     r = g_new0(Qcow2ReopenState, 1);
     state->opaque = r;
 
+    r->had_data_file = has_data_file(state->bs);
+
     ret = qcow2_update_options_prepare(state->bs, r, state->options,
                                        state->flags, errp);
     if (ret < 0) {
@@ -1966,7 +1969,18 @@ fail:
 
 static void qcow2_reopen_commit(BDRVReopenState *state)
 {
+    BDRVQcow2State *s = state->bs->opaque;
+    Qcow2ReopenState *r = state->opaque;
+
     qcow2_update_options_commit(state->bs, state->opaque);
+    if (!r->had_data_file && s->data_file != state->bs->file) {
+        /*
+         * If s->data_file is just a second pointer to bs->file (which is the
+         * case without an external data file), it may need to be updated.
+         */
+        s->data_file = state->bs->file;
+        assert(!has_data_file(state->bs));
+    }
     g_free(state->opaque);
 }
 
-- 
2.31.1

[PATCH v5 2/6] block: Add bdrv_reopen_queue_free()

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

Move the code to free a BlockReopenQueue to a separate function.
It will be used in a subsequent patch.

[ kwolf: Also free explicit_options and options, and explicitly
  qobject_ref() the value when it continues to be used. This avoids
  memory leaks as we saw them in two recent patches. ]

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  1 +
 block.c               | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 7ec77ecb1a..6d42992985 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -386,6 +386,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);
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp);
diff --git a/block.c b/block.c
index acd35cb0cb..ee9b46e95e 100644
--- a/block.c
+++ b/block.c
@@ -4095,6 +4095,19 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                    NULL, 0, keep_old_opts);
 }
 
+void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
+{
+    if (bs_queue) {
+        BlockReopenQueueEntry *bs_entry, *next;
+        QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+            qobject_unref(bs_entry->state.explicit_options);
+            qobject_unref(bs_entry->state.options);
+            g_free(bs_entry);
+        }
+        g_free(bs_queue);
+    }
+}
+
 /*
  * Reopen multiple BlockDriverStates atomically & transactionally.
  *
@@ -4197,15 +4210,10 @@ abort:
         if (bs_entry->prepared) {
             bdrv_reopen_abort(&bs_entry->state);
         }
-        qobject_unref(bs_entry->state.explicit_options);
-        qobject_unref(bs_entry->state.options);
     }
 
 cleanup:
-    QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-        g_free(bs_entry);
-    }
-    g_free(bs_queue);
+    bdrv_reopen_queue_free(bs_queue);
 
     return ret;
 }
@@ -4573,6 +4581,8 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     /* set BDS specific flags now */
     qobject_unref(bs->explicit_options);
     qobject_unref(bs->options);
+    qobject_ref(reopen_state->explicit_options);
+    qobject_ref(reopen_state->options);
 
     bs->explicit_options   = reopen_state->explicit_options;
     bs->options            = reopen_state->options;
-- 
2.31.1

[PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple()

[Kevin Wolf]

As the BlockReopenQueue can contain nodes in multiple AioContexts, only
one of which may be locked when AIO_WAIT_WHILE() can be called, we can't
let the caller lock the right contexts. Instead, individually lock the
AioContext of a single node when iterating the queue.

Reintroduce bdrv_reopen() as a wrapper for reopening a single node that
drains the node and temporarily drops the AioContext lock for
bdrv_reopen_multiple().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  2 ++
 block.c               | 49 ++++++++++++++++++++++++++++++++++++-------
 block/replication.c   |  7 +++++++
 blockdev.c            |  5 +++++
 qemu-io-cmds.c        |  7 +------
 5 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 6d42992985..3477290f9a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -388,6 +388,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     bool keep_old_opts);
 void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
+int bdrv_reopen(BlockDriverState *bs, QDict *opts, bool keep_old_opts,
+                Error **errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp);
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
diff --git a/block.c b/block.c
index ee9b46e95e..06fb69df5c 100644
--- a/block.c
+++ b/block.c
@@ -4124,19 +4124,26 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
  *
  * All affected nodes must be drained between bdrv_reopen_queue() and
  * bdrv_reopen_multiple().
+ *
+ * To be called from the main thread, with all other AioContexts unlocked.
  */
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 {
     int ret = -1;
     BlockReopenQueueEntry *bs_entry, *next;
+    AioContext *ctx;
     Transaction *tran = tran_new();
     g_autoptr(GHashTable) found = NULL;
     g_autoptr(GSList) refresh_list = NULL;
 
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
     assert(bs_queue != NULL);
 
     QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+        ctx = bdrv_get_aio_context(bs_entry->state.bs);
+        aio_context_acquire(ctx);
         ret = bdrv_flush(bs_entry->state.bs);
+        aio_context_release(ctx);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Error flushing drive");
             goto abort;
@@ -4145,7 +4152,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 
     QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
         assert(bs_entry->state.bs->quiesce_counter > 0);
+        ctx = bdrv_get_aio_context(bs_entry->state.bs);
+        aio_context_acquire(ctx);
         ret = bdrv_reopen_prepare(&bs_entry->state, bs_queue, tran, errp);
+        aio_context_release(ctx);
         if (ret < 0) {
             goto abort;
         }
@@ -4188,7 +4198,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
      * to first element.
      */
     QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
+        ctx = bdrv_get_aio_context(bs_entry->state.bs);
+        aio_context_acquire(ctx);
         bdrv_reopen_commit(&bs_entry->state);
+        aio_context_release(ctx);
     }
 
     tran_commit(tran);
@@ -4197,7 +4210,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
         BlockDriverState *bs = bs_entry->state.bs;
 
         if (bs->drv->bdrv_reopen_commit_post) {
+            ctx = bdrv_get_aio_context(bs);
+            aio_context_acquire(ctx);
             bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
+            aio_context_release(ctx);
         }
     }
 
@@ -4208,7 +4224,10 @@ abort:
     tran_abort(tran);
     QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
         if (bs_entry->prepared) {
+            ctx = bdrv_get_aio_context(bs_entry->state.bs);
+            aio_context_acquire(ctx);
             bdrv_reopen_abort(&bs_entry->state);
+            aio_context_release(ctx);
         }
     }
 
@@ -4218,23 +4237,39 @@ cleanup:
     return ret;
 }
 
-int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
-                              Error **errp)
+int bdrv_reopen(BlockDriverState *bs, QDict *opts, bool keep_old_opts,
+                Error **errp)
 {
-    int ret;
+    AioContext *ctx = bdrv_get_aio_context(bs);
     BlockReopenQueue *queue;
-    QDict *opts = qdict_new();
-
-    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+    int ret;
 
     bdrv_subtree_drained_begin(bs);
-    queue = bdrv_reopen_queue(NULL, bs, opts, true);
+    if (ctx != qemu_get_aio_context()) {
+        aio_context_release(ctx);
+    }
+
+    queue = bdrv_reopen_queue(NULL, bs, opts, keep_old_opts);
     ret = bdrv_reopen_multiple(queue, errp);
+
+    if (ctx != qemu_get_aio_context()) {
+        aio_context_acquire(ctx);
+    }
     bdrv_subtree_drained_end(bs);
 
     return ret;
 }
 
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+                              Error **errp)
+{
+    QDict *opts = qdict_new();
+
+    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+    return bdrv_reopen(bs, opts, true, errp);
+}
+
 /*
  * Take a BDRVReopenState and check if the value of 'backing' in the
  * reopen_state->options QDict is valid or not.
diff --git a/block/replication.c b/block/replication.c
index 52163f2d1f..774e15df16 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -390,7 +390,14 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
     }
 
     if (reopen_queue) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+        if (ctx != qemu_get_aio_context()) {
+            aio_context_release(ctx);
+        }
         bdrv_reopen_multiple(reopen_queue, errp);
+        if (ctx != qemu_get_aio_context()) {
+            aio_context_acquire(ctx);
+        }
     }
 
     bdrv_subtree_drained_end(s->hidden_disk->bs);
diff --git a/blockdev.c b/blockdev.c
index f08192deda..f657d090d3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3593,8 +3593,13 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
     ctx = bdrv_get_aio_context(bs);
     aio_context_acquire(ctx);
     bdrv_subtree_drained_begin(bs);
+    aio_context_release(ctx);
+
     queue = bdrv_reopen_queue(NULL, bs, qdict, false);
     bdrv_reopen_multiple(queue, errp);
+
+    ctx = bdrv_get_aio_context(bs);
+    aio_context_acquire(ctx);
     bdrv_subtree_drained_end(bs);
     aio_context_release(ctx);
 
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e8d862a426..46593d632d 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2116,8 +2116,6 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
     bool writethrough = !blk_enable_write_cache(blk);
     bool has_rw_option = false;
     bool has_cache_option = false;
-
-    BlockReopenQueue *brq;
     Error *local_err = NULL;
 
     while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
@@ -2210,10 +2208,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
         qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
     }
 
-    bdrv_subtree_drained_begin(bs);
-    brq = bdrv_reopen_queue(NULL, bs, opts, true);
-    bdrv_reopen_multiple(brq, &local_err);
-    bdrv_subtree_drained_end(bs);
+    bdrv_reopen(bs, opts, true, &local_err);
 
     if (local_err) {
         error_report_err(local_err);
-- 
2.31.1

[PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

[ kwolf: Fixed AioContext locking ]

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json                          | 18 +++--
 blockdev.c                                    | 81 ++++++++++---------
 tests/qemu-iotests/155                        |  9 ++-
 tests/qemu-iotests/165                        |  4 +-
 tests/qemu-iotests/245                        | 27 ++++---
 tests/qemu-iotests/248                        |  2 +-
 tests/qemu-iotests/248.out                    |  2 +-
 tests/qemu-iotests/296                        |  9 ++-
 tests/qemu-iotests/298                        |  4 +-
 .../tests/remove-bitmap-from-backing          | 18 +++--
 10 files changed, 99 insertions(+), 75 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4a46552816..052520331e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4221,13 +4221,15 @@
 ##
 # @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
+# Reopens one or more block devices 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.
+# error. All devices in the list are reopened in one transaction, so
+# if one of them fails then the whole transaction is cancelled.
 #
-# The top-level @node-name option (from BlockdevOptions) must be
+# The command receives a list of block devices to reopen. For each one
+# of them, 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
@@ -4247,8 +4249,8 @@
 #
 #  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.
+# Options (1) and (2) are supported in all cases. Option (3) is
+# supported for @file and @backing, and option (4) for @backing only.
 #
 # Unlike with blockdev-add, the @backing option must always be present
 # unless the node being reopened does not have a backing file and its
@@ -4258,7 +4260,7 @@
 # Since: 4.0
 ##
 { 'command': 'x-blockdev-reopen',
-  'data': 'BlockdevOptions', 'boxed': true }
+  'data': { 'options': ['BlockdevOptions'] } }
 
 ##
 # @blockdev-del:
diff --git a/blockdev.c b/blockdev.c
index f657d090d3..1e8c946828 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3560,51 +3560,60 @@ 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);
-    BlockReopenQueue *queue;
-    QDict *qdict;
-
-    /* Check for the selected node name */
-    if (!options->has_node_name) {
-        error_setg(errp, "node-name not specified");
-        goto fail;
-    }
+void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+{
+    BlockReopenQueue *queue = NULL;
+    GSList *drained = NULL;
+
+    /* Add each one of the BDS that we want to reopen to the queue */
+    for (; reopen_list != NULL; reopen_list = reopen_list->next) {
+        BlockdevOptions *options = reopen_list->value;
+        BlockDriverState *bs;
+        AioContext *ctx;
+        QObject *obj;
+        Visitor *v;
+        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, "Failed to find node with node-name='%s'",
+        bs = bdrv_find_node(options->node_name);
+        if (!bs) {
+            error_setg(errp, "Failed to find node with node-name='%s'",
                    options->node_name);
-        goto fail;
-    }
+            goto fail;
+        }
 
-    /* Put all options in a QDict and flatten it */
-    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
-    visit_complete(v, &obj);
-    qdict = qobject_to(QDict, obj);
+        /* Put all options in a QDict and flatten it */
+        v = qobject_output_visitor_new(&obj);
+        visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
+        visit_complete(v, &obj);
+        visit_free(v);
 
-    qdict_flatten(qdict);
+        qdict = qobject_to(QDict, obj);
 
-    /* Perform the reopen operation */
-    ctx = bdrv_get_aio_context(bs);
-    aio_context_acquire(ctx);
-    bdrv_subtree_drained_begin(bs);
-    aio_context_release(ctx);
+        qdict_flatten(qdict);
 
-    queue = bdrv_reopen_queue(NULL, bs, qdict, false);
-    bdrv_reopen_multiple(queue, errp);
+        ctx = bdrv_get_aio_context(bs);
+        aio_context_acquire(ctx);
 
-    ctx = bdrv_get_aio_context(bs);
-    aio_context_acquire(ctx);
-    bdrv_subtree_drained_end(bs);
-    aio_context_release(ctx);
+        bdrv_subtree_drained_begin(bs);
+        queue = bdrv_reopen_queue(queue, bs, qdict, false);
+        drained = g_slist_prepend(drained, bs);
+
+        aio_context_release(ctx);
+    }
+
+    /* Perform the reopen operation */
+    bdrv_reopen_multiple(queue, errp);
+    queue = NULL;
 
 fail:
-    visit_free(v);
+    bdrv_reopen_queue_free(queue);
+    g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
 }
 
 void qmp_blockdev_del(const char *node_name, Error **errp)
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index bafef9dd9a..3400b0312a 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,9 +261,12 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
             result = self.vm.qmp('blockdev-add', node_name="backing",
                                  driver="null-co")
             self.assert_qmp(result, 'return', {})
-            result = self.vm.qmp('x-blockdev-reopen', node_name="target",
-                                 driver=iotests.imgfmt, file="target-file",
-                                 backing="backing")
+            result = self.vm.qmp('x-blockdev-reopen', options = [{
+                                     'node-name': "target",
+                                     'driver': iotests.imgfmt,
+                                     'file': "target-file",
+                                     'backing': "backing"
+                                 }])
             self.assert_qmp(result, 'return', {})
 
 class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen):
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index abc4ffadd5..57aa88ecae 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         assert sha256_1 == self.getSha256()
 
         # Reopen to RW
-        result = self.vm.qmp('x-blockdev-reopen', **{
+        result = self.vm.qmp('x-blockdev-reopen', options = [{
             'node-name': 'node0',
             'driver': iotests.imgfmt,
             'file': {
@@ -145,7 +145,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
                 'filename': disk
             },
             'read-only': False
-        })
+        }])
         self.assert_qmp(result, 'return', {})
 
         # Check that bitmap is reopened to RW and we can write to it.
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 0295129cbb..b81fde8f12 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -85,8 +85,18 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                          "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
+    # Run x-blockdev-reopen on a list of block devices
+    def reopenMultiple(self, opts, errmsg = None):
+        result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
+        if errmsg:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+            self.assert_qmp(result, 'error/desc', errmsg)
+        else:
+            self.assert_qmp(result, 'return', {})
+
+    # Run x-blockdev-reopen on a single block device (specified by
+    # '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)
 
@@ -101,12 +111,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                 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', {})
+        self.reopenMultiple([ opts ], errmsg)
 
 
     # Run query-named-block-nodes and return the specified entry
@@ -142,10 +147,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         # We cannot change any of these
         self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node with node-name='not-found'")
         self.reopen(opts, {'node-name': ''}, "Failed to find node with node-name=''")
-        self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string")
+        self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'options[0].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, {'driver': None}, "Invalid parameter type for 'options[0].driver', expected: string")
         self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
         self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''")
         self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef")
@@ -154,7 +159,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         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")
+        self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string")
 
         # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
         del opts['node-name']
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 4daaed1530..03911333c4 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -63,7 +63,7 @@ vm.get_qmp_events()
 
 del blockdev_opts['file']['size']
 vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles],
-           **blockdev_opts)
+           options = [ blockdev_opts ])
 
 vm.qmp_log('block-job-resume', device='drive0')
 vm.event_wait('JOB_STATUS_CHANGE', timeout=1.0,
diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out
index 369b25bf26..893f625347 100644
--- a/tests/qemu-iotests/248.out
+++ b/tests/qemu-iotests/248.out
@@ -2,7 +2,7 @@
 {"return": {}}
 {"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}}
 {"return": {}}
-{"execute": "x-blockdev-reopen", "arguments": {"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}}
+{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
 {"return": {}}
 {"execute": "block-job-resume", "arguments": {"device": "drive0"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 7c65e987a1..74b74511b6 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -120,8 +120,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
 
         command = 'x-blockdev-reopen' if reOpen else 'blockdev-add'
 
-        result = vm.qmp(command, **
-            {
+        opts = {
                 'driver': iotests.imgfmt,
                 'node-name': id,
                 'read-only': readOnly,
@@ -131,7 +130,11 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
                     'filename': test_img,
                 }
             }
-        )
+
+        if reOpen:
+            result = vm.qmp(command, options = [ opts ])
+        else:
+            result = vm.qmp(command, **opts)
         self.assert_qmp(result, 'return', {})
 
 
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index d535946b5f..4efdb35b91 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase):
         self.check_big()
 
     def test_reopen_opts(self):
-        result = self.vm.qmp('x-blockdev-reopen', **{
+        result = self.vm.qmp('x-blockdev-reopen', options = [{
             'node-name': 'disk',
             'driver': iotests.imgfmt,
             'file': {
@@ -112,7 +112,7 @@ class TestPreallocateFilter(TestPreallocateBase):
                     'filename': disk
                 }
             }
-        })
+        }])
         self.assert_qmp(result, 'return', {})
 
         self.vm.hmp_qemu_io('drive0', 'write 0 1M')
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index 0ea4c36507..0b07f7e836 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -41,13 +41,15 @@ log('Trying to remove persistent bitmap from r-o base node, should fail:')
 vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
 
 new_base_opts = {
-    'node-name': 'base',
-    'driver': 'qcow2',
-    'file': {
-        'driver': 'file',
-        'filename':  base
-    },
-    'read-only': False
+    'options': [{
+        'node-name': 'base',
+        'driver': 'qcow2',
+        'file': {
+            'driver': 'file',
+            'filename':  base
+        },
+        'read-only': False
+    }]
 }
 
 # Don't want to bother with filtering qmp_log for reopen command
@@ -58,7 +60,7 @@ if result != {'return': {}}:
 log('Remove persistent bitmap from base node reopened to RW:')
 vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
 
-new_base_opts['read-only'] = True
+new_base_opts['options'][0]['read-only'] = True
 result = vm.qmp('x-blockdev-reopen', **new_base_opts)
 if result != {'return': {}}:
     log('Failed to reopen: ' + str(result))
-- 
2.31.1

[PATCH v5 5/6] iotests: Test reopening multiple devices at the same time

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This test swaps the images used by two active block devices.

This is now possible thanks to the new ability to run
x-blockdev-reopen on multiple devices at the same time.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/245     | 47 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/245.out |  4 ++--
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index b81fde8f12..6eff352099 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -649,6 +649,53 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                                          '-c', 'read -P 0x40 0x40008 1',
                                          '-c', 'read -P 0x80 0x40010 1', hd_path[0])
 
+    # Swap the disk images of two active block devices
+    def test_swap_files(self):
+        # Add hd0 and hd2 (none of them with backing files)
+        opts0 = hd_opts(0)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts0)
+        self.assert_qmp(result, 'return', {})
+
+        opts2 = hd_opts(2)
+        result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
+        self.assert_qmp(result, 'return', {})
+
+        # Write different data to both block devices
+        self.run_qemu_io("hd0", "write -P 0xa0 0 1k")
+        self.run_qemu_io("hd2", "write -P 0xa2 0 1k")
+
+        # Check that the data reads correctly
+        self.run_qemu_io("hd0", "read  -P 0xa0 0 1k")
+        self.run_qemu_io("hd2", "read  -P 0xa2 0 1k")
+
+        # It's not possible to make a block device use an image that
+        # is already being used by the other device.
+        self.reopen(opts0, {'file': 'hd2-file'},
+                    "Permission conflict on node 'hd2-file': permissions "
+                    "'write, resize' are both required by node 'hd2' (uses "
+                    "node 'hd2-file' as 'file' child) and unshared by node "
+                    "'hd0' (uses node 'hd2-file' as 'file' child).")
+        self.reopen(opts2, {'file': 'hd0-file'},
+                    "Permission conflict on node 'hd0-file': permissions "
+                    "'write, resize' are both required by node 'hd0' (uses "
+                    "node 'hd0-file' as 'file' child) and unshared by node "
+                    "'hd2' (uses node 'hd0-file' as 'file' child).")
+
+        # But we can swap the images if we reopen both devices at the
+        # same time
+        opts0['file'] = 'hd2-file'
+        opts2['file'] = 'hd0-file'
+        self.reopenMultiple([opts0, opts2])
+        self.run_qemu_io("hd0", "read  -P 0xa2 0 1k")
+        self.run_qemu_io("hd2", "read  -P 0xa0 0 1k")
+
+        # And we can of course come back to the original state
+        opts0['file'] = 'hd0-file'
+        opts2['file'] = 'hd2-file'
+        self.reopenMultiple([opts0, opts2])
+        self.run_qemu_io("hd0", "read  -P 0xa0 0 1k")
+        self.run_qemu_io("hd2", "read  -P 0xa2 0 1k")
+
     # Misc reopen tests with different block drivers
     @iotests.skip_if_unsupported(['quorum', 'throttle'])
     def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index daf1e51922..4eced19294 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -17,8 +17,8 @@ read 1/1 bytes at offset 262152
 read 1/1 bytes at offset 262160
 1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
-..............
+...............
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
2.31.1

[PATCH v5 6/6] block: Make blockdev-reopen stable API

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This patch drops the 'x-' prefix from x-blockdev-reopen.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json                                |  6 +++---
 blockdev.c                                          |  2 +-
 tests/qemu-iotests/155                              |  2 +-
 tests/qemu-iotests/165                              |  2 +-
 tests/qemu-iotests/245                              | 10 +++++-----
 tests/qemu-iotests/248                              |  2 +-
 tests/qemu-iotests/248.out                          |  2 +-
 tests/qemu-iotests/296                              |  2 +-
 tests/qemu-iotests/298                              |  2 +-
 tests/qemu-iotests/tests/remove-bitmap-from-backing |  4 ++--
 10 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 052520331e..2eb399f0d4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4219,7 +4219,7 @@
 { 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
 
 ##
-# @x-blockdev-reopen:
+# @blockdev-reopen:
 #
 # Reopens one or more block devices using the given set of options.
 # Any option not specified will be reset to its default value regardless
@@ -4257,9 +4257,9 @@
 # image does not have a default backing file name as part of its
 # metadata.
 #
-# Since: 4.0
+# Since: 6.0
 ##
-{ 'command': 'x-blockdev-reopen',
+{ 'command': 'blockdev-reopen',
   'data': { 'options': ['BlockdevOptions'] } }
 
 ##
diff --git a/blockdev.c b/blockdev.c
index 1e8c946828..7639f2108e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3560,7 +3560,7 @@ fail:
     visit_free(v);
 }
 
-void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
+void qmp_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
 {
     BlockReopenQueue *queue = NULL;
     GSList *drained = NULL;
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 3400b0312a..fec43d662d 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -261,7 +261,7 @@ class TestBlockdevMirrorReopen(MirrorBaseClass):
             result = self.vm.qmp('blockdev-add', node_name="backing",
                                  driver="null-co")
             self.assert_qmp(result, 'return', {})
-            result = self.vm.qmp('x-blockdev-reopen', options = [{
+            result = self.vm.qmp('blockdev-reopen', options = [{
                                      'node-name': "target",
                                      'driver': iotests.imgfmt,
                                      'file': "target-file",
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 57aa88ecae..92a431315b 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -137,7 +137,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         assert sha256_1 == self.getSha256()
 
         # Reopen to RW
-        result = self.vm.qmp('x-blockdev-reopen', options = [{
+        result = self.vm.qmp('blockdev-reopen', options = [{
             'node-name': 'node0',
             'driver': iotests.imgfmt,
             'file': {
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 6eff352099..28a116a6aa 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 # group: rw
 #
-# Test cases for the QMP 'x-blockdev-reopen' command
+# Test cases for the QMP 'blockdev-reopen' command
 #
 # Copyright (C) 2018-2019 Igalia, S.L.
 # Author: Alberto Garcia <berto@igalia.com>
@@ -85,16 +85,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
                          "Expected output of %d qemu-io commands, found %d" %
                          (found, self.total_io_cmds))
 
-    # Run x-blockdev-reopen on a list of block devices
+    # Run blockdev-reopen on a list of block devices
     def reopenMultiple(self, opts, errmsg = None):
-        result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)
+        result = self.vm.qmp('blockdev-reopen', conv_keys = False, options = opts)
         if errmsg:
             self.assert_qmp(result, 'error/class', 'GenericError')
             self.assert_qmp(result, 'error/desc', errmsg)
         else:
             self.assert_qmp(result, 'return', {})
 
-    # Run x-blockdev-reopen on a single block device (specified by
+    # Run blockdev-reopen on a single block device (specified by
     # 'opts') but applying 'newopts' on top of it. The original 'opts'
     # dict is unmodified
     def reopen(self, opts, newopts = {}, errmsg = None):
@@ -161,7 +161,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {'file.locking': 'off'}, "Cannot change the option 'locking'")
         self.reopen(opts, {'file.filename': None}, "Invalid parameter type for 'options[0].file.filename', expected: string")
 
-        # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it
+        # node-name is optional in BlockdevOptions, but blockdev-reopen needs it
         del opts['node-name']
         self.reopen(opts, {}, "node-name not specified")
 
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 03911333c4..2ec2416e8a 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -62,7 +62,7 @@ vm.event_wait('JOB_STATUS_CHANGE', timeout=3.0,
 vm.get_qmp_events()
 
 del blockdev_opts['file']['size']
-vm.qmp_log('x-blockdev-reopen', filters=[filter_qmp_testfiles],
+vm.qmp_log('blockdev-reopen', filters=[filter_qmp_testfiles],
            options = [ blockdev_opts ])
 
 vm.qmp_log('block-job-resume', device='drive0')
diff --git a/tests/qemu-iotests/248.out b/tests/qemu-iotests/248.out
index 893f625347..66e94ccd7e 100644
--- a/tests/qemu-iotests/248.out
+++ b/tests/qemu-iotests/248.out
@@ -2,7 +2,7 @@
 {"return": {}}
 {"execute": "blockdev-mirror", "arguments": {"device": "drive0", "on-target-error": "enospc", "sync": "full", "target": "target"}}
 {"return": {}}
-{"execute": "x-blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
+{"execute": "blockdev-reopen", "arguments": {"options": [{"driver": "qcow2", "file": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-target"}}, "node-name": "target"}]}}
 {"return": {}}
 {"execute": "block-job-resume", "arguments": {"device": "drive0"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 74b74511b6..9206ddb954 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -118,7 +118,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
     def openImageQmp(self, vm, id, file, secret,
                      readOnly = False, reOpen = False):
 
-        command = 'x-blockdev-reopen' if reOpen else 'blockdev-add'
+        command = 'blockdev-reopen' if reOpen else 'blockdev-add'
 
         opts = {
                 'driver': iotests.imgfmt,
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index 4efdb35b91..b4d8bd9b55 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -98,7 +98,7 @@ class TestPreallocateFilter(TestPreallocateBase):
         self.check_big()
 
     def test_reopen_opts(self):
-        result = self.vm.qmp('x-blockdev-reopen', options = [{
+        result = self.vm.qmp('blockdev-reopen', options = [{
             'node-name': 'disk',
             'driver': iotests.imgfmt,
             'file': {
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index 0b07f7e836..8d48fc0f3c 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -53,7 +53,7 @@ new_base_opts = {
 }
 
 # Don't want to bother with filtering qmp_log for reopen command
-result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+result = vm.qmp('blockdev-reopen', **new_base_opts)
 if result != {'return': {}}:
     log('Failed to reopen: ' + str(result))
 
@@ -61,7 +61,7 @@ log('Remove persistent bitmap from base node reopened to RW:')
 vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
 
 new_base_opts['options'][0]['read-only'] = True
-result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+result = vm.qmp('blockdev-reopen', **new_base_opts)
 if result != {'return': {}}:
     log('Failed to reopen: ' + str(result))
 
-- 
2.31.1

Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'

[Vladimir Sementsov-Ogievskiy]

06.07.2021 14:23, Kevin Wolf wrote:
> Without an external data file, s->data_file is a second pointer with the
> same value as bs->file. When changing bs->file to a different BdrvChild
> and freeing the old BdrvChild, s->data_file must also be updated,
> otherwise it points to freed memory and causes crashes.
> 
> This problem was caught by iotests case 245.
> 
> Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Still, some ideas below:

> ---
>   block/qcow2.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ee4530cdbd..cb459ef6a6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
>   }
>   
>   typedef struct Qcow2ReopenState {
> +    bool had_data_file;
>       Qcow2Cache *l2_table_cache;
>       Qcow2Cache *refcount_block_cache;
>       int l2_slice_size; /* Number of entries in a slice of the L2 table */
> @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
>       r = g_new0(Qcow2ReopenState, 1);
>       state->opaque = r;
>   
> +    r->had_data_file = has_data_file(state->bs);
> +

So, during reopen, at some moment s->data_file become invalid. So, we shouldn't rely on it..

Maybe we need

        s->data_file = NULL;

here..

>       ret = qcow2_update_options_prepare(state->bs, r, state->options,
>                                          state->flags, errp);
>       if (ret < 0) {
> @@ -1966,7 +1969,18 @@ fail:
>   
>   static void qcow2_reopen_commit(BDRVReopenState *state)
>   {
> +    BDRVQcow2State *s = state->bs->opaque;
> +    Qcow2ReopenState *r = state->opaque;
> +
>       qcow2_update_options_commit(state->bs, state->opaque);

Worth doing

        assert(r->had_data_file == has_data_file(state->bs));

here, to be double sure?

> +    if (!r->had_data_file && s->data_file != state->bs->file) {
> +        /*
> +         * If s->data_file is just a second pointer to bs->file (which is the
> +         * case without an external data file), it may need to be updated.
> +         */
> +        s->data_file = state->bs->file;
> +        assert(!has_data_file(state->bs));
> +    }
>       g_free(state->opaque);
>   }
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v5 2/6] block: Add bdrv_reopen_queue_free()

[Vladimir Sementsov-Ogievskiy]

06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia<berto@igalia.com>
> 
> Move the code to free a BlockReopenQueue to a separate function.
> It will be used in a subsequent patch.
> 
> [ kwolf: Also free explicit_options and options, and explicitly
>    qobject_ref() the value when it continues to be used. This avoids
>    memory leaks as we saw them in two recent patches. ]

Hmm, not clear from the context which two patches you mean

> 
> Signed-off-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'

[Kevin Wolf]

Am 06.07.2021 um 15:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.07.2021 14:23, Kevin Wolf wrote:
> > Without an external data file, s->data_file is a second pointer with the
> > same value as bs->file. When changing bs->file to a different BdrvChild
> > and freeing the old BdrvChild, s->data_file must also be updated,
> > otherwise it points to freed memory and causes crashes.
> > 
> > This problem was caught by iotests case 245.
> > 
> > Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Still, some ideas below:
> 
> > ---
> >   block/qcow2.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index ee4530cdbd..cb459ef6a6 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
> >   }
> >   typedef struct Qcow2ReopenState {
> > +    bool had_data_file;
> >       Qcow2Cache *l2_table_cache;
> >       Qcow2Cache *refcount_block_cache;
> >       int l2_slice_size; /* Number of entries in a slice of the L2 table */
> > @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
> >       r = g_new0(Qcow2ReopenState, 1);
> >       state->opaque = r;
> > +    r->had_data_file = has_data_file(state->bs);
> > +
> 
> So, during reopen, at some moment s->data_file become invalid. So, we
> shouldn't rely on it..
> 
> Maybe we need
> 
>        s->data_file = NULL;
> 
> here..

"need" is a strong word, but I guess we shouldn't access it between
prepare and commit, so I agree setting it to NULL would make bugs in
this area very visible.

In fact, we wouldn't even need r->had_data_file then because commit
could just set s->data_file = state->bs->file if it's NULL.

> >       ret = qcow2_update_options_prepare(state->bs, r, state->options,
> >                                          state->flags, errp);
> >       if (ret < 0) {
> > @@ -1966,7 +1969,18 @@ fail:
> >   static void qcow2_reopen_commit(BDRVReopenState *state)
> >   {
> > +    BDRVQcow2State *s = state->bs->opaque;
> > +    Qcow2ReopenState *r = state->opaque;
> > +
> >       qcow2_update_options_commit(state->bs, state->opaque);
> 
> Worth doing
> 
>        assert(r->had_data_file == has_data_file(state->bs));
> 
> here, to be double sure?

This would be wrong because at the time that this runs, state->bs->file
is already updated and this is what has_data_file() checks against. So
you can't use has_data_file() any more until it's synced again with the
code below.

In fact, this is why I even added r->had_data_file. At first I directly
used has_data_file(state->bs) here and watched it break.

> > +    if (!r->had_data_file && s->data_file != state->bs->file) {
> > +        /*
> > +         * If s->data_file is just a second pointer to bs->file (which is the
> > +         * case without an external data file), it may need to be updated.
> > +         */
> > +        s->data_file = state->bs->file;
> > +        assert(!has_data_file(state->bs));
> > +    }
> >       g_free(state->opaque);
> >   }

Kevin

Re: [PATCH v5 3/6] block: Acquire AioContexts during bdrv_reopen_multiple()

[Vladimir Sementsov-Ogievskiy]

06.07.2021 14:23, Kevin Wolf wrote:
> As the BlockReopenQueue can contain nodes in multiple AioContexts, only
> one of which may be locked when AIO_WAIT_WHILE() can be called, we can't
> let the caller lock the right contexts. Instead, individually lock the
> AioContext of a single node when iterating the queue.
> 
> Reintroduce bdrv_reopen() as a wrapper for reopening a single node that
> drains the node and temporarily drops the AioContext lock for
> bdrv_reopen_multiple().
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

I don't have the whole picture in mind. But looks clear:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v5 4/6] block: Support multiple reopening with x-blockdev-reopen

[Vladimir Sementsov-Ogievskiy]

06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia <berto@igalia.com>
> 
> [ kwolf: Fixed AioContext locking ]
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   qapi/block-core.json                          | 18 +++--
>   blockdev.c                                    | 81 ++++++++++---------
>   tests/qemu-iotests/155                        |  9 ++-
>   tests/qemu-iotests/165                        |  4 +-
>   tests/qemu-iotests/245                        | 27 ++++---
>   tests/qemu-iotests/248                        |  2 +-
>   tests/qemu-iotests/248.out                    |  2 +-
>   tests/qemu-iotests/296                        |  9 ++-

[..]

>   
>   ##
>   # @blockdev-del:
> diff --git a/blockdev.c b/blockdev.c
> index f657d090d3..1e8c946828 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[..]

>   
> -    bs = bdrv_find_node(options->node_name);
> -    if (!bs) {
> -        error_setg(errp, "Failed to find node with node-name='%s'",
> +        bs = bdrv_find_node(options->node_name);
> +        if (!bs) {
> +            error_setg(errp, "Failed to find node with node-name='%s'",
>                      options->node_name);

indentation broken

> -        goto fail;
> -    }
> +            goto fail;
> +        }
>   
> -    /* Put all options in a QDict and flatten it */
> -    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> -    visit_complete(v, &obj);
> -    qdict = qobject_to(QDict, obj);
> +        /* Put all options in a QDict and flatten it */
> +        v = qobject_output_visitor_new(&obj);
> +        visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> +        visit_complete(v, &obj);
> +        visit_free(v);

[..]

> +    # Run x-blockdev-reopen on a list of block devices
> +    def reopenMultiple(self, opts, errmsg = None):
> +        result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, options = opts)

I don't really care if this doesn't break iotest 297, but PEP8 doesn't like whitespaces around '=' for named arguments..

> +        if errmsg:
> +            self.assert_qmp(result, 'error/class', 'GenericError')
> +            self.assert_qmp(result, 'error/desc', errmsg)
> +        else:
> +            self.assert_qmp(result, 'return', {})



-- 
Best regards,
Vladimir

Re: [PATCH v5 5/6] iotests: Test reopening multiple devices at the same time

[Vladimir Sementsov-Ogievskiy]

06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia<berto@igalia.com>
> 
> This test swaps the images used by two active block devices.
> 
> This is now possible thanks to the new ability to run
> x-blockdev-reopen on multiple devices at the same time.
> 
> Signed-off-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>



Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v5 6/6] block: Make blockdev-reopen stable API

[Vladimir Sementsov-Ogievskiy]

06.07.2021 14:23, Kevin Wolf wrote:
> From: Alberto Garcia<berto@igalia.com>
> 
> This patch drops the 'x-' prefix from x-blockdev-reopen.
> 
> Signed-off-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir