Re: [Qemu-block] [Qemu-devel] [PATCH v8 02/14] block/dirty-bitmap: add locked version of bdrv_release_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> > ---
> >   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 <address@hidden>


--
Best regards,
Vladimir