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

[Daniel P . Berrangé]

On Wed, Sep 16, 2020 at 09:47:32AM +0200, Markus Armbruster wrote:
> 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.

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