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

[Max Reitz <mreitz@redhat.com>]

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

On 06.05.20 15:47, Kevin Wolf wrote:
> 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 <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  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?

Sounds all good.

> 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.

Maybe, I’ll see whether it looks good.  If it doesn’t, I could also
rename the _metadata function to _storage, so that it’s generally a
function that handles metadata+data children (i.e. defined to be
stricter than the _data function).

Max


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