| Message ID | 20171030163309.75770-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <vsementsov@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 <jsnow@redhat.com>
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 <vsementsov@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. > >> +} >> >> -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 <jsnow@redhat.com>
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 <vsementsov@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 <jsnow@redhat.com> > > Just make sure it compiles standalone by using the unused attribute and you can add the RB. --js
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 <vsementsov@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: how do you compile to get an error? and what is unused? > > __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 <jsnow@redhat.com>
On 11/17/2017 03:07 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 <vsementsov@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: > > how do you compile to get an error? and what is unused? > .../src/qemu/block/dirty-bitmap.c:368:13: error: ‘bdrv_release_dirty_bitmap_locked’ defined but not used [-Werror=unused-function] static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors I commented on the wrong prototype. The ((__unused__)) attribute just quiets this warning so it can compile without you having to refactor. --js
17.11.2017 21:25, John Snow wrote: > > On 11/17/2017 03:07 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 <vsementsov@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: >> how do you compile to get an error? and what is unused? >> > .../src/qemu/block/dirty-bitmap.c:368:13: error: > ‘bdrv_release_dirty_bitmap_locked’ defined but not used > [-Werror=unused-function] > static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > > I commented on the wrong prototype. The ((__unused__)) attribute just > quiets this warning so it can compile without you having to refactor. aha ok, you are right > > --js
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 */ +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(); } +} -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) {
It is needed to realize bdrv_dirty_bitmap_release_successor in the following patch. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/dirty-bitmap.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)