[Qemu-devel] [PATCH v2 0/2] blockdev: Overlays are not snapshots

[Qemu-devel] [PATCH v2 0/2] blockdev: Overlays are not snapshots

[Max Reitz]

QEMU’s always been confused over what a snapshot is: Is it the overlay?
Is it the backing image?

Confusion is rarely a good thing.  I can’t think of any objective reason
why the overlay would be a snapshot.  A snapshot is something that does
not change over time; the overlay does.

(I suppose historically the reason is that “Taking an overlay” makes no
sense, so the operations are called “Taking a snapshot”.  Somehow, this
meaning carried over to the new file that is created during that
operation; if “Creating a snapshot” creates a file, that file must be
the snapshot, right?  Well, no, it isn’t.)

Let’s fix this as best as we can.  Better Nate than lever.


v2:
- Don’t break the iotests for a change
  (kept Eric’s R-b, because it felt like the right thing to do)


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[----] [--] 'qapi/block-core: Overlays are not snapshots'
002/2:[0010] [FC] 'blockdev: Overlays are not snapshots'


Max Reitz (2):
  qapi/block-core: Overlays are not snapshots
  blockdev: Overlays are not snapshots

 qapi/block-core.json       | 20 ++++++++++----------
 blockdev.c                 | 10 +++++-----
 tests/qemu-iotests/085.out | 10 +++++-----
 3 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] qapi/block-core: Overlays are not snapshots

[Max Reitz]

A snapshot is something that reflects the state of something at a
certain point in time.  It does not change.

The file our snapshot commands create (or the node they install) is not
a snapshot, as it does change over time.  It is an overlay.  We cannot
do anything about the parameter names, but we can at least adjust the
descriptions to reflect that fact.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1defcde048..df52a90736 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1279,17 +1279,17 @@
 #
 # Either @device or @node-name must be set but not both.
 #
-# @device: the name of the device to generate the snapshot from.
+# @device: the name of the device to take a snapshot of.
 #
 # @node-name: graph node name to generate the snapshot from (Since 2.0)
 #
-# @snapshot-file: the target of the new image. If the file exists, or
-# if it is a device, the snapshot will be created in the existing
-# file/device. Otherwise, a new file will be created.
+# @snapshot-file: the target of the new overlay image. If the file
+# exists, or if it is a device, the overlay will be created in the
+# existing file/device. Otherwise, a new file will be created.
 #
 # @snapshot-node-name: the graph node name of the new image (Since 2.0)
 #
-# @format: the format of the snapshot image, default is 'qcow2'.
+# @format: the format of the overlay image, default is 'qcow2'.
 #
 # @mode: whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
@@ -1302,10 +1302,10 @@
 ##
 # @BlockdevSnapshot:
 #
-# @node: device or node name that will have a snapshot created.
+# @node: device or node name that will have a snapshot taken.
 #
 # @overlay: reference to the existing block device that will become
-#           the overlay of @node, as part of creating the snapshot.
+#           the overlay of @node, as part of taking the snapshot.
 #           It must not have a current backing file (this can be
 #           achieved by passing "backing": null to blockdev-add).
 #
@@ -1443,7 +1443,7 @@
 ##
 # @blockdev-snapshot-sync:
 #
-# Generates a synchronous snapshot of a block device.
+# Takes a synchronous snapshot of a block device.
 #
 # For the arguments, see the documentation of BlockdevSnapshotSync.
 #
@@ -1469,9 +1469,9 @@
 ##
 # @blockdev-snapshot:
 #
-# Generates a snapshot of a block device.
+# Takes a snapshot of a block device.
 #
-# Create a snapshot, by installing 'node' as the backing image of
+# Take 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.
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/2] blockdev: Overlays are not snapshots

[Max Reitz]

There are error messages which refer to an overlay node as the snapshot.
That is wrong, those are two different things.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c                 | 10 +++++-----
 tests/qemu-iotests/085.out | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6963270108..7de0b04fe7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1608,13 +1608,13 @@ static void external_snapshot_prepare(BlkActionState *common,
             s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
 
         if (node_name && !snapshot_node_name) {
-            error_setg(errp, "New snapshot node name missing");
+            error_setg(errp, "New overlay node name missing");
             goto out;
         }
 
         if (snapshot_node_name &&
             bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-            error_setg(errp, "New snapshot node name already in use");
+            error_setg(errp, "New overlay node name already in use");
             goto out;
         }
 
@@ -1656,7 +1656,7 @@ static void external_snapshot_prepare(BlkActionState *common,
     }
 
     if (bdrv_has_blk(state->new_bs)) {
-        error_setg(errp, "The snapshot is already in use");
+        error_setg(errp, "The overlay is already in use");
         goto out;
     }
 
@@ -1666,12 +1666,12 @@ static void external_snapshot_prepare(BlkActionState *common,
     }
 
     if (state->new_bs->backing != NULL) {
-        error_setg(errp, "The snapshot already has a backing image");
+        error_setg(errp, "The overlay already has a backing image");
         goto out;
     }
 
     if (!state->new_bs->drv->supports_backing) {
-        error_setg(errp, "The snapshot does not support backing images");
+        error_setg(errp, "The overlay does not support backing images");
         goto out;
     }
 
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 6edf107f55..2a5f256cd3 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -64,13 +64,13 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 
 === Invalid command - cannot create a snapshot using a file BDS ===
 
-{"error": {"class": "GenericError", "desc": "The snapshot does not support backing images"}}
+{"error": {"class": "GenericError", "desc": "The overlay does not support backing images"}}
 
 === Invalid command - snapshot node used as active layer ===
 
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}
+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}
+{"error": {"class": "GenericError", "desc": "The overlay is already in use"}}
 
 === Invalid command - snapshot node used as backing hd ===
 
