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

[Eric Blake]

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
> > add FOR_EACH_DIRTY_BITMAP macro.
> >
> > 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,
> anyway.
>
> 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