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