[Qemu-devel] [PATCH v7 0/5] Add 'blockdev-snapshot' command

[Qemu-devel] [PATCH v7 0/5] Add 'blockdev-snapshot' command

[Alberto Garcia]

This series adds a new 'blockdev-snapshot' command, that is similar to
'blockdev-snapshot-sync' but takes references to two existing block
devices.

This depends on Max's "BlockBackend and media" series:

https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00497.html

v7:
- Rebase on top of the current master.
  qmp_marshal_input_blockdev_snapshot is renamed to
  qmp_marshal_blockdev_snapshot in order to make it build.
- New patch to use bdrv_lookup_bs() instead of bdrv_find_node() in
  external_snapshot_prepare(). This way, if the user attempts to use
  blockdev-snapshot-sync using an existing device ID in the
  snapshot-node-name parameter, the code will detect the error before
  the new image is created.

v6: https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00575.html
- Update documentation and parameter names following Eric's
  suggestions: 'device' -> 'node', 'snapshot' -> 'overlay'.
- Rebased on top of Max's "BlockBackend and media" v5

v5: https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00483.html
- Don't delete the 'backing' option if it contains something different
  from an empty string.
- Rebase on top of the current master.

v4: https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00372.html
- Implement the support for 'backing': '', drop 'ignore-backing',
  and update iotest 085 accordingly.
- Include sample 'blockdev-add' call in the 'blockdev-snapshot'
  documentation.
- Clarify that the snapshot must not have a backing file in the
  BlockdevSnapshot documentation.
- Update error message ("...node name already existing" -> "...exists").

v3: https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00272.html
- 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 (5):
  block: check for existing device IDs in external_snapshot_prepare()
  block: rename BlockdevSnapshot to BlockdevSnapshotSync
  block: support passing 'backing': '' to 'blockdev-add'
  block: add a 'blockdev-snapshot' QMP command
  block: add tests for the 'blockdev-snapshot' command

 block.c                    |   7 ++
 blockdev.c                 | 166 ++++++++++++++++++++++++++++-----------------
 qapi-schema.json           |   4 +-
 qapi/block-core.json       |  34 +++++++++-
 qmp-commands.hx            |  38 +++++++++++
 tests/qemu-iotests/085     | 102 ++++++++++++++++++++++++++--
 tests/qemu-iotests/085.out |  34 +++++++++-
 7 files changed, 312 insertions(+), 73 deletions(-)

-- 
2.6.1

[Qemu-devel] [PATCH v7 1/5] block: check for existing device IDs in external_snapshot_prepare()

[Alberto Garcia]

The 'snapshot-node-name' parameter of blockdev-snapshot-sync allows
setting the node name of the image that is going to be created.

Before creating the image, external_snapshot_prepare() checks that the
name is not already being used. The check is however incomplete since
it only considers existing node names, but node names must not clash
with device IDs either because they share the same namespace.

If the user attempts to create a snapshot using the name of an
existing device for the 'snapshot-node-name' parameter the operation
will eventually fail, but only after the new image has been created.

This patch replaces bdrv_find_node() with bdrv_lookup_bs() to extend
the check to existing device IDs, and thus detect possible name
clashes before the new image is created.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 4731843..0898d1f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1570,8 +1570,9 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
-        error_setg(errp, "New snapshot node name already existing");
+    if (has_snapshot_node_name &&
+        bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
+        error_setg(errp, "New snapshot node name already in use");
         return;
     }
 
-- 
2.6.1

[Qemu-devel] [PATCH v7 2/5] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Alberto Garcia]

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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@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 0898d1f..12741a0 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 a05794e..65701dc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1534,7 +1534,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 5f12af7..6b5ac02 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.6.1

[Qemu-devel] [PATCH v7 3/5] block: support passing 'backing': '' to 'blockdev-add'

[Alberto Garcia]

Passing an empty string allows opening an image but not its backing
file. This was already described in the API documentation, only the
implementation was missing.

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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index c9e3c6c..31d1b6e 100644
--- a/block.c
+++ b/block.c
@@ -1406,6 +1406,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
     BlockDriverState *file = NULL, *bs;
     BlockDriver *drv = NULL;
     const char *drvname;
