[Qemu-devel] [PULL 03/71] block/snapshot.c: eliminate use of ID input in snapshot operations

[Kevin Wolf <kwolf@redhat.com>]

From: Daniel Henrique Barboza <danielhb413@gmail.com>

At this moment, QEMU attempts to create/load/delete snapshots
by using either an ID (id_str) or a name. The problem is that the code
isn't consistent of whether the entered argument is an ID or a name,
causing unexpected behaviors.

For example, when creating snapshots via savevm <arg>, what happens is that
"arg" is treated as both name and id_str. In a guest without snapshots, create
a single snapshot via savevm:

(qemu) savevm 0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                 VM SIZE                DATE       VM CLOCK
--        0                      741M 2018-07-31 13:39:56   00:41:25.313

A snapshot with name "0" is created. ID is hidden from the user, but the
ID is a non-zero integer that starts at "1". Thus, this snapshot has
id_str=1, TAG="0". Creating a second snapshot with arg = 1, the first one
is deleted:

(qemu) savevm 1
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                 VM SIZE                DATE       VM CLOCK
--        1                      741M 2018-07-31 13:42:14   00:41:55.252

What happened?

- when creating the second snapshot, a verification is done inside
bdrv_all_delete_snapshot to delete any existing snapshots that matches an
string argument. Here, the code calls bdrv_all_delete_snapshot("1", ...);

- bdrv_all_delete_snapshot calls bdrv_snapshot_find(..., "1") for each
BlockDriverState of the guest. And this is where things goes tilting:
bdrv_snapshot_find does a search by both id_str and name. It finds
out that there is a snapshot that has id_str = 1, stores a reference
to the snapshot in the sn_info pointer and then returns match found;

- since a match was found, a call to bdrv_snapshot_delete_by_id_or_name() is
made. This function ignores the pointer written by bdrv_snapshot_find. Instead,
it deletes the snapshot using bdrv_snapshot_delete() calling it first with
id_str = 1. If it fails to delete, then it calls it again with name = 1.

- after all that, QEMU creates the new snapshot, that has id_str = 1 and
name = 1. The user is left wondering that happened with the first snapshot
created. Similar bugs can be triggered when using loadvm and delvm.

Before contemplating discarding the use of ID input in these operations,
I've searched the code of what would be the implications. My findings
are:

- the RBD and Sheepdog drivers don't care. Both uses the 'name' field as
key in their logic, making id_str = name when appropriate.
replay-snapshot.c does not make any special use of id_str;

- qcow2 uses id_str as an unique identifier but it is automatically
calculated, not being influenced by user input. Other than that, there are
no distinguish operations made only with id_str;

- in blockdev.c, the delete operation uses a match of both id_str AND
name. Given that id_str is either a copy of 'name' or auto-generated,
we're fine here.

This gives motivation to not consider ID as a valid user input in HMP
commands - sticking with 'name' input only is more consistent. To
accomplish that, the following changes were made in this patch:

- bdrv_snapshot_find() does not match for id_str anymore, only 'name'. The
function is called in save_snapshot(), load_snapshot(), bdrv_all_delete_snapshot()
and bdrv_all_find_snapshot(). This change makes the search function more
predictable and does not change the behavior of any underlying code that uses
these affected functions, which are related to HMP (which is fine) and the
main loop inside vl.c (which doesn't care about it anyways);

- bdrv_all_delete_snapshot() does not call bdrv_snapshot_delete_by_id_or_name
anymore. Instead, it uses the pointer returned by bdrv_snapshot_find to
erase the snapshot with the exact match of id_str an name. This function
is called in save_snapshot and hmp_delvm, thus this change  produces the
intended effect;

- documentation changes to reflect the new behavior. I consider this to
be an API fix instead of an API change - the user was already creating
snapshots using 'name', but now he/she will also enjoy a consistent
behavior.

Ideally we would get rid of the id_str field entirely, but this would have
repercussions on existing snapshots. Another day perhaps.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c |  5 +++--
 hmp-commands.hx  | 32 ++++++++++++++++++++------------
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 3218a542df..e371d2243d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -63,7 +63,7 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     }
     for (i = 0; i < nb_sns; i++) {
         sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+        if (!strcmp(sn->name, name)) {
             *sn_info = *sn;
             ret = 0;
             break;
@@ -448,7 +448,8 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
         aio_context_acquire(ctx);
         if (bdrv_can_snapshot(bs) &&
                 bdrv_snapshot_find(bs, snapshot, name) >= 0) {
-            ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
+            ret = bdrv_snapshot_delete(bs, snapshot->id_str,
+                                       snapshot->name, err);
         }
         aio_context_release(ctx);
         if (ret < 0) {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index ba71558c25..e5fbc2ca59 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -350,49 +350,57 @@ ETEXI
     {
         .name       = "savevm",
         .args_type  = "name:s?",
-        .params     = "[tag|id]",
-        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .params     = "tag",
+        .help       = "save a VM snapshot. If no tag is provided, a new snapshot is created",
         .cmd        = hmp_savevm,
     },
 
 STEXI
-@item savevm [@var{tag}|@var{id}]
+@item savevm @var{tag}
 @findex savevm
 Create a snapshot of the whole virtual machine. If @var{tag} is
 provided, it is used as human readable identifier. If there is already
-a snapshot with the same tag or ID, it is replaced. More info at
+a snapshot with the same tag, it is replaced. More info at
 @ref{vm_snapshots}.
+
+Since 4.0, savevm stopped allowing the snapshot id to be set, accepting
+only @var{tag} as parameter.
 ETEXI
 
     {
         .name       = "loadvm",
         .args_type  = "name:s",
-        .params     = "tag|id",
-        .help       = "restore a VM snapshot from its tag or id",
+        .params     = "tag",
+        .help       = "restore a VM snapshot from its tag",
         .cmd        = hmp_loadvm,
         .command_completion = loadvm_completion,
     },
 
 STEXI
-@item loadvm @var{tag}|@var{id}
+@item loadvm @var{tag}
 @findex loadvm
 Set the whole virtual machine to the snapshot identified by the tag
-@var{tag} or the unique snapshot ID @var{id}.
+@var{tag}.
+
+Since 4.0, loadvm stopped accepting snapshot id as parameter.
 ETEXI
 
     {
         .name       = "delvm",
         .args_type  = "name:s",
-        .params     = "tag|id",
-        .help       = "delete a VM snapshot from its tag or id",
+        .params     = "tag",
+        .help       = "delete a VM snapshot from its tag",
         .cmd        = hmp_delvm,
         .command_completion = delvm_completion,
     },
 
 STEXI
-@item delvm @var{tag}|@var{id}
+@item delvm @var{tag}
 @findex delvm
-Delete the snapshot identified by @var{tag} or @var{id}.
+Delete the snapshot identified by @var{tag}.
+
+Since 4.0, delvm stopped deleting snapshots by snapshot id, accepting
+only @var{tag} as parameter.
 ETEXI
 
     {
-- 
2.20.1