Skip to contentSkip to conversation

Add 'blockdev-snapshot' command

PATCH Superseded Alberto Garcia 4 participants 18 messages Sep 10, 2015 – Sep 14, 2015

v3 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v3 cover Add 'blockdev-snapshot' command

Hi,

here's version 3 of the patchset that adds the 'blockdev-snapshot' QMP command. This one has a couple of important fixes plus some of the corrections suggested by Eric.

The most controversial change, I believe, is the addition of the 'ignore-backing' field to BlockdevOptionsGenericCOWFormat. This allows opening an image using 'blockdev-add' but not its backing chain. I expect that this will generate some debate so decided to go for a simple solution that would allow me to finish the rest of the series, but I'm of course open to go for an alternative API/solution.

Regards,

Berto

v3:
- Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat. This allows opening images but not their backing images.
- Check for op blockers in the snapshot node and make sure that it doesn't have any backing image.
- Remove extra check for the existence of the snapshot node:
bdrv_open() already does that.
- Extend iotest 085 to add tests for 'blockdev-snapshot'.
- Replace local_err with errp in some places where the former is unnecessary.
- Update command description.
- Add 'since' tag to the 'blockdev-snapshot' field in TransactionAction.

v2: https://lists.gnu.org/archive/html/qemu-block/2015-09/msg00094.html
- Add 'blockdev-snapshot' command instead of allowing passing options to 'blockdev-snapshot-sync'.
- Rename BlockdevSnapshot to BlockdevSnapshotSync

v1: https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00236.html

Alberto Garcia (4):
block: rename BlockdevSnapshot to BlockdevSnapshotSync
block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat
block: add a 'blockdev-snapshot' QMP command
block: add tests for the 'blockdev-snapshot' command

 block.c                    |   5 ++
 blockdev.c                 | 165 ++++++++++++++++++++++++++++-----------------
 qapi-schema.json           |   4 +-
 qapi/block-core.json       |  38 +++++++++--
 qmp-commands.hx            |  29 ++++++++
 tests/qemu-iotests/085     |  97 ++++++++++++++++++++++++--
 tests/qemu-iotests/085.out |  34 +++++++++-
 7 files changed, 298 insertions(+), 74 deletions(-)
Signature · 1 line
2.5.1
#1 lore raw

PATCH v3 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 v3 1/4
#2 lore raw

Re: [Qemu-devel] [PATCH v3 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 v3 2/4 block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

In reply to Alberto Garcia

If set to true, the image will be opened with the BDRV_O_NO_BACKING flag. This is useful for creating snapshots using images opened with blockdev-add, since they are not supposed to have a backing image before the operation.

---
 block.c              | 5 +++++
 qapi/block-core.json | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)
block.c +5 -0
index 22d3b0e..4be32fb 100644
@@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
 
     assert(drvname || !(flags & BDRV_O_PROTOCOL));
 
+    if (qdict_get_try_bool(options, "ignore-backing", false)) {
+        flags |= BDRV_O_NO_BACKING;
+    }
+    qdict_del(options, "ignore-backing");
+
     bs->open_flags = flags;
     bs->options = options;
     options = qdict_clone_shallow(options);
qapi/block-core.json +5 -1
index ec50f06..0f797d7 100644
@@ -1498,11 +1498,15 @@
 #               allowed to pass an empty string here in order to disable the
 #               default backing file.
 #
+# @ignore-backing: #optional if true, no backing file will be
+#                  opened. Defaults to false (Since 2.5)
+#
 # Since: 1.7
 ##
 { 'struct': 'BlockdevOptionsGenericCOWFormat',
   'base': 'BlockdevOptionsGenericFormat',
-  'data': { '*backing': 'BlockdevRef' } }
+  'data': { '*backing': 'BlockdevRef',
+            '*ignore-backing': 'bool' } }
 
 ##
 # @Qcow2OverlapCheckMode
