HMP/snapshot changes - do not use ID anymore

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

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          | 24 ++++++++++++------------
  include/block/snapshot.h |  3 ---
  qemu-img.c               | 15 +++++++++++----
  5 files changed, 26 insertions(+), 46 deletions(-)

Hey David,

Quoted 22 lines · Dr. David Alan Gilbert

Quoted 21 lines · * Daniel Henrique Barboza ([email protected]) wrote:

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 6 lines

Quoted 3 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Can you clarify a couple of things:
a) What is it about libvirt's use that means it's OK ? Does it never
use numeric tags?

I am glad you asked because my understanding in how Libvirt was dealing with snapshots was wrong, and I just looked into it further to answer your question. Luckily, this series fixes the situation for Libvirt as well.

I was misled by the fact that Libvirt does not show the same symptoms we see in
QEMU of this problem, but the bug is there. Here's a quick test with Libvirt with
"0" and "1" as snapshot names, considering a VM named with no snapshots, using QEMU 2.12 and Libvirt 4.0.0:

- create the "0" snapshot:

$ sudo virsh snapshot-create-as --name 0 dhb
Domain snapshot 0 created

$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    0  405M 2018-09-24 15:47:56 00:04:20.867

- created the "1" snapshot. Here, Libvirt shows both snapshots with snapshot-list,
but the snapshot was erased inside QEMU:

$ sudo virsh snapshot-create-as --name 1 dhb
Domain snapshot 1 created
$
$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running
1 2018-09-24 15:50:09 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    1  404M 2018-09-24 15:50:10 00:05:36.226

This is where I stopped checking out Libvirt at first, concluding wrongly that it
was immune to the bug.

Libvirt does not throw an error when trying to apply snapshot 0. In fact, it acts
like everything went fine:

$ sudo virsh snapshot-revert dhb 0

$ echo $?
0

Reverting back to snapshot "1" works as intended, restoring the VM state when it
was created.

(perhaps this is something we should let Libvirt be aware of ...)

This series fixes this behavior because the snapshot 0 isn't erased from QEMU, allowing
Libvirt to successfully restore it.

Quoted 2 lines

b) After this series are you always guaranteed to be able to fix
any existing oddly named snapshots?

The oddly named snapshots that already exists can be affected by the semantic
change in loadvm and delvm, in a way that the user can't load/del using the snap
ID, just the tag. Aside from that, I don't see any side effects with existing
snapshots and this patch series.

Cc libvir-list for review of the compatibility argument below.

Quoted 86 lines · Daniel Henrique Barboza

Hey David,

Quoted 22 lines · Dr. David Alan Gilbert

Quoted 21 lines · * Daniel Henrique Barboza ([email protected]) wrote:

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 6 lines

Quoted 3 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Can you clarify a couple of things:
a) What is it about libvirt's use that means it's OK ? Does it never
use numeric tags?

I am glad you asked because my understanding in how Libvirt was dealing with snapshots was wrong, and I just looked into it further to answer your question. Luckily, this series fixes the situation for Libvirt as well.

I was misled by the fact that Libvirt does not show the same symptoms we see in
QEMU of this problem, but the bug is there. Here's a quick test with Libvirt with
"0" and "1" as snapshot names, considering a VM named with no snapshots, using QEMU 2.12 and Libvirt 4.0.0:

- create the "0" snapshot:

$ sudo virsh snapshot-create-as --name 0 dhb
Domain snapshot 0 created

$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    0  405M 2018-09-24 15:47:56 00:04:20.867

- created the "1" snapshot. Here, Libvirt shows both snapshots with snapshot-list,
but the snapshot was erased inside QEMU:

$ sudo virsh snapshot-create-as --name 1 dhb
Domain snapshot 1 created
$
$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running
1 2018-09-24 15:50:09 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    1  404M 2018-09-24 15:50:10 00:05:36.226

This is where I stopped checking out Libvirt at first, concluding wrongly that it
was immune to the bug.

Libvirt does not throw an error when trying to apply snapshot 0. In fact, it acts
like everything went fine:

Quoted 4 lines

$ sudo virsh snapshot-revert dhb 0

$ echo $?
0

Is that a libvirt bug?

Quoted 7 lines

Reverting back to snapshot "1" works as intended, restoring the VM state when it
was created.

(perhaps this is something we should let Libvirt be aware of ...)

Quoted 11 lines

This series fixes this behavior because the snapshot 0 isn't erased from QEMU, allowing
Libvirt to successfully restore it.

Quoted 2 lines

b) After this series are you always guaranteed to be able to fix
any existing oddly named snapshots?

Quoted 7 lines

The oddly named snapshots that already exists can be affected by the semantic
change in loadvm and delvm, in a way that the user can't load/del using the snap
ID, just the tag. Aside from that, I don't see any side effects with existing
snapshots and this patch series.

