Re: [Qemu-devel] [PATCH for-4.0 1/3] block: continue until base is found in bdrv_freeze_backing_chain() et al

[Vladimir Sementsov-Ogievskiy]

26.03.2019 20:07, Alberto Garcia wrote:
> All three functions that handle the BdrvChild.frozen attribute walk
> the backing chain from 'bs' to 'base' and stop either when 'base' is
> found or at the end of the chain if 'base' is NULL.
> 
> However if 'base' is not found then the functions return without
> errors as if it was NULL.
> 
> This is wrong: if the caller passed an incorrect parameter that means
> that there is a bug in the code.
> 
> Signed-off-by: Alberto Garcia <address@hidden>

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0a93ee9ac8..3050854528 100644
> --- a/block.c
> +++ b/block.c
> @@ -4218,14 +4218,15 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
>   /*
>    * Return true if at least one of the backing links between @bs and
>    * @base is frozen. @errp is set if that's the case.
> + * @base must be reachable from @bs, or NULL.
>    */
>   bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState 
> *base,
>                                     Error **errp)
>   {
>       BlockDriverState *i;
>   
> -    for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> -        if (i->backing->frozen) {
> +    for (i = bs; i != base; i = backing_bs(i)) {
> +        if (i->backing && i->backing->frozen) {

may be a bit more plain conversion would be just add assert(i == base) after 
each loop,
but I'm OK with this too.

>               error_setg(errp, "Cannot change '%s' link from '%s' to '%s'",
>                          i->backing->name, i->node_name,
>                          backing_bs(i)->node_name);
> @@ -4240,6 +4241,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, 
> BlockDriverState *base,
>    * Freeze all backing links between @bs and @base.
>    * If any of the links is already frozen the operation is aborted and
>    * none of the links are modified.
> + * @base must be reachable from @bs, or NULL.
>    * Returns 0 on success. On failure returns < 0 and sets @errp.
>    */
>   int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> @@ -4251,8 +4253,10 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
> BlockDriverState *base,
>           return -EPERM;
>       }
>   
> -    for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> -        i->backing->frozen = true;
> +    for (i = bs; i != base; i = backing_bs(i)) {
> +        if (i->backing) {
> +            i->backing->frozen = true;
> +        }
>       }
>   
>       return 0;
> @@ -4261,14 +4265,17 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
> BlockDriverState *base,
>   /*
>    * Unfreeze all backing links between @bs and @base. The caller must
>    * ensure that all links are frozen before using this function.
> + * @base must be reachable from @bs, or NULL.
>    */
>   void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState 
> *base)
>   {
>       BlockDriverState *i;
>   
> -    for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> -        assert(i->backing->frozen);
> -        i->backing->frozen = false;
> +    for (i = bs; i != base; i = backing_bs(i)) {
> +        if (i->backing) {
> +            assert(i->backing->frozen);
> +            i->backing->frozen = false;
> +        }
>       }
>   }
>   
> 


-- 
Best regards,
Vladimir