Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore

[Daniel Henrique Barboza]

On 1/9/19 12:10 PM, Max Reitz wrote:
> On 06.09.18 13:11, Daniel Henrique Barboza 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.
> >
> > 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.


> 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".



> Max