Do all snapshots have a tag that is unique within their image? Even snapshots created by old versions of QEMU?

Quoted 90 lines · Markus Armbruster

Cc libvir-list for review of the compatibility argument below.

Quoted 86 lines · Daniel Henrique Barboza

Hey David,

Quoted 22 lines · Dr. David Alan Gilbert

Quoted 21 lines · * Daniel Henrique Barboza ([email protected]) wrote:

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 6 lines

Quoted 3 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Can you clarify a couple of things:
a) What is it about libvirt's use that means it's OK ? Does it never
use numeric tags?

I am glad you asked because my understanding in how Libvirt was dealing with snapshots was wrong, and I just looked into it further to answer your question. Luckily, this series fixes the situation for Libvirt as well.

I was misled by the fact that Libvirt does not show the same symptoms we see in
QEMU of this problem, but the bug is there. Here's a quick test with Libvirt with
"0" and "1" as snapshot names, considering a VM named with no snapshots, using QEMU 2.12 and Libvirt 4.0.0:

- create the "0" snapshot:

$ sudo virsh snapshot-create-as --name 0 dhb
Domain snapshot 0 created

$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    0  405M 2018-09-24 15:47:56 00:04:20.867

- created the "1" snapshot. Here, Libvirt shows both snapshots with snapshot-list,
but the snapshot was erased inside QEMU:

$ sudo virsh snapshot-create-as --name 1 dhb
Domain snapshot 1 created
$
$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running
1 2018-09-24 15:50:09 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    1  404M 2018-09-24 15:50:10 00:05:36.226

This is where I stopped checking out Libvirt at first, concluding wrongly that it
was immune to the bug.

Libvirt does not throw an error when trying to apply snapshot 0. In fact, it acts
like everything went fine:

Quoted 6 lines

Quoted 4 lines

$ sudo virsh snapshot-revert dhb 0

$ echo $?
0

Is that a libvirt bug?

Probably yes. The error handling from HMP sucks and can't really be fixed in all cases. This is for the handler which calls "loadvm":

    if (strstr(reply, "No block device supports snapshots") != NULL) {
        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                       _("this domain does not have a device to load snapshots"));
        goto cleanup;
    } else if (strstr(reply, "Could not find snapshot") != NULL) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       _("the snapshot '%s' does not exist, and was not loaded"),
                       name);
        goto cleanup;
    } else if (strstr(reply, "Snapshots not supported on device") != NULL) {
        virReportError(VIR_ERR_OPERATION_INVALID, "%s", reply);
        goto cleanup;
    } else if (strstr(reply, "Could not open VM state file") != NULL) {
        virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply);
        goto cleanup;
    } else if (strstr(reply, "Error") != NULL
             && strstr(reply, "while loading VM state") != NULL) {
        virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply);
        goto cleanup;
    } else if (strstr(reply, "Error") != NULL
             && strstr(reply, "while activating snapshot on") != NULL) {
        virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply);
        goto cleanup;
    }

If the above does not match the reported error, we report success. The same problem can happen with any of the 5 [1] HMP command handlers we still have.

Note that a similar abomination is also for the code which calls "savevm".

[1] We technically have 6 HMP handlers, but "cpu_set" is not used if yoy have a QEMU younger than 3 years. Soon also "drive_add" and "drive_del" will be replaced, so we'll be stuck with the 3 internal snapshot ones.

Quoted 7 lines

Quoted 7 lines

Reverting back to snapshot "1" works as intended, restoring the VM state when it
was created.

(perhaps this is something we should let Libvirt be aware of ...)

The error message from qemu which was wrongly ignored by qemu can be found in the libvirtd debug log. It would be helpful if you could provide it.

Quoted 19 lines

Quoted 18 lines

This series fixes this behavior because the snapshot 0 isn't erased from QEMU, allowing
Libvirt to successfully restore it.

Quoted 2 lines

b) After this series are you always guaranteed to be able to fix
any existing oddly named snapshots?

The oddly named snapshots that already exists can be affected by the semantic
change in loadvm and delvm, in a way that the user can't load/del using the snap
ID, just the tag. Aside from that, I don't see any side effects with existing
snapshots and this patch series.

Quoted 2 lines

Do all snapshots have a tag that is unique within their image? Even snapshots created by old versions of QEMU?

I remember there was a discussion which regarded problems with collisions between the name and the ID of the snapshot when dealing with loadvm/delvm commands but I can't seem to find it currently. Note that libvirt plainly issues loadvm/delvm $SNAPSHOTNAME. If the name is numeric it might clash. Similarly for inactive vms via qemu-img.

Quoted 126 lines · * Peter Krempa ([email protected]) wrote:

Quoted 90 lines · Markus Armbruster

Cc libvir-list for review of the compatibility argument below.

Quoted 86 lines · Daniel Henrique Barboza

Hey David,

