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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

Am 09.01.2019 um 18:05 hat Max Reitz geschrieben:
> On 09.01.19 17:57, Daniel Henrique Barboza wrote:
> > 
> > 
> > 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.
> 
> But you can get it through e.g. qemu-img info.
> 
> Snapshot list:
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1         0                      1.6M 2019-01-09 18:01:21   00:00:02.657
> 
> So it's not impossible to get.

Is there a reason why we should display the ID at all when you can't use
it any more to identify snapshots?

> >> 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".
> 
> True.  But it wouldn't be impossible to do, we'd need to deprecate
> numeric names, print a warning for two releases, and then we can make it
> an error.

This. Is. HMP.

Not a stable ABI, no deprecation period of two releases.

> Hm...  If we had a proper deprecation warning in this series, I suppose
> it wouldn't be dangerous anymore.  Can we just print a warning whenever
> the user specified an ID?  (i.e. if some snapshot's ID matches the
> string given by the user and the snapshot's name does not)

How is it less of a problem when a user gets QEMU from the distro and
skips five releases? They will never see the warning. This is different
from QMP where things are fixed in libvirt, which is tested with every
single QEMU release.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]