Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/8] dirty-bitmap: improve bdrv_dirty_bitmap_next_zero

[John Snow]

On 09/10/2018 10:59 AM, Eric Blake wrote:
> On 9/7/18 4:49 PM, John Snow wrote:
>>
>>
>> On 08/14/2018 08:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add bytes parameter to the function, to limit searched range.
>>>
>>
>> I'm going to assume that Eric Blake has been through here and commented
>> on the interface itself.
> 
> Actually, I haven't had time to look at this series in depth. Do you
> need me to?
> 

Not necessarily, it's just that I didn't read v1 or v2 so I was just
being cautious against recommending changes that maybe we already
recommended against in a different direction.

Historically you've cared the most about start/end/offset/bytes naming
and conventions, so I just made an assumption.

--js

>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   include/block/dirty-bitmap.h |  3 ++-
>>>   include/qemu/hbitmap.h       | 10 ++++++++--
>>>   block/backup.c               |  2 +-
>>>   block/dirty-bitmap.c         |  5 +++--
>>>   nbd/server.c                 |  2 +-
>>>   tests/test-hbitmap.c         |  2 +-
>>>   util/hbitmap.c               | 25 ++++++++++++++++++++-----
>>>   7 files changed, 36 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index 259bd27c40..27f5299c4e 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -98,7 +98,8 @@ bool
>>> bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>>                                           BdrvDirtyBitmap *bitmap);
>>>   char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error
>>> **errp);
>>> -int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap,
>>> uint64_t start);
>>> +int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap,
>>> uint64_t start,
>>> +                                    int64_t end);
> 
> It's already seeming a bit odd to mix uint64_t AND int64_t for the two
> parameters.  Is the intent to allow -1 to mean "end of the bitmap
> instead of a specific end range"? But you can get that with UINT64_MAX
> just as easily, and still get away with spelling it -1 in the source.
> 
> 
>>> + * the bitmap end.
>>>    */
>>> -int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start);
>>> +int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, int64_t
>>> end);
>>>   
>>
>> The interface looks weird because we can define a 'start' that's beyond
>> the 'end'.
>>
>> I realize that you need a signed integer for 'end' to signify EOF...
>> should we do a 'bytes' parameter instead? (Did you already do that in an
>> earlier version and we changed it?)
>>
>> Well, it's not a big deal to me personally.
> 
> It should always be possible to convert in either direction between
> [start,end) and [start,start+bytes); it boils down to a question of
> convenience (which form is easier for the majority of callers) and
> consistency (which form do we use more frequently in the block layer). I
> haven't checked closely, but I think start+bytes is more common than end
> in our public block layer APIs.
>