As of a future commit, bdrv_replace_child_noperm() will clear the
indirect BdrvChild pointer passed to it if the new child BDS is NULL.
bdrv_replace_child_tran() will want to let it do that, but revert this
change in its abort handler. For that, we need to have it receive a
BdrvChild ** pointer, too, and keep it stored in the
BdrvReplaceChildState object that we attach to the transaction.
Note that we do not need to store it in the BdrvReplaceChildState when
new_bs is not NULL, because then there is nothing to revert. This is
important so that bdrv_replace_node_noperm() can pass a pointer to a
loop-local variable to bdrv_replace_child_tran() without worrying that
this pointer will outlive one loop iteration.
(Of course, for that to work, bdrv_replace_node_noperm() and in turn
bdrv_replace_node() and its relatives may not be called with a NULL @to
node. Luckily, they already are not, but now we should assert this.)
bdrv_remove_file_or_backing_child() on the other hand needs to ensure
that the indirect pointer it passes will stay valid for the duration of
the transaction. Ensure this by keeping a strong reference to the BDS
whose >backing or >file it passes to bdrv_replace_child_tran(),
and giving up that reference only in the transaction .clean() handler.
Signed-off-by: Hanna Reitz
---
block.c | 83 ++---
1 file changed, 73 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 8da057f800..a40027161c 100644
--- a/block.c
+++ b/block.c
@@ -2254,6 +2254,7 @@ static int bdrv_drv_set_perm(BlockDriverState *bs,
uint64_t perm,
typedef struct BdrvReplaceChildState {
BdrvChild *child;
+BdrvChild **childp;
BlockDriverState *old_bs;
} BdrvReplaceChildState;
@@ -2269,7 +2270,29 @@ static void bdrv_replace_child_abort(void *opaque)
BdrvReplaceChildState *s = opaque;
BlockDriverState *new_bs = s->child->bs;
-/* old_bs reference is transparently moved from @s to @s->child */
+/*
+ * old_bs reference is transparently moved from @s to s->child.
+ *
+ * Pass >child here instead of s->childp, because:
+ * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not
+ * modify the BdrvChild * pointer we indirectly pass to it, i.e. it
+ * will not modify s->child. From that perspective, it does not matter
+ * whether we pass s->childp or >child.
+ * (TODO: Right now, bdrv_replace_child_noperm() never modifies that
+ * pointer anyway (though it will in the future), so at this point it
+ * absolutely does not matter whether we pass s->childp or >child.)
+ * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use
+ * it here.
+ * (3) If new_bs is NULL, *s->childp will have been NULLed by
+ * bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
+ * must not pass a NULL *s->childp here.
+ * (TODO: In its current state, bdrv_replace_child_noperm() will not
+ * have NULLed *s->childp, so this does not apply yet. It will in the
+ * future.)
+ *
+ * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
+ * any case, there is no reason to pass it anyway.
+ */
bdrv_replace_child_noperm(>child, s->old_bs);
bdrv_unref(new_bs);
}
@@ -2286,22 +2309,32 @@ static TransactionActionDrv bdrv_replace_child_drv = {
* Note: real unref of old_bs is done only on commit.
*
* The function doesn't update permissions, caller is responsible for this.
+ *
+ * Note that if new_bs == NULL, @childp is stored in a state object attached
+ * to @tran, so that the old child can be reinstated in the abort handler.
+ * Therefore, if @new_bs can be NULL, @childp must stay valid until the
+ * transaction is committed or aborted.
+ *
+ * (TODO: The reinstating does not happen yet, but it will once
+ * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
*/
-static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
+static void bdrv_replace_child_tran(BdrvChild **childp,
+BlockDriverState *new_bs,
Transaction *tran)
{
BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
*s = (BdrvReplaceChildState) {
-.child = child,
-.old_bs = child->bs,
+.child = *childp,
+.childp = new_bs == NULL ? childp : NULL,
+.old_bs = (*childp)->bs,
};
tran_add(tran, _replace_child_drv, s);
if (new_bs) {
bdrv_ref(new_bs);
}
-bdrv_replace_child_noperm(, new_bs);
-/* old_bs reference is transparently moved from @child to @s */
+bdrv_replace_child_noperm(childp, new_bs);
+/* old_bs reference is transparently moved from *childp to @s */
}
/*
@@ -4844,6 +4877,7 @@ static bool should_update_child(BdrvChild *c,
BlockDriverState *to)
typedef struct