Quoted 22 lines · Dr. David Alan Gilbert

Quoted 21 lines · * Daniel Henrique Barboza ([email protected]) wrote:

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 6 lines

Quoted 3 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Can you clarify a couple of things:
a) What is it about libvirt's use that means it's OK ? Does it never
use numeric tags?

I am glad you asked because my understanding in how Libvirt was dealing with snapshots was wrong, and I just looked into it further to answer your question. Luckily, this series fixes the situation for Libvirt as well.

I was misled by the fact that Libvirt does not show the same symptoms we see in
QEMU of this problem, but the bug is there. Here's a quick test with Libvirt with
"0" and "1" as snapshot names, considering a VM named with no snapshots, using QEMU 2.12 and Libvirt 4.0.0:

- create the "0" snapshot:

$ sudo virsh snapshot-create-as --name 0 dhb
Domain snapshot 0 created

$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    0  405M 2018-09-24 15:47:56 00:04:20.867

- created the "1" snapshot. Here, Libvirt shows both snapshots with snapshot-list,
but the snapshot was erased inside QEMU:

$ sudo virsh snapshot-create-as --name 1 dhb
Domain snapshot 1 created
$
$ sudo virsh snapshot-list dhb
Name Creation Time State
------------------------------------------------------------
0 2018-09-24 15:47:56 -0400 running
1 2018-09-24 15:50:09 -0400 running

$ sudo virsh qemu-monitor-command dhb --hmp info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
--    1  404M 2018-09-24 15:50:10 00:05:36.226

This is where I stopped checking out Libvirt at first, concluding wrongly that it
was immune to the bug.

Libvirt does not throw an error when trying to apply snapshot 0. In fact, it acts
like everything went fine:

Quoted 6 lines

Quoted 4 lines

$ sudo virsh snapshot-revert dhb 0

$ echo $?
0

Is that a libvirt bug?

Probably yes. The error handling from HMP sucks and can't really be fixed in all cases. This is for the handler which calls "loadvm":

    if (strstr(reply, "No block device supports snapshots") != NULL) {
        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                       _("this domain does not have a device to load snapshots"));
        goto cleanup;
    } else if (strstr(reply, "Could not find snapshot") != NULL) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       _("the snapshot '%s' does not exist, and was not loaded"),
                       name);
        goto cleanup;
    } else if (strstr(reply, "Snapshots not supported on device") != NULL) {
        virReportError(VIR_ERR_OPERATION_INVALID, "%s", reply);
        goto cleanup;
    } else if (strstr(reply, "Could not open VM state file") != NULL) {
        virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply);
        goto cleanup;
    } else if (strstr(reply, "Error") != NULL
             && strstr(reply, "while loading VM state") != NULL) {
        virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply);
        goto cleanup;
    } else if (strstr(reply, "Error") != NULL
             && strstr(reply, "while activating snapshot on") != NULL) {
        virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply);
        goto cleanup;
    }

Quoted 6 lines

If the above does not match the reported error, we report success. The same problem can happen with any of the 5 [1] HMP command handlers we still have.

Note that a similar abomination is also for the code which calls "savevm".

Would the following small qemu change make life a little safer:

hmp.c +1 -1

index 576253a01f..0729a8c7ed 100644
@@ -62,7 +62,7 @@ static void hmp_handle_error(Monitor *mon, Error **errp)
 {
     assert(errp);
     if (*errp) {
-        error_report_err(*errp);
+        error_reportf_err(*errp, "Error: ");
     }
 }
 

changing:

No block device supports snapshots

to:

Error: No block device supports snapshots

so you could check for Error: at the start and know that you've hit some unknown error?

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Quoted 21 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 3 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 26 lines · Daniel Henrique Barboza

Quoted 24 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Quoted 4 lines

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 59 lines

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 6 lines

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

No, my idea was to keep the resolution the same as it is; just to forbid creating new snapshots with numeric names. This would prevent users from getting into the whole situation.

Am 09.01.2019 um 15:54 hat Max Reitz geschrieben:

Quoted 67 lines · Kevin Wolf

Quoted 59 lines

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 6 lines

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 3 lines

No, my idea was to keep the resolution the same as it is; just to forbid creating new snapshots with numeric names. This would prevent users from getting into the whole situation.

That's the version with an even more complex resolution method C. :-)

