On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote: > It is needed to realize bdrv_dirty_bitmap_release_successor in > the following patch. >
OK, but... > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > block/dirty-bitmap.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 81adbeb6d4..981f99d362 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -326,13 +326,13 @@ static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap > *bitmap) > return !!bdrv_dirty_bitmap_name(bitmap); > } > > -/* Called with BQL taken. */ > -static void bdrv_do_release_matching_dirty_bitmap( > +/* Called within bdrv_dirty_bitmap_lock..unlock */ ...Add this so it will compile: __attribute__((__unused__)) > +static void bdrv_do_release_matching_dirty_bitmap_locked( > BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > bool (*cond)(BdrvDirtyBitmap *bitmap)) > { > BdrvDirtyBitmap *bm, *next; > - bdrv_dirty_bitmaps_lock(bs); > + > QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { > if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) { > assert(!bm->active_iterators); > @@ -344,18 +344,33 @@ static void bdrv_do_release_matching_dirty_bitmap( > g_free(bm); > > if (bitmap) { > - goto out; > + return; > } > } > } > + > if (bitmap) { > abort(); > } Do we have any style guide rules on using abort() instead of assert()? The rest of this function uses assert, and it'd be less lines to simply write: assert(!bitmap); which I think might also carry better semantic information for coverity beyond an actual runtime conditional branch. (I think. Please correct me if I am wrong, I'm a little hazy on this.) > +} > > -out: > +/* Called with BQL taken. */ > +static void bdrv_do_release_matching_dirty_bitmap( > + BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > + bool (*cond)(BdrvDirtyBitmap *bitmap)) > +{ > + bdrv_dirty_bitmaps_lock(bs); > + bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond); > bdrv_dirty_bitmaps_unlock(bs); > } > > +/* Called within bdrv_dirty_bitmap_lock..unlock */ > +static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap) > +{ > + bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL); > +} > + > /* Called with BQL taken. */ > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > If you agree with those two changes, you may add: Reviewed-by: John Snow <js...@redhat.com>