[Qemu-devel] [PATCH 0/3] qmp: Add "blockdev-backup"

[Qemu-devel] [PATCH 0/3] qmp: Add "blockdev-backup"

[Fam Zheng]

The existing drive-backup command accepts a target file path, but that
interface provides little flexibility on the properties of target block device,
compared to what is possible with "blockdev-add", "drive_add" or "-drive".

This is also a building block to allow image fleecing (creating a point in time
snapshot and export with nbd-server-add).

(For symmetry, blockdev-mirror will be added in a separate series.)

Fam

Fam Zheng (3):
  qmp: Add command 'blockdev-backup'
  block: Add blockdev-backup to transaction
  qemu-iotests: Test blockdev-backup in 055

 block/backup.c             |  26 +++++
 blockdev.c                 |  95 ++++++++++++++++
 qapi-schema.json           |   3 +
 qapi/block-core.json       |  50 ++++++++
 qmp-commands.hx            |  44 +++++++
 tests/qemu-iotests/055     | 277 ++++++++++++++++++++++++++++++++++++++-------
 tests/qemu-iotests/055.out |   4 +-
 7 files changed, 454 insertions(+), 45 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Fam Zheng]

Similar to drive-backup, but this command uses a device id as target
instead of creating/opening an image file.

Also add blocker on target bs, since the target is also a named device
now.

Add check and report error for bs == target which became possible but is
an illegal case with introduction of blockdev-backup.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c       | 26 ++++++++++++++++++++++++++
 blockdev.c           | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 167 insertions(+)

diff --git a/block/backup.c b/block/backup.c
index d0b0225..24e8ffc 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -344,6 +344,7 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
+    bdrv_op_unblock_all(target, job->common.blocker);
     bdrv_unref(target);
 
     block_job_completed(&job->common, ret);
@@ -362,6 +363,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     assert(target);
     assert(cb);
 
+    if (bs == target) {
+        error_setg(errp, "Source and target cannot be the same");
+        return;
+    }
+
     if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
          on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
         !bdrv_iostatus_is_enabled(bs)) {
@@ -369,6 +375,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name);
+        return;
+    }
+
+    if (!bdrv_is_inserted(target)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name);
+        return;
+    }
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+        return;
+    }
+
+    if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
+        return;
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -382,6 +406,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    bdrv_op_block_all(target, job->common.blocker);
+
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->target = target;
diff --git a/blockdev.c b/blockdev.c
index e919566..516de7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2046,6 +2046,8 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
+    /* Although backup_run has this check too, we need to use bs->drv below, so
+     * do an early check redundantly. */
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
@@ -2062,6 +2064,7 @@ void qmp_drive_backup(const char *device, const char *target,
         }
     }
 
+    /* Early check to avoid creating target */
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
         return;
     }
@@ -2124,6 +2127,50 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
     return bdrv_named_nodes_list();
 }
 
