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 ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
