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

[Vladimir Sementsov-Ogievskiy]

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

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