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

[Eric Blake]

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 @@
 #
 # Since 1.3
 ##
-{ '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);
     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);
 }

-- 
2.5.5