Re: [Qemu-devel] [PATCH v1 02/13] qcow2: is_zero_sectors(): return true if area is outside of backing file

[Eric Blake <eblake@redhat.com>]

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

On 05/22/2017 02:12 PM, Eric Blake wrote:

>> +++ b/block/qcow2.c
>> @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>>      int64_t res;
>>  
>>      if (start + count > bs->total_sectors) {
>> -        count = bs->total_sectors - start;
>> +        count = start < bs->total_sectors ? bs->total_sectors - start : 0;
>>      }
>>  
>>      if (!count) {
>> @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>>      }
>>      res = bdrv_get_block_status_above(bs, NULL, start, count,
>>                                        &nr, &file);
>> -    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
>> +    return res >= 0
>> +        && (((res & BDRV_BLOCK_ZERO) && nr == count)
>> +            || nr == 0);
> 
> The logic makes sense to me, although the formatting is unusual (we tend
> to split && and || with the operator at the tail of the previous line,
> not the head of the new line).
> 
> This quick check may make me revisit whether it is worth my my RFC
> series about adding BDRV_BLOCK_EOF for more quickly tracking reads
> beyond EOF (my solution in that series was able to make the same change
> to test 154, but by doing it at the block layer instead of the qcow2.c
> code).  https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html

Actually, re-reading my RFC - I was able to change 6 instances in test
154, while your tweak only affected 2 instances (you still left four
instances that were allocating).  So my approach may still be more
optimal in the long run.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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