I actually think the current behaviour is more confusing than helpful. Without looking into the code or trying it out, I couldn't even tell whether ID or name takes precedence if there is a matching snapshot for both. Considering your proposal, it's probably the ID, but how should a user know that? (If against all expectations documentation exists, it doesn't count because nobody reads that.)

Kevin

Am 09.01.2019 um 15:54 hat Max Reitz geschrieben:

Quoted 67 lines · Kevin Wolf

Quoted 59 lines

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 6 lines

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 3 lines

No, my idea was to keep the resolution the same as it is; just to forbid creating new snapshots with numeric names. This would prevent users from getting into the whole situation.

That's the version with an even more complex resolution method C. :-)

I actually think the current behaviour is more confusing than helpful. Without looking into the code or trying it out, I couldn't even tell whether ID or name takes precedence if there is a matching snapshot for both.

Considering your proposal, it's probably the ID, but how should a
user know that? (If against all expectations documentation exists, it doesn't count because nobody reads that.)

Am 09.01.2019 um 15:54 hat Max Reitz geschrieben:

Quoted 67 lines · Kevin Wolf

Quoted 65 lines

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 3 lines

No, my idea was to keep the resolution the same as it is; just to forbid creating new snapshots with numeric names. This would prevent users from getting into the whole situation.

That's the version with an even more complex resolution method C. :-)

I actually think the current behaviour is more confusing than helpful. Without looking into the code or trying it out, I couldn't even tell whether ID or name takes precedence if there is a matching snapshot for both. Considering your proposal, it's probably the ID, but how should a user know that? (If against all expectations documentation exists, it doesn't count because nobody reads that.)

Quoted 72 lines

Am 09.01.2019 um 15:54 hat Max Reitz geschrieben:

Quoted 70 lines · Kevin Wolf

Quoted 59 lines

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 6 lines

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

No, my idea was to keep the resolution the same as it is; just to forbid creating new snapshots with numeric names. This would prevent users from getting into the whole situation.

Quoted 1 line

That's the version with an even more complex resolution method C. :-)

How so if the resolution method stays the same? Because it already is too complex?

If so, yes, that is an argument. I was arguing for the simplest patch instead of the simplest code, true.

Quoted 6 lines

I actually think the current behaviour is more confusing than helpful. Without looking into the code or trying it out, I couldn't even tell whether ID or name takes precedence if there is a matching snapshot for both. Considering your proposal, it's probably the ID, but how should a user know that? (If against all expectations documentation exists, it doesn't count because nobody reads that.)

It isn't more confusing than it is right now. With my proposal, all current images are simply as confusing as they are right now (I think ID takes precedence, yes), but if you create new snapshots, it's clear, since you simply cannot create names that could be IDs.

Quoted 21 lines · Daniel Henrique Barboza

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 3 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

Max

Am 09.01.2019 um 17:27 hat Max Reitz geschrieben:

Quoted 73 lines · Kevin Wolf

Quoted 72 lines

Am 09.01.2019 um 15:54 hat Max Reitz geschrieben:

Quoted 70 lines · Kevin Wolf

Quoted 59 lines

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:

Quoted 30 lines · Kevin Wolf

Quoted 29 lines

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 28 lines · Daniel Henrique Barboza

Quoted 16 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

I don't know, but what I meant is that if you have scripts to do all this, you might have to adjust them with this change.

That's what the H in HMP means.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Quoted 6 lines

It wouldn't be incompatible with existing images if we'd just disallow creating such snapshots. I don't see how the identifier resolution would be more complex.

I don't know if it'd be better. I'd just find it simpler (for us, that is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Quoted 6 lines

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given identifier as its ID
- If the identifier is not a number, find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

No, my idea was to keep the resolution the same as it is; just to forbid creating new snapshots with numeric names. This would prevent users from getting into the whole situation.

Quoted 7 lines

Quoted 1 line

That's the version with an even more complex resolution method C. :-)

How so if the resolution method stays the same? Because it already is too complex?

If so, yes, that is an argument. I was arguing for the simplest patch instead of the simplest code, true.

Yes, because it already is too complex. Not even necessarily the code (even though that's true as well), but most importantly the interface.

Quoted 7 lines

Quoted 6 lines

I actually think the current behaviour is more confusing than helpful. Without looking into the code or trying it out, I couldn't even tell whether ID or name takes precedence if there is a matching snapshot for both. Considering your proposal, it's probably the ID, but how should a user know that? (If against all expectations documentation exists, it doesn't count because nobody reads that.)

Quoted 4 lines

It isn't more confusing than it is right now. With my proposal, all current images are simply as confusing as they are right now (I think ID takes precedence, yes), but if you create new snapshots, it's clear, since you simply cannot create names that could be IDs.

I agree. But wasn't the goal of the patch to make it less confusing than it is right now?

Quoted 23 lines · Max Reitz

Quoted 22 lines · Daniel Henrique Barboza

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

Quoted 4 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

That's true. My idea here was simple: the user can't reliably exclude via snapshot ID today
because we're hiding the ID field in info snapshots:

    (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

Thus, what will end up happening is that the user will be forced to use the TAG of the snapshot since this is the only available information.

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names.  How bad would that be?

This was my first idea when evaluating what to do in this case. I gave it up because
I found it to be too extreme. People would start complaining "I was able to do
savevm 0 and now I can't".

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names.  How bad would that be?

This was my first idea when evaluating what to do in this case. I gave it up because
I found it to be too extreme. People would start complaining "I was able to do
savevm 0 and now I can't".

Quoted 47 lines · Max Reitz

Quoted 23 lines · Daniel Henrique Barboza

Quoted 22 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

Quoted 4 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

That's true. My idea here was simple: the user can't reliably exclude via snapshot ID today
because we're hiding the ID field in info snapshots:

    (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

Quoted 2 lines

Thus, what will end up happening is that the user will be forced to use the TAG of the snapshot since this is the only available information.

But you can get it through e.g. qemu-img info.

Snapshot list:
ID TAG VM SIZE DATE VM CLOCK
1 0 1.6M 2019-01-09 18:01:21 00:00:02.657

So it's not impossible to get.

Quoted 9 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names.  How bad would that be?

This was my first idea when evaluating what to do in this case. I gave it up because
I found it to be too extreme. People would start complaining "I was able to do
savevm 0 and now I can't".

True. But it wouldn't be impossible to do, we'd need to deprecate numeric names, print a warning for two releases, and then we can make it an error.

Hm... If we had a proper deprecation warning in this series, I suppose it wouldn't be dangerous anymore. Can we just print a warning whenever the user specified an ID? (i.e. if some snapshot's ID matches the string given by the user and the snapshot's name does not)

Quoted 45 lines · Daniel Henrique Barboza

Quoted 23 lines · Max Reitz

Quoted 22 lines · Daniel Henrique Barboza

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

Quoted 4 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

That's true. My idea here was simple: the user can't reliably exclude via snapshot ID today
because we're hiding the ID field in info snapshots:

    (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

Quoted 2 lines

Thus, what will end up happening is that the user will be forced to use the TAG of the snapshot since this is the only available information.

But you can get it through e.g. qemu-img info.

Snapshot list:
ID TAG VM SIZE DATE VM CLOCK
1 0 1.6M 2019-01-09 18:01:21 00:00:02.657

So it's not impossible to get.

Quoted 8 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names.  How bad would that be?

This was my first idea when evaluating what to do in this case. I gave it up because
I found it to be too extreme. People would start complaining "I was able to do
savevm 0 and now I can't".

True. But it wouldn't be impossible to do, we'd need to deprecate numeric names, print a warning for two releases, and then we can make it an error.

Hm... If we had a proper deprecation warning in this series, I suppose it wouldn't be dangerous anymore. Can we just print a warning whenever the user specified an ID? (i.e. if some snapshot's ID matches the string given by the user and the snapshot's name does not)

Max

Am 09.01.2019 um 18:05 hat Max Reitz geschrieben:

Quoted 53 lines · Daniel Henrique Barboza

Quoted 47 lines · Max Reitz

Quoted 23 lines · Daniel Henrique Barboza

Quoted 22 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

Quoted 8 lines

Quoted 4 lines

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

That's true. My idea here was simple: the user can't reliably exclude via snapshot ID today
because we're hiding the ID field in info snapshots:

    (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

Quoted 2 lines

Thus, what will end up happening is that the user will be forced to use the TAG of the snapshot since this is the only available information.

But you can get it through e.g. qemu-img info.

Quoted 5 lines

Snapshot list:
ID TAG VM SIZE DATE VM CLOCK
1 0 1.6M 2019-01-09 18:01:21 00:00:02.657

So it's not impossible to get.

Is there a reason why we should display the ID at all when you can't use it any more to identify snapshots?

Quoted 10 lines

Quoted 9 lines

Quoted 2 lines

I think we had a short discussion about just disallowing numeric snapshot names.  How bad would that be?

This was my first idea when evaluating what to do in this case. I gave it up because
I found it to be too extreme. People would start complaining "I was able to do
savevm 0 and now I can't".

Quoted 3 lines

True. But it wouldn't be impossible to do, we'd need to deprecate numeric names, print a warning for two releases, and then we can make it an error.

This. Is. HMP.

Not a stable ABI, no deprecation period of two releases.

Quoted 4 lines

Hm... If we had a proper deprecation warning in this series, I suppose it wouldn't be dangerous anymore. Can we just print a warning whenever the user specified an ID? (i.e. if some snapshot's ID matches the string given by the user and the snapshot's name does not)

How is it less of a problem when a user gets QEMU from the distro and skips five releases? They will never see the warning. This is different from QMP where things are fixed in libvirt, which is tested with every single QEMU release.

<general whining>
Actually, to me what you're saying sounds more like "Our deprecation policy is useless" to which I wholeheartedly agree. I think we should only remove things in major releases, and only if it was deprecated in the previous major release already. (So if you deprecate something in 4.0, you can remove it in 5.0; but if you deprecate it in 4.1, you can remove it only in 6.0.) From a user standpoint I really think we deprecate stuff too irregularly.
</general whining>

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

This. Is. HMP.

Not a stable ABI, no deprecation period of two releases.

Well, if you want to do it.

This may be HMP, but this is also the only interface to savevm, so it's not like users have a choice to use a more stable interface. I know that was a conscious decision, more or less, but I don't see why we need to be so nasty when the hardest thing about doing a nice deprecation would be to remember to make it an error in half a year.

Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:

Quoted 25 lines · Daniel Henrique Barboza

Quoted 24 lines

changes in v2:
- removed the "RFC" marker;
- added a new patch (patch 2) that removes
bdrv_snapshot_delete_by_id_or_name from the code;
- made changes in patch 1 as suggested by Murilo;
- previous patch set link:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.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.

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 simplifying the meaning of the parameters of savevm/loadvm/delvm.

Quoted 4 lines

(Yes, very late reply, I'm sorry...)

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

Can there be snapshots that can't be identified by a snapshot name, but only by their ID?

Quoted 3 lines

I think we had a short discussion about just disallowing numeric snapshot names. How bad would that be?

It would be incompatible with existing images and result in a more complex snapshot identifier resolution. Why would it be any better?

Kevin

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

I do think it affects users of HMP, because right now you can delete snapshots with their ID, and after this series you cannot.

This. Is. HMP.

Not a stable ABI, no deprecation period of two releases.

Quoted 7 lines

Well, if you want to do it.

This may be HMP, but this is also the only interface to savevm, so it's not like users have a choice to use a more stable interface. I know that was a conscious decision, more or less, but I don't see why we need to be so nasty when the hardest thing about doing a nice deprecation would be to remember to make it an error in half a year.

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 8 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 5 lines

<general whining>
Actually, to me what you're saying sounds more like "Our deprecation policy is useless" to which I wholeheartedly agree.

Quoted 6 lines

I think we should only remove things in major releases, and only if it was deprecated in the previous major release already. (So if you deprecate something in 4.0, you can remove it in 5.0; but if you deprecate it in 4.1, you can remove it only in 6.0.) From a user standpoint I really think we deprecate stuff too irregularly. </general whining>

That's actually incorrect. Our current version numbering scheme is that the major version number is NOT synonymous with major releases: we just bump the major version number once per year, and ALL releases are on equal footing with no one release being more major than others. Thus, a policy that (at least) 2 releases is needed for a deprecation is consistent, where one that requires waiting for a bump in the major version number (which is as short as one release and as long as 3, given that we bump every year with about 3 releases per year) is the one that is less predictable and less meaningful (why is waiting for January better than waiting for 2 releases?).

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 18 lines · Kevin Wolf

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 7 lines

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Kevin

Quoted 50 lines · * Kevin Wolf ([email protected]) wrote:

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 15 lines · Kevin Wolf

Quoted 14 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Quoted 8 lines

Quoted 1 line

Or does libvirt really use snapshot IDs rather than names?

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

Quoted 4 lines

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Can't we break this loop for the savevm command fairly easily; it's currently documnted as:

savevm [tag|id]

If we made that:

savevm [-t] [-i] [tag|id]

then:
a) with neither -t or -i it would behave in the same roulette way as it does in the moment, and it might be a tag or id

b) with -t we'd explicitly treat the parameter as a tag and it would error if it wasn't found

c) With -i we'd explicitly treat the parameter as an id and it would error if it wasn't found

Since we still allow (a) it doesn't break any existing code.

Dave

Quoted 1 line

Kevin

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 53 lines

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 18 lines · Kevin Wolf

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 7 lines

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

Quoted 5 lines

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Can't we break this loop for the savevm command fairly easily; it's currently documnted as:

savevm [tag|id]

If we made that:

savevm [-t] [-i] [tag|id]

then:
a) with neither -t or -i it would behave in the same roulette way as it does in the moment, and it might be a tag or id

b) with -t we'd explicitly treat the parameter as a tag and it would error if it wasn't found

c) With -i we'd explicitly treat the parameter as an id and it would error if it wasn't found

Since we still allow (a) it doesn't break any existing code.

Am 10.01.2019 um 12:41 hat Dr. David Alan Gilbert geschrieben:

Quoted 79 lines · * Kevin Wolf ([email protected]) wrote:

Quoted 53 lines

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 18 lines · Kevin Wolf

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 7 lines

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

Quoted 5 lines

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Can't we break this loop for the savevm command fairly easily; it's currently documnted as:

savevm [tag|id]

If we made that:

savevm [-t] [-i] [tag|id]

then:
a) with neither -t or -i it would behave in the same roulette way as it does in the moment, and it might be a tag or id

b) with -t we'd explicitly treat the parameter as a tag and it would error if it wasn't found

c) With -i we'd explicitly treat the parameter as an id and it would error if it wasn't found

Quoted 1 line

Since we still allow (a) it doesn't break any existing code.

If you can explain why we need both tag and id?

And by keeping the current behaviour, we might not break hypothetically existing correct code, but we leave currently actually existing broken code like libvirt broken.

Kevin

Quoted 16 lines

Quoted 6 lines

savevm [-t] [-i] [tag|id]

then:
a) with neither -t or -i it would behave in the same roulette way as it does in the moment, and it might be a tag or id

Quoted 7 lines

b) with -t we'd explicitly treat the parameter as a tag and it would error if it wasn't found

