From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKv0x-0004vz-Q0 for qemu-devel@nongnu.org; Wed, 27 Mar 2013 14:24:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKv0w-0004Gl-CN for qemu-devel@nongnu.org; Wed, 27 Mar 2013 14:23:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKv0w-0004Gb-4P for qemu-devel@nongnu.org; Wed, 27 Mar 2013 14:23:58 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2RINvPr012755 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 27 Mar 2013 14:23:57 -0400 Message-ID: <5153393A.3030207@redhat.com> Date: Wed, 27 Mar 2013 19:23:54 +0100 From: Pavel Hrdina MIME-Version: 1.0 References: <5151FD7F.2060301@redhat.com> In-Reply-To: <5151FD7F.2060301@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 07/12] qapi: Convert savevm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 26.3.2013 20:56, Eric Blake wrote: > On 03/22/2013 07:16 AM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina >> --- >> hmp-commands.hx | 4 ++-- >> hmp.c | 9 +++++++++ >> hmp.h | 1 + >> include/sysemu/sysemu.h | 1 - >> qapi-schema.json | 18 ++++++++++++++++++ >> qmp-commands.hx | 29 +++++++++++++++++++++++++++++ >> savevm.c | 27 ++++++++++++--------------- >> 7 files changed, 71 insertions(+), 18 deletions(-) >> > >> +++ b/qapi-schema.json >> @@ -3442,3 +3442,21 @@ >> # Since: 1.5 >> ## >> { 'command': 'query-tpm', 'returns': ['TPMInfo'] } >> + >> +## >> +# @vm-snapshot-save: >> +# >> +# Create a snapshot of the whole virtual machine. If tag is provided as @name, >> +# it is used as human readable identifier. If there is already a snapshot >> +# with the same tag or ID, it is replaced. > > Any reason we have to fix this up later in the series, instead of > getting the interface right to begin with (in regards to having an > optional force argument)? It was supposed to introduce new feature for both, but I can make it as you suggesting. > >> +# >> +# The VM is automatically stopped and resumed and saving a snapshot can take >> +# a long time. > > Transactionally, are we exposing the right building blocks? While the > existing HMP command pauses the domain up front, I think the QMP > interface should be job-oriented. That is, it should be possible to > start a long-running job and have QMP return immediately, so that QMP > remains responsive, and lets us do a live migrate, live bandwidth > tuning, the ability to gracefully abort, and the ability to pause the > domain if live migrate isn't converging fast enough. HMP would then > preserve its existing interface by calling multiple QMP commands, > instead of trying to make QMP an exact analogue to the sub-par HMP > interface. > > Libvirt _really_ wants to be able to cancel an in-progress snapshot > creation action, but can't if all we expose is the same interface as HMP > has always had. > I'm working on live snapshots which will introduce this functionality, but I couldn't give you any deadline when I'll have it done. So if you (libvirt) really want to have savevm and vm-snapshot-save non-blocking before I'll finish live snapshots, then I could rewrite this patch. I know that current approach isn't good enough because of that blocking behavior. >> +# @name: #optional tag of new snapshot or tag|id of existing snapshot >> +# >> +# Returns: Nothing on success > > Bad. If @name is optional, and one is generated on your behalf, then > you need to return the 'name' that was generated. Also, even if 'name' > is provided, knowing which 'id' was allocated is useful, since later > APIs can operate on 'name' or 'id'. That's a good point. I'll fix this to return all information about the created snapshot. > >> +# >> +# Since: 1.5 >> +## >> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } > > In other words, this needs a return value. > > >> if (!bdrv_can_snapshot(bs)) { >> - monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n", >> - bdrv_get_device_name(bs)); >> + error_setg(errp, "Device '%s' is writable but does not support " >> + "snapshots.", bdrv_get_device_name(bs)); >> return; >> } >> } >> >> bs = bdrv_snapshots(); >> if (!bs) { >> - monitor_printf(mon, "No block device can accept snapshots\n"); >> + error_setg(errp, "No block device can accept snapshots."); > > Odd that we weren't consistent on whether errors ended with '.' when > using monitor_printf; your patch at least tried to be self-consistent, > even if opposite the normal usage of error_setg() :) > Thanks for review. I've fixed error messages and other issues you mentioned in other patches. Pavel