[Qemu-devel] [RFC PATCH v1 0/2] HMP/snapshot changes - do not use ID anymore

[Qemu-devel] [RFC PATCH v1 0/2] HMP/snapshot changes - do not use ID anymore

[Daniel Henrique Barboza]

I am marking the patch series as "RFC" because it was supposed to be
a discussion but, when I was investigating, it turned out to be
easier to send the patches right away.

It is not uncommon to see bugs being opened by testers that attempt to
create VM snapshots using HMP. It turns out that "0" and "1" are quite
common snapshot names and they trigger a lot of bugs. I gave an example
in the commit message of patch 1, but to sum up here: QEMU treats the
input of savevm/loadvm/delvm sometimes as 'ID', sometimes as 'name'. It
is documented as such, but this can lead to strange situations.

Given that it is strange for an API to consider a parameter to be 2 fields
at the same time, and inadvently treating them as one or the other, and
that removing the ID field is too drastic, my idea here is to keep the
ID field for internal control, but do not let the user set it.

I guess there's room for discussion about considering this change an API
change or not. It doesn't affect users of HMP and it doesn't affect Libvirt,
but I am simplifying the meaning of the parameters of savevm/loadvm/delvm.


Daniel Henrique Barboza (2):
  block/snapshot.c: eliminate use of ID input in snapshot operations
  qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call

 block/qcow2-snapshot.c |  5 -----
 block/snapshot.c       |  5 +++--
 hmp-commands.hx        | 20 ++++++++++----------
 3 files changed, 13 insertions(+), 17 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 1/2] block/snapshot.c: eliminate use of ID input in snapshot operations

[Daniel Henrique Barboza]

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. Libvirt does not care about this change either, as far as
I've tested.

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>
---
 block/snapshot.c |  5 +++--
 hmp-commands.hx  | 20 ++++++++++----------
 2 files changed, 13 insertions(+), 12 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 c1fc747403..e55b889868 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -350,17 +350,17 @@ 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}.
 ETEXI
 
@@ -368,31 +368,31 @@ ETEXI
         .name       = "loadvm",
         .args_type  = "name:s",
         .params     = "tag|id",
-        .help       = "restore a VM snapshot from its tag or id",
+        .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}.
 ETEXI
 
     {
         .name       = "delvm",
         .args_type  = "name:s",
         .params     = "tag|id",
-        .help       = "delete a VM snapshot from its tag or id",
+        .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}.
 ETEXI
 
     {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 1/2] block/snapshot.c: eliminate use of ID input in snapshot operations

[Murilo Opsfelder Araujo]

Hi, Daniel.

On Tue, Aug 21, 2018 at 06:00:23PM -0300, Daniel Henrique Barboza wrote:
> 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. Libvirt does not care about this change either, as far as
> I've tested.
> 
> 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>
> ---
>  block/snapshot.c |  5 +++--
>  hmp-commands.hx  | 20 ++++++++++----------
>  2 files changed, 13 insertions(+), 12 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 c1fc747403..e55b889868 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -350,17 +350,17 @@ 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}.
>  ETEXI
>  
> @@ -368,31 +368,31 @@ ETEXI
>          .name       = "loadvm",
>          .args_type  = "name:s",
>          .params     = "tag|id",

If we go ahead with this, you may want to update .params too.