+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)
+{
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    Error *local_err = NULL;
+
+    if (!has_speed) {
+        speed = 0;
+    }
+    if (!has_on_source_error) {
+        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+    if (!has_on_target_error) {
+        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    target_bs = bdrv_find(target);
+    if (!target_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+        return;
+    }
+
+    bdrv_ref(target_bs);
+    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+                 block_job_cb, bs, &local_err);
+    if (local_err != NULL) {
+        bdrv_unref(target_bs);
+        error_propagate(errp, local_err);
+    }
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a685d02..b953c7b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -669,6 +669,40 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockdevBackup
+#
+# @device: the name of the device which should be copied.
+#
+# @target: the name of the backup target device.
+#
+# @sync: what parts of the disk image should be copied to the destination
+#        (all the disk, only the sectors allocated in the topmost image, or
+#        only new I/O).
+#
+# @speed: #optional the maximum speed, in bytes per second.
+#
+# @on-source-error: #optional the action to take on an error on the source,
+#                   default 'report'.  'stop' and 'enospc' can only be used
+#                   if the block device supports io-status (see BlockInfo).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+#                   default 'report' (no limitations, since this applies to
+#                   a different block device than @device).
+#
+# Note that @on-source-error and @on-target-error only affect background I/O.
+# If an error occurs during a guest write request, the device's rerror/werror
+# actions will be used.
+#
+# Since: 2.2
+##
+{ 'type': 'BlockdevBackup',
+  'data': { 'device': 'str', 'target': 'str',
+            'sync': 'MirrorSyncMode',
+            '*speed': 'int',
+            '*on-source-error': 'BlockdevOnError',
+            '*on-target-error': 'BlockdevOnError' } }
+
+##
 # @blockdev-snapshot-sync
 #
 # Generates a synchronous snapshot of a block device.
@@ -788,6 +822,22 @@
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @blockdev-backup
+#
+# Block device version for drive-backup command. Use existing device as target
+# of backup.
+#
+# For the arguments, see the documentation of BlockdevBackup.
+#
+# Returns: Nothing on success.
+#          If @device or @target is not a valid block device, DeviceNotFound.
+#
+# Since 2.2
+##
+{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+
+
+##
 # @query-named-block-nodes
 #
 # Get the named block driver list
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7658d4b..fa40260 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1094,6 +1094,50 @@ Example:
                                                "sync": "full",
                                                "target": "backup.img" } }
 <- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "blockdev-backup",
+        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+                      "on-source-error:s?,on-target-error:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
+    },
+
+SQMP
+blockdev-backup
+------------
+
+The device version of drive-backup: this command takes a existing named device
+as backup target.
+
+Arguments:
+
+- "device": the name of the device which should be copied.
+            (json-string)
+- "target": the target of the new image. If the file exists, or if it is a
+            device, the existing file/device will be used as the new
+            destination.  If it does not exist, a new file will be created.
+            (json-string)
+- "sync": what parts of the disk image should be copied to the destination;
+          possibilities include "full" for all the disk, "top" for only the
+          sectors allocated in the topmost image, or "none" to only replicate
+          new I/O (MirrorSyncMode).
+- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "on-source-error": the action to take on an error on the source, default
+                     'report'.  'stop' and 'enospc' can only be used
+                     if the block device supports io-status.
+                     (BlockdevOnError, optional)
+- "on-target-error": the action to take on an error on the target, default
+                     'report' (no limitations, since this applies to
+                     a different block device than device).
+                     (BlockdevOnError, optional)
+
+Example:
+-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
+                                                  "target": "tgt-id" } }
+<- { "return": {} }
+
 EQMP
 
     {
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> Similar to drive-backup, but this command uses a device id as target
> instead of creating/opening an image file.
>
> Also add blocker on target bs, since the target is also a named device
> now.
>
> Add check and report error for bs == target which became possible but is
> an illegal case with introduction of blockdev-backup.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c       | 26 ++++++++++++++++++++++++++
>  blockdev.c           | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 167 insertions(+)
>
> diff --git a/block/backup.c b/block/backup.c
> index d0b0225..24e8ffc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -344,6 +344,7 @@ static void coroutine_fn backup_run(void *opaque)
>      hbitmap_free(job->bitmap);
>  
>      bdrv_iostatus_disable(target);
> +    bdrv_op_unblock_all(target, job->common.blocker);
>      bdrv_unref(target);
>  
>      block_job_completed(&job->common, ret);
> @@ -362,6 +363,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>      assert(target);
>      assert(cb);
>  
> +    if (bs == target) {
> +        error_setg(errp, "Source and target cannot be the same");
> +        return;
> +    }
> +
>      if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
>           on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
>          !bdrv_iostatus_is_enabled(bs)) {
> @@ -369,6 +375,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>          return;
>      }
>  
> +    if (!bdrv_is_inserted(bs)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name);
> +        return;
> +    }
> +
> +    if (!bdrv_is_inserted(target)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name);
> +        return;
> +    }
> +
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
> +        return;
> +    }
> +
> +    if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
> +        return;
> +    }
> +
>      len = bdrv_getlength(bs);
>      if (len < 0) {
>          error_setg_errno(errp, -len, "unable to get length for '%s'",
> @@ -382,6 +406,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>          return;
>      }
>  
> +    bdrv_op_block_all(target, job->common.blocker);
> +
>      job->on_source_error = on_source_error;
>      job->on_target_error = on_target_error;
>      job->target = target;
> diff --git a/blockdev.c b/blockdev.c
> index e919566..516de7f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2046,6 +2046,8 @@ void qmp_drive_backup(const char *device, const char *target,
>          return;
>      }
>  
> +    /* Although backup_run has this check too, we need to use bs->drv below, so
> +     * do an early check redundantly. */
>      if (!bdrv_is_inserted(bs)) {
>          error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>          return;
> @@ -2062,6 +2064,7 @@ void qmp_drive_backup(const char *device, const char *target,
>          }
>      }
>  
> +    /* Early check to avoid creating target */
>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
>          return;
>      }
> @@ -2124,6 +2127,50 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
>      return bdrv_named_nodes_list();
>  }
>  
> +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)
> +{
> +    BlockDriverState *bs;
> +    BlockDriverState *target_bs;
> +    Error *local_err = NULL;
> +
> +    if (!has_speed) {
> +        speed = 0;
> +    }
> +    if (!has_on_source_error) {
> +        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
> +    }
> +    if (!has_on_target_error) {
> +        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
> +    }
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    target_bs = bdrv_find(target);
> +    if (!target_bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
> +        return;
> +    }
> +
> +    bdrv_ref(target_bs);
> +    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
> +                 block_job_cb, bs, &local_err);
> +    if (local_err != NULL) {
> +        bdrv_unref(target_bs);
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
>  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
>  
>  void qmp_drive_mirror(const char *device, const char *target,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a685d02..b953c7b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -669,6 +669,40 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockdevBackup
> +#
> +# @device: the name of the device which should be copied.
> +#
> +# @target: the name of the backup target device.
> +#
> +# @sync: what parts of the disk image should be copied to the destination
> +#        (all the disk, only the sectors allocated in the topmost image, or
> +#        only new I/O).
> +#
> +# @speed: #optional the maximum speed, in bytes per second.
> +#
> +# @on-source-error: #optional the action to take on an error on the source,
> +#                   default 'report'.  'stop' and 'enospc' can only be used
> +#                   if the block device supports io-status (see BlockInfo).
> +#
> +# @on-target-error: #optional the action to take on an error on the target,
> +#                   default 'report' (no limitations, since this applies to
> +#                   a different block device than @device).
> +#
> +# Note that @on-source-error and @on-target-error only affect background I/O.
> +# If an error occurs during a guest write request, the device's rerror/werror
> +# actions will be used.

Changes from DriveBackup:

* @target is a device name, i.e. the name stored in BDS member
  device_name[].

* Therfore, @format and @mode don't apply, and are dropped.

Good.

> +#
> +# Since: 2.2
> +##
> +{ 'type': 'BlockdevBackup',
> +  'data': { 'device': 'str', 'target': 'str',
> +            'sync': 'MirrorSyncMode',
> +            '*speed': 'int',
> +            '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError' } }
> +
> +##
>  # @blockdev-snapshot-sync
>  #
>  # Generates a synchronous snapshot of a block device.
> @@ -788,6 +822,22 @@
>  { 'command': 'drive-backup', 'data': 'DriveBackup' }
>  
>  ##
> +# @blockdev-backup
> +#
> +# Block device version for drive-backup command. Use existing device as target
> +# of backup.
> +#
> +# For the arguments, see the documentation of BlockdevBackup.

blockdev-backup is the modern command we want people to use,
drive-backup is the old one we provide for compatibility, right?

If yes, blockdev-backup should be documented without referring to
drive-backup.  It's okay for drive-backup's documentation to refer to
block-backup, though.

> +#
> +# Returns: Nothing on success.
> +#          If @device or @target is not a valid block device, DeviceNotFound.

Do we really want error classes other than GenericError in new code
without a clear need?

> +#
> +# Since 2.2
> +##
> +{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
> +
> +
> +##
>  # @query-named-block-nodes
>  #
>  # Get the named block driver list
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7658d4b..fa40260 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1094,6 +1094,50 @@ Example:
>                                                 "sync": "full",
>                                                 "target": "backup.img" } }
>  <- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "blockdev-backup",
> +        .args_type  = "sync:s,device:B,target:B,speed:i?,"
> +                      "on-source-error:s?,on-target-error:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
> +    },
> +
> +SQMP
> +blockdev-backup
> +------------
> +
> +The device version of drive-backup: this command takes a existing named device
> +as backup target.
> +
> +Arguments:
> +
> +- "device": the name of the device which should be copied.
> +            (json-string)
> +- "target": the target of the new image. If the file exists, or if it is a
> +            device, the existing file/device will be used as the new
> +            destination.  If it does not exist, a new file will be created.
> +            (json-string)

