[2/3] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

Firstly, no reason to optimize failure path. Then, function name is
ambiguous: it checks for readonly and similar things, but someone may
think that it will ignore normal bitmaps which was just unchanged, and
this is in bad relation with the fact that we should drop IN_USE flag
for unchanged bitmaps in the image.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c         | 12 ------------
 block/qcow2-bitmap.c         | 23 +++++++++++++----------
 3 files changed, 13 insertions(+), 23 deletions(-)

On 5/23/19 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Firstly, no reason to optimize failure path. Then, function name is
> ambiguous: it checks for readonly and similar things, but someone may
> think that it will ignore normal bitmaps which was just unchanged, and
> this is in bad relation with the fact that we should drop IN_USE flag
> for unchanged bitmaps in the image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h |  1 -
>  block/dirty-bitmap.c         | 12 ------------
>  block/qcow2-bitmap.c         | 23 +++++++++++++----------
>  3 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8044ace63e..816022972b 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -105,7 +105,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>  bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
> -bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>  BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>                                          BdrvDirtyBitmap *bitmap);
>  char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 59e6ebb861..eca2eed0bf 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -775,18 +775,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
>      return bitmap->inconsistent;
>  }
>  
> -bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
> -{
> -    BdrvDirtyBitmap *bm;
> -    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> -        if (bm->persistent && !bm->readonly && !bm->migration) {

The loop down below has these conditionals for skipping bitmaps:

        if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
            bdrv_dirty_bitmap_readonly(bitmap) ||
            bdrv_dirty_bitmap_inconsistent(bitmap)) {
            continue;
        }

It looks like a semantic change, but hiding inside of get_persistence is
this:

bitmap->persistent && !bitmap->migration;

So this is equivalent, actually. It's not readily apparent at a glance.

> -            return true;
> -        }
> -    }
> -
> -    return false;
> -}
> -
>  BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>                                          BdrvDirtyBitmap *bitmap)
>  {
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 8a75366c92..2b84bfa007 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1457,16 +1457,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      Qcow2Bitmap *bm;
>      QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
>      Qcow2BitmapTable *tb, *tb_next;
> -
> -    if (!bdrv_has_changed_persistent_bitmaps(bs)) {
> -        /* nothing to do */
> -        return;
> -    }
> -
> -    if (!can_write(bs)) {
> -        error_setg(errp, "No write access");
> -        return;
> -    }
> +    bool need_write = false;
>  
>      QSIMPLEQ_INIT(&drop_tables);
>  
> @@ -1494,6 +1485,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>              continue;
>          }
>  
> +        need_write = true;
> +
>          if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
>              error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
>                            name);
> @@ -1532,6 +1525,15 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>          bm->dirty_bitmap = bitmap;
>      }
>  
> +    if (!need_write) {
> +        goto success;
> +    }
> +
> +    if (!can_write(bs)) {
> +        error_setg(errp, "No write access");
> +        goto fail;
> +    }
> +
>      /* allocate clusters and store bitmaps */
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>          if (bm->dirty_bitmap == NULL) {
> @@ -1573,6 +1575,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>          bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>      }
>  
> +success:
>      bitmap_list_free(bm_list);
>      return;
>  
> 

Alright, interesting. You're right that the function you're removing is
pretty badly named for what it actually does. I got confused by that not
too long ago.

The rationale against optimizing the error path works for me, too.

Seems like it's equivalent before and after, so:

Reviewed-by: John Snow <jsnow@redhat.com>