[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bit

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next
Date: Thu, 26 Sep 2019 14:28:53 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0

On 9/26/19 1:54 PM, John Snow wrote:

On 9/16/19 10:19 AM, Vladimir Sementsov-Ogievskiy wrote:
bdrv_dirty_bitmap_next is always used in same pattern. So, split it
into _next and _first, instead of combining two functions into one and

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
  include/block/dirty-bitmap.h   |  9 +++++++--
  block.c                        |  4 +---
  block/dirty-bitmap.c           | 11 +++++++----
  block/qcow2-bitmap.c           |  8 ++------
  migration/block-dirty-bitmap.c |  4 +---
  5 files changed, 18 insertions(+), 18 deletions(-)

I'm not as sure that this is an improvement.

  bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-                                        BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
+#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
+for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
+     bitmap = bdrv_dirty_bitmap_next(bitmap))

If you want the macro, you can do that without splitting the function in two:

#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; \
     bitmap = bdrv_dirty_bitmap_next(bs, bitmap))

Well, I guess explicit first and next functions is harder to mess up,

Reviewed-by: John Snow <address@hidden>

(Any other thoughts?)

I don't mind the macro as much (since we already use iteration macros for QTAILQ_FOREACH and such throughout the codebase, and since it somewhat isolates you from the calling conventions of passing NULL to prime the iteration), but introducing the macro does not imply that we need two functions. So I think this is, to some extent, a question of the maintainer's sense of aesthetics, and not an actual problem in the code.

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

reply via email to

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