20.09.2019 18:27, Max Reitz wrote:
> After a couple of follow-up patches, this function will replace
> bdrv_recurse_is_first_non_filter() in check_to_replace_node().
>
> bdrv_recurse_is_first_non_filter() is both not sufficiently specific for
> check_to_replace_node() (it allows cases that should not be allowed,
> like replacing child nodes of quorum with dissenting data that have more
> parents than just quorum), and it is too restrictive (it is perfectly
> fine to replace filters).
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
> include/block/block_int.h | 10 ++++++++++
> block.c | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 5fd4f17d93..0be7d12f04 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -103,6 +103,13 @@ struct BlockDriver {
> */
> bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
> BlockDriverState *candidate);
> + /*
> + * Return true if @to_replace can be replaced by a BDS with the
> + * same data as @bs without it affecting @bs's behavior (that is,
> + * without it being visible to @bs's parents).
> + */
> + bool (*bdrv_recurse_can_replace)(BlockDriverState *bs,
> + BlockDriverState *to_replace);
>
> int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char
> *filename);
> int (*bdrv_probe_device)(const char *filename);
> @@ -1254,6 +1261,9 @@ void bdrv_format_default_perms(BlockDriverState *bs,
> BdrvChild *c,
> uint64_t perm, uint64_t shared,
> uint64_t *nperm, uint64_t *nshared);
>
> +bool bdrv_recurse_can_replace(BlockDriverState *bs,
> + BlockDriverState *to_replace);
> +
> /*
> * Default implementation for drivers to pass bdrv_co_block_status() to
> * their file.
> diff --git a/block.c b/block.c
> index 7d99ca692c..a2deca4ac9 100644
> --- a/block.c
> +++ b/block.c
> @@ -6206,6 +6206,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState
> *bs,
> return false;
> }
>
> +/*
> + * This function checks whether the given @to_replace is allowed to be
> + * replaced by a node that always shows the same data as @bs. This is
> + * used for example to verify whether the mirror job can replace
> + * @to_replace by the target mirrored from @bs.
> + * To be replaceable, @bs and @to_replace may either be guaranteed to
> + * always show the same data (because they are only connected through
> + * filters), or some driver may allow replacing one of its children
> + * because it can guarantee that this child's data is not visible at
> + * all (for example, for dissenting quorum children that have no other
> + * parents).
> + */
> +bool bdrv_recurse_can_replace(BlockDriverState *bs,
> + BlockDriverState *to_replace)
> +{
> + if (!bs || !bs->drv) {
> + return false;
> + }
> +
> + if (bs == to_replace) {
> + return true;
> + }
> +
> + /* For filters, we can recurse on our own */
> + if (bs->drv->is_filter) {
> + BdrvChild *child = bs->file ?: bs->backing;
then, maybe asset(!bs->drv->bdrv_recurse_can_replace)
> + return bdrv_recurse_can_replace(child->bs, to_replace);
> + }
or, this may be filter-skipping loop instead of recursion, like
while (bs && bs->drv && bs->drv->is_filter) {
bs = (bs->file ?: bs->backing)->bs;
}
at function start.
either way:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> +
> + /* See what the driver can do */
> + if (bs->drv->bdrv_recurse_can_replace) {
> + return bs->drv->bdrv_recurse_can_replace(bs, to_replace);
> + }
> +
> + /* Safe default */
> + return false;
> +}
> +
> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> const char *node_name, Error **errp)
> {
>
--
Best regards,
Vladimir
