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

[Daniel Henrique Barboza]

On 1/9/19 12:21 PM, Kevin Wolf wrote:
> Am 09.01.2019 um 15:10 hat Max Reitz geschrieben:
> > 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.
> Can there be snapshots that can't be identified by a snapshot name, but
> only by their ID?

Hmmm I would need to read my notes back when I was working on this series.
In a quick look at the code and playing around with HMP, what I can tell 
is that:

- there is no way for a snapshot to have an empty TAG - an auto-generated
name is generated and used as TAG.

-  if you create a snapshot using an existing TAG, the older snapshot is 
overwritten
without warning (would be nice to fix it to at least provide a warning 
message - I can
do it in this series if a respin is needed)


I do not have the expertise of the whole block layer to see further 
implications of
the changes made in this series, but as far as HMP goes, I don't think 
there's too much
to it other than what I've said in the cover letter. The goal here is to 
remove the
ambiguity of save/load/del vm commands that would interpret the argument
either as tag or ID, leading to situations in which snapshots ended up 
getting
overwritten because the tag matched an existing ID.


I am not against providing a warning message if the user tries to create 
a snapshot
using a numerical tag. In the end, although I said otherwise in the 
cover letter, changing
the meaning of an input is still an API change, and I'd rather error on 
the cautious side
and warn the callers about it. Even if this has no/little impact (please 
feel free to prove
me wrong) on them.



Daniel






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