@@ -81,7 +81,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "The snapshot already has a backing image"}}
+{"error": {"class": "GenericError", "desc": "The overlay already has a backing image"}}
 
 === Invalid command - The node does not exist ===
 
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/2] blockdev: Overlays are not snapshots

[John Snow]

On 6/3/19 4:22 PM, Max Reitz wrote:
> QEMU’s always been confused over what a snapshot is: Is it the overlay?
> Is it the backing image?
> 
> Confusion is rarely a good thing.  I can’t think of any objective reason
> why the overlay would be a snapshot.  A snapshot is something that does
> not change over time; the overlay does.
> 
> (I suppose historically the reason is that “Taking an overlay” makes no
> sense, so the operations are called “Taking a snapshot”.  Somehow, this
> meaning carried over to the new file that is created during that
> operation; if “Creating a snapshot” creates a file, that file must be
> the snapshot, right?  Well, no, it isn’t.)
> 
> Let’s fix this as best as we can.  Better Nate than lever.
> 
> 
> v2:
> - Don’t break the iotests for a change
>   (kept Eric’s R-b, because it felt like the right thing to do)
> 
> 
> git backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/2:[----] [--] 'qapi/block-core: Overlays are not snapshots'
> 002/2:[0010] [FC] 'blockdev: Overlays are not snapshots'
> 
> 
> Max Reitz (2):
>   qapi/block-core: Overlays are not snapshots
>   blockdev: Overlays are not snapshots
> 
>  qapi/block-core.json       | 20 ++++++++++----------
>  blockdev.c                 | 10 +++++-----
>  tests/qemu-iotests/085.out | 10 +++++-----
>  3 files changed, 20 insertions(+), 20 deletions(-)
> 

Makes good sense to me.

There are only 3,283 things named "snapshot" in QEMU so one less is
probably not the worst.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/2] blockdev: Overlays are not snapshots

[Alberto Garcia]

On Mon 03 Jun 2019 10:22:34 PM CEST, Max Reitz wrote:
> QEMU’s always been confused over what a snapshot is: Is it the overlay?
> Is it the backing image?
>
> Confusion is rarely a good thing.  I can’t think of any objective reason
> why the overlay would be a snapshot.  A snapshot is something that does
> not change over time; the overlay does.
>
> (I suppose historically the reason is that “Taking an overlay” makes no
> sense, so the operations are called “Taking a snapshot”.  Somehow, this
> meaning carried over to the new file that is created during that
> operation; if “Creating a snapshot” creates a file, that file must be
> the snapshot, right?  Well, no, it isn’t.)
>
> Let’s fix this as best as we can.  Better Nate than lever.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 1/2] qapi/block-core: Overlays are not snapshots

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> A snapshot is something that reflects the state of something at a
> certain point in time.  It does not change.
>
> The file our snapshot commands create (or the node they install) is not
> a snapshot, as it does change over time.  It is an overlay.  We cannot
> do anything about the parameter names,

We certainly could: add new parameter, default to the old one, error out
when both are given, deprecate the old one.  Way more trouble than it's
worth.

If the QAPI machinery made such renames as simple as

    'overlay-file': {'type': 'str', 'alias': 'snapshot-file'}

we could consider it.  Of course, whether enhancing the machinery that
way would be worthwhile depends on complexity and on use.  I think we
got bigger fish to fry.

>                                        but we can at least adjust the
> descriptions to reflect that fact.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Since you already got competent review, I merely glanced at the patches,
and didn't check for completeness.  No objections.

Re: [Qemu-devel] [PATCH v2 0/2] blockdev: Overlays are not snapshots

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 805 bytes --]

On 03.06.19 22:22, Max Reitz wrote:
> QEMU’s always been confused over what a snapshot is: Is it the overlay?
> Is it the backing image?
> 
> Confusion is rarely a good thing.  I can’t think of any objective reason
> why the overlay would be a snapshot.  A snapshot is something that does
> not change over time; the overlay does.
> 
> (I suppose historically the reason is that “Taking an overlay” makes no
> sense, so the operations are called “Taking a snapshot”.  Somehow, this
> meaning carried over to the new file that is created during that
> operation; if “Creating a snapshot” creates a file, that file must be
> the snapshot, right?  Well, no, it isn’t.)
> 
> Let’s fix this as best as we can.  Better Nate than lever.

Applied to my block branch.

Max


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