Skip to contentSkip to conversation

Add 'blockdev-snapshot' command

PATCH Superseded Alberto Garcia 5 participants 14 messages Sep 14, 2015 – Sep 22, 2015

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v4 cover Add 'blockdev-snapshot' command

This version uses 'backing': '' instead of 'ignore-backing' as discussed in the previous thread.

Regards,

Berto

v4:
- 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 (4):
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                 | 165 ++++++++++++++++++++++++++++-----------------
 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, 311 insertions(+), 73 deletions(-)
Signature · 1 line
2.5.1
#1 lore raw

PATCH v4 1/4 block: rename BlockdevSnapshot to BlockdevSnapshotSync

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.

---
 blockdev.c           | 2 +-
 qapi-schema.json     | 2 +-
 qapi/block-core.json | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)
blockdev.c +1 -1
index 3f42863..6b787c1 100644
@@ -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,
qapi-schema.json +1 -1
index 1521eb7..c32dc20 100644
@@ -1496,7 +1496,7 @@
 ##
 { 'union': 'TransactionAction',
   'data': {
-       'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
        'drive-backup': 'DriveBackup',
        'blockdev-backup': 'BlockdevBackup',
        'abort': 'Abort',
qapi/block-core.json +4 -4
index cb99cad..ec50f06 100644
@@ -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
Signature · 1 line
2.5.1
Reviewed-by on v4 1/4
#2 lore raw

Re: [Qemu-devel] [PATCH v4 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync

Quoted 11 lines · 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.

---
 blockdev.c           | 2 +-
 qapi-schema.json     | 2 +-
 qapi/block-core.json | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)
#3 lore raw

PATCH v4 2/4 block: support passing 'backing': '' to 'blockdev-add'

In reply to 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.

---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)
block.c +7 -0
index 22d3b0e..ad1792d 100644
@@ -1402,6 +1402,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;
 
@@ -1469,6 +1470,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);
Signature · 1 line
2.5.1
#4 lore raw

Re: [Qemu-devel] [PATCH v4 2/4] block: support passing 'backing': '' to 'blockdev-add'

Quoted 42 lines · 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.

---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)
block.c +7 -0
index 22d3b0e..ad1792d 100644
@@ -1402,6 +1402,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;
 
@@ -1469,6 +1470,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);
Signature · 1 line
2.5.1

Specifying a non-empty "backing" will be a slient nop now, but it used to be an error before. Should we return an error?

Fam

#5 lore raw

Re: [Qemu-devel] [PATCH v4 2/4] block: support passing 'backing': '' to 'blockdev-add'

Quoted 9 lines · Fam Zheng

Review

Quoted 6 lines
(quoted diff) +6 -0
+    backing = qdict_get_try_str(options, "backing");
+    if (backing && *backing == '\0') {
+        flags |= BDRV_O_NO_BACKING;
+    }
+    qdict_del(options, "backing");
+

Specifying a non-empty "backing" will be a slient nop now, but it used to be an error before. Should we return an error?

Ah, yes, I think it should return an error. Thanks!

