Re: [PATCH v7 25/47] block: Def. impl.s for get_allocated_file_size

[Max Reitz]

On 19.08.20 12:57, Kevin Wolf wrote:
> Am 25.06.2020 um 17:21 hat Max Reitz geschrieben:
>> If every BlockDriver were to implement bdrv_get_allocated_file_size(),
>> there are basically three ways it would be handled:
>> (1) For protocol drivers: Figure out the actual allocated file size in
>>     some protocol-specific way
>> (2) For protocol drivers: If that is not possible (or we just have not
>>     bothered to implement it yet), return -ENOTSUP
>> (3) For drivers with children: Return the sum of some or all their
>>     children's sizes
>>
>> For the drivers we have, case (3) boils down to either:
>> (a) The sum of all children's sizes
>> (b) The size of the primary child
>>
>> (2), (3a) and (3b) can be implemented generically, so this patch adds
>> such generic implementations for drivers to use.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/block/block_int.h |  5 ++++
>>  block.c                   | 51 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 5da793bfc3..c963ee9f28 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1318,6 +1318,11 @@ int coroutine_fn 
>> bdrv_co_block_status_from_backing(BlockDriverState *bs,
>>                                                     int64_t *pnum,
>>                                                     int64_t *map,
>>                                                     BlockDriverState **file);
>> +
>> +int64_t bdrv_sum_allocated_file_size(BlockDriverState *bs);
>> +int64_t bdrv_primary_allocated_file_size(BlockDriverState *bs);
>> +int64_t bdrv_notsup_allocated_file_size(BlockDriverState *bs);
>> +
>>  const char *bdrv_get_parent_name(const BlockDriverState *bs);
>>  void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
>>  bool blk_dev_has_removable_media(BlockBackend *blk);
>> diff --git a/block.c b/block.c
>> index 1c71ecab7c..fc01ce90b3 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5003,6 +5003,57 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState 
>> *bs)
>>      return -ENOTSUP;
>>  }
>>  
>> +/**
>> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
>> + * block drivers that want it to sum all children they store data on.
>> + * (This excludes backing children.)
>> + */
>> +int64_t bdrv_sum_allocated_file_size(BlockDriverState *bs)
>> +{
>> +    BdrvChild *child;
>> +    int64_t child_size, sum = 0;
>> +
>> +    QLIST_FOREACH(child, &bs->children, next) {
>> +        if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
>> +                           BDRV_CHILD_FILTERED))
>> +        {
>> +            child_size = bdrv_get_allocated_file_size(child->bs);
>> +            if (child_size < 0) {
>> +                return child_size;
>> +            }
>> +            sum += child_size;
>> +        }
>> +    }
>> +
>> +    return sum;
>> +}
> 
> The only user apart from bdrv_get_allocated_file_size() is blkverify. As
> I argued that blkverify shouldn't use it, this can become static.
> 
>> +/**
>> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
>> + * block drivers that want it to return only the size of a node's
>> + * primary child.
>> + */
>> +int64_t bdrv_primary_allocated_file_size(BlockDriverState *bs)
>> +{
>> +    BlockDriverState *primary_bs;
>> +
>> +    primary_bs = bdrv_primary_bs(bs);
>> +    if (!primary_bs) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    return bdrv_get_allocated_file_size(primary_bs);
>> +}
> 
> This can become static, too (never used as a callback), and possibly
> even be inlined.
> 
>> +/**
>> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
>> + * protocol block drivers that just do not support it.
>> + */
>> +int64_t bdrv_notsup_allocated_file_size(BlockDriverState *bs)
>> +{
>> +    return -ENOTSUP;
>> +}
> 
> Also never used as a callback. I think inlining it would almost
> certainly make more sense.

I think they’re all artifacts from the development process, yeah.

Originally, I wanted to make .bdrv_get_allocated_file_size() mandatory,
but then I saw that led nowhere and could be done well generically.

Max

signature.asc
Description: OpenPGP digital signature