c) With -i we'd explicitly treat the parameter as an id and it would error if it wasn't found

Since we still allow (a) it doesn't break any existing code.

If you can explain why we need both tag and id?

Quoted 3 lines

And by keeping the current behaviour, we might not break hypothetically existing correct code, but we leave currently actually existing broken code like libvirt broken.

My only reason for leaving both tag & id was for the hypothetical existing current code; my assumption adding the above would be that we would then fix libvirt never to use (a), probably always (b).

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

savevm [-t] [-i] [tag|id]

then:
a) with neither -t or -i it would behave in the same roulette way as it does in the moment, and it might be a tag or id

Quoted 7 lines

b) with -t we'd explicitly treat the parameter as a tag and it would error if it wasn't found

c) With -i we'd explicitly treat the parameter as an id and it would error if it wasn't found

Since we still allow (a) it doesn't break any existing code.

If you can explain why we need both tag and id?

And by keeping the current behaviour, we might not break hypothetically existing correct code, but we leave currently actually existing broken code like libvirt broken.

Quoted 3 lines

My only reason for leaving both tag & id was for the hypothetical existing current code; my assumption adding the above would be that we would then fix libvirt never to use (a), probably always (b).

How? HMP is not introspectible, so libvirt can't know if 'savevm -t' works without trying it.

