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

[John Snow]

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:

__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>