Re: [Qemu-block] [Qemu-devel] [PATCH v6 05/18] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes

[Eric Blake]

On 09/08/2017 09:04 AM, Eric Blake wrote:

>>>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>>  {
>>>      BdrvDirtyBitmap *bitmap;
>>> -    uint64_t size = bdrv_nb_sectors(bs);
>>> +    int64_t size = bdrv_getlength(bs);
>>>
>>> +    assert(size >= 0);
>>
>> How can you assert that there will never be an error? Even if it's
>> correct (I don't know whether you can have dirty bitmaps on devices that
>> don't use the cached value), this needs at least a comment.
> 
> The old code wasn't checking for errors; if an error occurs, we have no
> way to report it. So I indeed need to audit whether all callers have a
> cached length at this point in time (it can't fail), or else change
> bdrv_dirty_bitmap_truncate() to be able to fail (pass failure along) and
> update all callers.  This may indeed be reason for a respin, depending
> on what I find.

Verdict - it can indeed fail; bdrv_truncate() was blindly calling the
dirty bitmap resize even after a failed refresh_total_sectors(), which
could then resize the dirty bitmap to -1.  At least bdrv_truncate()
itself still failed, but it's cleaner to fail up front rather than get
internal state even more botched in the meantime, so fixing that will be
a separate patch in v7.  Sadly, the failure is probably more
theoretical, and I did not quickly see an easy way to write an iotests
to expose it (which has been the case with a lot of our recent
bdrv_nb_sectors/bdrv_getlength failure cleanup patches).

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

signature.asc
Description: OpenPGP digital signature