From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48728) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1xBH-0006j8-Nx for qemu-devel@nongnu.org; Thu, 16 Aug 2012 06:20:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1xBG-0006wg-8f for qemu-devel@nongnu.org; Thu, 16 Aug 2012 06:19:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8549) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1xBG-0006wY-1E for qemu-devel@nongnu.org; Thu, 16 Aug 2012 06:19:58 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7GAJvMB016464 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 16 Aug 2012 06:19:57 -0400 Message-ID: <502CC94B.7030303@redhat.com> Date: Thu, 16 Aug 2012 12:19:55 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <582c2debbce809a86fe62d6a998d42bee8ef274c.1345016001.git.phrdina@redhat.com> <502C7E66.8040703@redhat.com> In-Reply-To: <502C7E66.8040703@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org 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.