Re: [Qemu-devel] [PATCH v4 01/23] block: Allow NULL file for bdrv_get_block_status()

[Eric Blake]

On 09/25/2017 05:43 PM, John Snow wrote:
> 
> 
> On 09/13/2017 12:03 PM, 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.
>> The only caller that does not care about pnum is bdrv_is_allocated,
>> as invoked by vvfat; we can likewise add assertions that the rest
>> of the stack does not have to worry about a NULL pnum.
>>
>> Furthermore, 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.
>>
> 
> Function reads slightly worse for the wear now with all of the return
> logic handled at various places within, but unifying it might be even
> stranger, perhaps..
> 
> Let's see if I hate this more:
> 
> out:
> bdrv_dec_in_flight(bs);
>     bdrv_dec_in_flight(bs);
>     if (ret >= 0 && sector_num + *pnum == total_sectors) {
>         ret |= BDRV_BLOCK_EOF;
>     }
> early_out:
>     if (file) {
>         *file = local_file;
>     }
>     return ret;
> 
> 
> and then earlier in the function, we can just:
> 
> if (total_sectors < 0) {
>   ret = total_sectors;
>   goto early_out;
> }

Seems reasonable enough, I'll work that in to v5, since there are other
reasons to respin the series anyway.

> 
> It's only shed paint, though:
> 
> Reviewed-by: John Snow <address@hidden>
> 
> I'm looking at the rest of the series now, so please stand by.
> 

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

signature.asc
Description: OpenPGP digital signature