Quoted 10 lines · Max Reitz

<general whining>
Actually, to me what you're saying sounds more like "Our deprecation policy is useless" to which I wholeheartedly agree. I think we should only remove things in major releases, and only if it was deprecated in the previous major release already. (So if you deprecate something in 4.0, you can remove it in 5.0; but if you deprecate it in 4.1, you can remove it only in 6.0.) From a user standpoint I really think we deprecate stuff too irregularly.
</general whining>

That's actually incorrect. Our current version numbering scheme is that the major version number is NOT synonymous with major releases: we just bump the major version number once per year, and ALL releases are on equal footing with no one release being more major than others. Thus, a policy that (at least) 2 releases is needed for a deprecation is consistent, where one that requires waiting for a bump in the major version number (which is as short as one release and as long as 3, given that we bump every year with about 3 releases per year) is the one that is less predictable and less meaningful (why is waiting for January better than waiting for 2 releases?).

Quoted 48 lines · * Kevin Wolf ([email protected]) wrote:

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 18 lines · Kevin Wolf

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 7 lines

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Quoted 4 lines

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

Quoted 5 lines

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Can't we break this loop for the savevm command fairly easily; it's currently documnted as:

savevm [tag|id]

If we made that:

savevm [-t] [-i] [tag|id]

then:
a) with neither -t or -i it would behave in the same roulette way as it does in the moment, and it might be a tag or id