+    const char *backing;
     Error *local_err = NULL;
     int snapshot_flags = 0;
 
@@ -1473,6 +1474,12 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
 
     assert(drvname || !(flags & BDRV_O_PROTOCOL));
 
+    backing = qdict_get_try_str(options, "backing");
+    if (backing && *backing == '\0') {
+        flags |= BDRV_O_NO_BACKING;
+        qdict_del(options, "backing");
+    }
+
     bs->open_flags = flags;
     bs->options = options;
     options = qdict_clone_shallow(options);
-- 
2.6.1

[Qemu-devel] [PATCH v7 4/5] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

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>
Cc: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           | 165 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  28 +++++++++
 qmp-commands.hx      |  38 ++++++++++++
 4 files changed, 172 insertions(+), 61 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 12741a0..b5470c9 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 *node, const char *overlay,
+                           Error **errp)
+{
+    BlockdevSnapshot snapshot_data = {
+        .node = (char *) node,
+        .overlay = (char *) overlay
+    };
+
+    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,58 +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->node;
+            node_name = s->node;
+            new_image_file = NULL;
+            snapshot_ref = s->overlay;
+        }
+        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_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-        error_setg(errp, "New snapshot node name already in use");
+    state->old_bs = bdrv_lookup_bs(device, node_name, errp);
+    if (!state->old_bs) {
         return;
     }
 
@@ -1602,35 +1604,70 @@ 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_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
+            error_setg(errp, "New snapshot node name already in use");
+            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");
     }
 }
 
@@ -1819,6 +1856,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 65701dc..326cc73 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1531,9 +1531,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 6b5ac02..2563ac9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -705,6 +705,21 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshot
+#
+# @node: device or node name that will have a snapshot created.
+#
+# @overlay: reference to the existing block device that will become
+#           the overlay of @node, as part of creating the snapshot.
+#           It must not have a current backing file (this can be
+#           achieved by passing "backing": "" to blockdev-add).
+#
+# Since 2.5
+##
+{ 'struct': 'BlockdevSnapshot',
+  'data': { 'node': 'str', 'overlay': 'str' } }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -800,6 +815,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 4f03d11..9921ce2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1461,6 +1461,44 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot",
+        .args_type  = "node:s,overlay:s",
+        .mhandler.cmd_new = qmp_marshal_blockdev_snapshot,
+    },
+
+SQMP
+blockdev-snapshot
+-----------------
+Since 2.5
+
+Create a snapshot, by installing 'node' as the backing image of
+'overlay'. Additionally, if 'node' is associated with a block
+device, the block device changes to using 'overlay' as its new active
+image.
+
+Arguments:
+
+- "node": device that will have a snapshot created (json-string)
+- "overlay": device that will have 'node' as its backing image (json-string)
+
+Example:
+
+-> { "execute": "blockdev-add",
+                "arguments": { "options": { "driver": "qcow2",
+                                            "node-name": "node1534",
+                                            "file": { "driver": "file",
+                                                      "filename": "hd1.qcow2" },
+                                            "backing": "" } } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-snapshot", "arguments": { "node": "ide-hd0",
+                                                    "overlay": "node1534" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-internal-sync",
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_blockdev_snapshot_internal_sync,
-- 
2.6.1

[Qemu-devel] [PATCH v7 5/5] block: add tests for the 'blockdev-snapshot' command

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/085     | 102 ++++++++++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/085.out |  34 ++++++++++++++-
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 56cd6f8..9484117 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,50 @@ function create_group_snapshot()
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
+# ${1}: unique identifier for the snapshot filename
+# ${2}: true: open backing images; false: don't open them (default)
+function add_snapshot_image()
+{
+    if [ "${2}" = "true" ]; then
+        extra_params=""
+    else
+        extra_params="'backing': '', "
+    fi
+    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}"', "${extra_params}"
+               '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': { 'node': 'virtio0',
+                                     'overlay':'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 +138,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 +167,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': { 'node':'virtio0',
+                                    'overlay':'virtio0' }
+                   }" "error"
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'node':'virtio0',
+                                    'overlay':'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} true
+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': { 'node':'nodevice',
+                                    'overlay':'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 a6cf19e..52292ea 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.6.1

Re: [Qemu-devel] [PATCH v7 1/5] block: check for existing device IDs in external_snapshot_prepare()

[Max Reitz]

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

On 12.10.2015 11:16, Alberto Garcia wrote:
> The 'snapshot-node-name' parameter of blockdev-snapshot-sync allows
> setting the node name of the image that is going to be created.
> 
> Before creating the image, external_snapshot_prepare() checks that the
> name is not already being used. The check is however incomplete since
> it only considers existing node names, but node names must not clash
> with device IDs either because they share the same namespace.
> 
> If the user attempts to create a snapshot using the name of an
> existing device for the 'snapshot-node-name' parameter the operation
> will eventually fail, but only after the new image has been created.
> 
> This patch replaces bdrv_find_node() with bdrv_lookup_bs() to extend
> the check to existing device IDs, and thus detect possible name
> clashes before the new image is created.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v7 4/5] block: add a 'blockdev-snapshot' QMP command

