From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1x7r-000421-N1 for qemu-devel@nongnu.org; Thu, 16 Aug 2012 06:16:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1x7q-0006XJ-GI for qemu-devel@nongnu.org; Thu, 16 Aug 2012 06:16:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1x7q-0006XD-7x for qemu-devel@nongnu.org; Thu, 16 Aug 2012 06:16:26 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7GAGPij015913 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 16 Aug 2012 06:16:25 -0400 Message-ID: <502CC877.2020202@redhat.com> Date: Thu, 16 Aug 2012 12:16:23 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <502BFD42.8070900@redhat.com> In-Reply-To: <502BFD42.8070900@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org On 08/15/2012 09:49 PM, Eric Blake wrote: > On 08/15/2012 01:41 AM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina >> --- > I'm focusing my review more on the public interface (since that's what > affects libvirt), and therefore glanced through 1 through 12 but did not > pay close attention to them. > >> hmp-commands.hx | 2 +- >> hmp.c | 10 ++++++++++ >> hmp.h | 1 + >> qapi-schema.json | 19 +++++++++++++++++++ >> qmp-commands.hx | 29 +++++++++++++++++++++++++++++ >> savevm.c | 25 +++++++++---------------- >> sysemu.h | 1 - >> 7 files changed, 69 insertions(+), 18 deletions(-) >> >> + >> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) >> +{ >> + const char *name = qdict_get_try_str(qdict, "name"); > In the cover letter, you said "and for QMP you have to always provide > name parameter" - but this says 'name' is optional and can still be > empty (id only). I said in cover letter that the last two patches introduce this functionality. >> +++ b/qapi-schema.json >> @@ -2356,3 +2356,22 @@ >> # Since: 1.2.0 >> ## >> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] } >> + >> +## >> +# @vm-snapshot-save: >> +# >> +# Create a snapshot of the whole virtual machine. If 'tag' is provided, > 'tag' is a misnomer, since you list @name as the parameter name. > >> +# it is used as human readable identifier. If there is already a snapshot >> +# with the same tag or ID, it is replaced. >> +# >> +# 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 > Here, you document @name as required, > >> +# >> +# Returns: Nothing on success >> +# If an error occurs, GenericError with error message >> +# >> +# Since: 1.2 > We missed 1.2 hard freeze. This better be 1.3. > >> +## >> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } > but here, you listed it as '*name' meaning optional. I'm okay with > leaving name optional, provided that it means that you end up allocating > the next unique id. But if that's the case, then returning nothing is > wrong; this command needs to return { 'id':'int', '*name':'str' }, so > that the user knows what snapshot just got allocated. If you apply whole patch-series, then you have to for QMP always provide @name. As I said above, this behaviour is introduced by last two patches. >> +SQMP >> +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. >> + >> +The VM is automatically stopped and resumed and saving a snapshot can take >> +a long time. > I don't like that this command can take a long time. Just today on > #virt IRC, someone was complaining that 'savevm' HMP cannot be canceled. > If we're going to create a new interface, it would be nicer to create a > command that starts the save process and returns immediately, as well as > commands to track progress, allow an early abort, and send an event on > completion. The HMP 'savevm' interface can issue multiple QMP commands > under the hood to continue with it's blocking behavior, but as long as > we are fixing things, I think the QMP interface should be more powerful. Well, I could try to do it...with this I'll probably introduce new command vm-snapshot-save-cancel or vm-snaphsot-cancel as we have migrate and migrate-cancel. >> + >> +Arguments: >> + >> +- "name": tag or id of new or existing snapshot >> + >> +Example: >> + >> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } >> +<- { "return": {} } > Again, you need to return the id of the just-created snapshot. > >> @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> } >> >> /* Delete old snapshots of the same name */ >> - if (name && del_existing_snapshots(name, NULL) < 0) { >> + if (has_name && del_existing_snapshots(name, errp) < 0) { > Here's hoping later in the series updates this to make saner decisions. > (Maybe I should peruse the entire series before commenting on > individual patches?) >