* [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command
@ 2015-09-10 13:39 Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync Alberto Garcia
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Alberto Garcia @ 2015-09-10 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz,
Stefan Hajnoczi
Hi,
here's version 3 of the patchset that adds the 'blockdev-snapshot' QMP
command. This one has a couple of important fixes plus some of the
corrections suggested by Eric.
The most controversial change, I believe, is the addition of the
'ignore-backing' field to BlockdevOptionsGenericCOWFormat. This allows
opening an image using 'blockdev-add' but not its backing chain. I
expect that this will generate some debate so decided to go for a
simple solution that would allow me to finish the rest of the series,
but I'm of course open to go for an alternative API/solution.
Regards,
Berto
v3:
- Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat. This
allows opening images but not their backing images.
- Check for op blockers in the snapshot node and make sure that it
doesn't have any backing image.
- Remove extra check for the existence of the snapshot node:
bdrv_open() already does that.
- Extend iotest 085 to add tests for 'blockdev-snapshot'.
- Replace local_err with errp in some places where the former is
unnecessary.
- Update command description.
- Add 'since' tag to the 'blockdev-snapshot' field in TransactionAction.
v2: https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00094.html
- Add 'blockdev-snapshot' command instead of allowing passing options
to 'blockdev-snapshot-sync'.
- Rename BlockdevSnapshot to BlockdevSnapshotSync
v1: https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00236.html
Alberto Garcia (4):
block: rename BlockdevSnapshot to BlockdevSnapshotSync
block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
block: add a 'blockdev-snapshot' QMP command
block: add tests for the 'blockdev-snapshot' command
block.c | 5 ++
blockdev.c | 165 ++++++++++++++++++++++++++++-----------------
qapi-schema.json | 4 +-
qapi/block-core.json | 38 +++++++++--
qmp-commands.hx | 29 ++++++++
tests/qemu-iotests/085 | 97 ++++++++++++++++++++++++--
tests/qemu-iotests/085.out | 34 +++++++++-
7 files changed, 298 insertions(+), 74 deletions(-)
--
2.5.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync
2015-09-10 13:39 [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command Alberto Garcia
@ 2015-09-10 13:39 ` Alberto Garcia
2015-09-11 17:15 ` Max Reitz
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat Alberto Garcia
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Alberto Garcia @ 2015-09-10 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz,
Stefan Hajnoczi
We will introduce the 'blockdev-snapshot' command that will require
its own struct for the parameters, so we need to rename this one in
order to avoid name clashes.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
blockdev.c | 2 +-
qapi-schema.json | 2 +-
qapi/block-core.json | 8 ++++----
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3f42863..6b787c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1166,7 +1166,7 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
bool has_format, const char *format,
bool has_mode, NewImageMode mode, Error **errp)
{
- BlockdevSnapshot snapshot = {
+ BlockdevSnapshotSync snapshot = {
.has_device = has_device,
.device = (char *) device,
.has_node_name = has_node_name,
diff --git a/qapi-schema.json b/qapi-schema.json
index 1521eb7..c32dc20 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1496,7 +1496,7 @@
##
{ 'union': 'TransactionAction',
'data': {
- 'blockdev-snapshot-sync': 'BlockdevSnapshot',
+ 'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
'drive-backup': 'DriveBackup',
'blockdev-backup': 'BlockdevBackup',
'abort': 'Abort',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cb99cad..ec50f06 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -682,7 +682,7 @@
'data': [ 'existing', 'absolute-paths' ] }
##
-# @BlockdevSnapshot
+# @BlockdevSnapshotSync
#
# Either @device or @node-name must be set but not both.
#
@@ -699,7 +699,7 @@
# @mode: #optional whether and how QEMU should create a new image, default is
# 'absolute-paths'.
##
-{ 'struct': 'BlockdevSnapshot',
+{ 'struct': 'BlockdevSnapshotSync',
'data': { '*device': 'str', '*node-name': 'str',
'snapshot-file': 'str', '*snapshot-node-name': 'str',
'*format': 'str', '*mode': 'NewImageMode' } }
@@ -790,7 +790,7 @@
#
# Generates a synchronous snapshot of a block device.
#
-# For the arguments, see the documentation of BlockdevSnapshot.
+# For the arguments, see the documentation of BlockdevSnapshotSync.
#
# Returns: nothing on success
# If @device is not a valid block device, DeviceNotFound
@@ -798,7 +798,7 @@
# Since 0.14.0
##
{ 'command': 'blockdev-snapshot-sync',
- 'data': 'BlockdevSnapshot' }
+ 'data': 'BlockdevSnapshotSync' }
##
# @change-backing-file
--
2.5.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-10 13:39 [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync Alberto Garcia
@ 2015-09-10 13:39 ` Alberto Garcia
2015-09-11 17:21 ` Max Reitz
2015-09-11 17:28 ` Eric Blake
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command Alberto Garcia
3 siblings, 2 replies; 18+ messages in thread
From: Alberto Garcia @ 2015-09-10 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz,
Stefan Hajnoczi
If set to true, the image will be opened with the BDRV_O_NO_BACKING
flag. This is useful for creating snapshots using images opened with
blockdev-add, since they are not supposed to have a backing image
before the operation.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 5 +++++
qapi/block-core.json | 6 +++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 22d3b0e..4be32fb 100644
--- a/block.c
+++ b/block.c
@@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
assert(drvname || !(flags & BDRV_O_PROTOCOL));
+ if (qdict_get_try_bool(options, "ignore-backing", false)) {
+ flags |= BDRV_O_NO_BACKING;
+ }
+ qdict_del(options, "ignore-backing");
+
bs->open_flags = flags;
bs->options = options;
options = qdict_clone_shallow(options);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ec50f06..0f797d7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1498,11 +1498,15 @@
# allowed to pass an empty string here in order to disable the
# default backing file.
#
+# @ignore-backing: #optional if true, no backing file will be
+# opened. Defaults to false (Since 2.5)
+#
# Since: 1.7
##
{ 'struct': 'BlockdevOptionsGenericCOWFormat',
'base': 'BlockdevOptionsGenericFormat',
- 'data': { '*backing': 'BlockdevRef' } }
+ 'data': { '*backing': 'BlockdevRef',
+ '*ignore-backing': 'bool' } }
##
# @Qcow2OverlapCheckMode
--
2.5.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command
2015-09-10 13:39 [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat Alberto Garcia
@ 2015-09-10 13:39 ` Alberto Garcia
2015-09-11 17:58 ` Eric Blake
2015-09-11 18:11 ` Max Reitz
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command Alberto Garcia
3 siblings, 2 replies; 18+ messages in thread
From: Alberto Garcia @ 2015-09-10 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz,
Stefan Hajnoczi
One of the limitations of the 'blockdev-snapshot-sync' command is that
it does not allow passing BlockdevOptions to the newly created
snapshots, so they are always opened using the default values.
Extending the command to allow passing options is not a practical
solution because there is overlap between those options and some of
the existing parameters of the command.
This patch introduces a new 'blockdev-snapshot' command with a simpler
interface: it just takes two references to existing block devices that
will be used as the source and target for the snapshot.
Since the main difference between the two commands is that one of them
creates and opens the target image, while the other uses an already
opened one, the bulk of the implementation is shared.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
blockdev.c | 163 ++++++++++++++++++++++++++++++++-------------------
qapi-schema.json | 2 +
qapi/block-core.json | 26 ++++++++
qmp-commands.hx | 29 +++++++++
4 files changed, 160 insertions(+), 60 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 6b787c1..78cfb79 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
&snapshot, errp);
}
+void qmp_blockdev_snapshot(const char *device, const char *snapshot,
+ Error **errp)
+{
+ BlockdevSnapshot snapshot_data = {
+ .device = (char *) device,
+ .snapshot = (char *) snapshot
+ };
+
+ blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
+ &snapshot_data, errp);
+}
+
void qmp_blockdev_snapshot_internal_sync(const char *device,
const char *name,
Error **errp)
@@ -1521,57 +1533,48 @@ typedef struct ExternalSnapshotState {
static void external_snapshot_prepare(BlkTransactionState *common,
Error **errp)
{
- int flags, ret;
- QDict *options;
+ int flags = 0, ret;
+ QDict *options = NULL;
Error *local_err = NULL;
- bool has_device = false;
+ /* Device and node name of the image to generate the snapshot from */
const char *device;
- bool has_node_name = false;
const char *node_name;
- bool has_snapshot_node_name = false;
- const char *snapshot_node_name;
+ /* Reference to the new image (for 'blockdev-snapshot') */
+ const char *snapshot_ref;
+ /* File name of the new image (for 'blockdev-snapshot-sync') */
const char *new_image_file;
- const char *format = "qcow2";
- enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
TransactionAction *action = common->action;
- /* get parameters */
- g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
-
- has_device = action->blockdev_snapshot_sync->has_device;
- device = action->blockdev_snapshot_sync->device;
- has_node_name = action->blockdev_snapshot_sync->has_node_name;
- node_name = action->blockdev_snapshot_sync->node_name;
- has_snapshot_node_name =
- action->blockdev_snapshot_sync->has_snapshot_node_name;
- snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
-
- new_image_file = action->blockdev_snapshot_sync->snapshot_file;
- if (action->blockdev_snapshot_sync->has_format) {
- format = action->blockdev_snapshot_sync->format;
- }
- if (action->blockdev_snapshot_sync->has_mode) {
- mode = action->blockdev_snapshot_sync->mode;
+ /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
+ * purpose but a different set of parameters */
+ switch (action->kind) {
+ case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
+ {
+ BlockdevSnapshot *s = action->blockdev_snapshot;
+ device = s->device;
+ node_name = s->device;
+ new_image_file = NULL;
+ snapshot_ref = s->snapshot;
+ }
+ break;
+ case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+ {
+ BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
+ device = s->has_device ? s->device : NULL;
+ node_name = s->has_node_name ? s->node_name : NULL;
+ new_image_file = s->snapshot_file;
+ snapshot_ref = NULL;
+ }
+ break;
+ default:
+ g_assert_not_reached();
}
/* start processing */
- state->old_bs = bdrv_lookup_bs(has_device ? device : NULL,
- has_node_name ? node_name : NULL,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
-
- if (has_node_name && !has_snapshot_node_name) {
- error_setg(errp, "New snapshot node name missing");
- return;
- }
-
- if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
- error_setg(errp, "New snapshot node name already existing");
+ state->old_bs = bdrv_lookup_bs(device, node_name, errp);
+ if (!state->old_bs) {
return;
}
@@ -1601,35 +1604,69 @@ static void external_snapshot_prepare(BlkTransactionState *common,
return;
}
- flags = state->old_bs->open_flags;
+ if (action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
+ BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
+ const char *format = s->has_format ? s->format : "qcow2";
+ enum NewImageMode mode;
+ const char *snapshot_node_name =
+ s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
- /* create new image w/backing file */
- if (mode != NEW_IMAGE_MODE_EXISTING) {
- bdrv_img_create(new_image_file, format,
- state->old_bs->filename,
- state->old_bs->drv->format_name,
- NULL, -1, flags, &local_err, false);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (node_name && !snapshot_node_name) {
+ error_setg(errp, "New snapshot node name missing");
return;
}
- }
- options = qdict_new();
- if (has_snapshot_node_name) {
- qdict_put(options, "node-name",
- qstring_from_str(snapshot_node_name));
+ if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
+ error_setg(errp, "New snapshot node name already existing");
+ return;
+ }
+
+ flags = state->old_bs->open_flags;
+
+ /* create new image w/backing file */
+ mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+ if (mode != NEW_IMAGE_MODE_EXISTING) {
+ bdrv_img_create(new_image_file, format,
+ state->old_bs->filename,
+ state->old_bs->drv->format_name,
+ NULL, -1, flags, &local_err, false);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ }
+
+ options = qdict_new();
+ if (s->has_snapshot_node_name) {
+ qdict_put(options, "node-name",
+ qstring_from_str(snapshot_node_name));
+ }
+ qdict_put(options, "driver", qstring_from_str(format));
+
+ flags |= BDRV_O_NO_BACKING;
}
- qdict_put(options, "driver", qstring_from_str(format));
- /* TODO Inherit bs->options or only take explicit options with an
- * extended QMP command? */
assert(state->new_bs == NULL);
- ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
- flags | BDRV_O_NO_BACKING, &local_err);
+ ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
+ flags, errp);
/* We will manually add the backing_hd field to the bs later */
if (ret != 0) {
- error_propagate(errp, local_err);
+ return;
+ }
+
+ if (state->new_bs->blk != NULL) {
+ error_setg(errp, "The snapshot is already in use by %s",
+ blk_name(state->new_bs->blk));
+ return;
+ }
+
+ if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+ errp)) {
+ return;
+ }
+
+ if (state->new_bs->backing_hd != NULL) {
+ error_setg(errp, "The snapshot already has a backing image");
}
}
@@ -1818,6 +1855,12 @@ static void abort_commit(BlkTransactionState *common)
}
static const BdrvActionOps actions[] = {
+ [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
+ .instance_size = sizeof(ExternalSnapshotState),
+ .prepare = external_snapshot_prepare,
+ .commit = external_snapshot_commit,
+ .abort = external_snapshot_abort,
+ },
[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
.instance_size = sizeof(ExternalSnapshotState),
.prepare = external_snapshot_prepare,
diff --git a/qapi-schema.json b/qapi-schema.json
index c32dc20..71dacea 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1493,9 +1493,11 @@
# abort since 1.6
# blockdev-snapshot-internal-sync since 1.7
# blockdev-backup since 2.3
+# blockdev-snapshot since 2.5
##
{ 'union': 'TransactionAction',
'data': {
+ 'blockdev-snapshot': 'BlockdevSnapshot',
'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
'drive-backup': 'DriveBackup',
'blockdev-backup': 'BlockdevBackup',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f797d7..7d05166 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -705,6 +705,19 @@
'*format': 'str', '*mode': 'NewImageMode' } }
##
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.
+#
+# @snapshot: reference to the existing block device that will be used
+# for the snapshot.
+#
+# Since 2.5
+##
+{ 'struct': 'BlockdevSnapshot',
+ 'data': { 'device': 'str', 'snapshot': 'str' } }
+
+##
# @DriveBackup
#
# @device: the name of the device which should be copied.
@@ -800,6 +813,19 @@
{ 'command': 'blockdev-snapshot-sync',
'data': 'BlockdevSnapshotSync' }
+
+##
+# @blockdev-snapshot
+#
+# Generates a snapshot of a block device.
+#
+# For the arguments, see the documentation of BlockdevSnapshot.
+#
+# Since 2.5
+##
+{ 'command': 'blockdev-snapshot',
+ 'data': 'BlockdevSnapshot' }
+
##
# @change-backing-file
#
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 495670b..05a5675 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1454,6 +1454,35 @@ Example:
EQMP
{
+ .name = "blockdev-snapshot",
+ .args_type = "device:s,snapshot:s",
+ .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot,
+ },
+
+SQMP
+blockdev-snapshot
+-----------------
+Since 2.5
+
+Create a snapshot, by installing 'device' as the backing image of
+'snapshot'. Additionally, if 'device' is associated with a block
+device, the block device changes to using 'snapshot' as its new active
+image.
+
+Arguments:
+
+- "device": snapshot source (json-string)
+- "snapshot": snapshot target (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
+ "snapshot": "node1534" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "blockdev-snapshot-internal-sync",
.args_type = "device:B,name:s",
.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
--
2.5.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command
2015-09-10 13:39 [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command Alberto Garcia
` (2 preceding siblings ...)
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command Alberto Garcia
@ 2015-09-10 13:39 ` Alberto Garcia
2015-09-11 18:02 ` Eric Blake
3 siblings, 1 reply; 18+ messages in thread
From: Alberto Garcia @ 2015-09-10 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz,
Stefan Hajnoczi
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
tests/qemu-iotests/085 | 97 +++++++++++++++++++++++++++++++++++++++++++---
tests/qemu-iotests/085.out | 34 +++++++++++++++-
2 files changed, 123 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 56cd6f8..2b0f85a 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -7,6 +7,7 @@
# snapshots are performed.
#
# Copyright (C) 2014 Red Hat, Inc.
+# Copyright (C) 2015 Igalia, S.L.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -34,17 +35,17 @@ status=1 # failure is the default!
snapshot_virt0="snapshot-v0.qcow2"
snapshot_virt1="snapshot-v1.qcow2"
-MAX_SNAPSHOTS=10
+SNAPSHOTS=10
_cleanup()
{
_cleanup_qemu
- for i in $(seq 1 ${MAX_SNAPSHOTS})
+ for i in $(seq 1 ${SNAPSHOTS})
do
rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
done
- _cleanup_test_img
+ rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
}
trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -85,18 +86,45 @@ function create_group_snapshot()
_send_qemu_cmd $h "${cmd}" "return"
}
+# ${1}: unique identifier for the snapshot filename
+# ${2}: true: ignore backing images (default); false: don't ignore them
+function add_snapshot_image()
+{
+ base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
+ snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
+ _make_test_img -b "${base_image}" "$size"
+ mv "${TEST_IMG}" "${snapshot_file}"
+ cmd="{ 'execute': 'blockdev-add', 'arguments':
+ { 'options':
+ { 'driver': 'qcow2', 'node-name': 'snap_"${1}"',
+ 'ignore-backing': "${2:-true}", 'file':
+ { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
+ _send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
+# ${2}: expected response, defaults to 'return'
+function blockdev_snapshot()
+{
+ cmd="{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'device': 'virtio0',
+ 'snapshot':'snap_"${1}"' } }"
+ _send_qemu_cmd $h "${cmd}" "${2:-return}"
+}
+
size=128M
_make_test_img $size
-mv "${TEST_IMG}" "${TEST_IMG}.orig"
+mv "${TEST_IMG}" "${TEST_IMG}.1"
_make_test_img $size
+mv "${TEST_IMG}" "${TEST_IMG}.2"
echo
echo === Running QEMU ===
echo
qemu_comm_method="qmp"
-_launch_qemu -drive file="${TEST_IMG}.orig",if=virtio -drive file="${TEST_IMG}",if=virtio
+_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
h=$QEMU_HANDLE
echo
@@ -105,6 +133,8 @@ echo
_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
+# Tests for the blockdev-snapshot-sync command
+
echo
echo === Create a single snapshot on virtio0 ===
echo
@@ -132,11 +162,66 @@ echo
echo === Create several transactional group snapshots ===
echo
-for i in $(seq 2 ${MAX_SNAPSHOTS})
+for i in $(seq 2 ${SNAPSHOTS})
do
create_group_snapshot ${i}
done
+# Tests for the blockdev-snapshot command
+
+echo
+echo === Create a couple of snapshots using blockdev-snapshot ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+blockdev_snapshot ${SNAPSHOTS}
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+blockdev_snapshot ${SNAPSHOTS}
+
+echo
+echo === Invalid command - snapshot node used as active layer ===
+echo
+
+blockdev_snapshot ${SNAPSHOTS} error
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'device':'virtio0',
+ 'snapshot':'virtio0' }
+ }" "error"
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'device':'virtio0',
+ 'snapshot':'virtio1' }
+ }" "error"
+
+echo
+echo === Invalid command - snapshot node used as backing hd ===
+echo
+
+blockdev_snapshot $((${SNAPSHOTS}-1)) error
+
+echo
+echo === Invalid command - snapshot node has a backing image ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS} false
+blockdev_snapshot ${SNAPSHOTS} error
+
+echo
+echo === Invalid command - The node does not exist ===
+echo
+
+blockdev_snapshot $((${SNAPSHOTS}+1)) error
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'device':'nodevice',
+ 'snapshot':'snap_"${SNAPSHOTS}"' }
+ }" "error"
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 5eb8b94..2238f6d 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -11,7 +11,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
=== Create a single snapshot on virtio0 ===
-Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2.orig' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2.1' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
{"return": {}}
=== Invalid command - missing device and nodename ===
@@ -26,7 +26,7 @@ Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file
=== Create several transactional group snapshots ===
Formatting 'TEST_DIR/2-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/1-snapshot-v0.qcow2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
-Formatting 'TEST_DIR/2-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/2-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/t.qcow2.2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
{"return": {}}
Formatting 'TEST_DIR/3-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/2-snapshot-v0.qcow2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
Formatting 'TEST_DIR/3-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/2-snapshot-v1.qcow2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
@@ -52,4 +52,34 @@ Formatting 'TEST_DIR/9-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file
Formatting 'TEST_DIR/10-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/9-snapshot-v0.qcow2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
Formatting 'TEST_DIR/10-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file='TEST_DIR/9-snapshot-v1.qcow2' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
{"return": {}}
+
+=== Create a couple of snapshots using blockdev-snapshot ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/10-snapshot-v0.IMGFMT'
+{"return": {}}
+{"return": {}}
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/11-snapshot-v0.IMGFMT'
+{"return": {}}
+{"return": {}}
+
+=== Invalid command - snapshot node used as active layer ===
+
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio1"}}
+
+=== Invalid command - snapshot node used as backing hd ===
+
+{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is used as backing hd of 'virtio0'"}}
+
+=== Invalid command - snapshot node has a backing image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/12-snapshot-v0.IMGFMT'
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "The snapshot already has a backing image"}}
+
+=== Invalid command - The node does not exist ===
+
+{"error": {"class": "GenericError", "desc": "Cannot find device=snap_14 nor node_name=snap_14"}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=nodevice nor node_name=nodevice"}}
*** done
--
2.5.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync Alberto Garcia
@ 2015-09-11 17:15 ` Max Reitz
0 siblings, 0 replies; 18+ messages in thread
From: Max Reitz @ 2015-09-11 17:15 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 531 bytes --]
On 10.09.2015 15:39, Alberto Garcia wrote:
> We will introduce the 'blockdev-snapshot' command that will require
> its own struct for the parameters, so we need to rename this one in
> order to avoid name clashes.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> blockdev.c | 2 +-
> qapi-schema.json | 2 +-
> qapi/block-core.json | 8 ++++----
> 3 files changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat Alberto Garcia
@ 2015-09-11 17:21 ` Max Reitz
2015-09-11 17:28 ` Kevin Wolf
2015-09-11 17:28 ` Eric Blake
1 sibling, 1 reply; 18+ messages in thread
From: Max Reitz @ 2015-09-11 17:21 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 726 bytes --]
On 10.09.2015 15:39, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 5 +++++
> qapi/block-core.json | 6 +++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty
string in order to achieve exactly that.
So why do we need the new flag? Because "backing: ''" is ugly?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat Alberto Garcia
2015-09-11 17:21 ` Max Reitz
@ 2015-09-11 17:28 ` Eric Blake
2015-09-11 17:30 ` [Qemu-devel] [Qemu-block] " Eric Blake
1 sibling, 1 reply; 18+ messages in thread
From: Eric Blake @ 2015-09-11 17:28 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]
On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 5 +++++
> qapi/block-core.json | 6 +++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 22d3b0e..4be32fb 100644
> --- a/block.c
> +++ b/block.c
> @@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>
> assert(drvname || !(flags & BDRV_O_PROTOCOL));
>
> + if (qdict_get_try_bool(options, "ignore-backing", false)) {
> + flags |= BDRV_O_NO_BACKING;
> + }
> + qdict_del(options, "ignore-backing");
What happens if the user specified "ignore-backing":true, "backing":...?
Should that be a hard error?
> { 'struct': 'BlockdevOptionsGenericCOWFormat',
> 'base': 'BlockdevOptionsGenericFormat',
> - 'data': { '*backing': 'BlockdevRef' } }
> + 'data': { '*backing': 'BlockdevRef',
> + '*ignore-backing': 'bool' } }
Depending on whether the answer to my question is that we already behave
sanely and don't leave a BlockdevRef dangling if the caller mixes the
two approaches, then:
Reviewed-by: Eric Blake <eblake@redhat.com>
But design-wise, would it make sense to support:
"backing":null
as an explicit request to not open a backing file? Right now, qapi does
not have a way to express 'null' as part of an alternate type; but if it
did, BlockdevRef would merely add 'null' as one of its allowed
alternates. Then we wouldn't need ignore-backing from the QMP
perspective. But I'm still not sure how it would map to the command line
perspective.
--
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: 604 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-11 17:21 ` Max Reitz
@ 2015-09-11 17:28 ` Kevin Wolf
2015-09-11 17:33 ` Max Reitz
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Wolf @ 2015-09-11 17:28 UTC (permalink / raw)
To: Max Reitz; +Cc: Stefan Hajnoczi, Alberto Garcia, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]
Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
> On 10.09.2015 15:39, Alberto Garcia wrote:
> > If set to true, the image will be opened with the BDRV_O_NO_BACKING
> > flag. This is useful for creating snapshots using images opened with
> > blockdev-add, since they are not supposed to have a backing image
> > before the operation.
> >
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> > block.c | 5 +++++
> > qapi/block-core.json | 6 +++++-
> > 2 files changed, 10 insertions(+), 1 deletion(-)
>
> Ignorant of any possible previous discussion that might have taken
> place: The documentation for @backing says it may be set to the empty
> string in order to achieve exactly that.
>
> So why do we need the new flag? Because "backing: ''" is ugly?
I guess it's just because you're the only one who actually reads the
documentation. When discussing this, I didn't remember that we already
had a way to express this (an additional bool wouldn't have been my
favourite solution anyway). Thanks for catching this.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-11 17:28 ` Eric Blake
@ 2015-09-11 17:30 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-09-11 17:30 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: Kevin Wolf, qemu-block, Stefan Hajnoczi, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 400 bytes --]
On 09/11/2015 11:28 AM, Eric Blake wrote:
> But design-wise, would it make sense to support:
>
> "backing":null
Just read Max's response; it sounds like we already have "backing":""
(and don't need "backing":null) for what we want. So maybe we don't need
this patch after all.
--
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: 604 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-11 17:28 ` Kevin Wolf
@ 2015-09-11 17:33 ` Max Reitz
2015-09-14 5:54 ` Alberto Garcia
0 siblings, 1 reply; 18+ messages in thread
From: Max Reitz @ 2015-09-11 17:33 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, Alberto Garcia, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]
On 11.09.2015 19:28, Kevin Wolf wrote:
> Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
>> On 10.09.2015 15:39, Alberto Garcia wrote:
>>> If set to true, the image will be opened with the BDRV_O_NO_BACKING
>>> flag. This is useful for creating snapshots using images opened with
>>> blockdev-add, since they are not supposed to have a backing image
>>> before the operation.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>> block.c | 5 +++++
>>> qapi/block-core.json | 6 +++++-
>>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> Ignorant of any possible previous discussion that might have taken
>> place: The documentation for @backing says it may be set to the empty
>> string in order to achieve exactly that.
>>
>> So why do we need the new flag? Because "backing: ''" is ugly?
>
> I guess it's just because you're the only one who actually reads the
> documentation. When discussing this, I didn't remember that we already
> had a way to express this (an additional bool wouldn't have been my
> favourite solution anyway). Thanks for catching this.
I read the patch, it was part of the context. ;-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command Alberto Garcia
@ 2015-09-11 17:58 ` Eric Blake
2015-09-14 14:08 ` Alberto Garcia
2015-09-11 18:11 ` Max Reitz
1 sibling, 1 reply; 18+ messages in thread
From: Eric Blake @ 2015-09-11 17:58 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 4447 bytes --]
On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
>
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
>
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
>
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> blockdev.c | 163 ++++++++++++++++++++++++++++++++-------------------
> qapi-schema.json | 2 +
> qapi/block-core.json | 26 ++++++++
> qmp-commands.hx | 29 +++++++++
> 4 files changed, 160 insertions(+), 60 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 6b787c1..78cfb79 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
> &snapshot, errp);
> }
>
> +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
> + Error **errp)
> +{
> + BlockdevSnapshot snapshot_data = {
> + .device = (char *) device,
> + .snapshot = (char *) snapshot
> + };
Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to
make this function have the saner signature of
void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);
rather than having to rebuild the struct yourself (with an annoying cast
to lose const) :)
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html
But no need to hold this series up waiting for the qapi review queue to
flush. We can simplify later.
>
> - options = qdict_new();
> - if (has_snapshot_node_name) {
> - qdict_put(options, "node-name",
> - qstring_from_str(snapshot_node_name));
> + if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> + error_setg(errp, "New snapshot node name already existing");
Pre-existing, but s/existing/exists/ while you are reindenting this.
> +++ b/qapi/block-core.json
> @@ -705,6 +705,19 @@
> '*format': 'str', '*mode': 'NewImageMode' } }
>
> ##
> +# @BlockdevSnapshot
> +#
> +# @device: device or node name to generate the snapshot from.
> +#
> +# @snapshot: reference to the existing block device that will be used
> +# for the snapshot.
Maybe mention that it must NOT have a current backing file, and point to
the "backing":"" trick to get it that way.
> +Create a snapshot, by installing 'device' as the backing image of
> +'snapshot'. Additionally, if 'device' is associated with a block
> +device, the block device changes to using 'snapshot' as its new active
> +image.
Still didn't answer the question from the earlier review of whether the
blockdev-snapshot-sync behavior of specifying the node name of an active
layer in order to not pivot the block device to the snapshot still makes
sense to support in the blockdev-snapshot case. But we could always add
an optional boolean flag later if someone comes up for a use case where
they'd need to create a snapshot of the active layer without the block
device pivoting, so I don't think it should hold up this patch.
> +
> +Arguments:
> +
> +- "device": snapshot source (json-string)
> +- "snapshot": snapshot target (json-string)
> +
> +Example:
> +
> +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
> + "snapshot": "node1534" } }
> +<- { "return": {} }
Maybe enhance the example to show the preliminary blockdev-add that
created node1534?
I've pointed out some potential wording improvements, but think they are
minor enough that you can have:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
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: 604 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command Alberto Garcia
@ 2015-09-11 18:02 ` Eric Blake
0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2015-09-11 18:02 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]
On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> tests/qemu-iotests/085 | 97 +++++++++++++++++++++++++++++++++++++++++++---
> tests/qemu-iotests/085.out | 34 +++++++++++++++-
> 2 files changed, 123 insertions(+), 8 deletions(-)
>
>
> +# ${1}: unique identifier for the snapshot filename
> +# ${2}: true: ignore backing images (default); false: don't ignore them
> +function add_snapshot_image()
> +{
> + base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
> + snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
> + _make_test_img -b "${base_image}" "$size"
> + mv "${TEST_IMG}" "${snapshot_file}"
> + cmd="{ 'execute': 'blockdev-add', 'arguments':
> + { 'options':
> + { 'driver': 'qcow2', 'node-name': 'snap_"${1}"',
> + 'ignore-backing': "${2:-true}", 'file':
Needs to be reworked to use 'backing':'' instead of 'ignore-backing'.
But overall looks like a sane set of tests to cover both positive and
negative expected behavior.
--
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: 604 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command Alberto Garcia
2015-09-11 17:58 ` Eric Blake
@ 2015-09-11 18:11 ` Max Reitz
1 sibling, 0 replies; 18+ messages in thread
From: Max Reitz @ 2015-09-11 18:11 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]
On 10.09.2015 15:39, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
>
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
>
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
>
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> blockdev.c | 163 ++++++++++++++++++++++++++++++++-------------------
> qapi-schema.json | 2 +
> qapi/block-core.json | 26 ++++++++
> qmp-commands.hx | 29 +++++++++
> 4 files changed, 160 insertions(+), 60 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-11 17:33 ` Max Reitz
@ 2015-09-14 5:54 ` Alberto Garcia
2015-09-14 8:45 ` Kevin Wolf
0 siblings, 1 reply; 18+ messages in thread
From: Alberto Garcia @ 2015-09-14 5:54 UTC (permalink / raw)
To: Max Reitz, Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, qemu-block
On Fri 11 Sep 2015 07:33:41 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>>> So why do we need the new flag? Because "backing: ''" is ugly?
>>
>> I guess it's just because you're the only one who actually reads the
>> documentation. When discussing this, I didn't remember that we
>> already had a way to express this (an additional bool wouldn't have
>> been my favourite solution anyway). Thanks for catching this.
>
> I read the patch, it was part of the context. ;-)
Oh, that was embarrassing :-) Yes, it was the discussion from two weeks
ago about passing empty strings as BlockdevRef that made me think that
this would be ugly.
Anyway, was this ever implemented? It seems that passing a string to the
'backing' parameter is only specified in the JSON schema, but no one
actually uses that.
So I'll implement that for the next version of my series.
Berto
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-14 5:54 ` Alberto Garcia
@ 2015-09-14 8:45 ` Kevin Wolf
2015-09-14 14:20 ` Alberto Garcia
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Wolf @ 2015-09-14 8:45 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Stefan Hajnoczi, qemu-devel, qemu-block, Max Reitz
Am 14.09.2015 um 07:54 hat Alberto Garcia geschrieben:
> On Fri 11 Sep 2015 07:33:41 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>
> >>> So why do we need the new flag? Because "backing: ''" is ugly?
> >>
> >> I guess it's just because you're the only one who actually reads the
> >> documentation. When discussing this, I didn't remember that we
> >> already had a way to express this (an additional bool wouldn't have
> >> been my favourite solution anyway). Thanks for catching this.
> >
> > I read the patch, it was part of the context. ;-)
>
> Oh, that was embarrassing :-) Yes, it was the discussion from two weeks
> ago about passing empty strings as BlockdevRef that made me think that
> this would be ugly.
>
> Anyway, was this ever implemented? It seems that passing a string to the
> 'backing' parameter is only specified in the JSON schema, but no one
> actually uses that.
>
> So I'll implement that for the next version of my series.
I have a patch that actually allows passing a node-name reference as a
string here. v1 was posted a few months ago; it just turned out that I
need to kill bdrv_swap() before that can work because bdrv_swap()
doesn't work with nodes that have a BlockBackend attached.
Of course, I didn't check what an empty string would do with my patch...
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command
2015-09-11 17:58 ` Eric Blake
@ 2015-09-14 14:08 ` Alberto Garcia
0 siblings, 0 replies; 18+ messages in thread
From: Alberto Garcia @ 2015-09-14 14:08 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, Max Reitz
On Fri 11 Sep 2015 07:58:11 PM CEST, Eric Blake wrote:
>> +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
>> + Error **errp)
>> +{
>> + BlockdevSnapshot snapshot_data = {
>> + .device = (char *) device,
>> + .snapshot = (char *) snapshot
>> + };
>
> Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to
> make this function have the saner signature of
>
> void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);
>
> rather than having to rebuild the struct yourself (with an annoying
> cast to lose const) :)
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html
That's cool! We can simplify this later as you suggest.
>> +Create a snapshot, by installing 'device' as the backing image of
>> +'snapshot'. Additionally, if 'device' is associated with a block
>> +device, the block device changes to using 'snapshot' as its new
>> +active image.
>
> Still didn't answer the question from the earlier review of whether
> the blockdev-snapshot-sync behavior of specifying the node name of an
> active layer in order to not pivot the block device to the snapshot
> still makes sense to support in the blockdev-snapshot case.
Sorry if I overlooked the explanation, but I still didn't get the use
case for this.
And particularly for this command, that doesn't create any image but
simply uses a previously-opened one.
What would 'pivot': false do ? As far as I can see, specifying the node
name of an active layer in blockdev-snapshot-sync seems to pivot the
block device to the snapshot.
{ "execute": "blockdev-snapshot-sync",
"arguments": { "node-name": "hd0",
"snapshot-file": "hd1.qcow2",
"snapshot-node-name": "hd1" } }
(qemu) info block
virtio0 (hd1): hd1.qcow2 (qcow2)
Cache mode: writeback
Backing file: hd0.qcow2 (chain depth: 1)
Berto
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
2015-09-14 8:45 ` Kevin Wolf
@ 2015-09-14 14:20 ` Alberto Garcia
0 siblings, 0 replies; 18+ messages in thread
From: Alberto Garcia @ 2015-09-14 14:20 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, qemu-block, Max Reitz
On Mon 14 Sep 2015 10:45:55 AM CEST, Kevin Wolf wrote:
>> >>> So why do we need the new flag? Because "backing: ''" is ugly?
>>
>> Anyway, was this ever implemented? It seems that passing a string to
>> the 'backing' parameter is only specified in the JSON schema, but no
>> one actually uses that.
>>
> I have a patch that actually allows passing a node-name reference as a
> string here. v1 was posted a few months ago; it just turned out that I
> need to kill bdrv_swap() before that can work because bdrv_swap()
> doesn't work with nodes that have a BlockBackend attached.
But there was no way to open a BDS without having a BlockBackend
attached, or was there? It's possible with Max's "BlockBackend and
media" series, that's why I am using it.
Berto
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-09-14 14:20 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-10 13:39 [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync Alberto Garcia
2015-09-11 17:15 ` Max Reitz
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat Alberto Garcia
2015-09-11 17:21 ` Max Reitz
2015-09-11 17:28 ` Kevin Wolf
2015-09-11 17:33 ` Max Reitz
2015-09-14 5:54 ` Alberto Garcia
2015-09-14 8:45 ` Kevin Wolf
2015-09-14 14:20 ` Alberto Garcia
2015-09-11 17:28 ` Eric Blake
2015-09-11 17:30 ` [Qemu-devel] [Qemu-block] " Eric Blake
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command Alberto Garcia
2015-09-11 17:58 ` Eric Blake
2015-09-14 14:08 ` Alberto Garcia
2015-09-11 18:11 ` Max Reitz
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command Alberto Garcia
2015-09-11 18:02 ` Eric Blake
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.