Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id

[seeteena <s1seetee@linux.vnet.ibm.com>]

On 12/19/2017 07:50 PM, Max Reitz wrote:
> On 2017-12-18 10:24, seeteena wrote:
>>
>> On 12/15/2017 02:48 PM, Max Reitz wrote:
>>> On 2017-12-13 05:50, seeteena wrote:
>>>> Hi David,
>>>>
>>>> While creating snapshots when tag name '0' used in the very first and followed by tag name '1' then snapshot created with tag name '0' is getting erased.
>>>> A snapshot is identified by a name computed either from an id, which is basically a numerical counter starting at 1 for qcow2, or from a tag, which is a string (provided
>>>> by the user or automatically computed).
>>>>
>>>> (qemu) savevm 0
>>>>
>>>> This creates a snapshot with tag '0' and id '1'.
>>> That's something someone has complained about already, as far as I
>>> remember, and this is indeed an issue.
>>>
>>>>> (qemu) savevm 1
>>>> This deletes snapshot with name '1' (ie, with id '1') and creates snapshot with tag '1' and id '1'.
>>> I think this is the issue, not info snapshots.
>>>
>>>>  From the output of 'info snapshots' id '1' is not seen for tag '0' instead seeing an empty field.
>>>>
>>>> ID        TAG                 VM SIZE                DATE       VM CLOCK
>>>> --        0                      338M 2017-10-16 13:44:35   00:02:07.491
>>>>
>>>> If an ID shown as '1' in the above then user can understand the above documented stuff i.e "If there is already a snapshot with the same tag or ID, it is replaced".
>>>> The '--' in ID field is annoying.
>>> Maybe, but this patch is wrong still.  Commit 3a1ee711904 says exactly why:
>>>
>>>> The patch uses snapshot name instead of snapshot id to determine whether a
>>>> snapshot is fully available and uses '--' instead of snapshot id in output
>>>> because the snapshot id is not guaranteed to be the same on all images.
>>> info snapshots first shows a list of snapshots that are present on all
>>> disks.  However, those are matched by name and not by ID, so the ID is
>>> not necessarily the same.
>>>
>>> Therefore, we can only print it if it is.  Sure, we can do that, but
>>> your patch is missing that check.
>>   As you referred I had checked Commit 3a1ee711904. Does it mean that we
>> need to add the check option for ID as well along with name to get the
>> list of snapshots that are present on all disks with my patch ?
>> so info snapshots shows list of snapshots that are present on all disks
>> that matched either by ID or name.
>>
>> like
>> if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) ||
>> (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0)
> Kind of true, but I think that first this is a separate issue and
> secondly, this can get rather hairy.  (See below under (3).)
>
> So there are three things:
>
> (1) We probably should not allow snapshot names that could be IDs.
> Easiest way to solve this: Names have to start with a non-digit.
>
> (2) If we want to print a global snapshot's common ID, we need to affirm
> that this ID is indeed the same on all disks before we can print it.
> Same for names, but currently the name is always the same on all disks
> because that is how we identify global snapshots.
>
> (3) You can give an ID to loadvm and then it will load the snapshot with
> that ID from all disks.  So if you have snapshots with a common ID on
> all disks, these are kind of global snapshots, too, even though they
> don't share a name.  Thus, they should probably be included in the
> listing (this is what you have just proposed).
> I don't like this at all, though.  A snapshot's ID is not really
> user-controlled, it's just some auto-generated number.  Therefore, just
> because the ID of a snapshot matches across multiple disks, this doesn't
> mean that they are related whatsoever.
> So, first, I don't think loadvm should work with IDs (at least not
> across multiple disks).  But I don't think this really needs to be fixed.
> On the other hand, I really don't think info snapshots should list
> snapshots that match by ID, because a matching ID does not mean that
> snapshots are actually related.  A matching name usually does, though,
> so I think what we currently do is sufficient and the right way to do it.
>
> Max
>

Sounds great.. Thank you Max for the detail explanation. I will go 
fixing (1) that would fix savevm issue (savevm overwrites snapshots when 
tags 0 and 1 is used consecutively) so we don't want ID to expose 
through info snapshots and will continue with the current implementation.