[Qemu-devel] [PATCH 0/3] snapshots: various updates

[Qemu-devel] [PATCH 0/3] snapshots: various updates

[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

Regards,

Miguel

Miguel Di Ciurcio Filho (3):
  monitor: make 'info snapshots' show only fully available snapshots
  savevm: Generate a name when run without one
  savevm: prevent snapshot overwriting

 qemu-monitor.hx |    7 ++--
 savevm.c        |   97 ++++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 76 insertions(+), 28 deletions(-)

[Qemu-devel] [PATCH 1/3] monitor: make 'info snapshots' show only fully available snapshots

[Miguel Di Ciurcio Filho]

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

[Qemu-devel] [PATCH 2/3] savevm: Generate a name when run without one

[Miguel Di Ciurcio Filho]

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

[Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting

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

[Qemu-devel] Re: [PATCH 3/3] savevm: prevent snapshot overwriting

[Kevin Wolf]

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

[Qemu-devel] Re: [PATCH 0/3] snapshots: various updates

[Kevin Wolf]

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