Re: [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Markus Armbruster]

Miguel Di Ciurcio Filho <address@hidden> writes:

> This patch address two issues.
>
> 1) 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.

Debatable.

> We add a '-f' parameter to savevm, to really force that to happen, in case the
> user really wants to.

Incompatible change, looks like it'll break libvirt.  Doesn't mean we
can't do it ever, but right now may not be the best time.  Perhaps after
savevm & friends are fully functional in QMP.

> 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.
>
> 2) When savevm is run without a name or id, the name stays blank.
>
> 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.

Huh?

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

Aha, now it makes sense.  Suggest to swap the paragraphs.

> (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'.
>
> TODO: I have no clue on how to create a timestamp string when using Windows.
>
> Signed-off-by: Miguel Di Ciurcio Filho <address@hidden>
> ---
>  qemu-monitor.hx |    9 ++++---
>  savevm.c        |   59 ++++++++++++++++++++++++++++++++----------------------
>  2 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 9c27b31..94e8178 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -275,14 +275,15 @@ 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] [name]",
> +        .help       = "save a VM snapshot. If no name is provided, a new one 
> will be generated"
> +                   "\n\t\t\t -f to overwrite an snapshot if it already 
> exists",
>          .mhandler.cmd = do_savevm,
>      },
>  
>  STEXI
> address@hidden savevm address@hidden|@var{id}]
> address@hidden savevm [-f] address@hidden
>  @findex savevm
>  Create a snapshot of the whole virtual machine. If @var{tag} is
>  provided, it is used as human readable identifier. If there is already
> diff --git a/savevm.c b/savevm.c
> index fb38e8a..ae6f29f 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1785,7 +1785,7 @@ static int del_existing_snapshots(Monitor *mon, const 
> char *name)
>  
>  void do_savevm(Monitor *mon, const QDict *qdict)
>  {
> -    BlockDriverState *bs, *bs1;
> +    BlockDriverState *bs_vm_state, *bs;

These renames make the patch harder to review.

>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
>      int ret;
>      QEMUFile *f;
> @@ -1796,7 +1796,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  #else
>      struct timeval tv;
>  #endif
> +    struct tm tm;
>      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;
> @@ -1813,8 +1815,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          }
>      }
>  
> -    bs = bdrv_snapshots();
> -    if (!bs) {
> +    bs_vm_state = bdrv_snapshots();
> +    if (!bs_vm_state) {
>          monitor_printf(mon, "No block device can accept snapshots\n");
>          return;
>      }
> @@ -1825,15 +1827,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
> @@ -1847,13 +1840,31 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  #endif
>      sn->vm_clock_nsec = qemu_get_clock(vm_clock);
>  
> -    /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(mon, name) < 0) {
> -        goto the_end;
> +    if (name) {
> +        ret = bdrv_snapshot_find(bs_vm_state, old_sn, 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 {
> +        /* TODO: handle Windows */
> +        localtime_r(&tv.tv_sec, &tm);
> +        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
>      }
>  
>      /* save the VM state */
> -    f = qemu_fopen_bdrv(bs, 1);
> +    f = qemu_fopen_bdrv(bs_vm_state, 1);
>      if (!f) {
>          monitor_printf(mon, "Could not open VM state file\n");
>          goto the_end;
> @@ -1867,23 +1878,23 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      /* create the snapshots */
> -
> -    bs1 = NULL;
> -    while ((bs1 = bdrv_next(bs1))) {
> -        if (bdrv_can_snapshot(bs1)) {
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {
> +        if (bdrv_can_snapshot(bs)) {
>              /* Write VM state size only to the image that contains the state 
> */
> -            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
> -            ret = bdrv_snapshot_create(bs1, sn);
> +            sn->vm_state_size = (bs_vm_state == bs ? vm_state_size : 0);
> +            ret = bdrv_snapshot_create(bs, sn);
>              if (ret < 0) {
>                  monitor_printf(mon, "Error while creating snapshot on 
> '%s'\n",
> -                               bdrv_get_device_name(bs1));
> +                               bdrv_get_device_name(bs));
>              }
>          }
>      }
>  
>   the_end:
> -    if (saved_vm_running)
> +    if (saved_vm_running) {
>          vm_start();
> +    }

I don't like style changes mixed up with functional changes in the same
patch.  It's fine if you have to touch the code anyway, but here you
don't.

>  }
>  
>  int load_vmstate(const char *name)