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

[Kevin Wolf]

Am 09.01.2019 um 15:27 hat Max Reitz geschrieben:
> On 09.01.19 15:21, 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?
> 
> I don't know, but what I meant is that if you have scripts to do all
> this, you might have to adjust them with this change.

That's what the H in HMP means.

> >> 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?
> 
> It wouldn't be incompatible with existing images if we'd just disallow
> creating such snapshots.  I don't see how the identifier resolution
> would be more complex.
> 
> I don't know if it'd be better.  I'd just find it simpler (for us, that
> is -- for users, I'm not sure).

Identifier resolution A:
- Find a snapshot that has the given identifier as a name
- If no such snapshot exists, it is an error

Identifier resolution B:
- If the identifier is a number, find a snapshot that has the given
  identifier as its ID
- If the identifier is not a number, find a snapshot that has the given
  identifier as a name
- If no such snapshot exists, it is an error

Isn't it rather obvious that B is more complex than A?

Kevin

signature.asc
Description: PGP signature