[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v1 0/2] HMP/snapshot changes - do not use ID
From: |
Murilo Opsfelder Araujo |
Subject: |
Re: [Qemu-devel] [RFC PATCH v1 0/2] HMP/snapshot changes - do not use ID anymore |
Date: |
Wed, 22 Aug 2018 19:04:40 -0300 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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