[PATCH hci-8.0 3/5] block: let bdrv_reopen_commit_post() report a failure #VSTOR-140073

["Denis V. Lunev" <den@openvz.org>]

From: Denis V. Lunev <den@openvz.org>

The callback runs after bdrv_reopen_multiple() has committed the
transaction, so it cannot reject the reopen. It can still find that
the node it has just made writable is unusable, and has no way to say
so: bdrv_reopen() returns success and the caller carries on.

Give it a return value and an Error argument. The reopen stays
committed, the error only reports that the node is gone. Every queued
node still gets its callback, the first error is the one reported. A
callback may leave its node without a driver, and so may the I/O of a
later bdrv_reopen_prepare(), so do not assume that the nodes still
ahead of it in the queue have one. qcow2 is the only implementation and
does not fail yet.

An error therefore means one of two things now, either that the reopen
was denied and nothing changed, or that it went through and left a tree
which cannot be used. Nothing is undone in the second case: the node is
beyond repair by another reopen, and a caller which reacts to the error
by reopening anything is making it worse. bdrv_reopen_multiple() says
so, nothing else changes.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Cc: qemu-stable@nongnu.org
---
 block.c                          | 24 +++++++++++++++++++++---
 block/qcow2.c                    |  4 +++-
 include/block/block_int-common.h |  9 +++++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 44afa191930..0978ab2a0ea 100644
--- a/block.c
+++ b/block.c
@@ -4622,6 +4622,10 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
  * If all devices prepare successfully, then the changes are committed
  * to all devices.
  *
+ * A failure means either that the reopen was denied and nothing changed,
+ * or that it went through and a driver then found the node unusable. In
+ * the second case nothing is undone and the tree is no longer usable.
+ *
  * All affected nodes must be drained between bdrv_reopen_queue() and
  * bdrv_reopen_multiple().
  *
@@ -4698,15 +4702,29 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     tran_commit(tran);
     bdrv_graph_wrunlock();
 
+    ret = 0;
     QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
         BlockDriverState *bs = bs_entry->state.bs;
+        Error *local_err = NULL;
+        int commit_ret;
 
-        if (bs->drv->bdrv_reopen_commit_post) {
-            bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
+        if (!bs->drv || !bs->drv->bdrv_reopen_commit_post) {
+            continue;
+        }
+
+        commit_ret = bs->drv->bdrv_reopen_commit_post(&bs_entry->state,
+                                                      &local_err);
+        assert(commit_ret >= 0 || local_err);
+
+        if (commit_ret < 0 && ret == 0) {
+            /* Committed already, so report the first failure and go on */
+            error_propagate(errp, local_err);
+            ret = commit_ret;
+        } else {
+            error_free(local_err);
         }
     }
 
-    ret = 0;
     goto cleanup;
 
 abort:
diff --git a/block/qcow2.c b/block/qcow2.c
index 3e0a219e156..ddad39a3084 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2233,7 +2233,7 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
     g_free(state->opaque);
 }
 
-static void qcow2_reopen_commit_post(BDRVReopenState *state)
+static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp)
 {
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
@@ -2251,6 +2251,8 @@ static void qcow2_reopen_commit_post(BDRVReopenState *state)
                               bdrv_get_node_name(state->bs));
         }
     }
+
+    return 0;
 }
 
 static void qcow2_reopen_abort(BDRVReopenState *state)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 50848de07f4..7afbe8c4ae5 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -262,8 +262,13 @@ struct BlockDriver {
         BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp);
     void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)(
         BDRVReopenState *reopen_state);
-    void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)(
-        BDRVReopenState *reopen_state);
+    /*
+     * Runs once the reopen is committed, so it cannot reject it. Returns 0,
+     * or a negative errno with @errp set to report that the node it has
+     * just reopened is unusable, which it may leave without a driver.
+     */
+    int GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)(
+        BDRVReopenState *reopen_state, Error **errp);
     void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)(
         BDRVReopenState *reopen_state);
     void (*bdrv_join_options)(QDict *options, QDict *old_options);
-- 
2.53.0