Signature · 1 line
2.5.1
#4 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Quoted 10 lines · Alberto Garcia

If set to true, the image will be opened with the BDRV_O_NO_BACKING flag. This is useful for creating snapshots using images opened with blockdev-add, since they are not supposed to have a backing image before the operation.

---
 block.c              | 5 +++++
 qapi/block-core.json | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty string in order to achieve exactly that.

So why do we need the new flag? Because "backing: ''" is ugly?

Max

#5 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:

Quoted 12 lines · Alberto Garcia
Quoted 10 lines

If set to true, the image will be opened with the BDRV_O_NO_BACKING flag. This is useful for creating snapshots using images opened with blockdev-add, since they are not supposed to have a backing image before the operation.

---
 block.c              | 5 +++++
 qapi/block-core.json | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)
Quoted 5 lines

Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty string in order to achieve exactly that.

So why do we need the new flag? Because "backing: ''" is ugly?

I guess it's just because you're the only one who actually reads the documentation. When discussing this, I didn't remember that we already had a way to express this (an additional bool wouldn't have been my favourite solution anyway). Thanks for catching this.

Kevin

#6 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Quoted 17 lines · Kevin Wolf

Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:

Quoted 16 lines · Alberto Garcia
Quoted 10 lines

If set to true, the image will be opened with the BDRV_O_NO_BACKING flag. This is useful for creating snapshots using images opened with blockdev-add, since they are not supposed to have a backing image before the operation.

---
 block.c              | 5 +++++
 qapi/block-core.json | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty string in order to achieve exactly that.

Quoted 6 lines
Quoted 1 line

So why do we need the new flag? Because "backing: ''" is ugly?

I guess it's just because you're the only one who actually reads the documentation. When discussing this, I didn't remember that we already had a way to express this (an additional bool wouldn't have been my favourite solution anyway). Thanks for catching this.

I read the patch, it was part of the context. ;-)

Max

#7 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Quoted 8 lines · Max Reitz
Quoted 6 lines
Quoted 1 line

So why do we need the new flag? Because "backing: ''" is ugly?

I guess it's just because you're the only one who actually reads the documentation. When discussing this, I didn't remember that we already had a way to express this (an additional bool wouldn't have been my favourite solution anyway). Thanks for catching this.

I read the patch, it was part of the context. ;-)

Oh, that was embarrassing :-) Yes, it was the discussion from two weeks ago about passing empty strings as BlockdevRef that made me think that this would be ugly.

Anyway, was this ever implemented? It seems that passing a string to the 'backing' parameter is only specified in the JSON schema, but no one actually uses that.

So I'll implement that for the next version of my series.

Berto

#8 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Am 14.09.2015 um 07:54 hat Alberto Garcia geschrieben:

Quoted 15 lines · Max Reitz
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines

So why do we need the new flag? Because "backing: ''" is ugly?

I guess it's just because you're the only one who actually reads the documentation. When discussing this, I didn't remember that we already had a way to express this (an additional bool wouldn't have been my favourite solution anyway). Thanks for catching this.

I read the patch, it was part of the context. ;-)

Oh, that was embarrassing :-) Yes, it was the discussion from two weeks ago about passing empty strings as BlockdevRef that made me think that this would be ugly.

Quoted 5 lines

Anyway, was this ever implemented? It seems that passing a string to the 'backing' parameter is only specified in the JSON schema, but no one actually uses that.

So I'll implement that for the next version of my series.

I have a patch that actually allows passing a node-name reference as a string here. v1 was posted a few months ago; it just turned out that I need to kill bdrv_swap() before that can work because bdrv_swap() doesn't work with nodes that have a BlockBackend attached.

Of course, I didn't check what an empty string would do with my patch...

Kevin

#9 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Quoted 10 lines · Kevin Wolf
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

So why do we need the new flag? Because "backing: ''" is ugly?

Anyway, was this ever implemented? It seems that passing a string to the 'backing' parameter is only specified in the JSON schema, but no one actually uses that.

