Am 06.03.2019 um 19:11 hat Alberto Garcia geschrieben: > This patch allows the user to change the backing file of an image that > is being reopened. Here's what it does: > > - In bdrv_reopen_prepare(): check that the value of 'backing' points > to an existing node or is null. If it points to an existing node it > also needs to make sure that replacing the backing file will not > create a cycle in the node graph (i.e. you cannot reach the parent > from the new backing file). > > - In bdrv_reopen_commit(): perform the actual node replacement by > calling bdrv_set_backing_hd(). > > There may be temporary implicit nodes between a BDS and its backing > file (e.g. a commit filter node). In these cases bdrv_reopen_prepare() > looks for the real (non-implicit) backing file and requires that the > 'backing' option points to it. Replacing or detaching a backing file > is forbidden if there are implicit nodes in the middle. > > Although x-blockdev-reopen is meant to be used like blockdev-add, > there's an important thing that must be taken into account: the only > way to set a new backing file is by using a reference to an existing > node (previously added with e.g. blockdev-add). If 'backing' contains > a dictionary with a new set of options ({"driver": "qcow2", "file": { > ... }}) then it is interpreted that the _existing_ backing file must > be reopened with those options. > > Signed-off-by: Alberto Garcia <be...@igalia.com>
> @@ -3294,6 +3315,98 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, > BlockDriverState *bs, > } > > /* > + * Take a BDRVReopenState and check if the value of 'backing' in the > + * reopen_state->options QDict is valid or not. > + * > + * If 'backing' is missing from the QDict then return 0. > + * > + * If 'backing' contains the node name of the backing file of > + * reopen_state->bs then return 0. > + * > + * If 'backing' contains a different node name (or is null) then check > + * whether the current backing file can be replaced with the new one. > + * If that's the case then reopen_state->replace_backing_bs is set to > + * true and reopen_state->new_backing_bs contains a pointer to the new > + * backing BlockDriverState (or NULL). > + * > + * Return 0 on success, otherwise return < 0 and set @errp. > + */ > +static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > + Error **errp) > +{ > + BlockDriverState *bs = reopen_state->bs; > + BlockDriverState *overlay_bs, *new_backing_bs; > + QObject *value; > + const char *str; > + > + value = qdict_get(reopen_state->options, "backing"); > + if (value == NULL) { > + return 0; > + } > + > + switch (qobject_type(value)) { > + case QTYPE_QNULL: > + new_backing_bs = NULL; > + break; > + case QTYPE_QSTRING: > + str = qobject_get_try_str(value); > + new_backing_bs = bdrv_lookup_bs(NULL, str, errp); > + if (new_backing_bs == NULL) { > + return -EINVAL; > + } else if (bdrv_recurse_has_child(new_backing_bs, bs)) { > + error_setg(errp, "Making '%s' a backing file of '%s' " > + "would create a cycle", str, bs->node_name); > + return -EINVAL; > + } > + break; > + default: > + /* 'backing' does not allow any other data type */ > + g_assert_not_reached(); > + } > + > + /* > + * TODO: before removing the x- prefix from x-blockdev-reopen we > + * should move the new backing file into the right AioContext > + * instead of returning an error. > + */ > + if (new_backing_bs) { > + if (bdrv_get_aio_context(new_backing_bs) != > bdrv_get_aio_context(bs)) { > + error_setg(errp, "Cannot use a new backing file " > + "with a different AioContext"); > + return -EINVAL; > + } > + } > + > + /* > + * Find the "actual" backing file by skipping all links that point > + * to an implicit node, if any (e.g. a commit filter node). > + */ > + overlay_bs = bs; > + while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) { > + overlay_bs = backing_bs(overlay_bs); > + } > + > + /* If we want to replace the backing file we need some extra checks */ > + if (new_backing_bs != backing_bs(overlay_bs)) { > + /* 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); > + return -EPERM; > + } > + /* Check if the backing link that we want to replace is frozen */ > + if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs), > + errp)) { > + return -EPERM; > + } > + reopen_state->replace_backing_bs = true; > + reopen_state->new_backing_bs = new_backing_bs; Do we need to bdrv_ref(new_backing_bs) here in case its only reference is dropped in the same reopen transaction? > + } > + > + return 0; > +} > + > +/* > * Prepares a BlockDriverState for reopen. All changes are staged in the > * 'opaque' field of the BDRVReopenState, which is used and allocated by > * the block driver layer .bdrv_reopen_prepare() > @@ -3427,6 +3540,17 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > goto error; > } > > + /* > + * Allow changing the 'backing' option. The new value can be > + * either a reference to an existing node (using its node name) > + * or NULL to simply detach the current backing file. > + */ > + ret = bdrv_reopen_parse_backing(reopen_state, errp); > + if (ret < 0) { > + goto error; > + } > + qdict_del(reopen_state->options, "backing"); > + > /* Options that are not handled are only okay if they are unchanged > * compared to the old state. It is expected that some options are only > * used for the initial open, but not reopen (e.g. filename) */ > @@ -3485,6 +3609,20 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > goto error; > } > > + /* Check if new_backing_bs would accept the new permissions */ > + if (reopen_state->replace_backing_bs && reopen_state->new_backing_bs) { > + uint64_t cur_perm, cur_shared; > + bdrv_child_perm(reopen_state->bs, reopen_state->new_backing_bs, > + NULL, &child_backing, NULL, Why do we pass NULL instead of queue here? Shouldn't the required permissions be calculated based on the post-reopen state? > + reopen_state->perm, reopen_state->shared_perm, > + &cur_perm, &cur_shared); > + ret = bdrv_check_update_perm(reopen_state->new_backing_bs, NULL, > + cur_perm, cur_shared, NULL, errp); > + if (ret < 0) { > + goto error; > + } > + } > + > ret = 0; > > /* Restore the original reopen_state->options QDict */ Kevin