| Message ID | 20190916141911.5255-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: some refactoring | expand |
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 <vsementsov@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. > > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 4c58d922e4..89e52db7ec 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -97,8 +97,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); > bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap); > bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap); > 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)) > + > char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp); > int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset, > uint64_t bytes); > diff --git a/block.c b/block.c > index 5944124845..96c2c5ae2d 100644 > --- a/block.c > +++ b/block.c > @@ -5363,9 +5363,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, > } > } > > - for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm; > - bm = bdrv_dirty_bitmap_next(bs, bm)) > - { > + FOR_EACH_DIRTY_BITMAP(bs, bm) { > bdrv_dirty_bitmap_skip_store(bm, false); > } ... and I kind of prefer loops with explicit function calls more than I like macro-loops. > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 76a8e59e61..e2df82af01 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -742,11 +742,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs) > return false; > } > > -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, > - BdrvDirtyBitmap *bitmap) > +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs) > { > - return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) : > - QLIST_NEXT(bitmap, list); > + return QLIST_FIRST(&bs->dirty_bitmaps); > +} > + > +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap) > +{ > + return QLIST_NEXT(bitmap, list); > } > > char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp) > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 6d795a2255..73ebd2ff6a 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1480,9 +1480,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > } > > /* check constraints and names */ > - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; > - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) > - { > + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { > const char *name = bdrv_dirty_bitmap_name(bitmap); > uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); > Qcow2Bitmap *bm; > @@ -1602,9 +1600,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) > return -EINVAL; > } > > - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; > - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) > - { > + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { > if (bdrv_dirty_bitmap_get_persistence(bitmap)) { > bdrv_dirty_bitmap_set_readonly(bitmap, true); > } > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > index 793f249aa5..7eafface61 100644 > --- a/migration/block-dirty-bitmap.c > +++ b/migration/block-dirty-bitmap.c > @@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void) > for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { > const char *name = bdrv_get_device_or_node_name(bs); > > - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; > - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) > - { > + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { > if (!bdrv_dirty_bitmap_name(bitmap)) { > continue; > } > Well, I guess explicit first and next functions is harder to mess up, anyway. Reviewed-by: John Snow <jsnow@redhat.com> (Any other thoughts?)
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 <vsementsov@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 <jsnow@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.
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 <vsementsov@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 <jsnow@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. Thanks, applied to my bitmaps tree: https://github.com/jnsnow/qemu/commits/bitmaps https://github.com/jnsnow/qemu.git --js
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 <vsementsov@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 <jsnow@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!
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 4c58d922e4..89e52db7ec 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -97,8 +97,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap); 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)) + char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp); int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t bytes); diff --git a/block.c b/block.c index 5944124845..96c2c5ae2d 100644 --- a/block.c +++ b/block.c @@ -5363,9 +5363,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, } } - for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm; - bm = bdrv_dirty_bitmap_next(bs, bm)) - { + FOR_EACH_DIRTY_BITMAP(bs, bm) { bdrv_dirty_bitmap_skip_store(bm, false); } diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 76a8e59e61..e2df82af01 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -742,11 +742,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs) return false; } -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap) +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs) { - return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) : - QLIST_NEXT(bitmap, list); + return QLIST_FIRST(&bs->dirty_bitmaps); +} + +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap) +{ + return QLIST_NEXT(bitmap, list); } char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 6d795a2255..73ebd2ff6a 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1480,9 +1480,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) } /* check constraints and names */ - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) - { + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { const char *name = bdrv_dirty_bitmap_name(bitmap); uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); Qcow2Bitmap *bm; @@ -1602,9 +1600,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) return -EINVAL; } - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) - { + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { if (bdrv_dirty_bitmap_get_persistence(bitmap)) { bdrv_dirty_bitmap_set_readonly(bitmap, true); } diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 793f249aa5..7eafface61 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void) for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { const char *name = bdrv_get_device_or_node_name(bs); - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) - { + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { if (!bdrv_dirty_bitmap_name(bitmap)) { continue; }
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 <vsementsov@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(-)