Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3722 bytes --]

On 2017-05-25 08:34, Fam Zheng wrote:
> On Wed, 05/24 15:28, Eric Blake wrote:
>> Not all callers care about which BDS owns the mapping for a given
>> range of the file.  This patch merely simplifies the callers by
>> consolidating the logic in the common call point, while guaranteeing
>> a non-NULL file to all the driver callbacks, for no semantic change.
>>
>> However, this will also set the stage for a future cleanup: when a
>> caller does not care about which BDS owns an offset, it would be
>> nice to allow the driver to optimize things to not have to return
>> BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
>> allocation (for example, it's fairly easy to create a qcow2 image
>> where consecutive guest addresses are not at consecutive host
>> addresses), the current contract requires bdrv_get_block_status()
>> to clamp *pnum to the limit where host addresses are no longer
>> consecutive, but allowing a NULL file means that *pnum could be
>> set to the full length of known-allocated data.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v2: new patch
> 
> Yes. any particular reason why this patch is useful, besides simplifying
> callers?
> 
> 
>> ---
>>  block/io.c     | 15 +++++++++------
>>  block/mirror.c |  3 +--
>>  block/qcow2.c  |  4 +---
>>  qemu-img.c     | 10 ++++------
>>  4 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 8e6c3fe..eea74cb 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>>  {
>>      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
>>      BlockDriverState *bs = child->bs;
>> -    BlockDriverState *file;
>>      int n;
>>
>>      target_sectors = bdrv_nb_sectors(bs);
>> @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>>          if (nb_sectors <= 0) {
>>              return 0;
>>          }
>> -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
>> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
>>          if (ret < 0) {
>>              error_report("error getting block status at sector %" PRId64 ": %s",
>>                           sector_num, strerror(-ret));
>> @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData {
>>   * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
>>   * beyond the end of the disk image it will be clamped.
>>   *
>> - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
>> - * points to the BDS which the sector range is allocated in.
>> + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
>> + * 'file' is non-NULL, then '*file' points to the BDS which the sector range
>> + * is allocated in.
> 
> Sounds good.
> 
>>   */
>>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>                                                       int64_t sector_num,
>> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>      int64_t total_sectors;
>>      int64_t n;
>>      int64_t ret, ret2;
>> +    BlockDriverState *tmpfile;
>>
>> +    if (!file) {
>> +        file = &tmpfile;
>> +    }
> 
> I don't like this hunk. Instead, how about replacing all "*file = ..." with
> "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning?

Sounds fine to me, but in that case I'd like to request a different
variable name. Maybe do what we have with errp/local_error and make it
local_file or something?

Max


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