Re: [PATCH v4 5/9] migration: control whether snapshots are ovewritten

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The traditional HMP "savevm" command will overwrite an existing snapshot
> if it already exists with the requested name. This new flag allows this
> to be controlled allowing for safer behaviour with a future QMP command.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/migration/snapshot.h | 2 +-
>  migration/savevm.c           | 4 ++--
>  monitor/hmp-cmds.c           | 2 +-
>  replay/replay-snapshot.c     | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
> index c85b6ec75b..d7db1174ef 100644
> --- a/include/migration/snapshot.h
> +++ b/include/migration/snapshot.h
> @@ -15,7 +15,7 @@
>  #ifndef QEMU_MIGRATION_SNAPSHOT_H
>  #define QEMU_MIGRATION_SNAPSHOT_H
>  
> -int save_snapshot(const char *name, Error **errp);
> +int save_snapshot(const char *name, bool overwrite, Error **errp);
>  int load_snapshot(const char *name, Error **errp);
>  
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 76972d69b0..2025e3e579 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)
>      return 0;
>  }
>  
> -int save_snapshot(const char *name, Error **errp)
> +int save_snapshot(const char *name, bool overwrite, Error **errp)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2685,7 +2685,7 @@ int save_snapshot(const char *name, Error **errp)
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name) {
> +    if (name && overwrite) {
>          if (bdrv_all_delete_snapshot(name, false, NULL, errp) < 0) {
>              return ret;
>          }

Are you sure this is sane?

To see what happens, I set a breakpoint on this function, set overwrite
to false.  I got a *second* snapshot with the same ID.

> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 98c98ae182..c1b8533d0c 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1131,7 +1131,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>  
> -    save_snapshot(qdict_get_try_str(qdict, "name"), &err);
> +    save_snapshot(qdict_get_try_str(qdict, "name"), true, &err);
>      hmp_handle_error(mon, err);
>  }
>  
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index e26fa4c892..8e7ff97d11 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -77,7 +77,7 @@ void replay_vmstate_init(void)
>  
>      if (replay_snapshot) {
>          if (replay_mode == REPLAY_MODE_RECORD) {
> -            if (save_snapshot(replay_snapshot, &err) != 0) {
> +            if (save_snapshot(replay_snapshot, true, &err) != 0) {
>                  error_report_err(err);
>                  error_report("Could not create snapshot for icount record");
>                  exit(1);