04.02.2021 00:33, Kevin Wolf wrote:
Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
bdrv_append is not very good for inserting filters: it does extra
permission update as part of bdrv_set_backing_hd(). During this update
filter may conflict with other parents of top_bs.
Instead, let's first do all graph modifications and after it update
permissions.
This sounds like it fixes a bug. If so, should we have a test like for
the other cases fixed by this series?
Hm. I considered it mostly like a lack not a bug. We just have to workaround this lack by
"inactive" mode of filters. But adding a test is good idea anyway. Will do.
Note: bdrv_append() is still only works for backing-child based
filters. It's something to improve later.
It simplifies the fact that bdrv_append() used to append new nodes,
without backing child. Let's add an assertion.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/block.c b/block.c
index 02da1a90bc..7094922509 100644
--- a/block.c
+++ b/block.c
@@ -4998,22 +4998,28 @@ int bdrv_replace_node(BlockDriverState *from,
BlockDriverState *to,
int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
Error **errp)
{
- Error *local_err = NULL;
+ int ret;
+ GSList *tran = NULL;
- bdrv_set_backing_hd(bs_new, bs_top, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return -EPERM;
+ assert(!bs_new->backing);
+
+ ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
+ &child_of_bds, bdrv_backing_role(bs_new),
+ &bs_new->backing, &tran, errp);
+ if (ret < 0) {
+ goto out;
}
I don't think changing bs->backing without bdrv_set_backing_hd() is
correct at the moment. We lose a few things:
1. The bdrv_is_backing_chain_frozen() check
2. Updating backing_hd->inherits_from if necessary
3. bdrv_refresh_limits()
If I'm not missing anything, all of these are needed in the context of
bdrv_append().
I decided that bdrv_append() is only for appending new nodes, so frozen and
inherts_from checks are not needed. And I've added assert(!bs_new->backing)...
Checking this now:
- appending filters is obvious
- bdrv_append_temp_snapshot() creates new qcow2 node based on tmp file, don't
see any backing initialization (and it would be rather strange)
- external_snapshot_prepare() do check if (bdrv_cow_child(state->new_bs)) {
error-out }
So everything is OK. I should describe it in commit message and add a comment
to bdrv_append.
- bdrv_replace_node(bs_top, bs_new, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- bdrv_set_backing_hd(bs_new, NULL, &error_abort);
- return -EPERM;
+ ret = bdrv_replace_node_noperm(bs_top, bs_new, true, &tran, errp);
+ if (ret < 0) {
+ goto out;
}
- return 0;
+ ret = bdrv_refresh_perms(bs_new, errp);
+out:
+ tran_finalize(tran, ret);
+
+ return ret;
}
Kevin
--
Best regards,
Vladimir