Am 17.03.2021 um 18:15 hat Alberto Garcia geschrieben: > When the x-blockdev-reopen was added it allowed reconfiguring the > graph by replacing backing files, but changing the 'file' option was > forbidden. Because of this restriction some operations are not > possible, notably inserting and removing block filters. > > This patch adds support for replacing the 'file' option. This is > similar to replacing the backing file and the user is likewise > responsible for the correctness of the resulting graph, otherwise this > can lead to data corruption. > > Signed-off-by: Alberto Garcia <be...@igalia.com>
> @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState > *reopen_state, > } > > /* If we want to replace the backing file we need some extra checks */ > - if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { > + if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) { I may be missing something, but I don't see how this whole block makes sense for changing 'file'. overlay_bs was found by going down the backing chain, so of course it will be different from new_child_bs (unless you use the same node as 'backing' and as 'file'). So we run all this code that seems to be concerned only with backing files. Probably overlay_bs should be found by starting from child and using bdrv_filter_or_cow_bs() only for the following loop iterations. > int ret; > > /* Check for implicit nodes between bs and its backing file */ > if (bs != overlay_bs) { > - error_setg(errp, "Cannot change backing link if '%s' has " > - "an implicit backing file", bs->node_name); > + error_setg(errp, "Cannot change %s link if '%s' has an implicit " > + "child", parse_file ? "file" : "backing", > bs->node_name); > return -EPERM; > } With fixed overlay_bs, this check makes sense, though the comment needs an update. > /* > @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState > *reopen_state, > * with bs->drv->supports_backing == true. > */ > if (bdrv_is_backing_chain_frozen(overlay_bs, > - child_bs(overlay_bs->backing), > errp)) > + bdrv_filter_or_cow_bs(overlay_bs), > + errp)) > { > return -EPERM; > } This checks if bs->backing is frozen (overlay_bs == bs because of the check above). What we really want to check is if child is frozen (i.e. bs->backing for updating 'backing', bs->file for updating 'file). So maybe this should be just written as that: if (child && child->frozen) { error_setg(errp, ...); return -EPERM; } Or factor this part out from bdrv_is_backing_chain_frozen() into a bdrv_is_child_frozen() or something like that. Either way, this might make the whole (outdated) comment above unnecessary because things would become a lot clearer. > - reopen_state->replace_backing_bs = true; > - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; > - ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, > - errp); > - if (ret < 0) { > - return ret; > + if (parse_file) { > + /* Store the old file bs, we'll need to refresh its permissions > */ > + reopen_state->old_file_bs = bs->file->bs; > + > + /* And finally replace the child */ > + bdrv_replace_child(bs->file, new_child_bs, tran); > + } else { > + reopen_state->replace_backing_bs = true; > + reopen_state->old_backing_bs = child_bs(bs->backing); > + ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp); > + if (ret < 0) { > + return ret; > + } > } > } Kevin