Re: [Qemu-block] [PATCH 6/7] block: Add *loosen_restrictions to *check*_perm()

[Kevin Wolf]

Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
> This patch makes three functions report whether the necessary permission
> change purely loosens restrictions or not.  These functions are:
> - bdrv_check_perm()
> - bdrv_check_update_perm()
> - bdrv_child_check_perm()
> 
> Callers can use this result to decide whether a failure is fatal or not
> (see the next patch).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block.c | 81 +++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 21e4514426..105866d15a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1686,9 +1686,12 @@ static int bdrv_fill_options(QDict **options, const 
> char *filename,
>  
>  static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
>                                   uint64_t perm, uint64_t shared,
> -                                 GSList *ignore_children, Error **errp);
> +                                 GSList *ignore_children,
> +                                 bool *loosen_restrictions, Error **errp);
>  static void bdrv_child_abort_perm_update(BdrvChild *c);
>  static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t 
> shared);
> +static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
> +                                     uint64_t *shared_perm);
>  
>  typedef struct BlockReopenQueueEntry {
>       bool prepared;
> @@ -1759,18 +1762,37 @@ static void bdrv_child_perm(BlockDriverState *bs, 
> BlockDriverState *child_bs,
>   * permissions of all its parents. This involves checking whether all 
> necessary
>   * permission changes to child nodes can be performed.
>   *
> + * Will set *loosen_restrictions to true if and only if no new permissions 
> have
> + * to be taken and no existing shared permissions are to be unshared.  In 
> this
> + * case, errors are not fatal, as long as the caller accepts that the
> + * restrictions remain tighter than they need to be.  The caller still has to
> + * abort the transaction.
> + *
>   * A call to this function must always be followed by a call to 
> bdrv_set_perm()
>   * or bdrv_abort_perm_update().
>   */
>  static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>                             uint64_t cumulative_perms,
>                             uint64_t cumulative_shared_perms,
> -                           GSList *ignore_children, Error **errp)
> +                           GSList *ignore_children,
> +                           bool *loosen_restrictions, Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
>      BdrvChild *c;
>      int ret;
>  
> +    if (loosen_restrictions) {
> +        uint64_t current_perms, current_shared;
> +        uint64_t added_perms, removed_shared_perms;
> +
> +        bdrv_get_cumulative_perm(bs, &current_perms, &current_shared);
> +
> +        added_perms = cumulative_perms & ~current_perms;
> +        removed_shared_perms = current_shared & ~cumulative_shared_perms;
> +
> +        *loosen_restrictions = !added_perms && !removed_shared_perms;
> +    }

(loosen_restrictions is a misnomer, just not changing permissions will
make it true, too)

>      /* Write permissions never work with read-only images */
>      if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
>          !bdrv_is_writable_after_reopen(bs, q))
    {
        error_setg(errp, "Block node is read-only");
        return -EPERM;
    }

This is an interesting case in the context of reopen. It could happen
that we're actually not taking any new permissions, but the node becomes
read-only in reopen, so we fail here while maintaining the old set of
options.

If this happens, we want the reopen operation to fail, so should we set
*loosen_restrictions = false here even though we're not literally taking
new permissions?

Hm, or actually, loosen_restrictions should always be NULL during
reopen, so it will never make a different. Maybe what we want then is
assert(!q || !loosen_restrictions) at the start of the function?

Kevin