Re: [Qemu-devel] [PATCH 13/13] vm-snapshot-save: add force parameter

[Pavel Hrdina]

On Wed, 2013-01-09 at 15:47 -0700, Eric Blake wrote:
> On 01/09/2013 08:18 AM, Pavel Hrdina wrote:
> > HMP command "savevm" now takes extra optional force parameter to specifi 
> > whether
> 
> s/specifi/specify/
> 
> > replace existing snapshot or not.
> > 
> > QMP command "vm-snapshot-save" has also extra optional force parameter and
> > name parameter isn't optional anymore.
> 
> The qcow2 format lets 'name' be optional; why are we insisting at the
> QMP layer that we cannot use this feature?  It may be a change worth
> making, but you'll need more arguments in the commit message; and you'll
> still need to make sure that qemu can handle an arbitrary qcow2 file
> that has an existing snapshot created by older qemu when name was still
> optional.
> 

Based on your comment I think that we should create this behavior:

We will have for HMP and QMP two parameters id and name and you can
choose only one of them next to the force option.

If name is specified and there is a snapshot with specified name, the
snapshot will be overwritten.

If name is specified and there isn't a snapshot with specified name, a
new snapshot will be written.

If id is specified and there is a snapshot with specified id, the
snapshot will be overwritten.

If id is specified and there isn't a snapshot with specified id,
the command ends with error message that there isn't snapshot to
overwrite with specified id.

If id or name is not specified the new snapshot with generated name and
id will be created.

In all successful cases the information about snapshot will be returned.

> > 
> > Signed-off-by: Pavel Hrdina <address@hidden>
> > ---
> >  hmp-commands.hx  | 15 ++++++++-------
> >  hmp.c            | 25 ++++++++++++++++++++++++-
> >  qapi-schema.json |  9 ++++++---
> >  qmp-commands.hx  | 15 +++++++++------
> >  savevm.c         | 36 ++++++++++++++----------------------
> >  5 files changed, 61 insertions(+), 39 deletions(-)
> > 
> 
> > address@hidden savevm address@hidden @var{tag}|@var{id}
> >  @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
> > -a snapshot with the same @var{tag} or @var{id}, it is replaced. More info 
> > at
> > +Create a snapshot of the whole virtual machine. Paramtere "name" is 
> > optional.
> 
> s/Paramtere/Parameter/
> 
> > +++ b/hmp.c
> > @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict 
> > *qdict)
> >  void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *name = qdict_get_try_str(qdict, "name");
> > +    char new_name[256];
> > +    bool force = qdict_get_try_bool(qdict, "force", 0);
> >      Error *err = NULL;
> > +#ifdef _WIN32
> > +    struct _timeb tb;
> > +    struct tm *ptm;
> > +#else
> > +    struct timeval tv;
> > +    struct tm tm;
> > +#endif
> > +
> > +    if (!name) {
> > +#ifdef _WIN32
> > +        time_t t = tb.time;
> > +        ptm = localtime(&t);
> > +        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm);
> 
> It feels like lots of pending patches are trying to touch this same area
> of code; make sure you coordinate the efforts (for example,
> http://patchwork.ozlabs.org/patch/210250/ should go in first).
> 
> > +++ b/qapi-schema.json
> > @@ -3023,15 +3023,18 @@
> >  #
> >  # Create a snapshot of the whole virtual machine. If tag is provided as 
> > @name,
> >  # it is used as human readable identifier. If there is already a snapshot
> > -# with the same tag or ID, it is replaced.
> > +# with the same tag or id, the force argument needs to be true to replace 
> > it.
> >  #
> >  # The VM is automatically stopped and resumed and saving a snapshot can 
> > take
> >  # a long time.
> >  #
> > -# @name: #optional tag of new snapshot or tag|id of existing snapshot
> > +# @name: tag of new snapshot or tag|id of existing snapshot
> 
> It feels like you are doing too much in this commit.  Either name should
> never have been optional earlier in the series, or the change to make
> name mandatory should be its own commit (since nothing in the one-line
> summary mentions making name mandatory).
> 
> > +    if (ret >= 0) {
> > +        if (has_force && force) {
> >              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> >              pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> > +
> > +            /* Delete old snapshots of the same name */
> > +            if (del_existing_snapshots(name, &local_err) < 0) {
> > +                error_propagate(errp, local_err);
> > +                goto the_end;
> > +            }
> >          } else {
> > -            pstrcpy(sn->name, sizeof(sn->name), name);
> > +            error_setg(errp, "Snapshot '%s' exist. For override add "
> 
> s/exist/exists/
>