Re: [Qemu-devel] [PATCH 17/22] qmp: add autoload parameter to block-dirty-bitmap-add

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5668 bytes --]

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Optional. Default is false.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  blockdev.c           | 22 ++++++++++++++++++++--
>  qapi/block-core.json |  7 ++++++-
>  qmp-commands.hx      |  5 ++++-
>  3 files changed, 30 insertions(+), 4 deletions(-)

Design question: I see that being able to specify these flags when
creating bitmaps is useful. However, would about a way for the user to
change these flags on an existing dirty bitmap? Would you consider that
useful?

(Of course, if so, it can always be added later, we don't need it now.)

> diff --git a/blockdev.c b/blockdev.c
> index ec0ec75..00da7a1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1992,6 +1992,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>      qmp_block_dirty_bitmap_add(action->node, action->name,
>                                 action->has_granularity, action->granularity,
>                                 action->has_persistent, action->persistent,
> +                               action->has_autoload, action->autoload,
>                                 &local_err);
>  
>      if (!local_err) {
> @@ -2696,6 +2697,7 @@ out:
>  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>                                  bool has_granularity, uint32_t granularity,
>                                  bool has_persistent, bool persistent,
> +                                bool has_autoload, bool autoload,
>                                  Error **errp)
>  {
>      AioContext *aio_context;
> @@ -2729,10 +2731,26 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>      if (!has_persistent) {
>          persistent = false;
>      }
> +    if (!has_autoload) {
> +        autoload = false;
> +    }
> +
> +    if (autoload && !persistent) {
> +        error_setg(errp, "Autoload flag must be used only for persistent"
> +                         "bitmaps");

Missing space between "persistent" and "bitmaps".

Also, technically I think you should throw this error if has_autoload is
true instead of autoload. I would consider it wrong if a user specified
autoload at all, even autoload=false, without setting persistent=true.
But if you disagree, then please keep the condition the way it is.

> +        goto out;
> +    }
>  
>      bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> -    if (bitmap != NULL) {
> -        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
> +    if (bitmap == NULL) {
> +        goto out;
> +    }
> +
> +    if (persistent) {
> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
> +        if (autoload) {
> +            bdrv_dirty_bitmap_set_autoload(bitmap, true);
> +        }

By the way, a simpler way to do the same would be just

bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
bdrv_dirty_bitmap_set_autoload(bitmap, autoload);

But if you prefer this explicit style, that's fine, too.

>      }
>  
>   out:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2bf56cd..087a681 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1239,11 +1239,16 @@
>  #              corresponding block device on it's close. Default is false.
>  #              For block-dirty-bitmap-add. (Since 2.8)
>  #
> +# @autoload: #optional the bitmap will be autoloaded on it's storage image
> +#            open.

I'd rephrase this like "The bitmap will be automatically loaded when the
image it is stored in is opened." (Or keep "autoloaded" instead of
"automatically loaded", that doesn't really matter.)

>                     This flag is only for persistent bitmap and needed to inform
> +#            block driver that bitmap should be autoloaded on the next image
> +#            open.

Again, the user doesn't really have to care how the internals work. All
they need to know is that the image will be automatically loaded when
the image is opened the next time, and that is something the first
sentence told them already.

(Of course, "This flag may only be specified for persistent bitmaps"
should stay.)

>                     Default is false. For block-dirty-bitmap-add. (Since 2.8)

Again, I don't see the point of the last sentence.

Max

> +#
>  # Since 2.4
>  ##
>  { 'struct': 'BlockDirtyBitmapAdd',
>    'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> -  '*persistent': 'bool' } }
> +  '*persistent': 'bool', '*autoload': 'bool' } }
>  
>  ##
>  # @block-dirty-bitmap-add
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 434b418..8f4e841 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?,persistent:b?",
> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?,autoload:b?",
>          .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>      },
>  
> @@ -1461,6 +1461,9 @@ Arguments:
>  - "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)
> +- "autoload": only for persistent bitmaps. Bitmap will be autoloaded on it's
> +              storage image open. (json-bool, optional, default false)
> +              (Since 2.8)
>  
>  Example:
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]