[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 01/23] block: Allow NULL file fo
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 01/23] block: Allow NULL file for bdrv_get_block_status() |
Date: |
Wed, 27 Sep 2017 16:46:14 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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
[Qemu-block] [PATCH v4 05/23] block: Switch bdrv_make_zero() to byte-based, Eric Blake, 2017/09/13