Re: [Qemu-devel] [PATCH v2 2/5] block: Uniform handling of 0-length bdrv_get_block_status()

[Eric Blake]

On 10/05/2017 09:35 AM, Stefan Hajnoczi wrote:
> On Tue, Oct 03, 2017 at 08:43:44PM -0500, Eric Blake wrote:
>> Handle a 0-length block status request up front, with a uniform
>> return value claiming the area is not allocated.
>>
>> Most callers don't pass a length of 0 to bdrv_get_block_status()
>> and friends; but it definitely happens with a 0-length read when
>> copy-on-read is enabled.  While we could audit all callers to
>> ensure that they never make a 0-length request, and then assert
>> that fact, it was just as easy to fix things to always report
>> success (as long as the callers are careful to not go into an
>> infinite loop).  However, we had inconsistent behavior on whether
>> the status is reported as allocated or defers to the backing
>> layer, depending on what callbacks the driver implements, and
>> possibly wasting quite a few CPU cycles to get to that answer.
>> Consistently reporting unallocated up front doesn't really hurt
>> anything, and makes it easier both for callers (0-length requests
>> now have well-defined behavior) and for drivers (drivers don't
>> have to deal with 0-length requests).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

> Reviewed-by: Stefan Hajnoczi <address@hidden>
> 
> If you respin, please consider using a separate if statement to make the
> code clearer:
> 
>   if (!nb_sectors) {
>       *pnum = 0;
>       return 0;
>   }
>   if (sector_num >= total_sectors) {
>       *pnum = 0;
>       return BDRV_BLOCK_EOF;
>   }

In fact, I think I would prefer to prioritize BDRV_BLOCK_EOF as the
first condition, rather than the second, by swapping those two
statements (that is, reading beyond EOF should set the EOF bit,
regardless of whether nb_sectors was non-zero).

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

signature.asc
Description: OpenPGP digital signature