Re: [Qemu-devel] [PATCH v4 13/23] qemu-img: Simplify logic in img_compare()

[Eric Blake]

On 09/27/2017 02:05 PM, John Snow wrote:
> 
> 
> On 09/13/2017 12:03 PM, Eric Blake wrote:
>> As long as we are querying the status for a chunk smaller than
>> the known image size, we are guaranteed that a successful return
>> will have set pnum to a non-zero size (pnum is zero only for
>> queries beyond the end of the file).  Use that to slightly
>> simplify the calculation of the current chunk size being compared.
>> Likewise, we don't have to shrink the amount of data operated on
>> until we know we have to read the file, and therefore have to fit
>> in the bounds of our buffer.  Also, note that 'total_sectors_over'
>> is equivalent to 'progress_base'.
>>
>> With these changes in place, sectors_to_process() is now dead code,
>> and can be removed.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>> @@ -1402,14 +1393,9 @@ static int img_compare(int argc, char **argv)
>>              goto out;
>>          }
>>          allocated2 = status2 & BDRV_BLOCK_ALLOCATED;
>> -        if (pnum1) {
>> -            nb_sectors = MIN(nb_sectors,
>> -                             DIV_ROUND_UP(pnum1, BDRV_SECTOR_SIZE));
>> -        }
>> -        if (pnum2) {
>> -            nb_sectors = MIN(nb_sectors,
>> -                             DIV_ROUND_UP(pnum2, BDRV_SECTOR_SIZE));
>> -        }
>> +
>> +        assert(pnum1 && pnum2);
>> +        nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);
> 
> In the apocalyptic future where non-sector sized returns are possible,
> does this math make sense?
> 
> e.g. say the return is zeroes, but it's not aligned anymore, so we
> assume we have an extra half a sector's worth of zeroes here.

Not introduced in this patch, but a good question for 12/23.  We want to
round up rather than down to ensure that we don't inf-loop on a partial
sector response; but at the same time, you're right that if we got a
report of a half-sector zero and we widen it, we can't guarantee that
the second half is zero.

On the bright side, this rounding goes away when later patches switch
img_compare to be byte-based, later in this series.  But you're right
that it is probably smarter to have 12/23 assert that things are already
aligned (and thus we don't need to round in the first place).

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

signature.asc
Description: OpenPGP digital signature