block.c +0 -0
@@ -1473,8 +1473,8 @@ static int bdrv_open_inherit(BlockDriverState

**pbs, const char *filename,

     backing = qdict_get_try_str(options, "backing");
     if (backing && *backing == '\0') {
         flags |= BDRV_O_NO_BACKING;
+        qdict_del(options, "backing");
     }

- qdict_del(options, "backing");

Berto

Reviewed-by
#6 lore raw

Re: [Qemu-devel] [PATCH v4 2/4] block: support passing 'backing': '' to 'blockdev-add'

Quoted 9 lines · Alberto Garcia
Quoted 6 lines · Fam Zheng
Quoted 6 lines
(quoted diff) +6 -0
+    backing = qdict_get_try_str(options, "backing");
+    if (backing && *backing == '\0') {
+        flags |= BDRV_O_NO_BACKING;
+    }
+    qdict_del(options, "backing");
+
Quoted 4 lines
Quoted 2 lines

Specifying a non-empty "backing" will be a slient nop now, but it used to be an error before. Should we return an error?

Ah, yes, I think it should return an error. Thanks!

With this squashed in,

Quoted 15 lines
block.c +1 -1
@@ -1473,8 +1473,8 @@ static int bdrv_open_inherit(BlockDriverState
 **pbs, const char *filename,
     backing = qdict_get_try_str(options, "backing");
     if (backing && *backing == '\0') {
         flags |= BDRV_O_NO_BACKING;
+        qdict_del(options, "backing");
     }
-    qdict_del(options, "backing");
 

Berto

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#7 lore raw

PATCH v4 3/4 block: add a 'blockdev-snapshot' QMP command

In reply to 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.

---
 blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  28 +++++++++
 qmp-commands.hx      |  38 ++++++++++++
 4 files changed, 171 insertions(+), 60 deletions(-)
blockdev.c +103 -60
index 6b787c1..18fdb5c 100644
@@ -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 exists");
+            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,
qapi-schema.json +2 -0
index c32dc20..71dacea 100644
@@ -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',
qapi/block-core.json +28 -0
index ec50f06..328c155 100644
@@ -705,6 +705,21 @@
             '*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. It must not have a current backing file
+#            (this can be achieved by passing "backing": "" to
+#            blockdev-add).
+#
+# Since 2.5
+##
+{ 'struct': 'BlockdevSnapshot',
+  'data': { 'device': 'str', 'snapshot': '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
 #
qmp-commands.hx +38 -0
index 495670b..516f935 100644
@@ -1454,6 +1454,44 @@ 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-add",
+                "arguments": { "options": { "driver": "qcow2",
+                                            "node-name": "node1534",
+                                            "file": { "driver": "file",
+                                                      "filename": "hd1.qcow2" },
+                                            "backing": "" } } }
+
+<- { "return": {} }
+
+-> { "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,
Signature · 1 line
2.5.1
Reviewed-by on v4 3/4
#8 lore raw

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

Quoted 19 lines · 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.

Quoted 6 lines
---
 blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  28 +++++++++
 qmp-commands.hx      |  38 ++++++++++++
 4 files changed, 171 insertions(+), 60 deletions(-)
#9 lore raw

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

In reply to Alberto Garcia
Quoted 23 lines · 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.

---

+++ b/qapi/block-core.json

qapi/block-core.json line 710

Quoted 6 lines
qapi/block-core.json +3 -0
@@ -705,6 +705,21 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.

I'm still wondering if 'node' is a better name than 'device' here.

qapi/block-core.json line 715

Quoted 5 lines
qapi/block-core.json +5 -0
+#
+# @snapshot: reference to the existing block device that will be used
+#            for the snapshot. It must not have a current backing file
+#            (this can be achieved by passing "backing": "" to
+#            blockdev-add).

Possibly confusing terminology.

Let's consider: the act of creating a snapshot says to go from:

image1 [read-write]

to

image1 [read-only] <- image2 [read-write]

that is, image1 is now the snapshot of the state in time we executed the command, and image2 is the delta from what happened since the snapshot.
Therefore, image2 is NOT the snapshot. Naming the command
'blockdev-snapshot' is fine, but I think we can have better names for the arguments.

Better wording might be:

@device: device or node that will have a snapshot created

@overlay: reference to existing block device that will become the overlay of device, as part of creating the snapshot. It must not have a current backing file...

Quoted 4 lines
qmp-commands.hx +4 -0
+Example:
+
+-> { "execute": "blockdev-add",
+                "arguments": { "options": { "driver": "qcow2",

qmp-commands.hx Example: · line 1490

Quoted 10 lines
qmp-commands.hx +10 -0
+                                            "node-name": "node1534",
+                                            "file": { "driver": "file",
+                                                      "filename": "hd1.qcow2" },
+                                            "backing": "" } } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
+                                                    "snapshot": "node1534" } }
+<- { "return": {} }

The example would need tweaking if we rename members of the command, but it definitely helps.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#10 lore raw

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

Quoted 5 lines · Eric Blake

qapi/block-core.json line 710

Quoted 3 lines
qapi/block-core.json +3 -0
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.

I'm still wondering if 'node' is a better name than 'device' here.

I don't have a strong preference. Kevin, Max, ... any opinions?

Quoted 22 lines

qapi/block-core.json line 715

Quoted 5 lines
qapi/block-core.json +5 -0
+#
+# @snapshot: reference to the existing block device that will be used
+#            for the snapshot. It must not have a current backing file
+#            (this can be achieved by passing "backing": "" to
+#            blockdev-add).

Possibly confusing terminology.

Let's consider: the act of creating a snapshot says to go from:

image1 [read-write]

to

image1 [read-only] <- image2 [read-write]

that is, image1 is now the snapshot of the state in time we executed the command, and image2 is the delta from what happened since the snapshot.
Therefore, image2 is NOT the snapshot. Naming the command
'blockdev-snapshot' is fine, but I think we can have better names for the arguments.

Quoted 7 lines

Better wording might be:

@device: device or node that will have a snapshot created

@overlay: reference to existing block device that will become the overlay of device, as part of creating the snapshot. It must not have a current backing file...

I think you are right. I'll update the descriptions as you suggest.

The problem is that, incorrectly or not, we are already using 'snapshot' to refer to the overlays, e.g in BlockdevSnapshotSync:

# @device: #optional the name of the device to generate the snapshot
# from.
# @node-name: #optional graph node name to generate the snapshot from
# (Since 2.0)
# @snapshot-file: the target of the new image. A new file will be
# created.
# @snapshot-node-name: #optional the graph node name of the new image
# (Since 2.0)

I'll wait a bit in case someone wants to say something about the names of the parameters.

Berto

#11 lore raw

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

Am 22.09.2015 um 11:21 hat Alberto Garcia geschrieben:

Quoted 9 lines · Eric Blake
Quoted 6 lines

qapi/block-core.json line 710

Quoted 4 lines
qapi/block-core.json +3 -0
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.

I'm still wondering if 'node' is a better name than 'device' here.

I don't have a strong preference. Kevin, Max, ... any opinions?

If I had to choose, I think I would go with 'node' indeed.

Quoted 26 lines
Quoted 26 lines

qapi/block-core.json line 715

Quoted 5 lines
qapi/block-core.json +5 -0
+#
+# @snapshot: reference to the existing block device that will be used
+#            for the snapshot. It must not have a current backing file
+#            (this can be achieved by passing "backing": "" to
+#            blockdev-add).

Possibly confusing terminology.

Let's consider: the act of creating a snapshot says to go from:

image1 [read-write]

to

image1 [read-only] <- image2 [read-write]

that is, image1 is now the snapshot of the state in time we executed the command, and image2 is the delta from what happened since the snapshot.
Therefore, image2 is NOT the snapshot. Naming the command
'blockdev-snapshot' is fine, but I think we can have better names for the arguments.

Better wording might be:

@device: device or node that will have a snapshot created

Quoted 8 lines
Quoted 3 lines

@overlay: reference to existing block device that will become the overlay of device, as part of creating the snapshot. It must not have a current backing file...

I think you are right. I'll update the descriptions as you suggest.

The problem is that, incorrectly or not, we are already using 'snapshot' to refer to the overlays, e.g in BlockdevSnapshotSync:

Let's not worry too much about legacy commands. We know that we made some decisions in the past that we wouldn't make today. As long as we don't introduce new commands, it makes little sense to clean them up, as the requirement to maintain compatibility would result in an even worse API in the end. But for new commands, we can choose whatever we think makes most sense in the future.

Kevin

#12 lore raw

PATCH v4 4/4 block: add tests for the 'blockdev-snapshot' command

In reply to Alberto Garcia
---
 tests/qemu-iotests/085     | 102 ++++++++++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/085.out |  34 ++++++++++++++-
 2 files changed, 128 insertions(+), 8 deletions(-)
tests/qemu-iotests/085 +96 -6
index 56cd6f8..532eb0e 100755
@@ -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': { '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 +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': { '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} 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': { 'device':'nodevice',
+                                    'snapshot':'snap_"${SNAPSHOTS}"' }
+                   }" "error"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
tests/qemu-iotests/085.out +32 -2
index 5eb8b94..2238f6d 100644
@@ -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
Signature · 1 line
2.5.1
#13 lore raw

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

Quoted 5 lines · Alberto Garcia
---
 tests/qemu-iotests/085     | 102 ++++++++++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/085.out |  34 ++++++++++++++-
 2 files changed, 128 insertions(+), 8 deletions(-)

Looks good, but the output diff needs a rebase due to fe646693.

Max