Re: [Qemu-block] [PATCH v3 04/44] block: Involve block drivers in permission granting

[Max Reitz]

On 28.02.2017 13:53, Kevin Wolf wrote:
> In many cases, the required permissions of one node on its children
> depend on what its parents require from it. For example, the raw format
> or most filter drivers only need to request consistent reads if that's
> something that one of their parents wants.
> 
> In order to achieve this, this patch introduces two new BlockDriver
> callbacks. The first one lets drivers first check (recursively) whether
> the requested permissions can be set; the second one actually sets the
> new permission bitmask.
> 
> Also add helper functions that drivers can use in their implementation
> of the callbacks to update their permissions on a specific child.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c                   | 206 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block_int.h |  61 ++++++++++++++
>  2 files changed, 263 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <address@hidden>

Some irrelevant nagging below.

> diff --git a/block.c b/block.c
> index 9628c7a..cf3534f 100644
> --- a/block.c
> +++ b/block.c

[...]

> -static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
> +static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
> +                               bool check_new_perm)

I can't say I like this parameter name very much, but I can't come up
with anything better.

>  {
>      BlockDriverState *old_bs = child->bs;
> +    uint64_t perm, shared_perm;
>  
>      if (old_bs) {
>          if (old_bs->quiesce_counter && child->role->drained_end) {
>              child->role->drained_end(child);
>          }
>          QLIST_REMOVE(child, next_parent);
> +
> +        /* Update permissions for old node. This is guaranteed to succeed
> +         * because we're just taking a parent away, so we're loosening
> +         * restrictions. */
> +        bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
> +        bdrv_check_perm(old_bs, perm, shared_perm, &error_abort);
> +        bdrv_set_perm(old_bs, perm, shared_perm);
>      }
>  
>      child->bs = new_bs;
> @@ -1376,6 +1564,12 @@ static void bdrv_replace_child(BdrvChild *child, 
> BlockDriverState *new_bs)
>          if (new_bs->quiesce_counter && child->role->drained_begin) {
>              child->role->drained_begin(child);
>          }
> +
> +        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
> +        if (check_new_perm) {
> +            bdrv_check_perm(new_bs, perm, shared_perm, &error_abort);

Yeah, well, error_abort is not very good. But can be fixed later...

(We should at least pass it through and make that decision in
change_parent_backing_link().)

> +        }
> +        bdrv_set_perm(new_bs, perm, shared_perm);
>      }
>  }
>  
> @@ -1390,6 +1584,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
> *child_bs,
>  
>      ret = bdrv_check_update_perm(child_bs, perm, shared_perm, NULL, errp);
>      if (ret < 0) {
> +        bdrv_abort_perm_update(child_bs);
>          return NULL;
>      }
>  
> @@ -1403,7 +1598,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
> *child_bs,
>          .opaque         = opaque,
>      };
>  
> -    bdrv_replace_child(child, child_bs);
> +    /* This performs the matching bdrv_set_perm() for the above check. */
> +    bdrv_replace_child(child, child_bs, false);
>  
>      return child;
>  }
> @@ -1434,7 +1630,7 @@ static void bdrv_detach_child(BdrvChild *child)
>          child->next.le_prev = NULL;
>      }
>  
> -    bdrv_replace_child(child, NULL);
> +    bdrv_replace_child(child, NULL, false);

The bool is basically ignored, but I'd rather set it to true here
because there is no check call before; so if a new child was attached
(which it clearly isn't), we would indeed want check_new_perm = true.

(You see, just irrelevant nagging.)

Max

>  
>      g_free(child->name);
>      g_free(child);

signature.asc
Description: OpenPGP digital signature