On 11/16/2017 03:56 AM, Vladimir Sementsov-Ogievskiy wrote: > 11.11.2017 01:52, John Snow wrote: >> >> 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__)) > > ok > >>> +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.) > > agree, but it is a preexisting code, so I'll fix it with an additional > patch. >
You're right. I didn't notice it was pre-existing where I was looking at it. I'll send my own little fixup. My mistake. >> >>> +} >>> -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: > > ok > >> >> Reviewed-by: John Snow <js...@redhat.com> > > Just make sure it compiles standalone by using the unused attribute and you can add the RB. --js