I have a patch that actually allows passing a node-name reference as a string here. v1 was posted a few months ago; it just turned out that I need to kill bdrv_swap() before that can work because bdrv_swap() doesn't work with nodes that have a BlockBackend attached.

But there was no way to open a BDS without having a BlockBackend attached, or was there? It's possible with Max's "BlockBackend and media" series, that's why I am using it.

Berto

Reviewed-by on v3 2/4
#10 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

In reply to Alberto Garcia
Quoted 17 lines · Alberto Garcia

If set to true, the image will be opened with the BDRV_O_NO_BACKING flag. This is useful for creating snapshots using images opened with blockdev-add, since they are not supposed to have a backing image before the operation.

---
 block.c              | 5 +++++
 qapi/block-core.json | 6 +++++-
 2 files changed, 10 insertions(+), 1 deletion(-)
block.c +0 -0
index 22d3b0e..4be32fb 100644

block.c static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, · line 1475

Quoted 7 lines
block.c +4 -0
@@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
 
     assert(drvname || !(flags & BDRV_O_PROTOCOL));
 
+    if (qdict_get_try_bool(options, "ignore-backing", false)) {
+        flags |= BDRV_O_NO_BACKING;
+    }
+    qdict_del(options, "ignore-backing");

What happens if the user specified "ignore-backing":true, "backing":...?
Should that be a hard error?

block.c static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, · line 1479

Quoted 5 lines
block.c +2 -1
 { 'struct': 'BlockdevOptionsGenericCOWFormat',
   'base': 'BlockdevOptionsGenericFormat',
-  'data': { '*backing': 'BlockdevRef' } }
+  'data': { '*backing': 'BlockdevRef',
+            '*ignore-backing': 'bool' } }

Depending on whether the answer to my question is that we already behave sanely and don't leave a BlockdevRef dangling if the caller mixes the two approaches, then:

But design-wise, would it make sense to support:

"backing":null

as an explicit request to not open a backing file? Right now, qapi does not have a way to express 'null' as part of an alternate type; but if it did, BlockdevRef would merely add 'null' as one of its allowed alternates. Then we wouldn't need ignore-backing from the QMP perspective. But I'm still not sure how it would map to the command line perspective.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

Quoted 3 lines · Eric Blake

But design-wise, would it make sense to support:

"backing":null

Just read Max's response; it sounds like we already have "backing":"" (and don't need "backing":null) for what we want. So maybe we don't need this patch after all.

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

PATCH v3 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 |  26 ++++++++
 qmp-commands.hx      |  29 +++++++++
 4 files changed, 160 insertions(+), 60 deletions(-)
blockdev.c +103 -60
index 6b787c1..78cfb79 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 existing");
+            return;
+        }
+
+        flags = state->old_bs->open_flags;
+
+        /* create new image w/backing file */
+        mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+        if (mode != NEW_IMAGE_MODE_EXISTING) {
+            bdrv_img_create(new_image_file, format,
+                            state->old_bs->filename,
+                            state->old_bs->drv->format_name,
+                            NULL, -1, flags, &local_err, false);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+
+        options = qdict_new();
+        if (s->has_snapshot_node_name) {
+            qdict_put(options, "node-name",
+                      qstring_from_str(snapshot_node_name));
+        }
+        qdict_put(options, "driver", qstring_from_str(format));
+
+        flags |= BDRV_O_NO_BACKING;
     }
-    qdict_put(options, "driver", qstring_from_str(format));
 
-    /* TODO Inherit bs->options or only take explicit options with an
-     * extended QMP command? */
     assert(state->new_bs == NULL);