b) with -t we'd explicitly treat the parameter as a tag and it would error if it wasn't found

c) With -i we'd explicitly treat the parameter as an id and it would error if it wasn't found

Since we still allow (a) it doesn't break any existing code.

Quoted 49 lines · * Kevin Wolf ([email protected]) wrote:

Quoted 48 lines

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 18 lines · Kevin Wolf

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 7 lines

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Quoted 4 lines

Quoted 4 lines

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

So you're trying to make a bug fix out of this now to get around deprecation? To me, changing behavior in qemu to fix a bug in libvirt doesn't equate to fixing a bug in qemu. So let's try to find a real bug in qemu.

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Quoted 4 lines

Can't we break this loop for the savevm command fairly easily; it's currently documnted as:

savevm [tag|id]

The bug starts here.
[... snipped long description of how horrible everything is ...]

So, yes, there are bugs in qemu, but to fix them, we would need to switch the comparison order in bdrv_snapshot_find() and fix bdrv_snapshot_delete_by_id_or_name() to always invoke bdrv_snapshot_delete() for both cases (ID and tag). And we should probably remove the special code path in savevm for overwriting an existing snapshot.

This would also alleviate the bug in libvirt (because the tag then always takes precedence, like the interface description suggests).

Sure, this series would fix the bugs as well, but it does more than that. It really isn't just a bug fix.

Am 11.01.2019 um 14:22 hat Max Reitz geschrieben:

Quoted 55 lines · Dr. David Alan Gilbert

Quoted 53 lines · * Kevin Wolf ([email protected]) wrote:

Quoted 52 lines

Am 09.01.2019 um 20:02 hat Eric Blake geschrieben:

