Re: [Qemu-devel] [PATCH v2 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[Max Reitz]

On 01.07.19 22:13, John Snow wrote:
> It is used to do transactional movement of the bitmap (which is
> possible in conjunction with merge command). Transactional bitmap
> movement is needed in scenarios with external snapshot, when we don't
> want to leave copy of the bitmap in the base image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> ---
>  qapi/transaction.json          |  2 +
>  include/block/dirty-bitmap.h   |  3 +-
>  block.c                        |  2 +-
>  block/dirty-bitmap.c           | 16 +++----
>  blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
>  migration/block-dirty-bitmap.c |  2 +-
>  6 files changed, 87 insertions(+), 17 deletions(-)

[...]

> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 95a9c2a5d8..8551f8219e 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap {
>      bool inconsistent;          /* bitmap is persistent, but inconsistent.
>                                     It cannot be used at all in any way, 
> except
>                                     a QMP user can remove it. */
> -    bool migration;             /* Bitmap is selected for migration, it 
> should
> -                                   not be stored on the next inactivation
> -                                   (persistent flag doesn't matter until next
> -                                   invalidation).*/
> +    bool squelch_persistence;   /* We are either migrating or deleting this
> +                                 * bitmap; it should not be stored on the 
> next
> +                                 * inactivation. */

I like the English lessons you give me, but why not just dont_store?  Or
skip_storing?

>      QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
>  

[...]

> diff --git a/blockdev.c b/blockdev.c
> index 01248252ca..4143ab7bbb 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->bitmap) {
> +        bdrv_dirty_bitmap_squelch_persistence(state->bitmap, false);
> +        bdrv_dirty_bitmap_set_busy(state->bitmap, false);

Don’t you need to undo the removal?  Like, re-add it to state->bs, and
re-store it?

[...]

> @@ -2892,13 +2944,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
> const char *name,
>          aio_context_acquire(aio_context);
>          bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>          aio_context_release(aio_context);
> +
>          if (local_err != NULL) {
>              error_propagate(errp, local_err);
> -            return;
> +            return NULL;
>          }
>      }
>  
> -    bdrv_release_dirty_bitmap(bs, bitmap);
> +    if (release) {
> +        bdrv_release_dirty_bitmap(bs, bitmap);
> +    }
> +
> +    if (bitmap_bs) {
> +        *bitmap_bs = bs;
> +    }
> +
> +    return bitmap;

I’d prefer “release ? NULL : bitmap”.

Max

> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> +                                   Error **errp)
> +{
> +    do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
>  }
>  
>  /**

signature.asc
Description: OpenPGP digital signature