Re: [Qemu-devel] [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add

[Max Reitz]

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
> Default is false.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> ---
>  blockdev.c           | 12 +++++++++++-
>  qapi/block-core.json |  7 ++++++-
>  qmp-commands.hx      |  5 ++++-
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 97062e3..ec0ec75 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1991,6 +1991,7 @@ static void 
> block_dirty_bitmap_add_prepare(BlkActionState *common,
>      /* AIO context taken and released within qmp_block_dirty_bitmap_add */
>      qmp_block_dirty_bitmap_add(action->node, action->name,
>                                 action->has_granularity, action->granularity,
> +                               action->has_persistent, action->persistent,
>                                 &local_err);
>  
>      if (!local_err) {
> @@ -2694,10 +2695,12 @@ out:
>  
>  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>                                  bool has_granularity, uint32_t granularity,
> +                                bool has_persistent, bool persistent,
>                                  Error **errp)
>  {
>      AioContext *aio_context;
>      BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
>  
>      if (!name || name[0] == '\0') {
>          error_setg(errp, "Bitmap name cannot be empty");
> @@ -2723,7 +2726,14 @@ void qmp_block_dirty_bitmap_add(const char *node, 
> const char *name,
>          granularity = bdrv_get_default_bitmap_granularity(bs);
>      }
>  
> -    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (!has_persistent) {
> +        persistent = false;
> +    }
> +
> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (bitmap != NULL) {
> +        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);

As I said before, I think this command should be able to return errors
and make use of that to reject making bitmaps persistent when the
respective block driver cannot handle them.

> +    }
>  
>   out:
>      aio_context_release(aio_context);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 31f9990..2bf56cd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1235,10 +1235,15 @@
>  # @granularity: #optional the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
>  #
> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
> +#              corresponding block device on it's close. Default is false.
> +#              For block-dirty-bitmap-add. (Since 2.8)

I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
because this whole struct is for block-dirty-bitmap-add (and for
block-dirty-bitmap-add transactions, to be exact, but @persistent will
surely work there, too, won't it?).

Also, I'd say "will be saved to the corresponding block device image
file" instead of just "block device", because in my understanding a
block device and its image file are two separate things.

> +#
>  # Since 2.4
>  ##
>  { 'struct': 'BlockDirtyBitmapAdd',
> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> +  '*persistent': 'bool' } }

I think normally we'd align the new line so that the opening ' of
'*persistent' is under the opening ' of 'node'.

>  
>  ##
>  # @block-dirty-bitmap-add
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba2a916..434b418 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1441,7 +1441,7 @@ EQMP
>  
>      {
>          .name       = "block-dirty-bitmap-add",
> -        .args_type  = "node:B,name:s,granularity:i?",
> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
>          .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>      },
>  
> @@ -1458,6 +1458,9 @@ Arguments:
>  - "node": device/node on which to create dirty bitmap (json-string)
>  - "name": name of the new dirty bitmap (json-string)
>  - "granularity": granularity to track writes with (int, optional)
> +- "persistent": bitmap will be saved to corresponding block device
> +                on it's close. Block driver should maintain persistent 
> bitmaps
> +                (json-bool, optional, default false) (Since 2.8)

And I don't know what the user is supposed to make of the information
that block drivers will take care of maintaining persistent bitmaps. All
they care about is that it will be stored in the corresponding image
file, so in my opinion it would be better to just omit the last sentence
here.

Max

>  
>  Example:
>  
>

signature.asc
Description: OpenPGP digital signature