Re: [PATCH v6 07/15] block: bdrv_check_perm(): process children anyway

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2177 bytes --]

On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Do generic processing even for drivers which define .bdrv_check_perm
> handler. It's needed for further preallocate filter: it will need to do
> additional action on bdrv_check_perm, but don't want to reimplement
> generic logic.
> 
> The patch doesn't change existing behaviour: the only driver that
> implements bdrv_check_perm is file-posix, but it never has any
> children.
> 
> Also, bdrv_set_perm() don't stop processing if driver has
> .bdrv_set_perm handler as well.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9538af4884..165c2d3cb2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1964,8 +1964,7 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
>  /*
>   * Check whether permissions on this node can be changed in a way that
>   * @cumulative_perms and @cumulative_shared_perms are the new cumulative
> - * permissions of all its parents. This involves checking whether all necessary
> - * permission changes to child nodes can be performed.
> + * permissions of all its parents.

Why do you want to remove this sentence?

>   *
>   * Will set *tighten_restrictions to true if and only if new permissions have to
>   * be taken or currently shared permissions are to be unshared.  Otherwise,
> @@ -2047,8 +2046,11 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>      }
>  
>      if (drv->bdrv_check_perm) {
> -        return drv->bdrv_check_perm(bs, cumulative_perms,
> -                                    cumulative_shared_perms, errp);
> +        ret = drv->bdrv_check_perm(bs, cumulative_perms,
> +                                   cumulative_shared_perms, errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }

Sounds good.  It’s also consistent with how bdrv_abort_perm_update() and
bdrv_set_perm() don’t return after calling the respective driver
functions, but always recurse to the children.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]