* Max Reitz (mre...@redhat.com) 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 / dgilb...@redhat.com / Manchester, UK
