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

[Kevin Wolf]

Am 09.01.2019 um 17:27 hat Max Reitz geschrieben:
> On 09.01.19 16:13, Kevin Wolf wrote:
> > Am 09.01.2019 um 15:54 hat Max Reitz geschrieben:
> >> On 09.01.19 15:48, Kevin Wolf wrote:
> >>> 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
> >>
> >> No, my idea was to keep the resolution the same as it is; just to forbid
> >> creating new snapshots with numeric names.  This would prevent users
> >> from getting into the whole situation.
> > 
> > That's the version with an even more complex resolution method C. :-)
> 
> How so if the resolution method stays the same?  Because it already is
> too complex?
> 
> If so, yes, that is an argument.  I was arguing for the simplest patch
> instead of the simplest code, true.

Yes, because it already is too complex. Not even necessarily the code
(even though that's true as well), but most importantly the interface.

> > I actually think the current behaviour is more confusing than helpful.
> > Without looking into the code or trying it out, I couldn't even tell
> > whether ID or name takes precedence if there is a matching snapshot for
> > both. Considering your proposal, it's probably the ID, but how should a
> > user know that? (If against all expectations documentation exists, it
> > doesn't count because nobody reads that.)
> 
> It isn't more confusing than it is right now.  With my proposal, all
> current images are simply as confusing as they are right now (I think ID
> takes precedence, yes), but if you create new snapshots, it's clear,
> since you simply cannot create names that could be IDs.

I agree. But wasn't the goal of the patch to make it less confusing than
it is right now?

Kevin

signature.asc
Description: PGP signature