[Max Reitz]

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

On 12.10.2015 11:16, 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>
> Cc: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c           | 165 ++++++++++++++++++++++++++++++++-------------------
>  qapi-schema.json     |   2 +
>  qapi/block-core.json |  28 +++++++++
>  qmp-commands.hx      |  38 ++++++++++++
>  4 files changed, 172 insertions(+), 61 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 12741a0..b5470c9 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -1521,58 +1533,48 @@ typedef struct ExternalSnapshotState {

[...]

>      }
>  
>      /* 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_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
> -        error_setg(errp, "New snapshot node name already in use");

There's a difference from v6 here...

> +    state->old_bs = bdrv_lookup_bs(device, node_name, errp);
> +    if (!state->old_bs) {
>          return;
>      }
>  
> @@ -1602,35 +1604,70 @@ 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_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
> +            error_setg(errp, "New snapshot node name already in use");

...and here, but how to resolve the conflict resulting from the newly
added patch 1 was obvious, so my R-b stands, of course.

Anyway, this is not why I'm replying, that's further down:

> +            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");
>      }

It's here: In case Kevin's series is applied before this one (which I'm
assuming since you were brave enough to base this series on my BB
series), this needs to be s/backing_hd/backing/. I'm just saying this so
you know you can keep my R-b then.

Max

>  }
>  


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

Re: [Qemu-devel] [PATCH v7 4/5] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Mon 12 Oct 2015 10:29:35 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>> -    if (has_snapshot_node_name &&
>> -        bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
>> -        error_setg(errp, "New snapshot node name already in use");
>
> There's a difference from v6 here...
[...]
>> -    options = qdict_new();
>> -    if (has_snapshot_node_name) {
>> -        qdict_put(options, "node-name",
>> -                  qstring_from_str(snapshot_node_name));
>> +        if (snapshot_node_name &&
>> +            bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
>> +            error_setg(errp, "New snapshot node name already in use");
>
> ...and here, but how to resolve the conflict resulting from the newly
> added patch 1 was obvious, so my R-b stands, of course.

The differences are because this patch is now rebased on top of the new
one, sorry if I overstepped by keeping your R-b here!

>> +    if (state->new_bs->backing_hd != NULL) {
>> +        error_setg(errp, "The snapshot already has a backing image");
>>      }
>
> It's here: In case Kevin's series is applied before this one (which
> I'm assuming since you were brave enough to base this series on my BB
> series), this needs to be s/backing_hd/backing/. I'm just saying this
> so you know you can keep my R-b then.

Sure, I was about to test your new version of the series.

Thanks,

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v7 1/5] block: check for existing device IDs in external_snapshot_prepare()

[Jeff Cody]

On Mon, Oct 12, 2015 at 12:16:13PM +0300, Alberto Garcia wrote:
> The 'snapshot-node-name' parameter of blockdev-snapshot-sync allows
> setting the node name of the image that is going to be created.
> 
> Before creating the image, external_snapshot_prepare() checks that the
> name is not already being used. The check is however incomplete since
> it only considers existing node names, but node names must not clash
> with device IDs either because they share the same namespace.
> 
> If the user attempts to create a snapshot using the name of an
> existing device for the 'snapshot-node-name' parameter the operation
> will eventually fail, but only after the new image has been created.
> 
> This patch replaces bdrv_find_node() with bdrv_lookup_bs() to extend
> the check to existing device IDs, and thus detect possible name
> clashes before the new image is created.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4731843..0898d1f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1570,8 +1570,9 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>          return;
>      }
>  
> -    if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> -        error_setg(errp, "New snapshot node name already existing");
> +    if (has_snapshot_node_name &&
> +        bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
> +        error_setg(errp, "New snapshot node name already in use");
>          return;
>      }
>  
> -- 
> 2.6.1
> 
>

A downfall is we don't communicate back if the collision is with a
device name or a node name, but I guess that is relatively minor (and
probably not worth splitting this into two different calls to
bdrv_lookup_bs() to differentiate).

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v7 2/5] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Jeff Cody]

On Mon, Oct 12, 2015 at 12:16:14PM +0300, 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@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 0898d1f..12741a0 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 a05794e..65701dc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1534,7 +1534,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 5f12af7..6b5ac02 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.6.1
> 
> 
Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v7 3/5] block: support passing 'backing': '' to 'blockdev-add'

[Jeff Cody]

On Mon, Oct 12, 2015 at 12:16:15PM +0300, Alberto Garcia wrote:
> Passing an empty string allows opening an image but not its backing
> file. This was already described in the API documentation, only the
> implementation was missing.
> 
> 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block.c b/block.c
> index c9e3c6c..31d1b6e 100644
> --- a/block.c
> +++ b/block.c
> @@ -1406,6 +1406,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>      BlockDriverState *file = NULL, *bs;
>      BlockDriver *drv = NULL;
>      const char *drvname;
> +    const char *backing;
>      Error *local_err = NULL;
>      int snapshot_flags = 0;
>  
> @@ -1473,6 +1474,12 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>  
>      assert(drvname || !(flags & BDRV_O_PROTOCOL));
>  
> +    backing = qdict_get_try_str(options, "backing");
> +    if (backing && *backing == '\0') {
> +        flags |= BDRV_O_NO_BACKING;
> +        qdict_del(options, "backing");
> +    }
> +
>      bs->open_flags = flags;
>      bs->options = options;
>      options = qdict_clone_shallow(options);
> -- 
> 2.6.1
> 
> 
Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v7 5/5] block: add tests for the 'blockdev-snapshot' command

[Jeff Cody]

On Mon, Oct 12, 2015 at 12:16:17PM +0300, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/085     | 102 ++++++++++++++++++++++++++++++++++++++++++---
>  tests/qemu-iotests/085.out |  34 ++++++++++++++-
>  2 files changed, 128 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> index 56cd6f8..9484117 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,50 @@ function create_group_snapshot()
>      _send_qemu_cmd $h "${cmd}" "return"
>  }
>  
> +# ${1}: unique identifier for the snapshot filename
> +# ${2}: true: open backing images; false: don't open them (default)
> +function add_snapshot_image()
> +{
> +    if [ "${2}" = "true" ]; then
> +        extra_params=""
> +    else
> +        extra_params="'backing': '', "
> +    fi
> +    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}"', "${extra_params}"
> +               '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': { 'node': 'virtio0',
> +                                     'overlay':'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 +138,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 +167,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': { 'node':'virtio0',
> +                                    'overlay':'virtio0' }
> +                   }" "error"
> +
> +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
> +                     'arguments': { 'node':'virtio0',
> +                                    'overlay':'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} true
> +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': { 'node':'nodevice',
> +                                    'overlay':'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 a6cf19e..52292ea 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.6.1
> 
>

Reviewed-by: Jeff Cody <jcody@redhat.com>