Re: [Qemu-devel] [Qemu-block] [PATCH v8 10/16] block: Simplify drive-mirror

[John Snow]

On 07/02/2016 10:58 PM, Eric Blake wrote:
> Now that we can support boxed commands, use it to greatly
> reduce the number of parameters (and likelihood of getting
> out of sync) when adjusting drive-mirror parameters.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v8: rebase, drop stale sentence in docs, don't rearrange initialiation
> v7: new patch
> ---
>  qapi/block-core.json | 20 +++++++++++---
>  blockdev.c           | 76 
> +++++++++++++++++++++++-----------------------------
>  hmp.c                | 25 ++++++++---------
>  3 files changed, 60 insertions(+), 61 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1bec29e..b91b07c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1108,6 +1108,21 @@
>  #
>  # Start mirroring a block device's writes to a new destination.
>  #
> +# See DriveMirror for parameter descriptions
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since 1.3
> +##
> +{ 'command': 'drive-mirror', 'box': true,
> +  'data': 'DriveMirror' }
> +
> +##
> +# DriveMirror
> +#
> +# A set of parameters describing drive mirror setup.
> +#
>  # @device:  the name of the device whose writes should be mirrored.
>  #
>  # @target: the target of the new image. If the file exists, or if it
> @@ -1154,12 +1169,9 @@
>  #         written. Both will result in identical contents.
>  #         Default is true. (Since 2.4)
>  #
> -# Returns: nothing on success
> -#          If @device is not a valid block device, DeviceNotFound
> -#
>  # Since 1.3

Should this still be "Since 1.3" for DriveMirror as a structure, since
it's being newly created?

(What color of shed would you like? Any color is fine for me.)

