[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: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next |
Date: |
Thu, 26 Sep 2019 17:44:01 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
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
[Qemu-block] [PATCH 1/4] block/dirty-bitmap: drop meta, Vladimir Sementsov-Ogievskiy, 2019/09/16
[Qemu-block] [PATCH 2/4] block/dirty-bitmap: add bs link, Vladimir Sementsov-Ogievskiy, 2019/09/16
[Qemu-block] [PATCH 3/4] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex, Vladimir Sementsov-Ogievskiy, 2019/09/16