Copied from drive-backup without change.  Please fix.

> +- "sync": what parts of the disk image should be copied to the destination;
> +          possibilities include "full" for all the disk, "top" for only the
> +          sectors allocated in the topmost image, or "none" to only replicate
> +          new I/O (MirrorSyncMode).
> +- "speed": the maximum speed, in bytes per second (json-int, optional)
> +- "on-source-error": the action to take on an error on the source, default
> +                     'report'.  'stop' and 'enospc' can only be used
> +                     if the block device supports io-status.
> +                     (BlockdevOnError, optional)
> +- "on-target-error": the action to take on an error on the target, default
> +                     'report' (no limitations, since this applies to
> +                     a different block device than device).
> +                     (BlockdevOnError, optional)
> +
> +Example:
> +-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
> +                                                  "target": "tgt-id" } }
> +<- { "return": {} }
> +
>  EQMP
>  
>      {

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Kevin Wolf]

Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
> Similar to drive-backup, but this command uses a device id as target
> instead of creating/opening an image file.
> 
> Also add blocker on target bs, since the target is also a named device
> now.
> 
> Add check and report error for bs == target which became possible but is
> an illegal case with introduction of blockdev-backup.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a685d02..b953c7b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -669,6 +669,40 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockdevBackup
> +#
> +# @device: the name of the device which should be copied.
> +#
> +# @target: the name of the backup target device.

Both of these are either a BlockBackend ID or a BDS node-name, right? Do
we have a standard way of expressing this? "name of the device" isn't
quite clear.

> +# @sync: what parts of the disk image should be copied to the destination
> +#        (all the disk, only the sectors allocated in the topmost image, or
> +#        only new I/O).
> +#
> +# @speed: #optional the maximum speed, in bytes per second.
> +#
> +# @on-source-error: #optional the action to take on an error on the source,
> +#                   default 'report'.  'stop' and 'enospc' can only be used
> +#                   if the block device supports io-status (see BlockInfo).
> +#
> +# @on-target-error: #optional the action to take on an error on the target,
> +#                   default 'report' (no limitations, since this applies to
> +#                   a different block device than @device).
> +#
> +# Note that @on-source-error and @on-target-error only affect background I/O.
> +# If an error occurs during a guest write request, the device's rerror/werror
> +# actions will be used.
> +#
> +# Since: 2.2
> +##
> +{ 'type': 'BlockdevBackup',
> +  'data': { 'device': 'str', 'target': 'str',
> +            'sync': 'MirrorSyncMode',
> +            '*speed': 'int',
> +            '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError' } }

Kevin

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Fam Zheng]

On Fri, 10/31 10:01, Kevin Wolf wrote:
> Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
> > Similar to drive-backup, but this command uses a device id as target
> > instead of creating/opening an image file.
> > 
> > Also add blocker on target bs, since the target is also a named device
> > now.
> > 
> > Add check and report error for bs == target which became possible but is
> > an illegal case with introduction of blockdev-backup.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index a685d02..b953c7b 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -669,6 +669,40 @@
> >              '*on-target-error': 'BlockdevOnError' } }
> >  
> >  ##
> > +# @BlockdevBackup
> > +#
> > +# @device: the name of the device which should be copied.
> > +#
> > +# @target: the name of the backup target device.
> 
> Both of these are either a BlockBackend ID or a BDS node-name, right? Do
> we have a standard way of expressing this? "name of the device" isn't
> quite clear.

"name of the device" is used everywhere to document the "device" parameters in
our json schema. Since we have BlockBackend now, device-name and node-name
could be better distinguished. All we have to do is giving a beautiful name to
both.

[This patch is only a copy&paste and is consistent with the rest part of the
file. So I'll leave it for now :]

Fam

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Kevin Wolf]

Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben:
> On Fri, 10/31 10:01, Kevin Wolf wrote:
> > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
> > > Similar to drive-backup, but this command uses a device id as target
> > > instead of creating/opening an image file.
> > > 
> > > Also add blocker on target bs, since the target is also a named device
> > > now.
> > > 
> > > Add check and report error for bs == target which became possible but is
> > > an illegal case with introduction of blockdev-backup.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index a685d02..b953c7b 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -669,6 +669,40 @@
> > >              '*on-target-error': 'BlockdevOnError' } }
> > >  
> > >  ##
> > > +# @BlockdevBackup
> > > +#
> > > +# @device: the name of the device which should be copied.
> > > +#
> > > +# @target: the name of the backup target device.
> > 
> > Both of these are either a BlockBackend ID or a BDS node-name, right? Do
> > we have a standard way of expressing this? "name of the device" isn't
> > quite clear.
> 
> "name of the device" is used everywhere to document the "device" parameters in
> our json schema. Since we have BlockBackend now, device-name and node-name
> could be better distinguished. All we have to do is giving a beautiful name to
> both.
> 
> [This patch is only a copy&paste and is consistent with the rest part of the
> file. So I'll leave it for now :]

The rest of the file doesn't accept node names. But looking at your
actual code, it seems that you are doing the same (by usign bdrv_find()
instead of bdrv_lookup_bs()).

Shouldn't a proper blockdev-* command accept node names as well?

Kevin

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Fam Zheng]

