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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3802 bytes --]

On 08/15/2012 01:41 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.

When HMP omits the name, what name are you using in it's place?  Either
a nameless snapshot is okay (and QMP must expose it), or it is an error
(and HMP must now be passing a reasonable name).

/me reads patch

Oh, there was always a name; the default name was a timestamp, and you
moved the timestamp creation into HMP.


> +++ b/hmp-commands.hx
> @@ -264,19 +264,19 @@ ETEXI
>  
>      {
>          .name       = "savevm",
> -        .args_type  = "name:s?",
> -        .params     = "[tag|id]",
> -        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> +        .args_type  = "name:s?,force:b?",
> +        .params     = "[tag|id] [on|off]",

Rather than sticking 'force' as an optional parameter at the end,
instead you want to add '-f' as a flag at the beginning.

.args_type = "force:-b,name:s?",
.params = "[-f] name"

By doing this, you no longer need 17/18.

> +        .help       = "save a VM snapshot. To replace existing snapshot use force parameter.",
>          .mhandler.cmd = hmp_vm_snapshot_save,
>      },
>  
>  STEXI
>  @item savevm [@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 tag or ID, it is replaced. More info at
> -@ref{vm_snapshots}.
> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.
> +If @var{tag} is provided, it is used as human readable identifier. If there is
> +already a snapshot with the same tag, force argument need to be "yes" to

s/force argument need/the force argument needs/

> +++ b/qapi-schema.json
> @@ -2394,21 +2394,23 @@
>  ##
>  # @vm-snapshot-save:
>  #
> -# Create a snapshot of the whole virtual machine. If 'tag' is provided,
> -# it is used as human readable identifier. If there is already a snapshot
> -# with the same tag or ID, it is replaced.
> +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
> +# human readable identifier. If there is already a snapshot with the same tag,
> +# force argument need to be "yes" to replace it.

s/need to be "yes"/needs to be 'true'/

>  #
>  # The VM is automatically stopped and resumed and saving a snapshot can take
>  # a long time.
>  #
> -# @name: tag or id of new or existing snapshot
> +# @name: tag of new or existing snapshot

Given your new semantics, @name must be tag when @force is false; but
the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1
regardless of the name, all while keeping the old tag.  Does the new HMP
code do an id lookup, and pass in the correct @name to the QMP code?
Should the QMP code allow the same semantics of being able to pass
@force=true and @name=id instead of the normal creation of @name=tag?

> +#
> +# @force: specify whether existing snapshot is replaced or not

Based on the JSON below, this needs an #optional marking, as well as
mentioning that the default is false.

>  #
>  # Returns: Nothing on success
>  #          If an error occurs, GenericError with error message

Knowing that a creation failed due to a snapshot already existing by the
same name or id might be worth a distinct error class (do we already
have an error class that we could reuse?)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]