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

[Dr. David Alan Gilbert]

* Max Reitz (address@hidden) 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.

Is this something we're in a position to change, or is it part of the
API that user programs can end up using?

Dave

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



--
Dr. David Alan Gilbert / address@hidden / Manchester, UK