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

[Eric Blake]

On 05/25/2017 01:34 AM, 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 <address@hidden>
>>
>> ---
>> v2: new patch
> 
> Yes. any particular reason why this patch is useful, besides simplifying
> callers?

I guess it is not directly useful to this series. I can delay it to
later, and include it as part of my v2 of changing
bdrv_get_block_status() to be byte-based.  As mentioned in the commit
message, it opens the door for a potential optimization for callers that
don't care about offsets, but only allocation status, so that they can
traverse the entire device address space with fewer queries.

I'm fine if we want to just go with 1,2,4,5 for now.

>> @@ -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?

Can do, particularly if I delay this patch to a later series, and we go
with the rest now.

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

signature.asc
Description: OpenPGP digital signature