* [Qemu-devel] [PATCH 1/3] monitor: make 'info snapshots' show only fully available snapshots
2010-08-04 17:55 [Qemu-devel] [PATCH 0/3] snapshots: various updates Miguel Di Ciurcio Filho
@ 2010-08-04 17:55 ` Miguel Di Ciurcio Filho
2010-08-04 17:55 ` [Qemu-devel] [PATCH 2/3] savevm: Generate a name when run without one Miguel Di Ciurcio Filho
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Miguel Di Ciurcio Filho @ 2010-08-04 17:55 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Miguel Di Ciurcio Filho, armbru, lcapitulino
The output generated by 'info snapshots' shows only snapshots that exist on the
block device that saves the VM state. This output can cause an user to
erroneously try to load an snapshot that is not available on all block devices.
$ qemu-img snapshot -l xxtest.qcow2
Snapshot list:
ID TAG VM SIZE DATE VM CLOCK
1 1.5M 2010-07-26 16:51:52 00:00:08.599
2 1.5M 2010-07-26 16:51:53 00:00:09.719
3 1.5M 2010-07-26 17:26:49 00:00:13.245
4 1.5M 2010-07-26 19:01:00 00:00:46.763
$ qemu-img snapshot -l xxtest2.qcow2
Snapshot list:
ID TAG VM SIZE DATE VM CLOCK
3 0 2010-07-26 17:26:49 00:00:13.245
4 0 2010-07-26 19:01:00 00:00:46.763
Current output:
$ qemu -hda xxtest.qcow2 -hdb xxtest2.qcow2 -monitor stdio -vnc :0
QEMU 0.12.4 monitor - type 'help' for more information
(qemu) info snapshots
Snapshot devices: ide0-hd0
Snapshot list (from ide0-hd0):
ID TAG VM SIZE DATE VM CLOCK
1 1.5M 2010-07-26 16:51:52 00:00:08.599
2 1.5M 2010-07-26 16:51:53 00:00:09.719
3 1.5M 2010-07-26 17:26:49 00:00:13.245
4 1.5M 2010-07-26 19:01:00 00:00:46.763
Snapshots 1 and 2 do not exist on xxtest2.qcow, but they are displayed anyway.
This patch sumarizes the output to only show fully available snapshots.
New output:
(qemu) info snapshots
ID TAG VM SIZE DATE VM CLOCK
3 1.5M 2010-07-26 17:26:49 00:00:13.245
4 1.5M 2010-07-26 19:01:00 00:00:46.763
Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
savevm.c | 59 +++++++++++++++++++++++++++++++++++++++++++----------------
1 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/savevm.c b/savevm.c
index 4c0e5d3..9291cfb 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2004,8 +2004,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
void do_info_snapshots(Monitor *mon)
{
BlockDriverState *bs, *bs1;
- QEMUSnapshotInfo *sn_tab, *sn;
- int nb_sns, i;
+ QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
+ int nb_sns, i, ret, available;
+ int total;
+ int *available_snapshots;
char buf[256];
bs = bdrv_snapshots();
@@ -2013,27 +2015,52 @@ void do_info_snapshots(Monitor *mon)
monitor_printf(mon, "No available block device supports snapshots\n");
return;
}
- monitor_printf(mon, "Snapshot devices:");
- bs1 = NULL;
- while ((bs1 = bdrv_next(bs1))) {
- if (bdrv_can_snapshot(bs1)) {
- if (bs == bs1)
- monitor_printf(mon, " %s", bdrv_get_device_name(bs1));
- }
- }
- monitor_printf(mon, "\n");
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
if (nb_sns < 0) {
monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
return;
}
- monitor_printf(mon, "Snapshot list (from %s):\n",
- bdrv_get_device_name(bs));
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
- for(i = 0; i < nb_sns; i++) {
+
+ if (nb_sns == 0) {
+ monitor_printf(mon, "There is no snapshot available.\n");
+ return;
+ }
+
+ available_snapshots = qemu_mallocz(sizeof(int) * nb_sns);
+ total = 0;
+ for (i = 0; i < nb_sns; i++) {
sn = &sn_tab[i];
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ available = 1;
+ bs1 = NULL;
+
+ while ((bs1 = bdrv_next(bs1))) {
+ if (bdrv_can_snapshot(bs1) && bs1 != bs) {
+ ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
+ if (ret < 0) {
+ available = 0;
+ break;
+ }
+ }
+ }
+
+ if (available) {
+ available_snapshots[total] = i;
+ total++;
+ }
}
+
+ if (total > 0) {
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ for (i = 0; i < total; i++) {
+ sn = &sn_tab[available_snapshots[i]];
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ }
+ } else {
+ monitor_printf(mon, "There is no suitable snapshot available\n");
+ }
+
qemu_free(sn_tab);
+ qemu_free(available_snapshots);
+
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 2/3] savevm: Generate a name when run without one
2010-08-04 17:55 [Qemu-devel] [PATCH 0/3] snapshots: various updates Miguel Di Ciurcio Filho
2010-08-04 17:55 ` [Qemu-devel] [PATCH 1/3] monitor: make 'info snapshots' show only fully available snapshots Miguel Di Ciurcio Filho
@ 2010-08-04 17:55 ` Miguel Di Ciurcio Filho
2010-08-04 17:55 ` [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting Miguel Di Ciurcio Filho
2010-08-30 14:39 ` [Qemu-devel] Re: [PATCH 0/3] snapshots: various updates Kevin Wolf
3 siblings, 0 replies; 6+ messages in thread
From: Miguel Di Ciurcio Filho @ 2010-08-04 17:55 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Miguel Di Ciurcio Filho, armbru, lcapitulino
When savevm is run without a name, the name stays blank and the snapshot is
saved anyway.
The new behavior is when savevm is run without parameters a name will be
created automaticaly, so the snapshot is accessible to the user without needing
the id when loadvm is run.
(qemu) savevm
(qemu) info snapshots
ID TAG VM SIZE DATE VM CLOCK
1 vm-20100728134640 978K 2010-07-28 13:46:40 00:00:08.603
We use a name with the format 'vm-YYYYMMDDHHMMSS'.
This is a first step to hide the internal id, because I don't see a reason to
expose this kind of internals to the user.
Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
savevm.c | 29 ++++++++++++++++++++---------
1 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/savevm.c b/savevm.c
index 9291cfb..025bee6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1799,8 +1799,10 @@ void do_savevm(Monitor *mon, const QDict *qdict)
uint32_t vm_state_size;
#ifdef _WIN32
struct _timeb tb;
+ struct tm *ptm;
#else
struct timeval tv;
+ struct tm tm;
#endif
const char *name = qdict_get_try_str(qdict, "name");
@@ -1831,15 +1833,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
vm_stop(0);
memset(sn, 0, sizeof(*sn));
- if (name) {
- ret = bdrv_snapshot_find(bs, old_sn, name);
- if (ret >= 0) {
- pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
- pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
- } else {
- pstrcpy(sn->name, sizeof(sn->name), name);
- }
- }
/* fill auxiliary fields */
#ifdef _WIN32
@@ -1853,6 +1846,24 @@ void do_savevm(Monitor *mon, const QDict *qdict)
#endif
sn->vm_clock_nsec = qemu_get_clock(vm_clock);
+ if (name) {
+ ret = bdrv_snapshot_find(bs, old_sn, name);
+ if (ret >= 0) {
+ pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
+ pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+ } else {
+ pstrcpy(sn->name, sizeof(sn->name), name);
+ }
+ } else {
+#ifdef _WIN32
+ ptm = localtime(&tb.time);
+ strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", ptm);
+#else
+ localtime_r(&tv.tv_sec, &tm);
+ strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
+#endif
+ }
+
/* Delete old snapshots of the same name */
if (name && del_existing_snapshots(mon, name) < 0) {
goto the_end;
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting
2010-08-04 17:55 [Qemu-devel] [PATCH 0/3] snapshots: various updates Miguel Di Ciurcio Filho
2010-08-04 17:55 ` [Qemu-devel] [PATCH 1/3] monitor: make 'info snapshots' show only fully available snapshots Miguel Di Ciurcio Filho
2010-08-04 17:55 ` [Qemu-devel] [PATCH 2/3] savevm: Generate a name when run without one Miguel Di Ciurcio Filho
@ 2010-08-04 17:55 ` Miguel Di Ciurcio Filho
2010-08-30 14:28 ` [Qemu-devel] " Kevin Wolf
2010-08-30 14:39 ` [Qemu-devel] Re: [PATCH 0/3] snapshots: various updates Kevin Wolf
3 siblings, 1 reply; 6+ messages in thread
From: Miguel Di Ciurcio Filho @ 2010-08-04 17:55 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Miguel Di Ciurcio Filho, armbru, lcapitulino
When savevm is run using an previously saved snapshot id or name, it will
delete the original and create a new one, using the same id and name and not
prompting the user of what just happened.
This behaviour is not good, IMHO.
We add a '-f' parameter to savevm, to really force that to happen, in case the
user really wants to.
New behavior:
(qemu) savevm snap1
An snapshot named 'snap1' already exists
(qemu) savevm -f snap1
We do better error reporting in case '-f' is used too than before and don't
reuse the previous id.
Note: This patch depends on "savevm: Generate a name when run without one"
Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
qemu-monitor.hx | 7 ++++---
savevm.c | 19 ++++++++++++++-----
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 2af3de6..683ac73 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -275,9 +275,10 @@ ETEXI
{
.name = "savevm",
- .args_type = "name:s?",
- .params = "[tag|id]",
- .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+ .args_type = "force:-f,name:s?",
+ .params = "[-f] [tag|id]",
+ .help = "save a VM snapshot. If no tag is provided, a new one is created"
+ "\n\t\t\t -f to overwrite an snapshot if it already exists",
.mhandler.cmd = do_savevm,
},
diff --git a/savevm.c b/savevm.c
index 025bee6..f0a4b78 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1805,6 +1805,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
struct tm tm;
#endif
const char *name = qdict_get_try_str(qdict, "name");
+ int force = qdict_get_try_bool(qdict, "force", 0);
/* Verify if there is a device that doesn't support snapshots and is writable */
bs = NULL;
@@ -1848,12 +1849,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
if (name) {
ret = bdrv_snapshot_find(bs, old_sn, name);
- if (ret >= 0) {
- pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
- pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
- } else {
- pstrcpy(sn->name, sizeof(sn->name), name);
+ if (ret == 0) {
+ if (force) {
+ ret = del_existing_snapshots(mon, name);
+ if (ret < 0) {
+ monitor_printf(mon, "Error deleting snapshot '%s', error: %d\n", name, ret);
+ goto the_end;
+ }
+ } else {
+ monitor_printf(mon, "An snapshot named '%s' already exists\n", name);
+ goto the_end;
+ }
}
+
+ pstrcpy(sn->name, sizeof(sn->name), name);
} else {
#ifdef _WIN32
ptm = localtime(&tb.time);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] Re: [PATCH 3/3] savevm: prevent snapshot overwriting
2010-08-04 17:55 ` [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting Miguel Di Ciurcio Filho
@ 2010-08-30 14:28 ` Kevin Wolf
0 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2010-08-30 14:28 UTC (permalink / raw)
To: Miguel Di Ciurcio Filho; +Cc: armbru, qemu-devel, lcapitulino
Am 04.08.2010 19:55, schrieb Miguel Di Ciurcio Filho:
> When savevm is run using an previously saved snapshot id or name, it will
> delete the original and create a new one, using the same id and name and not
> prompting the user of what just happened.
>
> This behaviour is not good, IMHO.
>
> We add a '-f' parameter to savevm, to really force that to happen, in case the
> user really wants to.
>
> New behavior:
> (qemu) savevm snap1
> An snapshot named 'snap1' already exists
>
> (qemu) savevm -f snap1
>
> We do better error reporting in case '-f' is used too than before and don't
> reuse the previous id.
>
> Note: This patch depends on "savevm: Generate a name when run without one"
>
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
I think what this patch is doing is not enough. It only takes
bs_snapshots into consideration, but will continue to silently overwrite
snapshots in other images. This is where I would consider this check
most valuable. I'd like to have this full check implemented before
applying this patch.
By the way, I've also hit yet another case which is similar, an ID
conflict. Assume I have hda with only one snapshot with ID 1 and hdb
with snapshots with IDs 1, 2 and 3. savevm will pick hda as
bs_snapshots, decide that ID 2 is free, start creating the snapshot and
fail when it tries to snapshot hdb.
Not requesting a fix for the latter, though, with UUIDs this is going to
be fixed anyway.
> ---
> qemu-monitor.hx | 7 ++++---
> savevm.c | 19 ++++++++++++++-----
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 2af3de6..683ac73 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -275,9 +275,10 @@ ETEXI
>
> {
> .name = "savevm",
> - .args_type = "name:s?",
> - .params = "[tag|id]",
> - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> + .args_type = "force:-f,name:s?",
> + .params = "[-f] [tag|id]",
> + .help = "save a VM snapshot. If no tag is provided, a new one is created"
> + "\n\t\t\t -f to overwrite an snapshot if it already exists",
> .mhandler.cmd = do_savevm,
> },
>
> diff --git a/savevm.c b/savevm.c
> index 025bee6..f0a4b78 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1805,6 +1805,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> struct tm tm;
> #endif
> const char *name = qdict_get_try_str(qdict, "name");
> + int force = qdict_get_try_bool(qdict, "force", 0);
>
> /* Verify if there is a device that doesn't support snapshots and is writable */
> bs = NULL;
> @@ -1848,12 +1849,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>
> if (name) {
> ret = bdrv_snapshot_find(bs, old_sn, name);
> - if (ret >= 0) {
> - pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> - pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
The id_str copy is completely dropped. Before the change, if you
overwrite a snapshot, you'd get a new one with the same ID. Now it's
assigned a new ID.
This is probably a good thing, as it's no longer compatible with an
older snapshot saved on a second disk which is currently not attached.
But it should be clearly mentioned in the commit message.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH 0/3] snapshots: various updates
2010-08-04 17:55 [Qemu-devel] [PATCH 0/3] snapshots: various updates Miguel Di Ciurcio Filho
` (2 preceding siblings ...)
2010-08-04 17:55 ` [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting Miguel Di Ciurcio Filho
@ 2010-08-30 14:39 ` Kevin Wolf
3 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2010-08-30 14:39 UTC (permalink / raw)
To: Miguel Di Ciurcio Filho; +Cc: armbru, qemu-devel, lcapitulino
Am 04.08.2010 19:55, schrieb Miguel Di Ciurcio Filho:
> Hi there!
>
> This series introduces updates the 'info snapshots' and 'savevm' commands.
>
> Patch 1 summarizes the output of 'info snapshots' to show only fully
> available snapshots.
>
> Patch 2 adds a default name to an snapshot in case the user did not provide one,
> using a template like vm-YYYYMMDDHHMMSS.
>
> Patch 3 adds -f to the 'savevm' command in case the use really wants to
> overwrite an snapshot.
>
> More details in each patch.
>
> Changelog from previous version:
> --------------------------------
> - libvirt is not affected by the change in savevm
> - Fixed some coding errors and do not rename the name of variables
Thanks, applied patch 1 and 2 to the block branch. Commented on patch 3.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread