From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52580) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1jan-0001Th-W3 for qemu-devel@nongnu.org; Wed, 15 Aug 2012 15:49:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T1jam-0004aQ-Ck for qemu-devel@nongnu.org; Wed, 15 Aug 2012 15:49:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49662) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T1jam-0004aJ-5Q for qemu-devel@nongnu.org; Wed, 15 Aug 2012 15:49:24 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7FJnNal007842 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 15 Aug 2012 15:49:23 -0400 Message-ID: <502BFD42.8070900@redhat.com> Date: Wed, 15 Aug 2012 13:49:22 -0600 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig717D89403160A6C344131912" Subject: Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig717D89403160A6C344131912 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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(-) >=20 > + > +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) > +{ > + const char *name =3D 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). > +++ 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 snaps= hot > +# with the same tag or ID, it is replaced. > +# > +# The VM is automatically stopped and resumed and saving a snapshot ca= n 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. > +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 snapsho= t > +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. > + > +Arguments: > + > +- "name": tag or id of new or existing snapshot > + > +Example: > + > +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapsho= t" } } > +<- { "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= ) > } > =20 > /* 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?) --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig717D89403160A6C344131912 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.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJQK/1CAAoJEKeha0olJ0Nq6OsH/ROQadUWtS50epb9ZOQweprg xw0By8YIptD4BOtFuLQPkPENoYgxY13FDgu1RyIJn0efoEcP4TrjL+y/QaJD1gcq 4PuHAkEcy5FNUYq4wjjYjFGZL5DaSzbyFywzJWF7l/QKAdkgbfnBCDDWfSbrjRZp it3GL9jYjnIqszrKgV/deoX53dh8aNw7dsqdi3PqN9C4UotXjPJSt8Yy+j9VlD/m U57pXec6epOpvKUvOicqPIDjUInq3RCVj4zDQlQkzpmu5WuOISsE8fn+mXvNQMm3 popPKmBWjLFgIA2T1vm3JTcXp3rndWI6bdph4HJAd38YpsIRxhpkXiqULrFPOv4= =9P4s -----END PGP SIGNATURE----- --------------enig717D89403160A6C344131912--