20.01.2020 16:14, Max Reitz wrote: > On 19.12.19 11:03, Vladimir Sementsov-Ogievskiy wrote: >> We have bdrv_dirty_bitmap_next_zero, let's add corresponding >> bdrv_dirty_bitmap_next_dirty, which is more comfortable to use than >> bitmap iterators in some cases. >> >> For test modify test_hbitmap_next_zero_check_range to check both >> next_zero and next_dirty and add some new checks. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> >> --- >> include/block/dirty-bitmap.h | 2 + >> include/qemu/hbitmap.h | 13 ++++ >> block/dirty-bitmap.c | 6 ++ >> tests/test-hbitmap.c | 130 ++++++++++++++++++++--------------- >> util/hbitmap.c | 60 ++++++++-------- >> 5 files changed, 126 insertions(+), 85 deletions(-) > > [...] > >> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h >> index b6e85f3d5d..a4b032b270 100644 >> --- a/include/qemu/hbitmap.h >> +++ b/include/qemu/hbitmap.h >> @@ -297,6 +297,19 @@ void hbitmap_free(HBitmap *hb); >> */ >> void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t >> first); >> >> +/* >> + * hbitmap_next_dirty: >> + * >> + * Find next dirty bit within selected range. If not found, return -1. >> + * >> + * @hb: The HBitmap to operate on >> + * @start: The bit to start from. >> + * @count: Number of bits to proceed. If @start+@count > bitmap size, the >> whole >> + * bitmap is looked through. You can use UINT64_MAX as @count to search up >> to > > I would’ve said s/looked through/scanned/, but it matches > hbitmap_next_zero()’s documentation, so it’s fine. > > But definitely s/UINT64_MAX/INT64_MAX/. > >> + * the bitmap end. >> + */ >> +int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t count); >> + >> /* hbitmap_next_zero: >> * >> * Find next not dirty bit within selected range. If not found, return -1. > > [...] > >> diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c >> index 0e1e5c64dd..e3f1b3f361 100644 >> --- a/tests/test-hbitmap.c >> +++ b/tests/test-hbitmap.c >> @@ -816,92 +816,108 @@ static void >> test_hbitmap_iter_and_reset(TestHBitmapData *data, >> hbitmap_iter_next(&hbi); >> } >> >> -static void test_hbitmap_next_zero_check_range(TestHBitmapData *data, >> - uint64_t start, >> - uint64_t count) >> +static void test_hbitmap_next_x_check_range(TestHBitmapData *data, >> + uint64_t start, >> + uint64_t count) > > Why not change the parameters to be signed while we’re already here? > > [...] > >> diff --git a/util/hbitmap.c b/util/hbitmap.c >> index df22f06be6..d23f4b9678 100644 >> --- a/util/hbitmap.c >> +++ b/util/hbitmap.c >> @@ -193,6 +193,30 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap >> *hb, uint64_t first) >> } >> } >> >> +int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t count) >> +{ >> + HBitmapIter hbi; >> + int64_t firt_dirty_off; > > Pre-existing, but isn’t this just a typo that you could fix here? (i.e. > s/firt/first/) > > Apart from this minor things:
Agree with them. > > Reviewed-by: Max Reitz <[email protected]> > >> + uint64_t end; >> + >> + assert(start >= 0 && count >= 0); >> + >> + if (start >= hb->orig_size || count == 0) { >> + return -1; >> + } >> + >> + end = count > hb->orig_size - start ? > -- Best regards, Vladimir
