From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKZzH-0004Xj-9h for qemu-devel@nongnu.org; Tue, 26 Mar 2013 15:56:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKZzF-0000j5-VC for qemu-devel@nongnu.org; Tue, 26 Mar 2013 15:56:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45825) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKZzF-0000iy-Mv for qemu-devel@nongnu.org; Tue, 26 Mar 2013 15:56:49 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2QJumtZ029528 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Mar 2013 15:56:48 -0400 Message-ID: <5151FD7F.2060301@redhat.com> Date: Tue, 26 Mar 2013 13:56:47 -0600 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2KKLFFEUOGCPOBASKLHKC" Subject: Re: [Qemu-devel] [PATCH v2 07/12] qapi: Convert savevm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2KKLFFEUOGCPOBASKLHKC Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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(-) >=20 > +++ 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 a= s @name, > +# it is used as human readable identifier. If there is already a snaps= hot > +# 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)? > +# > +# The VM is automatically stopped and resumed and saving a snapshot ca= n 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. > +# @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'. > +# > +# 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 sup= port " > + "snapshots.", bdrv_get_device_name(bs)); > return; > } > } > =20 > bs =3D 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() :) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2KKLFFEUOGCPOBASKLHKC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRUf1/AAoJEKeha0olJ0NqmAgH/0/5RfV/BCo9HqxfZ7EWAuBz Zw4/1RjnQisuQuNfpA0cblJ7O6E4MC1WAF6KXAgvyT4BW7DOEjEock3wwxXbKTMD P7HFXoikPt5KU59BeGGf4XoL7u8YDJwdc3/qfoucckw4F05hSBhYEFcNk6sRqcMj Dek1eekWYid8w7YwZOZzRsFDs7F9J+ApAPkqlQkJxspPgJK+BVjRs+dfgdxsgx2i QyNfHjW15GT35RlM7MYUcwb5+NwKCqLa6h1oHlM2FB6oPawPl+fbOiVGawGT1qnt ikVZW1bLHHWOz4ElW7ScHL7mhfUOlfNcNnq9CZshquD7x46UNjctoOelByG6d/M= =Ivh1 -----END PGP SIGNATURE----- ------enig2KKLFFEUOGCPOBASKLHKC--