Re: [Qemu-block] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Almost all format drivers have the same characteristics as far as
> permissions are concerned: They have one or more children for storing
> their own data and, more importantly, metadata (can be written to and
> grow even without external write requests, must be protected against
> other writers and present consistent data) and optionally a backing file
> (this is just data, so like for a filter, it only depends on what the
> parent nodes need).
> 
> This provides a default implementation that can be shared by most of
> our format drivers.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c                   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/block/block_int.h |  8 ++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 523cbd3..f2e7178 100644
> --- a/block.c
> +++ b/block.c
> @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>                 (c->shared_perm & DEFAULT_PERM_UNCHANGED);
>  }
>  
> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> +                               const BdrvChildRole *role,
> +                               uint64_t perm, uint64_t shared,
> +                               uint64_t *nperm, uint64_t *nshared)
> +{
> +    bool backing = (role == &child_backing);
> +    assert(role == &child_backing || role == &child_file);
> +
> +    if (!backing) {
> +        /* Apart from the modifications below, the same permissions are
> +         * forwarded and left alone as for filters */
> +        bdrv_filter_default_perms(bs, c, role, perm, shared, &perm, &shared);
> +
> +        /* Format drivers may touch metadata even if the guest doesn't write 
> */
> +        if (!bdrv_is_read_only(bs)) {
> +            perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> +        }
> +
> +        /* bs->file always needs to be consistent because of the metadata. We
> +         * can never allow other users to resize or write to it. */
> +        perm |= BLK_PERM_CONSISTENT_READ;
> +        shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> +    } else {
> +        /* We want consistent read from backing files if the parent needs it.
> +         * No other operations are performed on backing files. */
> +        perm &= BLK_PERM_CONSISTENT_READ;
> +
> +        /* If the parent can deal with changing data, we're okay with a
> +         * writable and resizable backing file. */
> +        if (shared & BLK_PERM_WRITE) {
> +            shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;

Wouldn't this break CONSISTENT_READ?

Max

> +        } else {
> +            shared = 0;
> +        }
> +
> +        shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
> +                  BLK_PERM_WRITE_UNCHANGED;
> +    }
> +
> +    *nperm = perm;
> +    *nshared = shared;
> +}
>  
>  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>  {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 17f4c2d..eb0598e 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -880,6 +880,14 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>                                 uint64_t perm, uint64_t shared,
>                                 uint64_t *nperm, uint64_t *nshared);
>  
> +/* Default implementation for BlockDriver.bdrv_child_perm() that can be used 
> by
> + * (non-raw) image formats: Like above for bs->backing, but for bs->file it
> + * requires WRITE | RESIZE for read-write images, always requires
> + * CONSISTENT_READ and doesn't share WRITE. */
> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> +                               const BdrvChildRole *role,
> +                               uint64_t perm, uint64_t shared,
> +                               uint64_t *nperm, uint64_t *nshared);
>  
>  const char *bdrv_get_parent_name(const BlockDriverState *bs);
>  void blk_dev_change_media_cb(BlockBackend *blk, bool load);
>

signature.asc
Description: OpenPGP digital signature