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

[Max Reitz]

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.

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

Max

signature.asc
Description: OpenPGP digital signature