[Qemu-devel] [PATCH v9.5 11/17] block: Simplify drive-mirror

[Eric Blake <eblake@redhat.com>]

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 <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>

---
v9.5: Rebase to commit 71aa9867 adding job-id
v9: s/box/boxed/, trivial enough to keep R-b
v8: rebase, drop stale sentence in docs, don't rearrange initialiation
v7: new patch
---
 qapi/block-core.json | 20 +++++++++++---
 blockdev.c           | 78 +++++++++++++++++++++++-----------------------------
 hmp.c                | 25 ++++++++---------
 3 files changed, 61 insertions(+), 62 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3d4b071..f4f3ef9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1120,6 +1120,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', 'boxed': true,
+  'data': 'DriveMirror' }
+
+##
+# DriveMirror
+#
+# A set of parameters describing drive mirror setup.
+#
 # @job-id: #optional identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
@@ -1169,12 +1184,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
 ##
-{ 'command': 'drive-mirror',
+{ 'struct': 'DriveMirror',
   'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
             '*format': 'str', '*node-name': 'str', '*replaces': 'str',
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
diff --git a/blockdev.c b/blockdev.c
index c5443c1..eafeba9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3466,19 +3466,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                  block_job_cb, bs, errp);
 }

-void qmp_drive_mirror(bool has_job_id, const char *job_id, 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;
@@ -3489,11 +3477,12 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
     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;
     }

@@ -3501,24 +3490,25 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
     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;
     }

@@ -3528,18 +3518,18 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
         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);
@@ -3558,26 +3548,26 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
         }
     }

-    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);
@@ -3593,8 +3583,8 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
     }

     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));
@@ -3603,22 +3593,22 @@ void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
     /* 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;
     }

     bdrv_set_aio_context(target_bs, aio_context);

-    blockdev_mirror_common(has_job_id ? job_id : NULL, 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,
+    blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
+                           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 ea17d49..cc2056e 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(false, NULL, 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);
 }

-- 
2.5.5