[v5,11/15] block: simplify blockdev-backup

Commit Message

Denis V. Lunev July 4, 2016, 12:28 p.m. UTC

From: Pavel Butsykin <[e-mail]>

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 blockdev-backup parameters.

Signed-off-by: Pavel Butsykin <[e-mail]>
Signed-off-by: Denis V. Lunev <[e-mail]>
CC: Jeff Cody <[e-mail]>
CC: Markus Armbruster <[e-mail]>
CC: Eric Blake <[e-mail]>
CC: John Snow <[e-mail]>
CC: Stefan Hajnoczi <[e-mail]>
CC: Kevin Wolf <[e-mail]>
---
 blockdev.c           | 65 +++++++++++++++-------------------------------------
 qapi/block-core.json |  6 ++++-
 2 files changed, 24 insertions(+), 47 deletions(-)

Comments

Stefan Hajnoczi July 14, 2016, 12:17 p.m. UTC | #1

On Mon, Jul 04, 2016 at 03:28:22PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <[e-mail]>
> 
> 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 blockdev-backup parameters.
> 
> Signed-off-by: Pavel Butsykin <[e-mail]>
> Signed-off-by: Denis V. Lunev <[e-mail]>
> CC: Jeff Cody <[e-mail]>
> CC: Markus Armbruster <[e-mail]>
> CC: Eric Blake <[e-mail]>
> CC: John Snow <[e-mail]>
> CC: Stefan Hajnoczi <[e-mail]>
> CC: Kevin Wolf <[e-mail]>
> ---
>  blockdev.c           | 65 +++++++++++++++-------------------------------------
>  qapi/block-core.json |  6 ++++-
>  2 files changed, 24 insertions(+), 47 deletions(-)

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Eric Blake July 14, 2016, 12:37 p.m. UTC | #2

On 07/04/2016 06:28 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <[e-mail]>
> 
> 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 blockdev-backup parameters.
> 
> Signed-off-by: Pavel Butsykin <[e-mail]>
> Signed-off-by: Denis V. Lunev <[e-mail]>
> CC: Jeff Cody <[e-mail]>
> CC: Markus Armbruster <[e-mail]>
> CC: Eric Blake <[e-mail]>
> CC: John Snow <[e-mail]>
> CC: Stefan Hajnoczi <[e-mail]>
> CC: Kevin Wolf <[e-mail]>
> ---
>  blockdev.c           | 65 +++++++++++++++-------------------------------------
>  qapi/block-core.json |  6 ++++-
>  2 files changed, 24 insertions(+), 47 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -1088,9 +1088,13 @@
>  #
>  # For the arguments, see the documentation of BlockdevBackup.
>  #
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
>  # Since 2.3
>  ##
> -{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
> +{ 'command': 'blockdev-backup', 'box': true,
> +  'data': 'BlockdevBackup' }

Will need a trivial rebase; my pending qapi v9 patches renamed the new
parameter 'boxed' instead of 'box'.

Patch

diff --git a/blockdev.c b/blockdev.c
index f002cc0..8f55590 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1903,14 +1903,8 @@  typedef struct BlockdevBackupState {
     AioContext *aio_context;
 } BlockdevBackupState;
 
-static void do_blockdev_backup(const char *device, const char *target,
-                               enum MirrorSyncMode sync,
-                               bool has_speed, int64_t speed,
-                               bool has_on_source_error,
-                               BlockdevOnError on_source_error,
-                               bool has_on_target_error,
-                               BlockdevOnError on_target_error,
-                               BlockJobTxn *txn, Error **errp);
+static void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
+                               Error **errp);
 
 static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
@@ -1950,12 +1944,7 @@  static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     state->bs = blk_bs(blk);
     bdrv_drained_begin(state->bs);
 
-    do_blockdev_backup(backup->device, backup->target,
-                       backup->sync,
-                       backup->has_speed, backup->speed,
-                       backup->has_on_source_error, backup->on_source_error,
-                       backup->has_on_target_error, backup->on_target_error,
-                       common->block_job_txn, &local_err);
+    do_blockdev_backup(backup, common->block_job_txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -3268,14 +3257,7 @@  BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
     return bdrv_named_nodes_list(errp);
 }
 
-void do_blockdev_backup(const char *device, const char *target,
-                         enum MirrorSyncMode sync,
-                         bool has_speed, int64_t speed,
-                         bool has_on_source_error,
-                         BlockdevOnError on_source_error,
-                         bool has_on_target_error,
-                         BlockdevOnError on_target_error,
-                         BlockJobTxn *txn, Error **errp)
+void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
@@ -3283,19 +3265,19 @@  void do_blockdev_backup(const char *device, const char *target,
     Error *local_err = NULL;
     AioContext *aio_context;
 
-    if (!has_speed) {
-        speed = 0;
+    if (!backup->has_speed) {
+        backup->speed = 0;
     }
-    if (!has_on_source_error) {
-        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+    if (!backup->has_on_source_error) {
+        backup->on_source_error = BLOCKDEV_ON_ERROR_REPORT;
     }
-    if (!has_on_target_error) {
-        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+    if (!backup->has_on_target_error) {
+        backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
+    blk = blk_by_name(backup->device);
     if (!blk) {
-        error_setg(errp, "Device '%s' not found", device);
+        error_setg(errp, "Device '%s' not found", backup->device);
         return;
     }
 
@@ -3303,12 +3285,12 @@  void do_blockdev_backup(const char *device, const char *target,
     aio_context_acquire(aio_context);
 
     if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
+        error_setg(errp, "Device '%s' has no medium", backup->device);
         goto out;
     }
     bs = blk_bs(blk);
 
-    target_bs = bdrv_lookup_bs(target, target, errp);
+    target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
     if (!target_bs) {
         goto out;
     }
@@ -3324,8 +3306,9 @@  void do_blockdev_backup(const char *device, const char *target,
             goto out;
         }
     }
-    backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
-                 on_target_error, block_job_cb, bs, txn, &local_err);
+    backup_start(bs, target_bs, backup->speed, backup->sync, NULL,
+                 backup->on_source_error, backup->on_target_error, block_job_cb,
+                 bs, txn, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
     }
@@ -3333,19 +3316,9 @@  out:
     aio_context_release(aio_context);
 }
 
-void qmp_blockdev_backup(const char *device, const char *target,
-                         enum MirrorSyncMode sync,
-                         bool has_speed, int64_t speed,
-                         bool has_on_source_error,
-                         BlockdevOnError on_source_error,
-                         bool has_on_target_error,
-                         BlockdevOnError on_target_error,
-                         Error **errp)
+void qmp_blockdev_backup(BlockdevBackup *arg, Error **errp)
 {
-    do_blockdev_backup(device, target, sync, has_speed, speed,
-                       has_on_source_error, on_source_error,
-                       has_on_target_error, on_target_error,
-                       NULL, errp);
+    do_blockdev_backup(arg, NULL, errp);
 }
 
 /* Parameter check and block job starting for drive mirroring.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6bd627e..8e0750a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1088,9 +1088,13 @@ 
 #
 # For the arguments, see the documentation of BlockdevBackup.
 #
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#
 # Since 2.3
 ##
-{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+{ 'command': 'blockdev-backup', 'box': true,
+  'data': 'BlockdevBackup' }
 
 
 ##