* [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup"
@ 2014-11-05 2:57 Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup' Fam Zheng
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Fam Zheng @ 2014-11-05 2:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi, mreitz
v3: Address Eric's comments on documentation.
Squashed 3/4 into 2/4.
v2: Address Markus' and Eric's comments:
Fix qapi schema documentation.
Fix versioning of transactions.
Improve test case code by dropping inelegnet bool.
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 | 28 ++++++
blockdev.c | 95 ++++++++++++++++++++
qapi-schema.json | 7 ++
qapi/block-core.json | 54 ++++++++++++
qmp-commands.hx | 44 ++++++++++
tests/qemu-iotests/055 | 211 +++++++++++++++++++++++++++++++++++++--------
tests/qemu-iotests/055.out | 4 +-
7 files changed, 404 insertions(+), 39 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup'
2014-11-05 2:57 [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Fam Zheng
@ 2014-11-05 2:57 ` Fam Zheng
2014-11-19 16:23 ` Stefan Hajnoczi
2014-12-17 9:36 ` Markus Armbruster
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction Fam Zheng
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Fam Zheng @ 2014-11-05 2:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi, mreitz
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 | 28 +++++++++++++++++++++++++++
blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
qapi/block-core.json | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 173 insertions(+)
diff --git a/block/backup.c b/block/backup.c
index 792e655..b944dd4 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -360,6 +360,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);
data = g_malloc(sizeof(*data));
data->ret = ret;
@@ -379,6 +380,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)) {
@@ -386,6 +392,26 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ if (!bdrv_is_inserted(bs)) {
+ error_setg(errp, "Devie is not inserted: %s",
+ bdrv_get_device_name(bs));
+ return;
+ }
+
+ if (!bdrv_is_inserted(target)) {
+ error_setg(errp, "Devie is not inserted: %s",
+ bdrv_get_device_name(target));
+ 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'",
@@ -399,6 +425,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 57910b8..2e5068c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2156,6 +2156,8 @@ void qmp_drive_backup(const char *device, const char *target,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ /* 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);
goto out;
@@ -2172,6 +2174,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)) {
goto out;
}
@@ -2239,6 +2242,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 77a0cfb..bc02bd7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -676,6 +676,41 @@
'*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. The default is 0,
+# for unlimited.
+#
+# @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.3
+##
+{ '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.
@@ -795,6 +830,25 @@
{ 'command': 'drive-backup', 'data': 'DriveBackup' }
##
+# @blockdev-backup
+#
+# Start a point-in-time copy of a block device to a new destination. The
+# status of ongoing blockdev-backup operations can be checked with
+# query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
+# The operation can be stopped before it has completed using the
+# block-job-cancel command.
+#
+# 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.3
+##
+{ '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 1abd619..518df0c 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 an 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction
2014-11-05 2:57 [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup' Fam Zheng
@ 2014-11-05 2:57 ` Fam Zheng
2014-12-17 9:40 ` Markus Armbruster
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
2014-12-17 10:44 ` [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Markus Armbruster
3 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2014-11-05 2:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi, mreitz
BTW add version info for other transaction types.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
blockdev.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 7 +++++++
2 files changed, 55 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index 2e5068c..6401850 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1495,6 +1495,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");
@@ -1517,6 +1560,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 24379ab..5fc6892 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1254,11 +1254,18 @@
#
# A discriminated record of operations that can be performed with
# @transaction.
+#
+# Since 1.1
+# drive-backup since 1.6
+# abort since 1.6
+# blockdev-snapshot-internal-sync since 1.7
+# blockdev-backup since 2.3
##
{ 'union': 'TransactionAction',
'data': {
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
+ 'blockdev-backup': 'BlockdevBackup',
'abort': 'Abort',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
} }
--
1.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Test blockdev-backup in 055
2014-11-05 2:57 [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup' Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction Fam Zheng
@ 2014-11-05 2:57 ` Fam Zheng
2014-12-17 10:41 ` Markus Armbruster
2014-12-17 10:44 ` [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Markus Armbruster
3 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2014-11-05 2:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi, mreitz
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 | 211 +++++++++++++++++++++++++++++++++++++--------
tests/qemu-iotests/055.out | 4 +-
2 files changed, 176 insertions(+), 39 deletions(-)
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 451b67d..d04dad5 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,41 @@ 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, cmd, target):
self.assert_no_active_block_jobs()
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ result = self.vm.qmp(cmd, device='drive0', target=target, 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('drive-backup', target_img)
+
+ def test_cancel_blockdev_backup(self):
+ self.do_test_cancel('blockdev-backup', 'drive1')
+
+ def do_test_pause(self, cmd, target, image):
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ result = self.vm.qmp(cmd, device='drive0',
+ target=target, sync='full')
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-pause', device='drive0')
@@ -86,14 +94,25 @@ class TestSingleDrive(iotests.QMPTestCase):
self.wait_until_completed()
self.vm.shutdown()
- self.assertTrue(iotests.compare_images(test_img, target_img),
+ self.assertTrue(iotests.compare_images(test_img, image),
'target image does not match source after backup')
+ def test_pause_drive_backup(self):
+ self.do_test_pause('drive-backup', target_img, target_img)
+
+ def test_pause_blockdev_backup(self):
+ self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
+
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')
+ def test_medium_not_found_blockdev_backup(self):
+ 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')
@@ -105,31 +124,53 @@ class TestSingleDrive(iotests.QMPTestCase):
format='spaghetti-noodles')
self.assert_qmp(result, 'error/class', 'GenericError')
- def test_device_not_found(self):
- result = self.vm.qmp('drive-backup', device='nonexistent',
- target=target_img, sync='full')
+ def do_test_device_not_found(self, cmd, **args):
+ result = self.vm.qmp(cmd, **args)
self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+ def test_device_not_found(self):
+ self.do_test_device_not_found('drive-backup', device='nonexistent',
+ target=target_img, sync='full')
+
+ self.do_test_device_not_found('blockdev-backup', device='nonexistent',
+ target='drive0', sync='full')
+
+ self.do_test_device_not_found('blockdev-backup', device='drive0',
+ target='nonexistent', sync='full')
+
+ self.do_test_device_not_found('blockdev-backup', device='nonexistent',
+ target='nonexistent', sync='full')
+
+ 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)
+ os.remove(blockdev_target_img)
+ try:
+ os.remove(target_img)
+ except OSError:
+ pass
- def test_set_speed(self):
+ def do_test_set_speed(self, cmd, target):
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ result = self.vm.qmp(cmd, device='drive0', target=target, sync='full')
self.assert_qmp(result, 'return', {})
# Default speed is 0
@@ -148,10 +189,10 @@ 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)
+ result = self.vm.qmp(cmd, device='drive0',
+ target=target, sync='full', speed=4*1024*1024)
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('query-block-jobs')
@@ -161,18 +202,24 @@ 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('drive-backup', target_img)
+
+ def test_set_speed_blockdev_backup(self):
+ self.do_test_set_speed('blockdev-backup', 'drive1')
+
+ def do_test_set_speed_invalid(self, cmd, target):
self.assert_no_active_block_jobs()
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full', speed=-1)
+ result = self.vm.qmp(cmd, device='drive0',
+ target=target, 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')
+ result = self.vm.qmp(cmd, device='drive0',
+ target=target, sync='full')
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
@@ -181,6 +228,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('drive-backup', target_img)
+
+ def test_set_speed_invalid_blockdev_backup(self):
+ self.do_test_set_speed_invalid('blockdev-backup', 'drive1')
+
class TestSingleTransaction(iotests.QMPTestCase):
image_len = 64 * 1024 * 1024 # MB
@@ -190,41 +243,50 @@ 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, cmd, target):
self.assert_no_active_block_jobs()
result = self.vm.qmp('transaction', actions=[{
- 'type': 'drive-backup',
+ 'type': cmd,
'data': { 'device': 'drive0',
- 'target': target_img,
+ 'target': target,
'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('drive-backup', target_img)
+
+ def test_cancel_blockdev_backup(self):
+ self.do_test_cancel('blockdev-backup', 'drive1')
+
+ def do_test_pause(self, cmd, target, image):
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
result = self.vm.qmp('transaction', actions=[{
- 'type': 'drive-backup',
+ 'type': cmd,
'data': { 'device': 'drive0',
- 'target': target_img,
+ 'target': target,
'sync': 'full' },
}
])
@@ -248,19 +310,31 @@ class TestSingleTransaction(iotests.QMPTestCase):
self.wait_until_completed()
self.vm.shutdown()
- self.assertTrue(iotests.compare_images(test_img, target_img),
+ self.assertTrue(iotests.compare_images(test_img, image),
'target image does not match source after backup')
- def test_medium_not_found(self):
+ def test_pause_drive_backup(self):
+ self.do_test_pause('drive-backup', target_img, target_img)
+
+ def test_pause_blockdev_backup(self):
+ self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
+
+ def do_test_medium_not_found(self, cmd, target):
result = self.vm.qmp('transaction', actions=[{
- 'type': 'drive-backup',
+ 'type': cmd,
'data': { 'device': 'ide1-cd0',
- 'target': target_img,
+ 'target': target,
'sync': 'full' },
}
])
self.assert_qmp(result, 'error/class', 'GenericError')
+ def test_medium_not_found_drive_backup(self):
+ self.do_test_medium_not_found('drive-backup', target_img)
+
+ def test_medium_not_found_blockdev_backup(self):
+ self.do_test_medium_not_found('blockdev-backup', 'drive1')
+
def test_image_not_found(self):
result = self.vm.qmp('transaction', actions=[{
'type': 'drive-backup',
@@ -283,6 +357,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 +409,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..42314e9 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-..............
+........................
----------------------------------------------------------------------
-Ran 14 tests
+Ran 24 tests
OK
--
1.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup'
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup' Fam Zheng
@ 2014-11-19 16:23 ` Stefan Hajnoczi
2014-12-17 9:36 ` Markus Armbruster
1 sibling, 0 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2014-11-19 16:23 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, mreitz, qemu-devel, Stefan Hajnoczi, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]
On Wed, Nov 05, 2014 at 10:57:09AM +0800, Fam Zheng wrote:
> +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);
> + }
> +}
Please make sure to acquire AioContext so that this is safe with
dataplane.
Here the tricky thing is that bs and target_bs must have the same
AioContext. The easiest thing is to refuse if their AioContexts are not
identical, but that puts the onus on the user to put the
BlockDriverStates into the same AioContext.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup'
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup' Fam Zheng
2014-11-19 16:23 ` Stefan Hajnoczi
@ 2014-12-17 9:36 ` Markus Armbruster
2014-12-17 10:41 ` Fam Zheng
1 sibling, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2014-12-17 9:36 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
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 | 28 +++++++++++++++++++++++++++
> blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> qapi/block-core.json | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 173 insertions(+)
>
> diff --git a/block/backup.c b/block/backup.c
> index 792e655..b944dd4 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -360,6 +360,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);
>
> data = g_malloc(sizeof(*data));
> data->ret = ret;
> @@ -379,6 +380,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)) {
> @@ -386,6 +392,26 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> return;
> }
>
> + if (!bdrv_is_inserted(bs)) {
> + error_setg(errp, "Devie is not inserted: %s",
s/Devie/Device/
> + bdrv_get_device_name(bs));
> + return;
> + }
> +
> + if (!bdrv_is_inserted(target)) {
> + error_setg(errp, "Devie is not inserted: %s",
Likewise.
> + bdrv_get_device_name(target));
> + 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'",
> @@ -399,6 +425,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 57910b8..2e5068c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2156,6 +2156,8 @@ void qmp_drive_backup(const char *device, const char *target,
> aio_context = bdrv_get_aio_context(bs);
> aio_context_acquire(aio_context);
>
> + /* 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);
> goto out;
> @@ -2172,6 +2174,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)) {
> goto out;
> }
> @@ -2239,6 +2242,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 77a0cfb..bc02bd7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -676,6 +676,41 @@
> '*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. The default is 0,
> +# for unlimited.
> +#
> +# @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.3
> +##
> +{ '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.
> @@ -795,6 +830,25 @@
> { 'command': 'drive-backup', 'data': 'DriveBackup' }
>
> ##
> +# @blockdev-backup
> +#
> +# Start a point-in-time copy of a block device to a new destination. The
> +# status of ongoing blockdev-backup operations can be checked with
> +# query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
> +# The operation can be stopped before it has completed using the
> +# block-job-cancel command.
> +#
> +# For the arguments, see the documentation of BlockdevBackup.
> +#
> +# Returns: Nothing on success.
> +# If @device or @target is not a valid block device, DeviceNotFound.
Why do you need an error classes other than GenericError here?
> +#
> +# Since 2.3
> +##
> +{ '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 1abd619..518df0c 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 an 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)
Still copied from drive-backup without change. I don't think it's
correct here. Either fix it, or explain to me why I'm wrong :)
> +- "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
>
> {
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction Fam Zheng
@ 2014-12-17 9:40 ` Markus Armbruster
2014-12-17 10:44 ` Fam Zheng
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2014-12-17 9:40 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
Fam Zheng <famz@redhat.com> writes:
> BTW add version info for other transaction types.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> blockdev.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> qapi-schema.json | 7 +++++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 2e5068c..6401850 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1495,6 +1495,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");
> @@ -1517,6 +1560,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 24379ab..5fc6892 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1254,11 +1254,18 @@
> #
> # A discriminated record of operations that can be performed with
> # @transaction.
> +#
> +# Since 1.1
> +# drive-backup since 1.6
> +# abort since 1.6
> +# blockdev-snapshot-internal-sync since 1.7
I'd do these belated doc updates in a separate patch preceding this one.
Up to you.
> +# blockdev-backup since 2.3
> ##
> { 'union': 'TransactionAction',
> 'data': {
> 'blockdev-snapshot-sync': 'BlockdevSnapshot',
> 'drive-backup': 'DriveBackup',
> + 'blockdev-backup': 'BlockdevBackup',
> 'abort': 'Abort',
> 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> } }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup'
2014-12-17 9:36 ` Markus Armbruster
@ 2014-12-17 10:41 ` Fam Zheng
2014-12-19 8:20 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2014-12-17 10:41 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
On Wed, 12/17 10:36, Markus Armbruster wrote:
> 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 | 28 +++++++++++++++++++++++++++
> > blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> > qapi/block-core.json | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 173 insertions(+)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 792e655..b944dd4 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -360,6 +360,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);
> >
> > data = g_malloc(sizeof(*data));
> > data->ret = ret;
> > @@ -379,6 +380,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)) {
> > @@ -386,6 +392,26 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> > return;
> > }
> >
> > + if (!bdrv_is_inserted(bs)) {
> > + error_setg(errp, "Devie is not inserted: %s",
>
> s/Devie/Device/
OK.
>
> > + bdrv_get_device_name(bs));
> > + return;
> > + }
> > +
> > + if (!bdrv_is_inserted(target)) {
> > + error_setg(errp, "Devie is not inserted: %s",
>
> Likewise.
OK.
>
> > + bdrv_get_device_name(target));
> > + 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'",
> > @@ -399,6 +425,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 57910b8..2e5068c 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2156,6 +2156,8 @@ void qmp_drive_backup(const char *device, const char *target,
> > aio_context = bdrv_get_aio_context(bs);
> > aio_context_acquire(aio_context);
> >
> > + /* 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);
> > goto out;
> > @@ -2172,6 +2174,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)) {
> > goto out;
> > }
> > @@ -2239,6 +2242,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 77a0cfb..bc02bd7 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -676,6 +676,41 @@
> > '*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. The default is 0,
> > +# for unlimited.
> > +#
> > +# @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.3
> > +##
> > +{ '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.
> > @@ -795,6 +830,25 @@
> > { 'command': 'drive-backup', 'data': 'DriveBackup' }
> >
> > ##
> > +# @blockdev-backup
> > +#
> > +# Start a point-in-time copy of a block device to a new destination. The
> > +# status of ongoing blockdev-backup operations can be checked with
> > +# query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
> > +# The operation can be stopped before it has completed using the
> > +# block-job-cancel command.
> > +#
> > +# For the arguments, see the documentation of BlockdevBackup.
> > +#
> > +# Returns: Nothing on success.
> > +# If @device or @target is not a valid block device, DeviceNotFound.
>
> Why do you need an error classes other than GenericError here?
>
Because this is what other block job commands do, and I followed the style.
> > +#
> > +# Since 2.3
> > +##
> > +{ '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 1abd619..518df0c 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 an 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)
>
> Still copied from drive-backup without change. I don't think it's
> correct here. Either fix it, or explain to me why I'm wrong :)
Overlooked by my eyes, will fix in v5 (I posted v4 earlier in Dec and got
comments from Max that are pending to be addressed).
Thanks for reviewing!
Fam
>
> > +- "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
> >
> > {
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Test blockdev-backup in 055
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
@ 2014-12-17 10:41 ` Markus Armbruster
0 siblings, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2014-12-17 10:41 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
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 | 211 +++++++++++++++++++++++++++++++++++++--------
> tests/qemu-iotests/055.out | 4 +-
> 2 files changed, 176 insertions(+), 39 deletions(-)
I rather like how you changed this from v1. Diffstat then was
236 insertions(+), 45 deletions(-).
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction
2014-12-17 9:40 ` Markus Armbruster
@ 2014-12-17 10:44 ` Fam Zheng
0 siblings, 0 replies; 13+ messages in thread
From: Fam Zheng @ 2014-12-17 10:44 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
On Wed, 12/17 10:40, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
>
> > BTW add version info for other transaction types.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > blockdev.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > qapi-schema.json | 7 +++++++
> > 2 files changed, 55 insertions(+)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 2e5068c..6401850 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1495,6 +1495,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");
> > @@ -1517,6 +1560,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 24379ab..5fc6892 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1254,11 +1254,18 @@
> > #
> > # A discriminated record of operations that can be performed with
> > # @transaction.
> > +#
> > +# Since 1.1
> > +# drive-backup since 1.6
> > +# abort since 1.6
> > +# blockdev-snapshot-internal-sync since 1.7
>
> I'd do these belated doc updates in a separate patch preceding this one.
> Up to you.
Okay, I remember one previous reviewer said this is fine, but since we are
talking about it, why not split it.
Fam
>
> > +# blockdev-backup since 2.3
> > ##
> > { 'union': 'TransactionAction',
> > 'data': {
> > 'blockdev-snapshot-sync': 'BlockdevSnapshot',
> > 'drive-backup': 'DriveBackup',
> > + 'blockdev-backup': 'BlockdevBackup',
> > 'abort': 'Abort',
> > 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> > } }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup"
2014-11-05 2:57 [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Fam Zheng
` (2 preceding siblings ...)
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
@ 2014-12-17 10:44 ` Markus Armbruster
3 siblings, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2014-12-17 10:44 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
Fam Zheng <famz@redhat.com> writes:
> v3: Address Eric's comments on documentation.
> Squashed 3/4 into 2/4.
>
> v2: Address Markus' and Eric's comments:
> Fix qapi schema documentation.
> Fix versioning of transactions.
> Improve test case code by dropping inelegnet bool.
>
> 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.)
Trivial typos/pastos in PATCH 1, and a question on GenericError. If you
address these, you can add my R-by.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup'
2014-12-17 10:41 ` Fam Zheng
@ 2014-12-19 8:20 ` Markus Armbruster
2014-12-23 2:10 ` Fam Zheng
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2014-12-19 8:20 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
Fam Zheng <famz@redhat.com> writes:
> On Wed, 12/17 10:36, Markus Armbruster wrote:
>> 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.
[...]
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 57910b8..2e5068c 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -2156,6 +2156,8 @@ void qmp_drive_backup(const char *device, const char *target,
>> > aio_context = bdrv_get_aio_context(bs);
>> > aio_context_acquire(aio_context);
>> >
>> > + /* 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);
>> > goto out;
>> > @@ -2172,6 +2174,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)) {
>> > goto out;
>> > }
>> > @@ -2239,6 +2242,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;
>> > + }
New use of a QERR_ macro. See below.
>> > +
>> > + 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 77a0cfb..bc02bd7 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -676,6 +676,41 @@
>> > '*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. The default is 0,
>> > +# for unlimited.
>> > +#
>> > +# @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.3
>> > +##
>> > +{ '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.
>> > @@ -795,6 +830,25 @@
>> > { 'command': 'drive-backup', 'data': 'DriveBackup' }
>> >
>> > ##
>> > +# @blockdev-backup
>> > +#
>> > +# Start a point-in-time copy of a block device to a new destination. The
>> > +# status of ongoing blockdev-backup operations can be checked with
>> > +# query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
>> > +# The operation can be stopped before it has completed using the
>> > +# block-job-cancel command.
>> > +#
>> > +# For the arguments, see the documentation of BlockdevBackup.
>> > +#
>> > +# Returns: Nothing on success.
>> > +# If @device or @target is not a valid block device, DeviceNotFound.
>>
>> Why do you need an error classes other than GenericError here?
>>
>
> Because this is what other block job commands do, and I followed the style.
Following existing practice is a good idea, except when you're following
bad examples.
When we scrapped the misguided "rich error" idea, we undid some, but not
all of its damage (series around commit de253f1). In particular, we got
rid of most, but not all ErrorClass values. We had to keep a few to
avoid breaking existing QMP clients. They've been sneaking into new
code ever since, hiding in their QERR_ macros.
Quoting the Code Transitions Wiki page:
* Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
you have a specific reason. Prefer error_setg() & friends over
error_set() & friends.
* The QError macros QERR_ are a hack to help with the transition to the
current error.h API (human-readable message rather than JSON argument,
see commit df1e608). Avoid them in new code, use simple message
strings instead.
We're going to deprecate a number of block-related commands for other
reasons. I want their replacements avoid ErrorClass values other than
ERROR_CLASS_GENERIC_ERROR. Same for new commands.
You don't have to respin for this, I can fix it up on top.
[*] http://wiki.qemu.org/CodeTransitions#Error_reporting
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup'
2014-12-19 8:20 ` Markus Armbruster
@ 2014-12-23 2:10 ` Fam Zheng
0 siblings, 0 replies; 13+ messages in thread
From: Fam Zheng @ 2014-12-23 2:10 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, mreitz
On Fri, 12/19 09:20, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
>
> > On Wed, 12/17 10:36, Markus Armbruster wrote:
> >> 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.
> [...]
> >> > diff --git a/blockdev.c b/blockdev.c
> >> > index 57910b8..2e5068c 100644
> >> > --- a/blockdev.c
> >> > +++ b/blockdev.c
> >> > @@ -2156,6 +2156,8 @@ void qmp_drive_backup(const char *device, const char *target,
> >> > aio_context = bdrv_get_aio_context(bs);
> >> > aio_context_acquire(aio_context);
> >> >
> >> > + /* 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);
> >> > goto out;
> >> > @@ -2172,6 +2174,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)) {
> >> > goto out;
> >> > }
> >> > @@ -2239,6 +2242,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;
> >> > + }
>
> New use of a QERR_ macro. See below.
>
> >> > +
> >> > + 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 77a0cfb..bc02bd7 100644
> >> > --- a/qapi/block-core.json
> >> > +++ b/qapi/block-core.json
> >> > @@ -676,6 +676,41 @@
> >> > '*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. The default is 0,
> >> > +# for unlimited.
> >> > +#
> >> > +# @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.3
> >> > +##
> >> > +{ '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.
> >> > @@ -795,6 +830,25 @@
> >> > { 'command': 'drive-backup', 'data': 'DriveBackup' }
> >> >
> >> > ##
> >> > +# @blockdev-backup
> >> > +#
> >> > +# Start a point-in-time copy of a block device to a new destination. The
> >> > +# status of ongoing blockdev-backup operations can be checked with
> >> > +# query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
> >> > +# The operation can be stopped before it has completed using the
> >> > +# block-job-cancel command.
> >> > +#
> >> > +# For the arguments, see the documentation of BlockdevBackup.
> >> > +#
> >> > +# Returns: Nothing on success.
> >> > +# If @device or @target is not a valid block device, DeviceNotFound.
> >>
> >> Why do you need an error classes other than GenericError here?
> >>
> >
> > Because this is what other block job commands do, and I followed the style.
>
> Following existing practice is a good idea, except when you're following
> bad examples.
>
> When we scrapped the misguided "rich error" idea, we undid some, but not
> all of its damage (series around commit de253f1). In particular, we got
> rid of most, but not all ErrorClass values. We had to keep a few to
> avoid breaking existing QMP clients. They've been sneaking into new
> code ever since, hiding in their QERR_ macros.
>
> Quoting the Code Transitions Wiki page:
>
> * Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
> you have a specific reason. Prefer error_setg() & friends over
> error_set() & friends.
>
> * The QError macros QERR_ are a hack to help with the transition to the
> current error.h API (human-readable message rather than JSON argument,
> see commit df1e608). Avoid them in new code, use simple message
> strings instead.
>
> We're going to deprecate a number of block-related commands for other
> reasons. I want their replacements avoid ErrorClass values other than
> ERROR_CLASS_GENERIC_ERROR. Same for new commands.
>
> You don't have to respin for this, I can fix it up on top.
>
>
> [*] http://wiki.qemu.org/CodeTransitions#Error_reporting
OK, thanks for explaining and taking care of it. I'll follow your future
patches.
Fam
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-12-23 2:10 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-05 2:57 [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 1/3] qmp: Add command 'blockdev-backup' Fam Zheng
2014-11-19 16:23 ` Stefan Hajnoczi
2014-12-17 9:36 ` Markus Armbruster
2014-12-17 10:41 ` Fam Zheng
2014-12-19 8:20 ` Markus Armbruster
2014-12-23 2:10 ` Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 2/3] block: Add blockdev-backup to transaction Fam Zheng
2014-12-17 9:40 ` Markus Armbruster
2014-12-17 10:44 ` Fam Zheng
2014-11-05 2:57 ` [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
2014-12-17 10:41 ` Markus Armbruster
2014-12-17 10:44 ` [Qemu-devel] [PATCH v3 0/3] qmp: Add "blockdev-backup" Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.