On Mon, 11/03 15:32, Kevin Wolf wrote:
> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben:
> > On Fri, 10/31 10:01, Kevin Wolf wrote:
> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
> > > > Similar to drive-backup, but this command uses a device id as target
> > > > instead of creating/opening an image file.
> > > > 
> > > > Also add blocker on target bs, since the target is also a named device
> > > > now.
> > > > 
> > > > Add check and report error for bs == target which became possible but is
> > > > an illegal case with introduction of blockdev-backup.
> > > > 
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > 
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index a685d02..b953c7b 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -669,6 +669,40 @@
> > > >              '*on-target-error': 'BlockdevOnError' } }
> > > >  
> > > >  ##
> > > > +# @BlockdevBackup
> > > > +#
> > > > +# @device: the name of the device which should be copied.
> > > > +#
> > > > +# @target: the name of the backup target device.
> > > 
> > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do
> > > we have a standard way of expressing this? "name of the device" isn't
> > > quite clear.
> > 
> > "name of the device" is used everywhere to document the "device" parameters in
> > our json schema. Since we have BlockBackend now, device-name and node-name
> > could be better distinguished. All we have to do is giving a beautiful name to
> > both.
> > 
> > [This patch is only a copy&paste and is consistent with the rest part of the
> > file. So I'll leave it for now :]
> 
> The rest of the file doesn't accept node names. But looking at your
> actual code, it seems that you are doing the same (by usign bdrv_find()
> instead of bdrv_lookup_bs()).

Yes, to be consistent with drive-backup.

> 
> Shouldn't a proper blockdev-* command accept node names as well?
> 

I am not sure, it's still blockdev-backup, not blocknode-backup.

I think that may be another thing, to changed drive-*'s @device parameter, and
blockdev-*'s @device and @target to accept node names, altogether.

Fam

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 11/03 15:32, Kevin Wolf wrote:
>> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben:
>> > On Fri, 10/31 10:01, Kevin Wolf wrote:
>> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
>> > > > Similar to drive-backup, but this command uses a device id as target
>> > > > instead of creating/opening an image file.
>> > > > 
>> > > > Also add blocker on target bs, since the target is also a named device
>> > > > now.
>> > > > 
>> > > > Add check and report error for bs == target which became possible but is
>> > > > an illegal case with introduction of blockdev-backup.
>> > > > 
>> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
>> > > 
>> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > > > index a685d02..b953c7b 100644
>> > > > --- a/qapi/block-core.json
>> > > > +++ b/qapi/block-core.json
>> > > > @@ -669,6 +669,40 @@
>> > > >              '*on-target-error': 'BlockdevOnError' } }
>> > > >  
>> > > >  ##
>> > > > +# @BlockdevBackup
>> > > > +#
>> > > > +# @device: the name of the device which should be copied.
>> > > > +#
>> > > > +# @target: the name of the backup target device.
>> > > 
>> > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do
>> > > we have a standard way of expressing this? "name of the device" isn't
>> > > quite clear.
>> > 
>> > "name of the device" is used everywhere to document the "device"
>> > parameters in
>> > our json schema. Since we have BlockBackend now, device-name and node-name
>> > could be better distinguished. All we have to do is giving a
>> > beautiful name to
>> > both.
>> > 
>> > [This patch is only a copy&paste and is consistent with the rest part of the
>> > file. So I'll leave it for now :]
>> 
>> The rest of the file doesn't accept node names. But looking at your
>> actual code, it seems that you are doing the same (by usign bdrv_find()
>> instead of bdrv_lookup_bs()).
>
> Yes, to be consistent with drive-backup.
>
>> 
>> Shouldn't a proper blockdev-* command accept node names as well?
>> 
>
> I am not sure, it's still blockdev-backup, not blocknode-backup.
>
> I think that may be another thing, to changed drive-*'s @device parameter, and
> blockdev-*'s @device and @target to accept node names, altogether.

We have many commands identifying nodes by some name: root nodes by BDS
device_name[] (now BB name), inner nodes by "file name" (ugh), and
arbitrary nodes by BDS name_name[].

An obvious task is deprecating "file name" in favor of node names.

Less obvious is where to accept a node name instead of / in addition to
a device name.

Want me to start a thread on this?

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Fam Zheng]

