[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 
  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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]