Re: [Qemu-block] [PATCH v6 09/42] block: Include filters when freezing backing chain

[Vladimir Sementsov-Ogievskiy]

09.08.2019 19:13, Max Reitz wrote:
> In order to make filters work in backing chains, the associated
> functions must be able to deal with them and freeze all filter links, be
> they COW or R/W filter links.
> 
> In the process, rename these functions to reflect that they now act on
> generalized chains of filter nodes instead of backing chains alone.
> 
> While at it, add some comments that note which functions require their
> caller to ensure that a given child link is not frozen, and how the
> callers do so.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   include/block/block.h | 10 +++---
>   block.c               | 81 +++++++++++++++++++++++++------------------
>   block/commit.c        |  8 ++---
>   block/mirror.c        |  4 +--
>   block/stream.c        |  8 ++---
>   5 files changed, 62 insertions(+), 49 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 50a07c1c33..f6f09b95cd 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -364,11 +364,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
> BlockDriverState *base,
>   BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>                                       BlockDriverState *bs);
>   BlockDriverState *bdrv_find_base(BlockDriverState *bs);
> -bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState 
> *base,
> -                                  Error **errp);
> -int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> -                              Error **errp);
> -void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState 
> *base);
> +bool bdrv_is_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
> +                          Error **errp);
> +int bdrv_freeze_chain(BlockDriverState *bs, BlockDriverState *base,
> +                      Error **errp);
> +void bdrv_unfreeze_chain(BlockDriverState *bs, BlockDriverState *base);
>   
>   
>   typedef struct BdrvCheckResult {
> diff --git a/block.c b/block.c
> index adf82efb0e..650c00d182 100644
> --- a/block.c
> +++ b/block.c
> @@ -2303,12 +2303,15 @@ static void bdrv_replace_child_noperm(BdrvChild 
> *child,
>    * If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as 
> this
>    * function uses bdrv_set_perm() to update the permissions according to the 
> new
>    * reference that @new_bs gets.
> + *
> + * Callers must ensure that child->frozen is false.
>    */
>   static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>   {
>       BlockDriverState *old_bs = child->bs;
>       uint64_t perm, shared_perm;
>   
> +    /* Asserts that child->frozen == false */
>       bdrv_replace_child_noperm(child, new_bs);
>   
>       /*
> @@ -2468,6 +2471,7 @@ static void bdrv_detach_child(BdrvChild *child)
>       g_free(child);
>   }
>   
> +/* Callers must ensure that child->frozen is false. */
>   void bdrv_root_unref_child(BdrvChild *child)
>   {
>       BlockDriverState *child_bs;
> @@ -2477,10 +2481,6 @@ void bdrv_root_unref_child(BdrvChild *child)
>       bdrv_unref(child_bs);
>   }
>   
> -/**
> - * Clear all inherits_from pointers from children and grandchildren of
> - * @root that point to @root, where necessary.
> - */

Hmm, unrelated chunk? Without it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

>   static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild 
> *child)
>   {
>       BdrvChild *c;
> @@ -2505,6 +2505,7 @@ static void bdrv_unset_inherits_from(BlockDriverState 
> *root, BdrvChild *child)

[..]

-- 
Best regards,
Vladimir