Re: [PATCH v3 18/33] block: Add bdrv_default_perms()

[Kevin Wolf]

Am 18.02.2020 um 13:42 hat Max Reitz geschrieben:
> This callback can be used by BDSs that use child_of_bds with the
> appropriate BdrvChildRole for their children.
> 
> Also, make bdrv_format_default_perms() use it for child_of_bds children
> (just a temporary solution until we can drop bdrv_format_default_perms()
> altogether).
> 
> Signed-off-by: Max Reitz <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  block.c                   | 46 ++++++++++++++++++++++++++++++++-------
>  include/block/block_int.h | 11 ++++++++++
>  2 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c0ba274743..3e5b0bc345 100644
> --- a/block.c
> +++ b/block.c
> @@ -2383,14 +2383,12 @@ static void 
> bdrv_default_perms_for_metadata(BlockDriverState *bs, BdrvChild *c,
>      *nshared = shared;
>  }
>  
> -/* TODO: Use */
> -static void __attribute__((unused))
> -bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c,
> -                            const BdrvChildClass *child_class,
> -                            BdrvChildRole role,
> -                            BlockReopenQueue *reopen_queue,
> -                            uint64_t perm, uint64_t shared,
> -                            uint64_t *nperm, uint64_t *nshared)
> +static void bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c,
> +                                        const BdrvChildClass *child_class,
> +                                        BdrvChildRole role,
> +                                        BlockReopenQueue *reopen_queue,
> +                                        uint64_t perm, uint64_t shared,
> +                                        uint64_t *nperm, uint64_t *nshared)
>  {
>      assert(child_class == &child_of_bds && (role & BDRV_CHILD_DATA));
>  
> @@ -2425,6 +2423,13 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>                                 uint64_t *nperm, uint64_t *nshared)
>  {
>      bool backing = (child_class == &child_backing);
> +
> +    if (child_class == &child_of_bds) {
> +        bdrv_default_perms(bs, c, child_class, role, reopen_queue,
> +                           perm, shared, nperm, nshared);
> +        return;
> +    }
> +
>      assert(child_class == &child_backing || child_class == &child_file);
>  
>      if (!backing) {
> @@ -2436,6 +2441,31 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>      }
>  }
>  
> +void bdrv_default_perms(BlockDriverState *bs, BdrvChild *c,
> +                        const BdrvChildClass *child_class, BdrvChildRole 
> role,
> +                        BlockReopenQueue *reopen_queue,
> +                        uint64_t perm, uint64_t shared,
> +                        uint64_t *nperm, uint64_t *nshared)
> +{
> +    assert(child_class == &child_of_bds);
> +
> +    if (role & BDRV_CHILD_FILTERED) {
> +        bdrv_filter_default_perms(bs, c, child_class, role, reopen_queue,
> +                                  perm, shared, nperm, nshared);
> +    } else if (role & BDRV_CHILD_COW) {
> +        bdrv_default_perms_for_backing(bs, c, child_class, role, 
> reopen_queue,
> +                                       perm, shared, nperm, nshared);
> +    } else if (role & BDRV_CHILD_METADATA) {
> +        bdrv_default_perms_for_metadata(bs, c, child_class, role, 
> reopen_queue,
> +                                        perm, shared, nperm, nshared);
> +    } else if (role & BDRV_CHILD_DATA) {
> +        bdrv_default_perms_for_data(bs, c, child_class, role, reopen_queue,
> +                                    perm, shared, nperm, nshared);
> +    } else {
> +        g_assert_not_reached();
> +    }
> +}

Here the discussion from the start of the series becomes relevant: Which
flags can be combined with which other flags, and if multiple flags are
set, which one takes precedence?

First undocumented requirement: You must set at least one of FILTERED,
COW, METADATA and DATA.

Then, for FILTERED we decided to document that DATA shouldn't be set at
the same time. I guess neither should COW and METADATA. Apart for
documentation, worth an assertion?

COW seems to be exclusive in practice, too. I guess you could construct
a driver that somehow keeps its own (meta)data in its backing file,
maybe in the VM state area. It also sounds like a really bad idea. So
forbid it, document it and assert it, too?

METADATA and DATA can be set at the same time. As your previous patch
shows, the function for DATA is a laxer version of the one for METADATA,
requesting a subset of the METADATA permissions and sharing a superset.
So the order in the code makes sense.

But can we make sure that this condition will be true in the future?
Imagine we find a need for a new permission that is used for data files,
but not for metadata. I think at the very least, this deserves a
comment. But probably it means that both should stay a single function
that can check each flag for the exact permission bits it influences.

Kevin