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

[John Snow]

On 09/14/2018 01:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.09.2018 20:39, John Snow wrote:
>>
>> On 09/10/2018 01:00 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 10.09.2018 19:55, Eric Blake wrote:
>>>> On 9/10/18 11:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>
>>>>>>> -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.
>>>>> interface with constant end parameter is more comfortable for loop:
>>>>> we don't need to update 'bytes' parameter on each iteration
>>>> But there's still the question of WHO should be calculating end. Your
>>>> interface argues for the caller:
>>>>
>>>> hbitmap_next_zero(start, start + bytes)
>>>>
>>>> int64_t hbitmap_next_zero(...)
>>>> {
>>>>      while (offset != end) ...
>>>> }
>>>>
>>>> while we're asking about a consistent interface for the caller (if
>>>> most callers already have a 'bytes' rather than an 'end' computed):
>>>>
>>>> hbitmap_next_zero(start, bytes)
>>>>
>>>> int64_t hbitmap_next_zero(...)
>>>> {
>>>>      int64_t end = start + bytes;
>>>>      while (offset != end) ...
>>>> }
>>>>
>>> Yes, that's an issue. Ok, if you are not comfortable with start,end, I
>>> can switch to start,bytes.
>>>
>> The series looks pretty close, I can merge the next version if you think
>> it's worth changing the interface.
>>
>> --js
> 
> I've started to change interface and found a bug in bitmap_to_extents
> (patch sent
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01804.html).
> So, next version will be based on this patch, which will go through
> Eric's tree..
> 

ah, I see. you can send it to the list anyway with the requires: header
and I can have Eric stage it to make the eventual merge easier for Peter.

--js