|
From: | Eric Blake |
Subject: | Re: [Qemu-devel] [PATCH v3 1/8] dirty-bitmap: improve bdrv_dirty_bitmap_next_zero |
Date: | Mon, 10 Sep 2018 09:59:48 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
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?
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.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Prev in Thread] | Current Thread | [Next in Thread] |