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 <mre...@redhat.com>
>> Reviewed-by: Eric Blake <ebl...@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
signature.asc
Description: OpenPGP digital signature