> -        .help       = "restore a VM snapshot from its tag or id",
> +        .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}.
>  ETEXI
>  
>      {
>          .name       = "delvm",
>          .args_type  = "name:s",
>          .params     = "tag|id",

Same here.

> -        .help       = "delete a VM snapshot from its tag or id",
> +        .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}.
>  ETEXI
>  
>      {
> -- 
> 2.17.1
> 
> 

-- 
Murilo

Re: [Qemu-devel] [PATCH v1 1/2] block/snapshot.c: eliminate use of ID input in snapshot operations

[Daniel Henrique Barboza]

Hey,

On 08/22/2018 07:06 PM, Murilo Opsfelder Araujo wrote:
> Hi, Daniel.
>
> On Tue, Aug 21, 2018 at 06:00:23PM -0300, Daniel Henrique Barboza wrote:
>> 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. Libvirt does not care about this change either, as far as
>> I've tested.
>>
>> 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>
>> ---
>>   block/snapshot.c |  5 +++--
>>   hmp-commands.hx  | 20 ++++++++++----------
>>   2 files changed, 13 insertions(+), 12 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 c1fc747403..e55b889868 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -350,17 +350,17 @@ 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}.
>>   ETEXI
>>   
>> @@ -368,31 +368,31 @@ ETEXI
>>           .name       = "loadvm",
>>           .args_type  = "name:s",
>>           .params     = "tag|id",
> If we go ahead with this, you may want to update .params too.
>
>> -        .help       = "restore a VM snapshot from its tag or id",
>> +        .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}.
>>   ETEXI
>>   
>>       {
>>           .name       = "delvm",
>>           .args_type  = "name:s",
>>           .params     = "tag|id",
> Same here.

Good catch. I'll update that in v2.
>
>> -        .help       = "delete a VM snapshot from its tag or id",
>> +        .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}.
>>   ETEXI
>>   
>>       {
>> -- 
>> 2.17.1
>>
>>

Re: [Qemu-devel] [PATCH v1 1/2] block/snapshot.c: eliminate use of ID input in snapshot operations

[Murilo Opsfelder Araujo]

Hi, Daniel.

On Tue, Aug 21, 2018 at 06:00:23PM -0300, Daniel Henrique Barboza wrote:
> 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. Libvirt does not care about this change either, as far as
> I've tested.
> 
> 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>
> ---
>  block/snapshot.c |  5 +++--
>  hmp-commands.hx  | 20 ++++++++++----------
>  2 files changed, 13 insertions(+), 12 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);

I see there is another use of bdrv_snapshot_delete_by_id_or_name() in
qemu-img.c.  Don't we want the same behavior there too?

If we remove these two uses, we can perhaps get rid of this function thoroughly.

>          }
>          aio_context_release(ctx);
>          if (ret < 0) {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index c1fc747403..e55b889868 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -350,17 +350,17 @@ 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}.
>  ETEXI
>  
> @@ -368,31 +368,31 @@ ETEXI
>          .name       = "loadvm",
>          .args_type  = "name:s",
>          .params     = "tag|id",
> -        .help       = "restore a VM snapshot from its tag or id",
> +        .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}.
>  ETEXI
>  
>      {
>          .name       = "delvm",
>          .args_type  = "name:s",
>          .params     = "tag|id",
> -        .help       = "delete a VM snapshot from its tag or id",
> +        .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}.
>  ETEXI
>  
>      {
> -- 
> 2.17.1
> 
> 

-- 
Murilo

Re: [Qemu-devel] [PATCH v1 1/2] block/snapshot.c: eliminate use of ID input in snapshot operations

[Daniel Henrique Barboza]

On 08/23/2018 04:27 PM, Murilo Opsfelder Araujo wrote:
> Hi, Daniel.
>
> On Tue, Aug 21, 2018 at 06:00:23PM -0300, Daniel Henrique Barboza wrote:
>> 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. Libvirt does not care about this change either, as far as
>> I've tested.
>>
>> 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>
>> ---
>>   block/snapshot.c |  5 +++--
>>   hmp-commands.hx  | 20 ++++++++++----------
>>   2 files changed, 13 insertions(+), 12 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);
> I see there is another use of bdrv_snapshot_delete_by_id_or_name() in
> qemu-img.c.  Don't we want the same behavior there too?

Yeah, I've considered changing the behavior there as well. Need to
check in detail how qemu-img parses the command line arguments.


>
> If we remove these two uses, we can perhaps get rid of this function thoroughly.
>
>>           }
>>           aio_context_release(ctx);
>>           if (ret < 0) {
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index c1fc747403..e55b889868 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -350,17 +350,17 @@ 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}.
>>   ETEXI
>>   
>> @@ -368,31 +368,31 @@ ETEXI
>>           .name       = "loadvm",
>>           .args_type  = "name:s",
>>           .params     = "tag|id",
>> -        .help       = "restore a VM snapshot from its tag or id",
>> +        .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}.
>>   ETEXI
>>   
>>       {
>>           .name       = "delvm",
>>           .args_type  = "name:s",
>>           .params     = "tag|id",
>> -        .help       = "delete a VM snapshot from its tag or id",
>> +        .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}.
>>   ETEXI
>>   
>>       {
>> -- 
>> 2.17.1
>>
>>

[Qemu-devel] [PATCH v1 2/2] qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call

[Daniel Henrique Barboza]

In qcow2_snapshot_create there is the following code block:

    /* Generate an ID */
    find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));

    /* Check that the ID is unique */
    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
        return -EEXIST;
    }

