HMP/snapshot changes - do not use ID anymore
v3 cover HMP/snapshot changes - do not use ID anymore
changes in v3:
- rebased to v3.1.0-rc0 tag
- hmp-commands.hx documentation now mentions the change of semantics starting version 3.2.
- previous version link:
http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg00629.html
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.
This series simplifies the meaning of savevm/loadvm/delvm to be up to par to what the QEMU code (and Libvirt) is already doing: snapshot operations using "tag" semantics only, leaving the "id" to be automatically calculated by the block drivers and used internally only.
This change of semantics does not affect existing snapshots. What changes is that any HMP operations with them will use the updated semantics.
Daniel Henrique Barboza (3):
block/snapshot.c: eliminate use of ID input in snapshot operations block/snapshot: remove bdrv_snapshot_delete_by_id_or_name
qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call
block/qcow2-snapshot.c | 5 ----- block/snapshot.c | 25 +++---------------------- hmp-commands.hx | 32 ++++++++++++++++++++------------ include/block/snapshot.h | 3 --- qemu-img.c | 15 +++++++++++---- 5 files changed, 34 insertions(+), 46 deletions(-)
Signature · 1 line
2.17.2
PATCH v3 1/3 block/snapshot.c: eliminate use of ID input in snapshot operations
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.
--- block/snapshot.c | 5 +++-- hmp-commands.hx | 32 ++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 14 deletions(-)
block/snapshot.c +3 -2
@@ -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 ) {
hmp-commands.hx +20 -12
@@ -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 3.2, 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 3.2, 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 3.2, delvm stopped deleting snapshots by snapshot id, accepting +only @var{tag} as parameter. ETEXI {
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH for-3.2 v3 1/3] block/snapshot.c: eliminate use of ID input in snapshot operations
Quoted 8 lines · * Daniel Henrique Barboza ([email protected]) 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:
I'm OK with the HMP side, and I think OK with the idea so:
but this is mainly block code, so let Kevin or Max review it fully.
Dave
Quoted 185 lines
(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.313A 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.252What happened?
- when creating the second snapshot, a verification is done inside
bdrv_all_delete_snapshotto delete any existing snapshots that matches an string argument. Here, the code callsbdrv_all_delete_snapshot("1", ...);-
bdrv_all_delete_snapshotcallsbdrv_snapshot_find(..., "1")for each BlockDriverState of the guest. And this is where things goes tilting:bdrv_snapshot_finddoes a search by bothid_strand name. It finds out that there is a snapshot that hasid_str= 1, stores a reference to the snapshot in thesn_infopointer 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 bybdrv_snapshot_find. Instead, it deletes the snapshot usingbdrv_snapshot_delete()calling it first withid_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.cdoes not make any special use ofid_str;- qcow2 uses
id_stras an unique identifier but it is automatically calculated, not being influenced by user input. Other than that, there are no distinguish operations made only withid_str;- in
blockdev.c, the delete operation uses a match of bothid_strAND name. Given thatid_stris 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 forid_stranymore, only 'name'. The function is called insave_snapshot(),load_snapshot(),bdrv_all_delete_snapshot()andbdrv_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 insidevl.c(which doesn't care about it anyways);-
bdrv_all_delete_snapshot()does not callbdrv_snapshot_delete_by_id_or_nameanymore. Instead, it uses the pointer returned bybdrv_snapshot_findto erase the snapshot with the exact match ofid_stran name. This function is called insave_snapshotandhmp_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_strfield entirely, but this would have repercussions on existing snapshots. Another day perhaps.Signed-off-by Daniel Henrique Barboza <[email protected]>--- block/snapshot.c | 5 +++-- hmp-commands.hx | 32 ++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 14 deletions(-)block/snapshot.c +3
-2@@ -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 ) {hmp-commands.hx +20
-12@@ -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 3.2, 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 3.2, 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 3.2, delvm stopped deleting snapshots by snapshot id, accepting +only @var{tag} as parameter. ETEXI {Signature · 1 line
2.17.2
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH for-3.2 v3 1/3] block/snapshot.c: eliminate use of ID input in snapshot operations
In reply to Daniel Henrique BarbozaQuoted 12 lines · 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.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
hmp-commands.hx ETEXI · line 366
Quoted 5 lines
hmp-commands.hx +3
-1-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 3.2, savevm stopped allowing the snapshot id to be set, accepting
s/3.2/4.0/
hmp-commands.hx ETEXI · line 403
Quoted 4 lines
hmp-commands.hx +3
-0+only @var{tag} as parameter. +Since 3.2, delvm stopped deleting snapshots by snapshot id, accepting +only @var{tag} as parameter.
and again
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-3.2 v3 1/3] block/snapshot.c: eliminate use of ID input in snapshot operations
Am 15.02.2019 um 17:21 hat Eric Blake geschrieben:
Quoted 21 lines · Daniel Henrique Barboza
Quoted 12 lines
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.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 alreadyhmp-commands.hx ETEXI · line 366
Quoted 5 lines
hmp-commands.hx +3
-1-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 3.2, savevm stopped allowing the snapshot id to be set, acceptings/3.2/4.0/
Quoted 6 lines
hmp-commands.hx ETEXI · line 403
Quoted 4 lines
hmp-commands.hx +3
-0+only @var{tag} as parameter. +Since 3.2, delvm stopped deleting snapshots by snapshot id, accepting +only @var{tag} as parameter.and again
Sorry, I should have mentioned that I changed this while applying.
Kevin
PATCH v3 2/3 block/snapshot: remove bdrv_snapshot_delete_by_id_or_name
In reply to Daniel Henrique BarbozaAfter the previous patch, the only instance of this function left is inside qemu-img.c.
qemu-img is using it inside the 'img_snapshot' function to delete snapshots in the SNAPSHOT_DELETE case, based on a "snapshot_name" string that refers to the tag, not ID, of the QEMUSnapshotInfo struct. This can be verified by checking the SNAPSHOT_CREATE case that comes shortly before SNAPSHOT_DELETE. In that case, the same "snapshot_name" variable is being strcpy to the 'name' field of the QEMUSnapshotInfo struct sn:
pstrcpy(sn.name, sizeof(sn.name), snapshot_name);
Based on that, it is unlikely that "snapshot_name" might contain an "id" in SNAPSHOT_DELETE.
This patch changes SNAPSHOT_DELETE to use snapshot_find() and snapshot_delete() instead of bdrv_snapshot_delete_by_id_or_name. After that, there is no instances left of bdrv_snapshot_delete_by_id_or_name in the code, so it is safe to remove it entirely.
--- block/snapshot.c | 20 -------------------- include/block/snapshot.h | 3 --- qemu-img.c | 15 +++++++++++---- 3 files changed, 11 insertions(+), 27 deletions(-)
block/snapshot.c +0 -20
@@ -301,26 +301,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,return ret; } -int bdrv_snapshot_delete_by_id_or_name (BlockDriverState *bs, -const char *id_or_name, -Error **errp) -{ -int ret; -Error *local_err =NULL ; - - ret =bdrv_snapshot_delete (bs, id_or_name,NULL , &local_err); -if (ret == -ENOENT || ret == -EINVAL ) { -error_free (local_err); - local_err =NULL ; - ret =bdrv_snapshot_delete (bs,NULL , id_or_name, &local_err); - } - -if (ret <0 ) { -error_propagate (errp, local_err); - } -return ret; -} -int bdrv_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_info) {
include/block/snapshot.h +0 -3
@@ -61,9 +61,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,const char *snapshot_id, const char *name, Error **errp); -int bdrv_snapshot_delete_by_id_or_name (BlockDriverState *bs, -const char *id_or_name, -Error **errp);int bdrv_snapshot_list (BlockDriverState *bs,QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs,
qemu-img.c +11 -4
@@ -3121,11 +3121,18 @@ static int img_snapshot(int argc, char **argv)break ;case SNAPSHOT_DELETE : -bdrv_snapshot_delete_by_id_or_name (bs, snapshot_name, &err); -if (err) { -error_reportf_err (err,"Could not delete snapshot '%s': " , - snapshot_name); + ret =bdrv_snapshot_find (bs, &sn, snapshot_name); +if (ret <0 ) { +error_report ("Could not delete snapshot '%s': snapshot not " +"found" , snapshot_name); ret =1 ; + }else { + ret =bdrv_snapshot_delete (bs, sn.id_str, sn.name, &err); +if (ret <0 ) { +error_reportf_err (err,"Could not delete snapshot '%s': " , + snapshot_name); + ret =1 ; + } }break ; }
Signature · 1 line
2.17.2
PATCH v3 3/3 qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call
In reply to Daniel Henrique BarbozaIn 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.
--- block/qcow2-snapshot.c | 5 ----- 1 file changed, 5 deletions(-)
block/qcow2-snapshot.c +0 -5
@@ -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);
Signature · 1 line
2.17.2
Ping
Quoted 43 lines · Daniel Henrique Barboza
changes in v3:
- rebased to v3.1.0-rc0 tag
- hmp-commands.hx documentation now mentions the change of semantics starting version 3.2.
- previous version link:
http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg00629.htmlIt 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.
This series simplifies the meaning of savevm/loadvm/delvm to be up to par to what the QEMU code (and Libvirt) is already doing: snapshot operations using "tag" semantics only, leaving the "id" to be automatically calculated by the block drivers and used internally only.
This change of semantics does not affect existing snapshots. What changes is that any HMP operations with them will use the updated semantics.
Daniel Henrique Barboza (3):
block/snapshot.c: eliminate use of ID input in snapshot operations block/snapshot: removebdrv_snapshot_delete_by_id_or_name
qcow2-snapshot: remove redundantfind_snapshot_by_id_and_namecallblock/qcow2-snapshot.c | 5 ----- block/snapshot.c | 25 +++---------------------- hmp-commands.hx | 32 ++++++++++++++++++++------------ include/block/snapshot.h | 3 --- qemu-img.c | 15 +++++++++++---- 5 files changed, 34 insertions(+), 46 deletions(-)
Ping
I believe all the discussion that happened in v2 applies here as well. Do we have a plan for this series? Should I add something else (warning message or doc note) to indicate a deprecation of the old meaning of the arguments?
Thanks,
DHB
Quoted 43 lines · Daniel Henrique Barboza
changes in v3:
- rebased to v3.1.0-rc0 tag
- hmp-commands.hx documentation now mentions the change of semantics starting version 3.2.
- previous version link:
http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg00629.htmlIt 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.
This series simplifies the meaning of savevm/loadvm/delvm to be up to par to what the QEMU code (and Libvirt) is already doing: snapshot operations using "tag" semantics only, leaving the "id" to be automatically calculated by the block drivers and used internally only.
This change of semantics does not affect existing snapshots. What changes is that any HMP operations with them will use the updated semantics.
Daniel Henrique Barboza (3):
block/snapshot.c: eliminate use of ID input in snapshot operations block/snapshot: removebdrv_snapshot_delete_by_id_or_name
qcow2-snapshot: remove redundantfind_snapshot_by_id_and_namecallblock/qcow2-snapshot.c | 5 ----- block/snapshot.c | 25 +++---------------------- hmp-commands.hx | 32 ++++++++++++++++++++------------ include/block/snapshot.h | 3 --- qemu-img.c | 15 +++++++++++---- 5 files changed, 34 insertions(+), 46 deletions(-)
Kevin Wolf
AppliedAm 07.11.2018 um 14:09 hat Daniel Henrique Barboza geschrieben:
Quoted 26 lines
changes in v3:
- rebased to v3.1.0-rc0 tag
- hmp-commands.hx documentation now mentions the change of semantics starting version 3.2.
- previous version link:
http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg00629.htmlIt 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.
This series simplifies the meaning of savevm/loadvm/delvm to be up to par to what the QEMU code (and Libvirt) is already doing: snapshot operations using "tag" semantics only, leaving the "id" to be automatically calculated by the block drivers and used internally only.
Quoted 3 lines
This change of semantics does not affect existing snapshots. What changes is that any HMP operations with them will use the updated semantics.
Thanks, applied to the block branch.
Kevin