Re: [PATCH v4 5/9] migration: control whether snapshots are ovewritten
On Wed, Sep 16, 2020 at 09:47:32AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé 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é
> > ---
> > 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.
Sigh. No, it doesn't do what I was meaning it to, and I forgot to add
a test case for this scenario in the last patch.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 5/9] migration: control whether snapshots are ovewritten
Daniel P. Berrangé 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é
> ---
> 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);
[PATCH v4 5/9] migration: control whether snapshots are ovewritten
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é
---
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;
}
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);
--
2.26.2
