(qemu) info snapshots
List of snapshots present on all disks:
ID TAG VM SIZE DATE VM CLOCK
-- 1 314M 2017-11-15 15:22:18 00:02:25.695
-- 2 319M 2017-11-15 15:23:03 00:02:45.970
-- 3 319M 2017-11-15 15:23:37 00:02:56.328
ID field is showing -- with no information.
Signed-off-by: Seeteena Thoufeek <s1seetee@linux.vnet.ibm.com>
---
hmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hmp.c b/hmp.c
index 35a7041..1921b3b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1454,7 +1454,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
/* The ID is not guaranteed to be the same on all images, so
* overwrite it.
*/
- pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
+ pstrcpy(sn->id_str, sizeof(sn->id_str), sn->id_str);
bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
monitor_printf(mon, "\n");
}
--
1.8.3.1
* Seeteena Thoufeek (s1seetee@linux.vnet.ibm.com) wrote: > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLOCK > -- 1 314M 2017-11-15 15:22:18 00:02:25.695 > -- 2 319M 2017-11-15 15:23:03 00:02:45.970 > -- 3 319M 2017-11-15 15:23:37 00:02:56.328 > > ID field is showing -- with no information. > > Signed-off-by: Seeteena Thoufeek <s1seetee@linux.vnet.ibm.com> Hi Seeteena, Your change doesn't seem to be consistent with the comment above it. I don't know the detailed history of snapshots, but that comment and the pstrcpy comes from Lin Ma's commit 3a1ee711 which seems to be saying the ID can't be trusted. Dave > --- > hmp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hmp.c b/hmp.c > index 35a7041..1921b3b 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1454,7 +1454,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > /* The ID is not guaranteed to be the same on all images, so > * overwrite it. > */ > - pstrcpy(sn->id_str, sizeof(sn->id_str), "--"); > + pstrcpy(sn->id_str, sizeof(sn->id_str), sn->id_str); > bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); > monitor_printf(mon, "\n"); > } > -- > 1.8.3.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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'. > (qemu) savevm 1 This deletes snapshot with name '1' (ie, with id '1') and creates snapshot with tag '1' and id '1'. 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. On 12/12/2017 10:11 PM, Dr. David Alan Gilbert wrote: > * Seeteena Thoufeek (s1seetee@linux.vnet.ibm.com) wrote: >> (qemu) info snapshots >> List of snapshots present on all disks: >> ID TAG VM SIZE DATE VM CLOCK >> -- 1 314M 2017-11-15 15:22:18 00:02:25.695 >> -- 2 319M 2017-11-15 15:23:03 00:02:45.970 >> -- 3 319M 2017-11-15 15:23:37 00:02:56.328 >> >> ID field is showing -- with no information. >> >> Signed-off-by: Seeteena Thoufeek <s1seetee@linux.vnet.ibm.com> > Hi Seeteena, > Your change doesn't seem to be consistent with the comment above it. > I don't know the detailed history of snapshots, but that comment and the > pstrcpy comes from Lin Ma's commit 3a1ee711 which seems to be saying the > ID can't be trusted. > > Dave > >> --- >> hmp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hmp.c b/hmp.c >> index 35a7041..1921b3b 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -1454,7 +1454,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) >> /* The ID is not guaranteed to be the same on all images, so >> * overwrite it. >> */ >> - pstrcpy(sn->id_str, sizeof(sn->id_str), "--"); >> + pstrcpy(sn->id_str, sizeof(sn->id_str), sn->id_str); >> bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); >> monitor_printf(mon, "\n"); >> } >> -- >> 1.8.3.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
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. Example: $ qemu-img create -f qcow2 foo.qcow2 64M Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img create -f qcow2 bar.qcow2 64M Formatting 'bar.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img snapshot -c common_snapshot foo.qcow2 $ qemu-img snapshot -c bar_snapshot bar.qcow2 $ qemu-img snapshot -c common_snapshot bar.qcow2 $ qemu-system-x86_64 -hda foo.qcow2 -hdb bar.qcow2 -monitor stdio $ qemu-img snapshot -l foo.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 common_snapshot 0 2017-12-15 10:09:17 00:00:00.000 $ qemu-img snapshot -l bar.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 bar_snapshot 0 2017-12-15 10:09:25 00:00:00.000 2 common_snapshot 0 2017-12-15 10:09:28 00:00:00.000 QEMU 2.9.1 monitor - type 'help' for more information (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- common_snapshot 0 2017-12-15 10:09:17 00:00:00.000 List of partial (non-loadable) snapshots on 'ide0-hd1': ID TAG VM SIZE DATE VM CLOCK 1 bar_snapshot 0 2017-12-15 10:09:25 00:00:00.000 With your patch: (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK 1 common_snapshot 0 2017-12-15 10:09:17 00:00:00.000 List of partial (non-loadable) snapshots on 'ide0-hd1': ID TAG VM SIZE DATE VM CLOCK 1 bar_snapshot 0 2017-12-15 10:09:25 00:00:00.000 Max
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) > > Example: > > $ qemu-img create -f qcow2 foo.qcow2 64M > Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > $ qemu-img create -f qcow2 bar.qcow2 64M > Formatting 'bar.qcow2', fmt=qcow2 size=67108864 encryption=off > cluster_size=65536 lazy_refcounts=off refcount_bits=16 > $ qemu-img snapshot -c common_snapshot foo.qcow2 > $ qemu-img snapshot -c bar_snapshot bar.qcow2 > $ qemu-img snapshot -c common_snapshot bar.qcow2 > $ qemu-system-x86_64 -hda foo.qcow2 -hdb bar.qcow2 -monitor stdio > $ qemu-img snapshot -l foo.qcow2 > Snapshot list: > ID TAG VM SIZE DATE VM CLOCK > 1 common_snapshot 0 2017-12-15 10:09:17 00:00:00.000 > $ qemu-img snapshot -l bar.qcow2 > Snapshot list: > ID TAG VM SIZE DATE VM CLOCK > 1 bar_snapshot 0 2017-12-15 10:09:25 00:00:00.000 > 2 common_snapshot 0 2017-12-15 10:09:28 00:00:00.000 > QEMU 2.9.1 monitor - type 'help' for more information > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLOCK > -- common_snapshot 0 2017-12-15 10:09:17 00:00:00.000 > > List of partial (non-loadable) snapshots on 'ide0-hd1': > ID TAG VM SIZE DATE VM CLOCK > 1 bar_snapshot 0 2017-12-15 10:09:25 00:00:00.000 > > With your patch: > > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLOCK > 1 common_snapshot 0 2017-12-15 10:09:17 00:00:00.000 > > List of partial (non-loadable) snapshots on 'ide0-hd1': > ID TAG VM SIZE DATE VM CLOCK > 1 bar_snapshot 0 2017-12-15 10:09:25 00:00:00.000 > > Max >
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
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.
* Max Reitz (mreitz@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 / dgilbert@redhat.com / Manchester, UK
On 12/19/2017 08:20 AM, Max Reitz wrote: > 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. Yes, that would be a nice change. It is not strictly backwards compatible (so we'd still have to cope with images that didn't follow the rule, whether created by older qemu or by non-qemu implementations of qcow2), but would alleviate a lot of confusion. > > (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 > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes: > On 12/19/2017 08:20 AM, Max Reitz wrote: > >> 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. > > Yes, that would be a nice change. It is not strictly backwards > compatible (so we'd still have to cope with images that didn't follow > the rule, whether created by older qemu or by non-qemu implementations > of qcow2), but would alleviate a lot of confusion. I recommend to restrict ID strings to letters, digits, '-', '.', '_', starting with a letter. Use id_wellformed() to check. If backward compatibility is an issue, deprecate offending IDs (with a suitable warning), and kill them off after the customary grace period. IDs embedded in image files and such you may have to keep working somehow indefinitely.
On 12/22/2017 01:07 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 12/19/2017 08:20 AM, Max Reitz wrote: >> >>> 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. >> Yes, that would be a nice change. It is not strictly backwards >> compatible (so we'd still have to cope with images that didn't follow >> the rule, whether created by older qemu or by non-qemu implementations >> of qcow2), but would alleviate a lot of confusion. > I recommend to restrict ID strings to letters, digits, '-', '.', '_', > starting with a letter. Use id_wellformed() to check. char *id_generate(IdSubSystems id) autogenates ID in the format eg- "#block146" with starting with '#' always. restricting ID strings to use id_wellformed() means we need to force ID to start with letter. #define ID_SPECIAL_CHAR '#' needs to change to (some letter) #define ID_SPECIAL_CHAR 'A' > > If backward compatibility is an issue, deprecate offending IDs (with a > suitable warning), and kill them off after the customary grace period. > > IDs embedded in image files and such you may have to keep working > somehow indefinitely. >
seeteena <s1seetee@linux.vnet.ibm.com> writes: > On 12/22/2017 01:07 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 12/19/2017 08:20 AM, Max Reitz wrote: >>> >>>> 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. >>> Yes, that would be a nice change. It is not strictly backwards >>> compatible (so we'd still have to cope with images that didn't follow >>> the rule, whether created by older qemu or by non-qemu implementations >>> of qcow2), but would alleviate a lot of confusion. >> I recommend to restrict ID strings to letters, digits, '-', '.', '_', >> starting with a letter. Use id_wellformed() to check. > char *id_generate(IdSubSystems id) autogenates ID in the format eg- > "#block146" with starting with '#' always. > restricting ID strings to use id_wellformed() means we need to force > ID to start with letter. > > #define ID_SPECIAL_CHAR '#' needs to change to (some letter) > #define ID_SPECIAL_CHAR 'A' Ensuring auto-generated IDs cannot clash with IDs chosen by the user is a good idea. We generally fail to do that in QEMU. An obvious way to do it is to require the user's IDs to start with a letter, and auto-generated IDs to start with another suitable character. In theory, starting with '#' is problematic, because it needs to be quoted in shell at the beginning of words. Only matters if auto-generated IDs make sense in command lines, which I guess they currently don't. I'd prefer a 'shell-safe' character all the same. >> If backward compatibility is an issue, deprecate offending IDs (with a >> suitable warning), and kill them off after the customary grace period. >> >> IDs embedded in image files and such you may have to keep working >> somehow indefinitely.
On 01/02/2018 01:54 PM, Markus Armbruster wrote: > seeteena <s1seetee@linux.vnet.ibm.com> writes: > >> On 12/22/2017 01:07 PM, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> On 12/19/2017 08:20 AM, Max Reitz wrote: >>>> >>>>> 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. >>>> Yes, that would be a nice change. It is not strictly backwards >>>> compatible (so we'd still have to cope with images that didn't follow >>>> the rule, whether created by older qemu or by non-qemu implementations >>>> of qcow2), but would alleviate a lot of confusion. >>> I recommend to restrict ID strings to letters, digits, '-', '.', '_', >>> starting with a letter. Use id_wellformed() to check. >> char *id_generate(IdSubSystems id) autogenates ID in the format eg- >> "#block146" with starting with '#' always. >> restricting ID strings to use id_wellformed() means we need to force >> ID to start with letter. >> >> #define ID_SPECIAL_CHAR '#' needs to change to (some letter) >> #define ID_SPECIAL_CHAR 'A' > Ensuring auto-generated IDs cannot clash with IDs chosen by the user is > a good idea. We generally fail to do that in QEMU. > > An obvious way to do it is to require the user's IDs to start with a > letter, and auto-generated IDs to start with another suitable character. > In theory, starting with '#' is problematic, because it needs to be > quoted in shell at the beginning of words. Only matters if > auto-generated IDs make sense in command lines, which I guess they > currently don't. I'd prefer a 'shell-safe' character all the same. I was using loaned system from other team for recreate and it appears there is some issue in that system and I am not able to recreate the issue now. QEMU 2.11.50 monitor - type 'help' for more information (qemu) savevm 0 Error while creating snapshot on 'rootdisk' (qemu) savevm 0 Error while creating snapshot on 'rootdisk' (qemu) savevm 0 Error while creating snapshot on 'rootdisk' (qemu) savevm 1 Error while creating snapshot on 'rootdisk' (qemu) > >>> If backward compatibility is an issue, deprecate offending IDs (with a >>> suitable warning), and kill them off after the customary grace period. >>> >>> IDs embedded in image files and such you may have to keep working >>> somehow indefinitely.
© 2016 - 2024 Red Hat, Inc.