find_new_snapshot_id cycles through all snapshots, getting the id_str
as an unsigned long int, calculating the max id_max value of all the
existing id_strs and writing in the id_str pointer id_max + 1:

    for(i = 0; i < s->nb_snapshots; i++) {
        sn = s->snapshots + i;
        id = strtoul(sn->id_str, NULL, 10);
        if (id > id_max)
            id_max = id;
    }
    snprintf(id_str, id_str_size, "%lu", id_max + 1);

Here, sn_info->id_str will have the unique value id_max + 1. Right
after that, find_snapshot_by_id_and_name is called with
id = sn_info->id_str and name = NULL. This will cause the function
to execute the following:

    } else if (id) {
        for (i = 0; i < s->nb_snapshots; i++) {
            if (!strcmp(s->snapshots[i].id_str, id)) {
                return i;
            }
        }
    }

In short, we're searching the existing snapshots to see if sn_info->id_str
matches any existing id, right after we set in the previous line a
sn_info->id_str value that is already unique.

The first code block goes way back to commit 585f8587ad, a 2006 commit from
Fabrice Bellard that simply says "new qcow2 disk image format". No more
info is provided about this logic in any subsequent commits that moved
this code block around.

I can't say about the original design, but the current logic is redundant.
bdrv_snapshot_create is called in aio_context lock, forbidding any
concurrent call to accidentally create a new snapshot between
the find_new_snapshot_id and find_snapshot_by_id_and_name calls. What
we're ending up doing is to cycle through the snapshots two times
for no viable reason.

This patch eliminates the redundancy by removing the 'id is unique'
check that calls find_snapshot_by_id_and_name.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/qcow2-snapshot.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index bb6a5b7516..20e8472191 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -358,11 +358,6 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     /* Generate an ID */
     find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
 
-    /* Check that the ID is unique */
-    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
-        return -EEXIST;
-    }
-
     /* Populate sn with passed data */
     sn->id_str = g_strdup(sn_info->id_str);
     sn->name = g_strdup(sn_info->name);
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH v1 0/2] HMP/snapshot changes - do not use ID anymore

[Murilo Opsfelder Araujo]

Hi, Daniel.

On Tue, Aug 21, 2018 at 06:00:22PM -0300, Daniel Henrique Barboza wrote:
> I am marking the patch series as "RFC" because it was supposed to be
> a discussion but, when I was investigating, it turned out to be
> easier to send the patches right away.
> 
> It is not uncommon to see bugs being opened by testers that attempt to
> create VM snapshots using HMP. It turns out that "0" and "1" are quite
> common snapshot names and they trigger a lot of bugs. I gave an example
> in the commit message of patch 1, but to sum up here: QEMU treats the
> input of savevm/loadvm/delvm sometimes as 'ID', sometimes as 'name'. It
> is documented as such, but this can lead to strange situations.
> 
> Given that it is strange for an API to consider a parameter to be 2 fields
> at the same time, and inadvently treating them as one or the other, and
> that removing the ID field is too drastic, my idea here is to keep the
> ID field for internal control, but do not let the user set it.
> 
> I guess there's room for discussion about considering this change an API
> change or not. It doesn't affect users of HMP and it doesn't affect Libvirt,
> but I am simplifying the meaning of the parameters of savevm/loadvm/delvm.

What if we harden ->id_str to be a UUID?

The example you mentioned is that user gave a snapshot the name "1", which was
the ->id_str of the first snapshot created.

If we harden ->id_str to be a UUID, that situation would only happen if user
entered a UUID already in use or a tag of their preference.  It would be a bit
harder for user to guess an ->id_str (UUID) by mistake.

So, in this example, the name "1" of the second snapshot wouldn't match the
->id_str of the first snapshot and a second snapshot (with a different UUID)
would be created with tag "1".

Having *_snapshot_find() to still match ->id_str or ->name is good for user
experience, where short names or tags can still be used to operate on snapshots.

Internally, code could handle only UUIDs of the snapshots.  So
*_snapshot_delete() would receive a UUID, and *_snapshot_find() would still
match ->id_str or ->name, still allowing savevm/loadvm/delvm to operate on both
IDs and tags.

> 
> 
> Daniel Henrique Barboza (2):
>   block/snapshot.c: eliminate use of ID input in snapshot operations
>   qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call
> 
>  block/qcow2-snapshot.c |  5 -----
>  block/snapshot.c       |  5 +++--
>  hmp-commands.hx        | 20 ++++++++++----------
>  3 files changed, 13 insertions(+), 17 deletions(-)
> 
> -- 
> 2.17.1
> 
> 

