[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id |
Date: |
Wed, 20 Dec 2017 13:18:49 +0000 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
* 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
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, Dr. David Alan Gilbert, 2017/12/12
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, seeteena, 2017/12/12
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, Max Reitz, 2017/12/15
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, seeteena, 2017/12/18
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, Max Reitz, 2017/12/19
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, seeteena, 2017/12/19
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id,
Dr. David Alan Gilbert <=
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, Eric Blake, 2017/12/21
- Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id, Markus Armbruster, 2017/12/22