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

[Pavel Hrdina <phrdina@redhat.com>]

On 08/16/2012 07:00 AM, Eric Blake wrote:
> 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.
Thanks, I'll rewrite it for v2.
>> +        .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?
You can override existing snapshot specified by id. I'll fix the 
documentation.
>> +#
>> +# @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?)
>
Regarding the Luiz's new error handling, for new errors we should use 
one error class.