On Tue, 11/04 07:47, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Mon, 11/03 15:32, Kevin Wolf wrote:
> >> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben:
> >> > On Fri, 10/31 10:01, Kevin Wolf wrote:
> >> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
> >> > > > Similar to drive-backup, but this command uses a device id as target
> >> > > > instead of creating/opening an image file.
> >> > > > 
> >> > > > Also add blocker on target bs, since the target is also a named device
> >> > > > now.
> >> > > > 
> >> > > > Add check and report error for bs == target which became possible but is
> >> > > > an illegal case with introduction of blockdev-backup.
> >> > > > 
> >> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> >> > > 
> >> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> > > > index a685d02..b953c7b 100644
> >> > > > --- a/qapi/block-core.json
> >> > > > +++ b/qapi/block-core.json
> >> > > > @@ -669,6 +669,40 @@
> >> > > >              '*on-target-error': 'BlockdevOnError' } }
> >> > > >  
> >> > > >  ##
> >> > > > +# @BlockdevBackup
> >> > > > +#
> >> > > > +# @device: the name of the device which should be copied.
> >> > > > +#
> >> > > > +# @target: the name of the backup target device.
> >> > > 
> >> > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do
> >> > > we have a standard way of expressing this? "name of the device" isn't
> >> > > quite clear.
> >> > 
> >> > "name of the device" is used everywhere to document the "device"
> >> > parameters in
> >> > our json schema. Since we have BlockBackend now, device-name and node-name
> >> > could be better distinguished. All we have to do is giving a
> >> > beautiful name to
> >> > both.
> >> > 
> >> > [This patch is only a copy&paste and is consistent with the rest part of the
> >> > file. So I'll leave it for now :]
> >> 
> >> The rest of the file doesn't accept node names. But looking at your
> >> actual code, it seems that you are doing the same (by usign bdrv_find()
> >> instead of bdrv_lookup_bs()).
> >
> > Yes, to be consistent with drive-backup.
> >
> >> 
> >> Shouldn't a proper blockdev-* command accept node names as well?
> >> 
> >
> > I am not sure, it's still blockdev-backup, not blocknode-backup.
> >
> > I think that may be another thing, to changed drive-*'s @device parameter, and
> > blockdev-*'s @device and @target to accept node names, altogether.
> 
> We have many commands identifying nodes by some name: root nodes by BDS
> device_name[] (now BB name), inner nodes by "file name" (ugh), and
> arbitrary nodes by BDS name_name[].
> 
> An obvious task is deprecating "file name" in favor of node names.
> 
> Less obvious is where to accept a node name instead of / in addition to
> a device name.
> 
> Want me to start a thread on this?

Yes please, it's good to review the situation and make some concrete
convention and plan for the transition.

Fam

Re: [Qemu-devel] [PATCH 1/3] qmp: Add command 'blockdev-backup'

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Tue, 11/04 07:47, Markus Armbruster wrote:
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > On Mon, 11/03 15:32, Kevin Wolf wrote:
>> >> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben:
>> >> > On Fri, 10/31 10:01, Kevin Wolf wrote:
>> >> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben:
>> >> > > > Similar to drive-backup, but this command uses a device id as target
>> >> > > > instead of creating/opening an image file.
>> >> > > > 
>> >> > > > Also add blocker on target bs, since the target is also a
>> >> > > > named device
>> >> > > > now.
>> >> > > > 
>> >> > > > Add check and report error for bs == target which became
>> >> > > > possible but is
>> >> > > > an illegal case with introduction of blockdev-backup.
>> >> > > > 
>> >> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
>> >> > > 
>> >> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> > > > index a685d02..b953c7b 100644
>> >> > > > --- a/qapi/block-core.json
>> >> > > > +++ b/qapi/block-core.json
>> >> > > > @@ -669,6 +669,40 @@
>> >> > > >              '*on-target-error': 'BlockdevOnError' } }
>> >> > > >  
>> >> > > >  ##
>> >> > > > +# @BlockdevBackup
>> >> > > > +#
>> >> > > > +# @device: the name of the device which should be copied.
>> >> > > > +#
>> >> > > > +# @target: the name of the backup target device.
>> >> > > 
>> >> > > Both of these are either a BlockBackend ID or a BDS
>> >> > > node-name, right? Do
>> >> > > we have a standard way of expressing this? "name of the device" isn't
>> >> > > quite clear.
>> >> > 
>> >> > "name of the device" is used everywhere to document the "device"
>> >> > parameters in
>> >> > our json schema. Since we have BlockBackend now, device-name
>> >> > and node-name
>> >> > could be better distinguished. All we have to do is giving a
>> >> > beautiful name to
>> >> > both.
>> >> > 
>> >> > [This patch is only a copy&paste and is consistent with the
>> >> > rest part of the
>> >> > file. So I'll leave it for now :]
>> >> 
>> >> The rest of the file doesn't accept node names. But looking at your
>> >> actual code, it seems that you are doing the same (by usign bdrv_find()
>> >> instead of bdrv_lookup_bs()).
>> >
>> > Yes, to be consistent with drive-backup.
>> >
>> >> 
>> >> Shouldn't a proper blockdev-* command accept node names as well?
>> >> 
>> >
>> > I am not sure, it's still blockdev-backup, not blocknode-backup.
>> >
>> > I think that may be another thing, to changed drive-*'s @device
>> > parameter, and
>> > blockdev-*'s @device and @target to accept node names, altogether.
>> 
>> We have many commands identifying nodes by some name: root nodes by BDS
>> device_name[] (now BB name), inner nodes by "file name" (ugh), and
>> arbitrary nodes by BDS name_name[].
>> 
>> An obvious task is deprecating "file name" in favor of node names.
>> 
>> Less obvious is where to accept a node name instead of / in addition to
>> a device name.
>> 
>> Want me to start a thread on this?
>
> Yes please, it's good to review the situation and make some concrete
> convention and plan for the transition.

Done: Review of monitor commands identifying BDS / BB by name

[Qemu-devel] [PATCH 2/3] block: Add blockdev-backup to transaction

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 516de7f..58565d7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1424,6 +1424,49 @@ static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
+typedef struct BlockdevBackupState {
+    BlkTransactionState common;
+    BlockDriverState *bs;
+    BlockJob *job;
+} BlockdevBackupState;
+
+static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
+{
+    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+    BlockdevBackup *backup;
+    Error *local_err = NULL;
+
+    assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
+    backup = common->action->blockdev_backup;
+
+    qmp_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,
+                        &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        state->bs = NULL;
+        state->job = NULL;
+        return;
+    }
+
+    state->bs = bdrv_find(backup->device);
+    state->job = state->bs->job;
+}
+
+static void blockdev_backup_abort(BlkTransactionState *common)
+{
+    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+    BlockDriverState *bs = state->bs;
+
+    /* Only cancel if it's the job we started */
+    if (bs && bs->job && bs->job == state->job) {
+        block_job_cancel_sync(bs->job);
+    }
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -1446,6 +1489,11 @@ static const BdrvActionOps actions[] = {
         .prepare = drive_backup_prepare,
         .abort = drive_backup_abort,
     },
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
+        .instance_size = sizeof(BlockdevBackupState),
+        .prepare = blockdev_backup_prepare,
+        .abort = blockdev_backup_abort,
+    },
     [TRANSACTION_ACTION_KIND_ABORT] = {
         .instance_size = sizeof(BlkTransactionState),
         .prepare = abort_prepare,
diff --git a/qapi-schema.json b/qapi-schema.json
index 689b548..e36bdc3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1251,11 +1251,14 @@
 #
 # A discriminated record of operations that can be performed with
 # @transaction.
+#
+# Since 1.1, blockdev-backup since 2.1
 ##
 { 'union': 'TransactionAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
        'drive-backup': 'DriveBackup',
+       'blockdev-backup': 'BlockdevBackup',
        'abort': 'Abort',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
    } }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] block: Add blockdev-backup to transaction

[Markus Armbruster]

I'm not familiar with transactions, so all I can do is match your code
against the DriveBackup action.  Passes that sanity check.

Re: [Qemu-devel] [PATCH 2/3] block: Add blockdev-backup to transaction

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On 09/10/2014 11:05 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c       | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  3 +++
>  2 files changed, 51 insertions(+)
> 

> +++ b/qapi-schema.json
> @@ -1251,11 +1251,14 @@
>  #
>  # A discriminated record of operations that can be performed with
>  # @transaction.
> +#
> +# Since 1.1, blockdev-backup since 2.1

If we're going to do this, we should be accurate.  s/2.1/2.2.  Furthermore:

drive-backup and abort were added in 1.6
blockdev-snapshot-internal-sync was added in 1.7

