Re: [Qemu-devel] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror

[Fam Zheng <famz@redhat.com>]

On Wed, 06/24 18:34, Max Reitz wrote:
> On 09.06.2015 04:13, Fam Zheng wrote:
> >This is the part that will be reused by blockdev-mirror.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  blockdev.c | 158 +++++++++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 92 insertions(+), 66 deletions(-)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index b573e56..c32a9a9 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -2883,29 +2883,22 @@ out:
> >  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
> >-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,
> >-                      Error **errp)
> >+/* Parameter check and block job starting for mirroring.
> >+ * Caller should hold @device and @target's aio context (They must to be on the
> >+ * same aio context). */
> >+static void blockdev_mirror_common(BlockDriverState *bs,
> >+                                   BlockDriverState *target,
> >+                                   bool has_replaces, const char *replaces,
> >+                                   enum MirrorSyncMode sync,
> >+                                   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,
> >+                                   Error **errp)
> >  {
> >-    BlockBackend *blk;
> >-    BlockDriverState *bs;
> >-    BlockDriverState *source, *target_bs;
> >-    AioContext *aio_context;
> >-    BlockDriver *drv = NULL;
> >-    Error *local_err = NULL;
> >-    QDict *options = NULL;
> >-    int flags;
> >-    int64_t size;
> >-    int ret;
> >      if (!has_speed) {
> >          speed = 0;
> >@@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char *target,
> >      if (!has_on_target_error) {
> >          on_target_error = BLOCKDEV_ON_ERROR_REPORT;
> >      }
> >-    if (!has_mode) {
> >-        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> >-    }
> >      if (!has_granularity) {
> >          granularity = 0;
> >      }
> >@@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          return;
> >      }
> >-    blk = blk_by_name(device);
> >-    if (!blk) {
> >-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >-        return;
> >-    }
> >-
> >-    aio_context = blk_get_aio_context(blk);
> >-    aio_context_acquire(aio_context);
> >-
> >-    if (!blk_is_available(blk)) {
> >-        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> >-        goto out;
> >-    }
> >-    bs = blk_bs(blk);
> >-
> >-    if (!has_format) {
> >-        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> >-    }
> >-    if (format) {
> >-        drv = bdrv_find_format(format);
> >-        if (!drv) {
> >-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> >-            goto out;
> >-        }
> >-    }
> >-
> >      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> >+        return;
> >+    }
> >+
> >+    if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) {
> >+        sync = MIRROR_SYNC_MODE_FULL;
> >+    }
> >+
> >+    /* pass the node name to replace to mirror start since it's loose coupling
> >+     * and will allow to check whether the node still exist at mirror completion
> >+     */
> >+    mirror_start(bs, target,
> >+                 has_replaces ? replaces : NULL,
> >+                 speed, granularity, buf_size, sync,
> >+                 on_source_error, on_target_error,
> >+                 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,
> >+                      Error **errp)
> >+{
> >+    BlockDriverState *bs;
> >+    BlockBackend *blk;
> >+    BlockDriverState *source, *target_bs;
> >+    AioContext *aio_context;
> >+    BlockDriver *drv = NULL;
> >+    Error *local_err = NULL;
> >+    QDict *options = NULL;
> >+    int flags;
> >+    int64_t size;
> >+    int ret;
> >+
> >+    blk = blk_by_name(device);
> >+    if (!blk) {
> >+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >+        return;
> >+    }
> >+
> >+    aio_context = blk_get_aio_context(blk);
> >+    aio_context_acquire(aio_context);
> >+
> >+    if (!blk_is_available(blk)) {
> >+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> >          goto out;
> >      }
> >+    bs = blk_bs(blk);
> >+
> >+    if (!has_format) {
> >+        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> >+    }
> >+    if (format) {
> >+        drv = bdrv_find_format(format);
> >+        if (!drv) {
> >+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> >+            goto out;
> >+        }
> >+    }
> >      flags = bs->open_flags | BDRV_O_RDWR;
> >      source = bs->backing_hd;
> >@@ -2989,14 +3017,14 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          if (!has_node_name) {
> >              error_setg(errp, "a node-name must be provided when replacing a"
> >                               " named node of the graph");
> >-            goto out;
> >+            return;
> >          }
> >          to_replace_bs = check_to_replace_node(replaces, &local_err);
> >          if (!to_replace_bs) {
> >              error_propagate(errp, local_err);
> >-            goto out;
> >+            return;
> >          }
> >          replace_aio_context = bdrv_get_aio_context(to_replace_bs);
> >@@ -3007,7 +3035,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          if (size != replace_size) {
> >              error_setg(errp, "cannot replace image with a mirror image of "
> >                               "different size");
> >-            goto out;
> >+            return;
> >          }
> >      }
> 
> There are still a couple of "return;" statements left in the "if
> (has_replaces)" block. I think they should be replaced, too.

You're right! Will fix.

Fam

> 
> Max