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

[Eric Blake]

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.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature