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

[Eric Blake <eblake@redhat.com>]

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

On 10/10/2017 08:59 AM, Kevin Wolf wrote:
> Am 04.10.2017 um 04:00 hat Eric Blake geschrieben:
>> 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.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v5: use second label for cleaner exit logic [John], use local_pnum

>> @@ -1811,16 +1811,19 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>      int64_t total_sectors;
>>      int64_t n;
>>      int64_t ret, ret2;
>> +    BlockDriverState *local_file = NULL;
>> +    int local_pnum = 0;
> 
> I don't quite see what the point of local_pnum is if we assert anyway
> that the real pnum is non-NULL.

I did it in parallel with fallout from John's review on v4:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06958.html

but since it wasn't specifically asked for, and is now getting
questions, I'm fine with not having it in v6.


>>
>> -out:
>> + out:
> 
> git grep tells me that the old style (unindented label, as opposed to
> indented by a single space) is a lot more common both within the block
> layer and across qemu as a whole.

emacs likes to add the space (so that grepping for content in column 1
sees only function declarations, not mid-function labels - which in turn
makes 'diff -p' output nicer to read).  But I can override emacs'
insistence, and go with the label flush to the left.

-- 
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: 619 bytes --]