Re: [Qemu-block] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker

[Vladimir Sementsov-Ogievskiy]

26.09.2018 02:49, John Snow wrote:
> Instead of both frozen and qmp_locked checks, wrap it into one check.
> frozen implies the bitmap is split in two (for backup), and shouldn't
> be modified. qmp_locked implies it's being used by another operation,
> like being exported over NBD. In both cases it means we shouldn't allow
> the user to modify it in any meaningful way.
>
> Replace any usages where we check both frozen and qmp_locked with the
> new check.
>
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/dirty-bitmap.c         |  6 ++++++
>   blockdev.c                   | 29 ++++++++---------------------
>   include/block/dirty-bitmap.h |  1 +
>   3 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 8ac933cf1c..fc10543ab0 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
>       return bitmap->successor;
>   }
>   
> +/* Both conditions disallow user-modification via QMP. */
> +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap) {
> +    return !(bdrv_dirty_bitmap_frozen(bitmap) ||
> +             bdrv_dirty_bitmap_qmp_locked(bitmap));
> +}

to reduce number of '!', we may use opposite check, for ex 
"bdrv_dirty_bitmap_user_locked".

anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> +
>   void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool 
> qmp_locked)
>   {
>       qemu_mutex_lock(bitmap->mutex);
> diff --git a/blockdev.c b/blockdev.c
> index 670ae5bbde..dedcebb0fa 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2009,11 +2009,8 @@ static void 
> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> -        error_setg(errp, "Cannot modify a frozen bitmap");
> -        return;
> -    } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
> -        error_setg(errp, "Cannot modify a locked bitmap");
> +    if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
> +        error_setg(errp, "Cannot modify a bitmap in-use by another operation");
>           return;
>       } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
>           error_setg(errp, "Cannot clear a disabled bitmap");
> @@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
> const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +    if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
>           error_setg(errp,
> -                   "Bitmap '%s' is currently frozen and cannot be removed",
> -                   name);
> -        return;
> -    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently locked and cannot be removed",
> -                   name);
> +                   "Bitmap '%s' is currently in-use by another operation and"
> +                   " cannot be removed", name);
>           return;
>       }
>   
> @@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +    if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
>           error_setg(errp,
> -                   "Bitmap '%s' is currently frozen and cannot be modified",
> -                   name);
> -        return;
> -    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently locked and cannot be modified",
> -                   name);
> +                   "Bitmap '%s' is currently in-use by another operation"
> +                   " and cannot be cleared", name);
>           return;
>       } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>           error_setg(errp,
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 201ff7f20b..92cf7e39d2 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap);
>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>                                           BdrvDirtyBitmap *bitmap);


--
Best regards,
Vladimir