>  ##
> -{ 'command': 'drive-mirror',
> +{ 'struct': 'DriveMirror',
>    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>              '*node-name': 'str', '*replaces': 'str',
>              'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> diff --git a/blockdev.c b/blockdev.c
> index ddf30e1..f23bf99 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3458,19 +3458,7 @@ static void blockdev_mirror_common(BlockDriverState 
> *bs,
>                   block_job_cb, bs, errp);
>  }
> 
> -void qmp_drive_mirror(const char *device, const char *target,
> -                      bool has_format, const char *format,
> -                      bool has_node_name, const char *node_name,
> -                      bool has_replaces, const char *replaces,
> -                      enum MirrorSyncMode sync,
> -                      bool has_mode, enum NewImageMode mode,
> -                      bool has_speed, int64_t speed,
> -                      bool has_granularity, uint32_t granularity,
> -                      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)
> +void qmp_drive_mirror(DriveMirror *arg, Error **errp)

It's like a symphony!

>  {
>      BlockDriverState *bs;
>      BlockBackend *blk;
> @@ -3481,11 +3469,12 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      QDict *options = NULL;
>      int flags;
>      int64_t size;
> +    const char *format = arg->format;
> 
> -    blk = blk_by_name(device);
> +    blk = blk_by_name(arg->device);
>      if (!blk) {
>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", device);
> +                  "Device '%s' not found", arg->device);
>          return;
>      }
> 
> @@ -3493,24 +3482,25 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      aio_context_acquire(aio_context);
> 
>      if (!blk_is_available(blk)) {
> -        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, arg->device);
>          goto out;
>      }
>      bs = blk_bs(blk);
> -    if (!has_mode) {
> -        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +    if (!arg->has_mode) {
> +        arg->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>      }
> 
> -    if (!has_format) {
> -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : 
> bs->drv->format_name;
> +    if (!arg->has_format) {
> +        format = (arg->mode == NEW_IMAGE_MODE_EXISTING
> +                  ? NULL : bs->drv->format_name);
>      }
> 
>      flags = bs->open_flags | BDRV_O_RDWR;
>      source = backing_bs(bs);
> -    if (!source && sync == MIRROR_SYNC_MODE_TOP) {
> -        sync = MIRROR_SYNC_MODE_FULL;
> +    if (!source && arg->sync == MIRROR_SYNC_MODE_TOP) {
> +        arg->sync = MIRROR_SYNC_MODE_FULL;
>      }
> -    if (sync == MIRROR_SYNC_MODE_NONE) {
> +    if (arg->sync == MIRROR_SYNC_MODE_NONE) {
>          source = bs;
>      }
> 
> @@ -3520,18 +3510,18 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>          goto out;
>      }
> 
> -    if (has_replaces) {
> +    if (arg->has_replaces) {
>          BlockDriverState *to_replace_bs;
>          AioContext *replace_aio_context;
>          int64_t replace_size;
> 
> -        if (!has_node_name) {
> +        if (!arg->has_node_name) {
>              error_setg(errp, "a node-name must be provided when replacing a"
>                               " named node of the graph");
>              goto out;
>          }
> 
> -        to_replace_bs = check_to_replace_node(bs, replaces, &local_err);
> +        to_replace_bs = check_to_replace_node(bs, arg->replaces, &local_err);
> 
>          if (!to_replace_bs) {
>              error_propagate(errp, local_err);
> @@ -3550,26 +3540,26 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>          }
>      }
> 
> -    if (mode == NEW_IMAGE_MODE_ABSOLUTE_PATHS) {
> +    if (arg->mode == NEW_IMAGE_MODE_ABSOLUTE_PATHS) {
>          backing_mode = MIRROR_SOURCE_BACKING_CHAIN;
>      } else {
>          backing_mode = MIRROR_OPEN_BACKING_CHAIN;
>      }
> 
> -    if ((sync == MIRROR_SYNC_MODE_FULL || !source)
> -        && mode != NEW_IMAGE_MODE_EXISTING)
> +    if ((arg->sync == MIRROR_SYNC_MODE_FULL || !source)
> +        && arg->mode != NEW_IMAGE_MODE_EXISTING)
>      {
>          /* create new image w/o backing file */
>          assert(format);
> -        bdrv_img_create(target, format,
> +        bdrv_img_create(arg->target, format,
>                          NULL, NULL, NULL, size, flags, &local_err, false);
>      } else {
> -        switch (mode) {
> +        switch (arg->mode) {
>          case NEW_IMAGE_MODE_EXISTING:
>              break;
>          case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
>              /* create new image with backing file */
> -            bdrv_img_create(target, format,
> +            bdrv_img_create(arg->target, format,
>                              source->filename,
>                              source->drv->format_name,
>                              NULL, size, flags, &local_err, false);
> @@ -3585,8 +3575,8 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      }
> 
>      options = qdict_new();
> -    if (has_node_name) {
> -        qdict_put(options, "node-name", qstring_from_str(node_name));
> +    if (arg->has_node_name) {
> +        qdict_put(options, "node-name", qstring_from_str(arg->node_name));
>      }
>      if (format) {
>          qdict_put(options, "driver", qstring_from_str(format));
> @@ -3595,8 +3585,8 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      /* Mirroring takes care of copy-on-write using the source's backing
>       * file.
>       */
> -    target_bs = bdrv_open(target, NULL, options, flags | BDRV_O_NO_BACKING,
> -                          errp);
> +    target_bs = bdrv_open(arg->target, NULL, options,
> +                          flags | BDRV_O_NO_BACKING, errp);
>      if (!target_bs) {
>          goto out;
>      }
> @@ -3604,13 +3594,13 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      bdrv_set_aio_context(target_bs, aio_context);
> 
>      blockdev_mirror_common(bs, target_bs,
> -                           has_replaces, replaces, sync, backing_mode,
> -                           has_speed, speed,
> -                           has_granularity, granularity,
> -                           has_buf_size, buf_size,
> -                           has_on_source_error, on_source_error,
> -                           has_on_target_error, on_target_error,
> -                           has_unmap, unmap,
> +                           arg->has_replaces, arg->replaces, arg->sync,
> +                           backing_mode, arg->has_speed, arg->speed,
> +                           arg->has_granularity, arg->granularity,
> +                           arg->has_buf_size, arg->buf_size,
> +                           arg->has_on_source_error, arg->on_source_error,
> +                           arg->has_on_target_error, arg->on_target_error,
> +                           arg->has_unmap, arg->unmap,
>                             &local_err);
>      bdrv_unref(target_bs);
>      error_propagate(errp, local_err);
> diff --git a/hmp.c b/hmp.c
> index c7ca776..4819abc 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1077,31 +1077,28 @@ void hmp_block_resize(Monitor *mon, const QDict 
> *qdict)
> 
>  void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>  {
> -    const char *device = qdict_get_str(qdict, "device");
>      const char *filename = qdict_get_str(qdict, "target");
>      const char *format = qdict_get_try_str(qdict, "format");
>      bool reuse = qdict_get_try_bool(qdict, "reuse", false);
>      bool full = qdict_get_try_bool(qdict, "full", false);
> -    enum NewImageMode mode;
>      Error *err = NULL;
> +    DriveMirror mirror = {
> +        .device = (char *)qdict_get_str(qdict, "device"),
> +        .target = (char *)filename,
> +        .has_format = !!format,
> +        .format = (char *)format,
> +        .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
> +        .has_mode = true,
> +        .mode = reuse ? NEW_IMAGE_MODE_EXISTING : 
> NEW_IMAGE_MODE_ABSOLUTE_PATHS,
> +        .unmap = true,
> +    };
> 
>      if (!filename) {
>          error_setg(&err, QERR_MISSING_PARAMETER, "target");
>          hmp_handle_error(mon, &err);
>          return;
>      }
> -
> -    if (reuse) {
> -        mode = NEW_IMAGE_MODE_EXISTING;
> -    } else {
> -        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> -    }
> -
> -    qmp_drive_mirror(device, filename, !!format, format,
> -                     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, false, true, &err);
> +    qmp_drive_mirror(&mirror, &err);
>      hmp_handle_error(mon, &err);
>  }
> 

Looks good to me.

Reviewed-by: John Snow <address@hidden>