Re: [Qemu-devel] [PATCH v4 2/8] qmp: Add optional bool "unmap" to drive-mirror

[Eric Blake <eblake@redhat.com>]

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

On 05/21/2015 09:40 PM, Fam Zheng wrote:
> If specified as "true", it allows discarding on target sectors where source is
> not allocated.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c            | 7 +++++--
>  blockdev.c                | 5 +++++
>  hmp.c                     | 2 +-
>  include/block/block_int.h | 2 ++
>  qapi/block-core.json      | 5 ++++-
>  qmp-commands.hx           | 3 +++
>  6 files changed, 20 insertions(+), 4 deletions(-)
> 

> +++ b/blockdev.c
> @@ -2632,6 +2632,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>                        bool has_buf_size, int64_t buf_size,
>                        bool has_on_source_error, BlockdevOnError on_source_error,
>                        bool has_on_target_error, BlockdevOnError on_target_error,
> +                      bool has_unmap, bool unmap,
>                        Error **errp)
>  {
>      BlockBackend *blk;
> @@ -2663,6 +2664,9 @@ void qmp_drive_mirror(const char *device, const char *target,
>      if (!has_buf_size) {
>          buf_size = DEFAULT_MIRROR_BUF_SIZE;
>      }
> +    if (!has_unmap) {
> +        unmap = true;
> +    }

So drive-mirror defaults to true...

> +++ b/hmp.c
> @@ -1056,7 +1056,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>                       false, NULL, false, NULL,
>                       full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
>                       true, mode, false, 0, false, 0, false, 0,
> -                     false, 0, false, 0, &err);
> +                     false, 0, false, 0, false, false, &err);

...even though you are passing unmap=false here.  It might be a bit less
confusing to pass unmap=true, even though the has_unmap=false means to
fall back to the default, so that someone reading this code doesn't get
confused about the unmap choice being changed by the defaulting mechanism.

>      hmp_handle_error(mon, &err);
>  }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index db29b74..d136d34 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -587,6 +587,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>   * @mode: Whether to collapse all images in the chain to the target.
>   * @on_source_error: The action to take upon error reading from the source.
>   * @on_target_error: The action to take upon error writing to the target.
> + * @unmap: Whether unmap target where source sectors only contain zeroes.

s/Whether/Whether to/

> +++ b/qapi/block-core.json
> @@ -954,6 +954,8 @@
>  # @on-target-error: #optional the action to take on an error on the target,
>  #                   default 'report' (no limitations, since this applies to
>  #                   a different block device than @device).
> +# @unmap: #optional Whether unmap target sectors where source has zero. Default

s/Whether/Whether to/

> +#         is true. (Since 2.4)

That says what it does, but not why I would want to ever specify false.
 Is it worth a bit more explanation?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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