(but yes, I'm glad that you are wanting to add version information).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH 3/3] qemu-iotests: Test blockdev-backup in 055

[Fam Zheng]

This applies cases on drive-backup on blockdev-backup, except cases with
target format and mode.

Also add a case to check source == target.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/055     | 277 ++++++++++++++++++++++++++++++++++++++-------
 tests/qemu-iotests/055.out |   4 +-
 2 files changed, 236 insertions(+), 45 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 451b67d..ab47d59 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -1,8 +1,8 @@
 #!/usr/bin/env python
 #
-# Tests for drive-backup
+# Tests for drive-backup and blockdev-backup
 #
-# Copyright (C) 2013 Red Hat, Inc.
+# Copyright (C) 2013, 2014 Red Hat, Inc.
 #
 # Based on 041.
 #
@@ -27,6 +27,7 @@ from iotests import qemu_img, qemu_io
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
+blockdev_target_img = os.path.join(iotests.test_dir, 'blockdev-target.img')
 
 class TestSingleDrive(iotests.QMPTestCase):
     image_len = 64 * 1024 * 1024 # MB
@@ -38,34 +39,48 @@ class TestSingleDrive(iotests.QMPTestCase):
         qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
         qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
         qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+        qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
 
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
         self.vm.launch()
 
     def tearDown(self):
         self.vm.shutdown()
         os.remove(test_img)
+        os.remove(blockdev_target_img)
         try:
             os.remove(target_img)
         except OSError:
             pass
 
-    def test_cancel(self):
+    def do_test_cancel(self, test_drive_backup):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+        if test_drive_backup:
+            result = self.vm.qmp('drive-backup', device='drive0',
+                                 target=target_img, sync='full')
+        else:
+            result = self.vm.qmp('blockdev-backup', device='drive0',
+                                 target='drive1', sync='full')
         self.assert_qmp(result, 'return', {})
 
         event = self.cancel_and_wait()
         self.assert_qmp(event, 'data/type', 'backup')
 
-    def test_pause(self):
+    def test_cancel(self):
+        self.do_test_cancel(True)
+        self.do_test_cancel(False)
+
+    def do_test_pause(self, test_drive_backup):
         self.assert_no_active_block_jobs()
 
         self.vm.pause_drive('drive0')
-        result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+        if test_drive_backup:
+            result = self.vm.qmp('drive-backup', device='drive0',
+                                 target=target_img, sync='full')
+        else:
+            result = self.vm.qmp('blockdev-backup', device='drive0',
+                                 target='drive1', sync='full')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-pause', device='drive0')
@@ -86,14 +101,28 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.wait_until_completed()
 
         self.vm.shutdown()
-        self.assertTrue(iotests.compare_images(test_img, target_img),
-                        'target image does not match source after backup')
+        if test_drive_backup:
+            self.assertTrue(iotests.compare_images(test_img, target_img),
+                            'target image does not match source after backup')
+        else:
+            self.assertTrue(iotests.compare_images(test_img, blockdev_target_img),
+                            'target image does not match source after backup')
+
+    def test_pause_drive_backup(self):
+        self.do_test_pause(True)
+
+    def test_pause_blockdev_backup(self):
+        self.do_test_pause(False)
 
     def test_medium_not_found(self):
         result = self.vm.qmp('drive-backup', device='ide1-cd0',
                              target=target_img, sync='full')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
+        result = self.vm.qmp('blockdev-backup', device='ide1-cd0',
+                             target='drive1', sync='full')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
     def test_image_not_found(self):
         result = self.vm.qmp('drive-backup', device='drive0',
                              target=target_img, sync='full', mode='existing')
@@ -110,26 +139,56 @@ class TestSingleDrive(iotests.QMPTestCase):
                              target=target_img, sync='full')
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+        result = self.vm.qmp('blockdev-backup', device='nonexistent',
+                             target='drive0', sync='full')
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+        result = self.vm.qmp('blockdev-backup', device='drive0',
+                             target='nonexistent', sync='full')
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+        result = self.vm.qmp('blockdev-backup', device='nonexistent',
+                             target='nonexistent', sync='full')
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_target_is_source(self):
+        result = self.vm.qmp('blockdev-backup', device='drive0',
+                             target='drive0', sync='full')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
 class TestSetSpeed(iotests.QMPTestCase):
     image_len = 80 * 1024 * 1024 # MB
 
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSetSpeed.image_len))
         qemu_io('-c', 'write -P1 0 512', test_img)
-        self.vm = iotests.VM().add_drive(test_img)
+        qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
+
+        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
         self.vm.launch()
 
     def tearDown(self):
         self.vm.shutdown()
         os.remove(test_img)
-        os.remove(target_img)
+        try:
+            os.remove(blockdev_target_img)
+        except OSError:
+            pass
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
 
-    def test_set_speed(self):
+    def do_test_set_speed(self, test_drive_backup):
         self.assert_no_active_block_jobs()
 
         self.vm.pause_drive('drive0')
-        result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+        if test_drive_backup:
+            result = self.vm.qmp('drive-backup', device='drive0',
+                                 target=target_img, sync='full')
+        else:
+            result = self.vm.qmp('blockdev-backup', device='drive0',
+                                 target='drive1', sync='full')
         self.assert_qmp(result, 'return', {})
 
         # Default speed is 0
@@ -148,10 +207,14 @@ class TestSetSpeed(iotests.QMPTestCase):
         event = self.cancel_and_wait(resume=True)
         self.assert_qmp(event, 'data/type', 'backup')
 
-        # Check setting speed in drive-backup works
+        # Check setting speed option works
         self.vm.pause_drive('drive0')
-        result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full', speed=4*1024*1024)
+        if test_drive_backup:
+            result = self.vm.qmp('drive-backup', device='drive0',
+                                 target=target_img, sync='full', speed=4*1024*1024)
+        else:
+            result = self.vm.qmp('blockdev-backup', device='drive0',
+                                 target='drive1', sync='full', speed=4*1024*1024)
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block-jobs')
@@ -161,18 +224,32 @@ class TestSetSpeed(iotests.QMPTestCase):
         event = self.cancel_and_wait(resume=True)
         self.assert_qmp(event, 'data/type', 'backup')
 