-    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
-                    flags | BDRV_O_NO_BACKING, &local_err);
+    ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
+                    flags, errp);
     /* We will manually add the backing_hd field to the bs later */
     if (ret != 0) {
-        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (state->new_bs->blk != NULL) {
+        error_setg(errp, "The snapshot is already in use by %s",
+                   blk_name(state->new_bs->blk));
+        return;
+    }
+
+    if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+                           errp)) {
+        return;
+    }
+
+    if (state->new_bs->backing_hd != NULL) {
+        error_setg(errp, "The snapshot already has a backing image");
     }
 }
 
@@ -1818,6 +1855,12 @@ static void abort_commit(BlkTransactionState *common)
 }
 
 static const BdrvActionOps actions[] = {
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
+        .instance_size = sizeof(ExternalSnapshotState),
+        .prepare  = external_snapshot_prepare,
+        .commit   = external_snapshot_commit,
+        .abort = external_snapshot_abort,
+    },
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
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 +26 -0
index 0f797d7..7d05166 100644
@@ -705,6 +705,19 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.
+#
+# @snapshot: reference to the existing block device that will be used
+#            for the snapshot.
+#
+# Since 2.5
+##
+{ 'struct': 'BlockdevSnapshot',
+  'data': { 'device': 'str', 'snapshot': 'str' } }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -800,6 +813,19 @@
 { 'command': 'blockdev-snapshot-sync',
   'data': 'BlockdevSnapshotSync' }
 
+
+##
+# @blockdev-snapshot
+#
+# Generates a snapshot of a block device.
+#
+# For the arguments, see the documentation of BlockdevSnapshot.
+#
+# Since 2.5
+##
+{ 'command': 'blockdev-snapshot',
+  'data': 'BlockdevSnapshot' }
+
 ##
 # @change-backing-file
 #
