We are going to reimplement this behavior (clear bs->file / bs->backing
pointers automatically when child->bs is cleared) in a nicer way.
This reverts commit b0a9f6fed3d80de610dcd04a7e66f9f30a04174f.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block.c | 102 +++++++++++++-------------------------------------------
1 file changed, 23 insertions(+), 79 deletions(-)
diff --git a/block.c b/block.c
index a97720c5eb..69c20c729a 100644
--- a/block.c
+++ b/block.c
@@ -87,10 +87,8 @@ static BlockDriverState *bdrv_open_inherit(const char
*filename,
static bool bdrv_recurse_has_child(BlockDriverState *bs,
BlockDriverState *child);
-static void bdrv_child_free(BdrvChild *child);
static void bdrv_replace_child_noperm(BdrvChild **child,
- BlockDriverState *new_bs,
- bool free_empty_child);
+ BlockDriverState *new_bs);
static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
Transaction *tran);
@@ -2258,16 +2256,12 @@ typedef struct BdrvReplaceChildState {
BdrvChild *child;
BdrvChild **childp;
BlockDriverState *old_bs;
- bool free_empty_child;
} BdrvReplaceChildState;
static void bdrv_replace_child_commit(void *opaque)
{
BdrvReplaceChildState *s = opaque;
- if (s->free_empty_child && !s->child->bs) {
- bdrv_child_free(s->child);
- }
bdrv_unref(s->old_bs);
}
@@ -2284,26 +2278,22 @@ static void bdrv_replace_child_abort(void *opaque)
* 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 &s->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 &s->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(&s->child, s->old_bs, true);
- /*
- * The child was pre-existing, so s->old_bs must be non-NULL, and
- * s->child thus must not have been freed
- */
- assert(s->child != NULL);
- if (!new_bs) {
- /* As described above, *s->childp was cleared, so restore it */
- assert(s->childp != NULL);
- *s->childp = s->child;
- }
+ bdrv_replace_child_noperm(&s->child, s->old_bs);
bdrv_unref(new_bs);
}
@@ -2320,44 +2310,30 @@ static TransactionActionDrv bdrv_replace_child_drv = {
*
* The function doesn't update permissions, caller is responsible for this.
*
- * (*childp)->bs must not be NULL.
- *
* 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.
*
- * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
- * freed (on commit). @free_empty_child should only be false if the
- * caller will free the BDrvChild themselves (which may be important
- * if this is in turn called in another transactional context).
+ * (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 **childp,
BlockDriverState *new_bs,
- Transaction *tran,
- bool free_empty_child)
+ Transaction *tran)
{
BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
*s = (BdrvReplaceChildState) {
.child = *childp,
.childp = new_bs == NULL ? childp : NULL,
.old_bs = (*childp)->bs,
- .free_empty_child = free_empty_child,
};
tran_add(tran, &bdrv_replace_child_drv, s);
- /* The abort handler relies on this */
- assert(s->old_bs != NULL);
-
if (new_bs) {
bdrv_ref(new_bs);
}
- /*
- * Pass free_empty_child=false, we will free the child (if
- * necessary) in bdrv_replace_child_commit() (if our
- * @free_empty_child parameter was true).
- */
- bdrv_replace_child_noperm(childp, new_bs, false);
+ bdrv_replace_child_noperm(childp, new_bs);
/* old_bs reference is transparently moved from *childp to @s */
}
@@ -2729,22 +2705,8 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission
qapi_perm)
return permissions[qapi_perm];
}
-/**
- * Replace (*childp)->bs by @new_bs.
- *
- * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents
- * generally cannot handle a BdrvChild with .bs == NULL, so clearing
- * BdrvChild.bs should generally immediately be followed by the
- * BdrvChild pointer being cleared as well.
- *
- * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
- * freed. @free_empty_child should only be false if the caller will
- * free the BdrvChild themselves (this may be important in a
- * transactional context, where it may only be freed on commit).
- */
static void bdrv_replace_child_noperm(BdrvChild **childp,
- BlockDriverState *new_bs,
- bool free_empty_child)
+ BlockDriverState *new_bs)
{
BdrvChild *child = *childp;
BlockDriverState *old_bs = child->bs;
@@ -2781,9 +2743,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
}
child->bs = new_bs;
- if (!new_bs) {
- *childp = NULL;
- }
if (new_bs) {
QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
@@ -2813,10 +2772,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
bdrv_parent_drained_end_single(child);
drain_saldo++;
}
-
- if (free_empty_child && !child->bs) {
- bdrv_child_free(child);
- }
}
/**
@@ -2846,14 +2801,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
BdrvChild *child = *s->child;
BlockDriverState *bs = child->bs;
- /*
- * Pass free_empty_child=false, because we still need the child
- * for the AioContext operations on the parent below; those
- * BdrvChildClass methods all work on a BdrvChild object, so we
- * need to keep it as an empty shell (after this function, it will
- * not be attached to any parent, and it will not have a .bs).
- */
- bdrv_replace_child_noperm(s->child, NULL, false);
+ bdrv_replace_child_noperm(s->child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -2875,6 +2823,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
bdrv_unref(bs);
bdrv_child_free(child);
+ *s->child = NULL;
}
static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -2952,9 +2901,7 @@ static int bdrv_attach_child_common(BlockDriverState
*child_bs,
}
bdrv_ref(child_bs);
- bdrv_replace_child_noperm(&new_child, child_bs, true);
- /* child_bs was non-NULL, so new_child must not have been freed */
- assert(new_child != NULL);
+ bdrv_replace_child_noperm(&new_child, child_bs);
*child = new_child;
@@ -3013,7 +2960,8 @@ static void bdrv_detach_child(BdrvChild **childp)
{
BlockDriverState *old_bs = (*childp)->bs;
- bdrv_replace_child_noperm(childp, NULL, true);
+ bdrv_replace_child_noperm(childp, NULL);
+ bdrv_child_free(*childp);
if (old_bs) {
/*
@@ -5024,11 +4972,7 @@ static void
bdrv_remove_file_or_backing_child(BlockDriverState *bs,
}
if (child->bs) {
- /*
- * Pass free_empty_child=false, we will free the child in
- * bdrv_remove_filter_or_cow_child_commit()
- */
- bdrv_replace_child_tran(childp, NULL, tran, false);
+ bdrv_replace_child_tran(childp, NULL, tran);
}
s = g_new(BdrvRemoveFilterOrCowChild, 1);
@@ -5038,6 +4982,8 @@ static void
bdrv_remove_file_or_backing_child(BlockDriverState *bs,
.is_backing = (childp == &bs->backing),
};
tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
+
+ *childp = NULL;
}
/*
@@ -5080,7 +5026,7 @@ static int bdrv_replace_node_noperm(BlockDriverState
*from,
* Passing a pointer to the local variable @c is fine here, because
* @to is not NULL, and so &c will not be attached to the transaction.
*/
- bdrv_replace_child_tran(&c, to, tran, true);
+ bdrv_replace_child_tran(&c, to, tran);
}
return 0;
@@ -5236,9 +5182,7 @@ int bdrv_replace_child_bs(BdrvChild *child,
BlockDriverState *new_bs,
bdrv_drained_begin(old_bs);
bdrv_drained_begin(new_bs);
- bdrv_replace_child_tran(&child, new_bs, tran, true);
- /* @new_bs must have been non-NULL, so @child must not have been freed */
- assert(child != NULL);
+ bdrv_replace_child_tran(&child, new_bs, tran);
found = g_hash_table_new(NULL, NULL);
refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
--
2.31.1