-    def test_set_speed_invalid(self):
+    def test_set_speed_drive_backup(self):
+        self.do_test_set_speed(True)
+
+    def test_set_speed_blockdev_backup(self):
+        self.do_test_set_speed(False)
+
+    def do_test_set_speed_invalid(self, test_drive_backup):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full', speed=-1)
+        if test_drive_backup:
+            result = self.vm.qmp('drive-backup', device='drive0',
+                                 target=target_img, sync='full', speed=-1)
+        else:
+            result = self.vm.qmp('blockdev-backup', device='drive0',
+                                 target='drive1', sync='full', speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
         self.assert_no_active_block_jobs()
 
         self.vm.pause_drive('drive0')
-        result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+        if test_drive_backup:
+            result = self.vm.qmp('drive-backup', device='drive0',
+                                 target=target_img, sync='full')
+        else:
+            result = self.vm.qmp('blockdev-backup', device='drive0',
+                                 target='drive1', sync='full')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
@@ -181,6 +258,12 @@ class TestSetSpeed(iotests.QMPTestCase):
         event = self.cancel_and_wait(resume=True)
         self.assert_qmp(event, 'data/type', 'backup')
 
+    def test_set_speed_invalid_drive_backup(self):
+        self.do_test_set_speed_invalid(True)
+
+    def test_set_speed_invalid_blockdev_backup(self):
+        self.do_test_set_speed_invalid(False)
+
 class TestSingleTransaction(iotests.QMPTestCase):
     image_len = 64 * 1024 * 1024 # MB
 
@@ -190,44 +273,71 @@ class TestSingleTransaction(iotests.QMPTestCase):
         qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
         qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
         qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+        qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
 
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
         self.vm.launch()
 
     def tearDown(self):
         self.vm.shutdown()
         os.remove(test_img)
+        os.remove(blockdev_target_img)
         try:
             os.remove(target_img)
         except OSError:
             pass
 
-    def test_cancel(self):
+    def do_test_cancel(self, test_drive_backup):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('transaction', actions=[{
-                'type': 'drive-backup',
-                'data': { 'device': 'drive0',
-                          'target': target_img,
-                          'sync': 'full' },
-            }
-        ])
+        if test_drive_backup:
+            result = self.vm.qmp('transaction', actions=[{
+                    'type': 'drive-backup',
+                    'data': { 'device': 'drive0',
+                              'target': target_img,
+                              'sync': 'full' },
+                }
+            ])
+        else:
+            result = self.vm.qmp('transaction', actions=[{
+                    'type': 'blockdev-backup',
+                    'data': { 'device': 'drive0',
+                              'target': 'drive1',
+                              'sync': 'full' },
+                }
+            ])
+
         self.assert_qmp(result, 'return', {})
 
         event = self.cancel_and_wait()
         self.assert_qmp(event, 'data/type', 'backup')
 
-    def test_pause(self):
+    def test_cancel_drive_backup(self):
+        self.do_test_cancel(True)
+
+    def test_cancel_blockdev_backup(self):
+        self.do_test_cancel(False)
+
+    def do_test_pause(self, test_drive_backup):
         self.assert_no_active_block_jobs()
 
         self.vm.pause_drive('drive0')
-        result = self.vm.qmp('transaction', actions=[{
-                'type': 'drive-backup',
-                'data': { 'device': 'drive0',
-                          'target': target_img,
-                          'sync': 'full' },
-            }
-        ])
+        if test_drive_backup:
+            result = self.vm.qmp('transaction', actions=[{
+                    'type': 'drive-backup',
+                    'data': { 'device': 'drive0',
+                              'target': target_img,
+                              'sync': 'full' },
+                }
+            ])
+        else:
+            result = self.vm.qmp('transaction', actions=[{
+                    'type': 'blockdev-backup',
+                    'data': { 'device': 'drive0',
+                              'target': 'drive1',
+                              'sync': 'full' },
+                }
+            ])
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-pause', device='drive0')
@@ -248,8 +358,18 @@ class TestSingleTransaction(iotests.QMPTestCase):
         self.wait_until_completed()
 
         self.vm.shutdown()
-        self.assertTrue(iotests.compare_images(test_img, target_img),
-                        'target image does not match source after backup')
+        if test_drive_backup:
+            self.assertTrue(iotests.compare_images(test_img, target_img),
+                            'target image does not match source after backup')
+        else:
+            self.assertTrue(iotests.compare_images(test_img, blockdev_target_img),
+                            'target image does not match source after backup')
+
+    def test_pause_drive_backup(self):
+        self.do_test_pause(True)
+
+    def test_pause_blockdev_backup(self):
+        self.do_test_pause(False)
 
     def test_medium_not_found(self):
         result = self.vm.qmp('transaction', actions=[{
@@ -260,6 +380,14 @@ class TestSingleTransaction(iotests.QMPTestCase):
             }
         ])
         self.assert_qmp(result, 'error/class', 'GenericError')
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'ide1-cd0',
+                          'target': 'drive1',
+                          'sync': 'full' },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_image_not_found(self):
         result = self.vm.qmp('transaction', actions=[{
@@ -283,6 +411,43 @@ class TestSingleTransaction(iotests.QMPTestCase):
         ])
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'nonexistent',
+                          'target': 'drive1',
+                          'sync': 'full' },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'drive0',
+                          'target': 'nonexistent',
+                          'sync': 'full' },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'nonexistent',
+                          'target': 'nonexistent',
+                          'sync': 'full' },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_target_is_source(self):
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'drive0',
+                          'target': 'drive0',
+                          'sync': 'full' },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
     def test_abort(self):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
@@ -298,5 +463,31 @@ class TestSingleTransaction(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_no_active_block_jobs()
 
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'nonexistent',
+                          'target': 'drive1',
+                          'sync': 'full' },
+            }, {
+                'type': 'Abort',
+                'data': {},
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'blockdev-backup',
+                'data': { 'device': 'drive0',
+                          'target': 'nonexistent',
+                          'sync': 'full' },
+            }, {
+                'type': 'Abort',
+                'data': {},
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_no_active_block_jobs()
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 6323079..c6a10f8 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-..............
+.....................
 ----------------------------------------------------------------------
-Ran 14 tests
+Ran 21 tests
 
 OK
-- 
1.9.3

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test blockdev-backup in 055

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> This applies cases on drive-backup on blockdev-backup, except cases with
> target format and mode.
>
> Also add a case to check source == target.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/055     | 277 ++++++++++++++++++++++++++++++++++++++-------
>  tests/qemu-iotests/055.out |   4 +-
>  2 files changed, 236 insertions(+), 45 deletions(-)
>
> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> index 451b67d..ab47d59 100755
> --- a/tests/qemu-iotests/055
> +++ b/tests/qemu-iotests/055
> @@ -1,8 +1,8 @@
>  #!/usr/bin/env python
>  #
> -# Tests for drive-backup
> +# Tests for drive-backup and blockdev-backup
>  #
> -# Copyright (C) 2013 Red Hat, Inc.
> +# Copyright (C) 2013, 2014 Red Hat, Inc.
>  #
>  # Based on 041.
>  #
> @@ -27,6 +27,7 @@ from iotests import qemu_img, qemu_io
>  
>  test_img = os.path.join(iotests.test_dir, 'test.img')
>  target_img = os.path.join(iotests.test_dir, 'target.img')
> +blockdev_target_img = os.path.join(iotests.test_dir, 'blockdev-target.img')
>  
>  class TestSingleDrive(iotests.QMPTestCase):
>      image_len = 64 * 1024 * 1024 # MB
> @@ -38,34 +39,48 @@ class TestSingleDrive(iotests.QMPTestCase):
>          qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
>          qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
>          qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
> +        qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
>  
> -        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
>          self.vm.launch()
>  
>      def tearDown(self):
>          self.vm.shutdown()
>          os.remove(test_img)
> +        os.remove(blockdev_target_img)
>          try:
>              os.remove(target_img)
>          except OSError:
>              pass
>  
> -    def test_cancel(self):
> +    def do_test_cancel(self, test_drive_backup):
>          self.assert_no_active_block_jobs()
>  
> -        result = self.vm.qmp('drive-backup', device='drive0',
> -                             target=target_img, sync='full')
> +        if test_drive_backup:
> +            result = self.vm.qmp('drive-backup', device='drive0',
> +                                 target=target_img, sync='full')
> +        else:
> +            result = self.vm.qmp('blockdev-backup', device='drive0',
> +                                 target='drive1', sync='full')
>          self.assert_qmp(result, 'return', {})
>  
>          event = self.cancel_and_wait()
>          self.assert_qmp(event, 'data/type', 'backup')
>  
> -    def test_pause(self):
> +    def test_cancel(self):
> +        self.do_test_cancel(True)
> +        self.do_test_cancel(False)
> +

The bool parameter feels inelegant.  Perhaps you'd prefer something like
this (untested):

    def do_test_cancel(self, cmd, **args):
        self.assert_no_active_block_jobs()

        result = self.vm.qmp(cmd, **args)
        self.assert_qmp(result, 'return', {})

        event = self.cancel_and_wait()
        self.assert_qmp(event, 'data/type', 'backup')


    def test_cancel(self):
        self.do_test_cancel('drive-backup', device='drive0',
                            target=target_img, sync='full'))
        self.do_test_cancel('blockdev-backup', device='drive0',
                            target='drive1', sync='full')