qmp-commands.hx +29 -0
index 495670b..05a5675 100644
@@ -1454,6 +1454,35 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot",
+        .args_type  = "device:s,snapshot:s",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot,
+    },
+
+SQMP
+blockdev-snapshot
+-----------------
+Since 2.5
+
+Create a snapshot, by installing 'device' as the backing image of
+'snapshot'. Additionally, if 'device' is associated with a block
+device, the block device changes to using 'snapshot' as its new active
+image.
+
+Arguments:
+
+- "device": snapshot source (json-string)
+- "snapshot": snapshot target (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
+                                                    "snapshot": "node1534" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-internal-sync",
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
Signature · 1 line
2.5.1
Reviewed-by on v3 3/4
#13 lore raw

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

Quoted 29 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.

---
 blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  26 ++++++++
 qmp-commands.hx      |  29 +++++++++
 4 files changed, 160 insertions(+), 60 deletions(-)
blockdev.c +0 -0
index 6b787c1..78cfb79 100644

blockdev.c void qmp_blockdev_snapshot_sync(bool has_device, const char *device, · line 1192

Quoted 10 lines
blockdev.c +7 -0
@@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot(const char *device, const char *snapshot,
+                           Error **errp)
+{
+    BlockdevSnapshot snapshot_data = {
+        .device = (char *) device,
+        .snapshot = (char *) snapshot
+    };

Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to make this function have the saner signature of

void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);

rather than having to rebuild the struct yourself (with an annoying cast to lose const) :)
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html

But no need to hold this series up waiting for the qapi review queue to flush. We can simplify later.

blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1620

Quoted 7 lines
blockdev.c +2 -4
-    options = qdict_new();
-    if (has_snapshot_node_name) {
-        qdict_put(options, "node-name",
-                  qstring_from_str(snapshot_node_name));
+        if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
+            error_setg(errp, "New snapshot node name already existing");

Pre-existing, but s/existing/exists/ while you are reindenting this.

blockdev.c line 713

Quoted 11 lines

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

blockdev.c +6 -0
@@ -705,6 +705,19 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.
+#
+# @snapshot: reference to the existing block device that will be used
+#            for the snapshot.

Maybe mention that it must NOT have a current backing file, and point to the "backing":"" trick to get it that way.

blockdev.c line 717

Quoted 4 lines
blockdev.c +4 -0
+Create a snapshot, by installing 'device' as the backing image of
+'snapshot'. Additionally, if 'device' is associated with a block
+device, the block device changes to using 'snapshot' as its new active
+image.

Still didn't answer the question from the earlier review of whether the blockdev-snapshot-sync behavior of specifying the node name of an active layer in order to not pivot the block device to the snapshot still makes sense to support in the blockdev-snapshot case. But we could always add an optional boolean flag later if someone comes up for a use case where they'd need to create a snapshot of the active layer without the block device pivoting, so I don't think it should hold up this patch.

blockdev.c

Quoted 11 lines
blockdev.c +11 -0
+
+Arguments:
+
+- "device": snapshot source (json-string)
+- "snapshot": snapshot target (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
+                                                    "snapshot": "node1534" } }
+<- { "return": {} }

Maybe enhance the example to show the preliminary blockdev-add that created node1534?

I've pointed out some potential wording improvements, but think they are minor enough that you can have:

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

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

Quoted 11 lines · Eric Blake

blockdev.c void qmp_blockdev_snapshot_sync(bool has_device, const char *device, · line 1192

Quoted 7 lines
blockdev.c +7 -0
+void qmp_blockdev_snapshot(const char *device, const char *snapshot,
+                           Error **errp)
+{
+    BlockdevSnapshot snapshot_data = {
+        .device = (char *) device,
+        .snapshot = (char *) snapshot
+    };

Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to make this function have the saner signature of

Quoted 5 lines

void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);

rather than having to rebuild the struct yourself (with an annoying cast to lose const) :)
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html

That's cool! We can simplify this later as you suggest.

Quoted 9 lines

qmp-commands.hx Example:

Quoted 4 lines
qmp-commands.hx +4 -0
+Create a snapshot, by installing 'device' as the backing image of
+'snapshot'. Additionally, if 'device' is associated with a block
+device, the block device changes to using 'snapshot' as its new
+active image.

Still didn't answer the question from the earlier review of whether the blockdev-snapshot-sync behavior of specifying the node name of an active layer in order to not pivot the block device to the snapshot still makes sense to support in the blockdev-snapshot case.

Sorry if I overlooked the explanation, but I still didn't get the use case for this.

And particularly for this command, that doesn't create any image but simply uses a previously-opened one.

What would 'pivot': false do ? As far as I can see, specifying the node name of an active layer in blockdev-snapshot-sync seems to pivot the block device to the snapshot.

{ "execute": "blockdev-snapshot-sync",

             "arguments": { "node-name": "hd0",
                            "snapshot-file": "hd1.qcow2",
                            "snapshot-node-name": "hd1" } }

(qemu) info block
virtio0 (hd1): hd1.qcow2 (qcow2)
Cache mode: writeback
Backing file: hd0.qcow2 (chain depth: 1)

Berto

Reviewed-by on v3 3/4
#15 lore raw

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

In reply to Alberto Garcia
Quoted 16 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 7 lines
---
 blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  26 ++++++++
 qmp-commands.hx      |  29 +++++++++
 4 files changed, 160 insertions(+), 60 deletions(-)
#16 lore raw

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

In reply to Alberto Garcia
---
 tests/qemu-iotests/085     | 97 +++++++++++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/085.out | 34 +++++++++++++++-
 2 files changed, 123 insertions(+), 8 deletions(-)
tests/qemu-iotests/085 +91 -6
index 56cd6f8..2b0f85a 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,45 @@ function create_group_snapshot()
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
+# ${1}: unique identifier for the snapshot filename
+# ${2}: true: ignore backing images (default); false: don't ignore them
+function add_snapshot_image()
+{
+    base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
+    snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
+    _make_test_img -b "${base_image}" "$size"
+    mv "${TEST_IMG}" "${snapshot_file}"
+    cmd="{ 'execute': 'blockdev-add', 'arguments':
+           { 'options':
+             { 'driver': 'qcow2', 'node-name': 'snap_"${1}"',
+               'ignore-backing': "${2:-true}", 'file':
+               { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
+    _send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
+# ${2}: expected response, defaults to 'return'
+function blockdev_snapshot()
+{
+    cmd="{ 'execute': 'blockdev-snapshot',
+                      'arguments': { 'device': 'virtio0',
+                                     'snapshot':'snap_"${1}"' } }"
+    _send_qemu_cmd $h "${cmd}" "${2:-return}"
+}
+
 size=128M
 
 _make_test_img $size
-mv "${TEST_IMG}" "${TEST_IMG}.orig"
+mv "${TEST_IMG}" "${TEST_IMG}.1"
 _make_test_img $size
+mv "${TEST_IMG}" "${TEST_IMG}.2"
 
 echo
 echo === Running QEMU ===
 echo
 
 qemu_comm_method="qmp"
-_launch_qemu -drive file="${TEST_IMG}.orig",if=virtio -drive file="${TEST_IMG}",if=virtio
+_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
 h=$QEMU_HANDLE
 
 echo
@@ -105,6 +133,8 @@ echo
 
 _send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
 
+# Tests for the blockdev-snapshot-sync command
+
 echo
 echo === Create a single snapshot on virtio0 ===
 echo
@@ -132,11 +162,66 @@ echo
 echo === Create several transactional group snapshots ===
 echo
 
-for i in $(seq 2 ${MAX_SNAPSHOTS})
+for i in $(seq 2 ${SNAPSHOTS})
 do
     create_group_snapshot ${i}
 done
 
+# Tests for the blockdev-snapshot command
+
+echo
+echo === Create a couple of snapshots using blockdev-snapshot ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+blockdev_snapshot ${SNAPSHOTS}
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+blockdev_snapshot ${SNAPSHOTS}
+
+echo
+echo === Invalid command - snapshot node used as active layer ===
+echo
+
+blockdev_snapshot ${SNAPSHOTS} error
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'device':'virtio0',
+                                    'snapshot':'virtio0' }
+                   }" "error"
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'device':'virtio0',
+                                    'snapshot':'virtio1' }
+                   }" "error"
+
+echo
+echo === Invalid command - snapshot node used as backing hd ===
+echo
+
+blockdev_snapshot $((${SNAPSHOTS}-1)) error
+
+echo
+echo === Invalid command - snapshot node has a backing image ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS} false
+blockdev_snapshot ${SNAPSHOTS} error
+
+echo
+echo === Invalid command - The node does not exist ===
+echo
+
+blockdev_snapshot $((${SNAPSHOTS}+1)) error
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'device':'nodevice',
+                                    'snapshot':'snap_"${SNAPSHOTS}"' }
+                   }" "error"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
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
#17 lore raw

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

Quoted 10 lines · Alberto Garcia
---
 tests/qemu-iotests/085     | 97 +++++++++++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/085.out | 34 +++++++++++++++-
 2 files changed, 123 insertions(+), 8 deletions(-)
tests/qemu-iotests/085 +2 -0
+# ${1}: unique identifier for the snapshot filename
+# ${2}: true: ignore backing images (default); false: don't ignore them

tests/qemu-iotests/085 function create_group_snapshot() · line 100

Quoted 10 lines
tests/qemu-iotests/085 +10 -0
+function add_snapshot_image()
+{
+    base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
+    snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
+    _make_test_img -b "${base_image}" "$size"
+    mv "${TEST_IMG}" "${snapshot_file}"
+    cmd="{ 'execute': 'blockdev-add', 'arguments':
+           { 'options':
+             { 'driver': 'qcow2', 'node-name': 'snap_"${1}"',
+               'ignore-backing': "${2:-true}", 'file':

Needs to be reworked to use 'backing':'' instead of 'ignore-backing'.

But overall looks like a sane set of tests to cover both positive and negative expected behavior.

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