Quoted 18 lines · Kevin Wolf

Quoted 10 lines

Quoted 9 lines

Indeed, and libvirt IS using 'savevm' via HMP via QMP's human-monitor-command, since there is no QMP counterpart for internal snapshot. Even though lately we consistently tell people that internal snapshots are underdeveloped and you should use external snapshots, it does not get away from the fact that libvirt has been using 'savevm' to drive internal snapshots for years now, and that we MUST consider back-compat and/or add an introspectible QMP interface before making changes that would break libvirt.

Quoted 6 lines

Okay, so what does libvirt do when you request a snapshot with a numerical name? Without having looked at the code, the best case I would expect that it forbids them, and more realistically I suspect that we may actually fix a bug for libvirt by changing the semantics.

Or does libvirt really use snapshot IDs rather than names?

Quoted 7 lines

At the moment, libvirt does not place any restriction on internal snapshot names, but passes the user's name through without thought of whether it is an id or a name.

So yes, arguably tightening the semantics fixes a libvirt bug for libvirt having allowed internal snapshots to get into an inconsistent state.

So there are two scenarios to consider with respect to breaking backwards compatibility:

1. There may be code out there that relies on numeric arguments being interpreted as IDs. This code will break if we make this change and numeric snapshot names exist. That such code exists is speculation (even though plausible) and we don't know how widespread it is.

2. There may be code out there that blindly assumes that whatever it passes is interpreted as a name. Nobody considered that with a numeric snapshot name, it maybe misinterpreted as an ID. We know that this is true for libvirt, the single most used management tool for QEMU. More code like this may (and probably does) exist.

Essentially the same two categories exist for human users: those who somehow found out that QEMU accepts IDs as monitor command arguments and are using those (mitigated by not displaying IDs any more), and those who are trapped because they wanted to access a numeric name, but surprisingly got it interpreted as an ID. Both are speculation to some degree, but my guess is that the latter group is larger.

Given all this, this is a no-brainer for me. We simplify the interface, we simplify the code, we make things less confusing for manual users and we fix the management tool that everybody uses. How could we not make this change?

Quoted 4 lines

So you're trying to make a bug fix out of this now to get around deprecation? To me, changing behavior in qemu to fix a bug in libvirt doesn't equate to fixing a bug in qemu. So let's try to find a real bug in qemu.

I'm not making this a bug fix, but a interface cleanup that fixes a bug

as a side effect, which makes it only more appealing. I'm also not sure what exactly you mean by "get around deprecation".

If you mean the formal two releases deprecation period that is required by our deprecation policy, it doesn't apply because this is HMP and not a stable interface.

If you mean deprecation not because we must but because we're considering actual users, then I have described how making the change fixes things for more users than it could potentially break things

In
addition, I pointed out how a deprecation period is useless for user-facing changes.

I'm concluding that a "voluntary" deprecation for
two releases isn't helpful at all.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

But again, it falls in the category of "properly fixing this requires a lot of auditing, time, mental power, and unit tests", which makes it a rather daunting task to state for certain.

Fix the world before we can fix anything?

Quoted 7 lines

Quoted 4 lines

Can't we break this loop for the savevm command fairly easily; it's currently documnted as:

savevm [tag|id]

The bug starts here.
[... snipped long description of how horrible everything is ...]

Yes, it's horrible. Let's radically simplify the interface (and with it the code) by throwing away the useless ID stuff. Which happens to be exactly what this series was getting at.

There is no point in spending time for changing the current interface to make a little more sense while keeping its fundamental insanity unaddressed, then deprecate it, and then change it again to _actually_ make sense. Let's convert it directly to something that makes sense.

Quoted 9 lines

So, yes, there are bugs in qemu, but to fix them, we would need to switch the comparison order in bdrv_snapshot_find() and fix bdrv_snapshot_delete_by_id_or_name() to always invoke bdrv_snapshot_delete() for both cases (ID and tag). And we should probably remove the special code path in savevm for overwriting an existing snapshot.

This would also alleviate the bug in libvirt (because the tag then always takes precedence, like the interface description suggests).

Yes, at least tools which don't use IDs wouldn't be broken any more. But you removed none of the insanity. The same way addressing by tag was broken before, addressing by ID is broken now (previously, you need to check first that no ID exists while you want to address a tag; now, you need to check first that no tag exists while you want to address an ID).

In fact, if you're willing to go there, you break exactly the same users that would be broken if we dropped IDs from the interface completely. So why not do the full thing instead of stopping halfway? You're taking the potential problems without the advantages you could get.

Quoted 2 lines

Sure, this series would fix the bugs as well, but it does more than that. It really isn't just a bug fix.

It was never meant to be a bug fix (unless, of course, you consider insane interfaces a bug). But that it fixes a bug as a side effect contributes to the fact that breaking compatibility has not only downsides and that a deprecation period would be useless or even counterproductive.