Entirely up to you.  More of the same below.

> +    def do_test_pause(self, test_drive_backup):
>          self.assert_no_active_block_jobs()
>  
>          self.vm.pause_drive('drive0')
> -        result = self.vm.qmp('drive-backup', device='drive0',
> -                             target=target_img, sync='full')
> +        if test_drive_backup:
> +            result = self.vm.qmp('drive-backup', device='drive0',
> +                                 target=target_img, sync='full')
> +        else:
> +            result = self.vm.qmp('blockdev-backup', device='drive0',
> +                                 target='drive1', sync='full')
>          self.assert_qmp(result, 'return', {})
>  
>          result = self.vm.qmp('block-job-pause', device='drive0')
> @@ -86,14 +101,28 @@ class TestSingleDrive(iotests.QMPTestCase):
>          self.wait_until_completed()
>  
>          self.vm.shutdown()
> -        self.assertTrue(iotests.compare_images(test_img, target_img),
> -                        'target image does not match source after backup')
> +        if test_drive_backup:
> +            self.assertTrue(iotests.compare_images(test_img, target_img),
> +                            'target image does not match source after backup')
> +        else:
> +            self.assertTrue(iotests.compare_images(test_img, blockdev_target_img),
> +                            'target image does not match source after backup')
> +
> +    def test_pause_drive_backup(self):
> +        self.do_test_pause(True)
> +
> +    def test_pause_blockdev_backup(self):
> +        self.do_test_pause(False)
>  
>      def test_medium_not_found(self):
>          result = self.vm.qmp('drive-backup', device='ide1-cd0',
>                               target=target_img, sync='full')
>          self.assert_qmp(result, 'error/class', 'GenericError')
>  
> +        result = self.vm.qmp('blockdev-backup', device='ide1-cd0',
> +                             target='drive1', sync='full')
> +        self.assert_qmp(result, 'error/class', 'GenericError')
> +
>      def test_image_not_found(self):
>          result = self.vm.qmp('drive-backup', device='drive0',
>                               target=target_img, sync='full', mode='existing')
> @@ -110,26 +139,56 @@ class TestSingleDrive(iotests.QMPTestCase):
>                               target=target_img, sync='full')
>          self.assert_qmp(result, 'error/class', 'DeviceNotFound')
>  
> +        result = self.vm.qmp('blockdev-backup', device='nonexistent',
> +                             target='drive0', sync='full')
> +        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
> +
> +        result = self.vm.qmp('blockdev-backup', device='drive0',
> +                             target='nonexistent', sync='full')
> +        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
> +
> +        result = self.vm.qmp('blockdev-backup', device='nonexistent',
> +                             target='nonexistent', sync='full')
> +        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
> +
> +    def test_target_is_source(self):
> +        result = self.vm.qmp('blockdev-backup', device='drive0',
> +                             target='drive0', sync='full')
> +        self.assert_qmp(result, 'error/class', 'GenericError')
> +
>  class TestSetSpeed(iotests.QMPTestCase):
>      image_len = 80 * 1024 * 1024 # MB
>  
>      def setUp(self):
>          qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSetSpeed.image_len))
>          qemu_io('-c', 'write -P1 0 512', test_img)
> -        self.vm = iotests.VM().add_drive(test_img)
> +        qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
> +
> +        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
>          self.vm.launch()
>  
>      def tearDown(self):
>          self.vm.shutdown()
>          os.remove(test_img)
> -        os.remove(target_img)
> +        try:
> +            os.remove(blockdev_target_img)
> +        except OSError:
> +            pass

Elsewhere, you remove blockdev_target_img without a try.  Why not here?

> +        try:
> +            os.remove(target_img)
> +        except OSError:
> +            pass

Why do you need to wrap removal of target_img in a try now?

>  
> -    def test_set_speed(self):
> +    def do_test_set_speed(self, test_drive_backup):
>          self.assert_no_active_block_jobs()
>  
>          self.vm.pause_drive('drive0')
> -        result = self.vm.qmp('drive-backup', device='drive0',
> -                             target=target_img, sync='full')
> +        if test_drive_backup:
> +            result = self.vm.qmp('drive-backup', device='drive0',
> +                                 target=target_img, sync='full')
> +        else:
> +            result = self.vm.qmp('blockdev-backup', device='drive0',
> +                                 target='drive1', sync='full')
>          self.assert_qmp(result, 'return', {})
>  
>          # Default speed is 0
[...]

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test blockdev-backup in 055

[Fam Zheng]

Thanks a lot for the reviewing! I'm going to send another revision soon.

On Fri, 10/10 14:07, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> > +        try:
> > +            os.remove(target_img)
> > +        except OSError:
> > +            pass
> 
> Why do you need to wrap removal of target_img in a try now?

It's not present for blockdev-backup tests.

Fam