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 <ebl...@redhat.com>
>
> ---
> 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 <js...@redhat.com>
