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

[Murilo Opsfelder Araujo]

Hi, Daniel.

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

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

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

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

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

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

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

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

-- 
Murilo