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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature