27.09.2019 0:44, John Snow wrote: > > > On 9/26/19 3:28 PM, Eric Blake wrote: >> 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 <vsement...@virtuozzo.com> >>>> --- >>>> 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 <js...@redhat.com> >>> >>> (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. >> > No harm in taking it and it's easier than not taking it.
I don't insist, consider last patch as optional. > > Thanks, applied to my bitmaps tree: > > https://github.com/jnsnow/qemu/commits/bitmaps > https://github.com/jnsnow/qemu.git > Thank you! -- Best regards, Vladimir
