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

[Kevin Wolf <kwolf@redhat.com>]

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