-- 
Murilo

Re: [Qemu-devel] [RFC PATCH v1 0/2] HMP/snapshot changes - do not use ID anymore

[Daniel Henrique Barboza]

Hi,

On 08/22/2018 07:04 PM, Murilo Opsfelder Araujo wrote:
> Hi, Daniel.
>
> On Tue, Aug 21, 2018 at 06:00:22PM -0300, Daniel Henrique Barboza wrote:
>> I am marking the patch series as "RFC" because it was supposed to be
>> a discussion but, when I was investigating, it turned out to be
>> easier to send the patches right away.
>>
>> It is not uncommon to see bugs being opened by testers that attempt to
>> create VM snapshots using HMP. It turns out that "0" and "1" are quite
>> common snapshot names and they trigger a lot of bugs. I gave an example
>> in the commit message of patch 1, but to sum up here: QEMU treats the
>> input of savevm/loadvm/delvm sometimes as 'ID', sometimes as 'name'. It
>> is documented as such, but this can lead to strange situations.
>>
>> Given that it is strange for an API to consider a parameter to be 2 fields
>> at the same time, and inadvently treating them as one or the other, and
>> that removing the ID field is too drastic, my idea here is to keep the
>> ID field for internal control, but do not let the user set it.
>>
>> I guess there's room for discussion about considering this change an API
>> change or not. It doesn't affect users of HMP and it doesn't affect Libvirt,
>> but I am simplifying the meaning of the parameters of savevm/loadvm/delvm.
> What if we harden ->id_str to be a UUID?
>
> The example you mentioned is that user gave a snapshot the name "1", which was
> the ->id_str of the first snapshot created.
>
> If we harden ->id_str to be a UUID, that situation would only happen if user
> entered a UUID already in use or a tag of their preference.  It would be a bit
> harder for user to guess an ->id_str (UUID) by mistake.
>
> So, in this example, the name "1" of the second snapshot wouldn't match the
> ->id_str of the first snapshot and a second snapshot (with a different UUID)
> would be created with tag "1".
>
> Having *_snapshot_find() to still match ->id_str or ->name is good for user
> experience, where short names or tags can still be used to operate on snapshots.
>
> Internally, code could handle only UUIDs of the snapshots.  So
> *_snapshot_delete() would receive a UUID, and *_snapshot_find() would still
> match ->id_str or ->name, still allowing savevm/loadvm/delvm to operate on both
> IDs and tags.

This is a valid alternative to avoid the bug (in most cases anyway) without
changing the meaning of the APIs.

However, my idea with this series is to start a discussion about removing
the ID usage in the user side, showing that the world will not collapse if
we do it. The idea was based on an old wiki page with snapshot
improvements that were being worked on:

https://wiki.qemu.org/Features/SnapshottingImprovements#ID_and_TAG_confusion

As said there, other virtualization engines do not have this idea of 
snapshot
ids being exposed to the user. QEMU is still doing that, and in my opinion
it is actually worse than it was because now we're obscuring the ID 
information:


(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                 VM SIZE                DATE       VM CLOCK
--        running                121M 2018-08-09 18:00:57 00:21:22.444
--        vm-20180809180123      121M 2018-08-09 18:01:23 00:21:48.114
--        vm-20180809180127      121M 2018-08-09 18:01:27 00:21:51.391


Even if the user is aware of the ID/TAG duality of the savevm API, how 
can you
issue "savevm 100" without knowing if any of these snapshots has ID = 100?
And remember that user can be any management system (Libvirt uses this
API, for example) so even if we use an UUID there is no guarantee that a
collision wouldn't occur eventually. Why take the chances if we can simply
ignore the ID in these APIs?

I stand by this proposal, but I believe we need input from someone 
experienced
in this code (Kevin for example) to see if I am missing something 
trivial here.


Thanks,


Daniel



>
>>
>> Daniel Henrique Barboza (2):
>>    block/snapshot.c: eliminate use of ID input in snapshot operations
>>    qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call
>>
>>   block/qcow2-snapshot.c |  5 -----
>>   block/snapshot.c       |  5 +++--
>>   hmp-commands.hx        | 20 ++++++++++----------
>>   3 files changed, 13 insertions(+), 17 deletions(-)
>>
>> -- 
>> 2.17.1
>>
>>