Re: [Qemu-block] [Qemu-devel] [PATCH v7 09/15] block: Simplify drive-mirror

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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>
>
> ---
> v7: new patch
> ---
>  qapi/block-core.json | 17 ++++++++++++-
>  blockdev.c           | 72 
> ++++++++++++++++++++++------------------------------
>  hmp.c                | 27 +++++++++-----------
>  3 files changed, 59 insertions(+), 57 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 26f7c0e..885a75a 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
> +#
> +# The parameters for the drive-mirror command.
> +#
>  # @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
> @@ -1159,7 +1174,7 @@
   #
   # Returns: nothing on success
   #          If @device is not a valid block device, DeviceNotFound

You forgot to delete this part.

   #
>  #
>  # Since 1.3

Kind of (same in previous patch).

>  ##
> -{ '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 b8db496..94850fd 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3457,19 +3457,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)
>  {
>      BlockDriverState *bs;
>      BlockBackend *blk;
> @@ -3480,11 +3468,12 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      int flags;
>      int64_t size;
>      int ret;
> +    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;
>      }
>
> @@ -3492,24 +3481,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;
>      }
>
> @@ -3519,18 +3509,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);
> @@ -3549,20 +3539,20 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>          }
>      }
>
> -    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);
> @@ -3578,8 +3568,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));
> @@ -3589,7 +3579,7 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>       * file.
>       */
>      target_bs = NULL;
> -    ret = bdrv_open(&target_bs, target, NULL, options,
> +    ret = bdrv_open(&target_bs, arg->target, NULL, options,
>                      flags | BDRV_O_NO_BACKING, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> @@ -3599,13 +3589,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,
> -                           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,
> +                           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);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/hmp.c b/hmp.c
> index a0c3f4e..c8f744b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1059,31 +1059,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;
> +    bool reuse = qdict_get_try_bool(qdict, "reuse", false);

Any particular reason to swap reuse and full?

>      Error *err = NULL;
> +    DriveMirror mirror = {
> +        .device = (char *) qdict_get_str(qdict, "device"),
> +        .target = (char *) filename,

No space between the parenthesized type and the experession in type
casts, please.

> +        .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);
>  }