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

[John Snow]

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 <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.
> 
No harm in taking it and it's easier than not taking it.

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js