From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43971) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBr9G-0007p4-1I for qemu-devel@nongnu.org; Wed, 23 Dec 2015 16:40:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBr9B-0002cF-IV for qemu-devel@nongnu.org; Wed, 23 Dec 2015 16:40:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36318) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBr9B-0002c4-Ai for qemu-devel@nongnu.org; Wed, 23 Dec 2015 16:40:37 -0500 References: <1449240275-26196-1-git-send-email-den@openvz.org> <1449240275-26196-3-git-send-email-den@openvz.org> From: Eric Blake Message-ID: <567B14CE.90508@redhat.com> Date: Wed, 23 Dec 2015 14:40:30 -0700 MIME-Version: 1.0 In-Reply-To: <1449240275-26196-3-git-send-email-den@openvz.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="73wKKDwdHa1pGh8s5FdgbnPmWaHn6MpOj" Subject: Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Amit Shah , Markus Armbruster , qemu-devel@nongnu.org, quintela@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --73wKKDwdHa1pGh8s5FdgbnPmWaHn6MpOj Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12/04/2015 07:44 AM, Denis V. Lunev wrote: > 'name' attribute is made mandatory in distinction with HMP command. >=20 > The patch also moves hmp_savevm implementation into hmp.c. This functio= n > is just a simple wrapper now and does not have knowledge about > migration internals. >=20 > Signed-off-by: Denis V. Lunev > CC: Juan Quintela > CC: Amit Shah > CC: Markus Armbruster > CC: Eric Blake > --- > hmp.c | 12 ++++++++++++ > migration/savevm.c | 13 +------------ > qapi-schema.json | 13 +++++++++++++ > qmp-commands.hx | 25 +++++++++++++++++++++++++ > 4 files changed, 51 insertions(+), 12 deletions(-) >=20 > diff --git a/hmp.c b/hmp.c > index 2140605..c9c7100 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -32,6 +32,7 @@ > #include "ui/console.h" > #include "block/qapi.h" > #include "qemu-io.h" > +#include "sysemu/sysemu.h" What is this header needed for? > =20 > #ifdef CONFIG_SPICE > #include > @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, cons= t QDict *qdict) > =20 > qapi_free_RockerOfDpaGroupList(list); > } > + > +void hmp_savevm(Monitor *mon, const QDict *qdict) > +{ > + Error *local_err =3D NULL; I've been favoring 'err' rather than 'local_err', but that's not a show-stopper since we still have both spellings mixed in the tree. > + > + qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err); qdict_get_try_str() can return NULL;, but qmp_savevm() requires a non-NULL string. You'll need to invent a name in HMP if the user didn't give you one. > +++ b/migration/savevm.c > @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f) > return ret; > } > =20 > -static void do_savevm(const char *name, Error **errp) > +void qmp_savevm(const char *name, Error **errp) > { This function still has some 'if (name)' conditionals; but according to the qapi contract, name should never be NULL. You need to hoist the name creation aspect to the caller (hmp_savevm), and in qmp_savevm you can then assert(name). > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo sn1, *sn =3D &sn1, old_sn1, *old_sn =3D &old_sn1;= > @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **= errp) > } > } > =20 > -void hmp_savevm(Monitor *mon, const QDict *qdict) > -{ > - Error *local_err =3D NULL; > - > - do_savevm(qdict_get_try_str(qdict, "name"), &local_err); Hmm. You are doing code motion of code you just added in 1/5; wouldn't it be better if 1/5 stuck it in the right file to begin with? And my argument about needing the name logic in HMP (not QMP) means that maybe the split in 1/5 isn't ideal, after all. > +++ b/qapi-schema.json > @@ -3971,3 +3971,16 @@ > ## > { 'enum': 'ReplayMode', > 'data': [ 'none', 'record', 'play' ] } > + > +## > +# @savevm > +# > +# Save a VM snapshot. Without a name new snapshot is created", Stale sentence, since a name is required. > +# > +# @name: identifier of a snapshot to be created > +# > +# Returns: Nothing on success > +# > +# Since 2.6 > +## > +{ 'command': 'savevm', 'data': {'name': 'str'} } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 9d8b42f..f216c5e 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -4739,3 +4739,28 @@ Example: > {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 38= 40, > "pop-vlan": 1, "id": 251658240} > ]} > + > +EQMP > + > +SQMP > +savevm > +------------------ Line too long. Make it match the length of the line above. > + > +Save a VM snapshot. If no tag or id are provided, a new snapshot is cr= eated Stale sentence, since you are requiring a name. Why does QMP not allow specifying an id? Should it? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --73wKKDwdHa1pGh8s5FdgbnPmWaHn6MpOj Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWexTOAAoJEKeha0olJ0Nqu9QH/RtdbJleOJy30vEk5PP2mjVd M9+FkNwhaFZ5txi9lhbQeR18byn6KTbQfOB6x3i2aBBeJ1oSGNbluXnQUWveIM7X Cxgph2ymSr2ykTeD9C7M401V222uro1JefCq3wHjiVzZVsM3Em4h7Q8hJDLQaLlX NVTOJcqHdey0grlmIZNegdfE+tXrObsbI6FS1jVhUHHrmxsWBaqEiCn5zu0FyYZp EhnNpV1oUfwA//TZCEmsJdibPpm9BwAvDBXyPK7EJUnUW/gCfFYpG/XqiGWKXk77 zIN1IHpjB3lbpOeh53GZHooQtJ72nJOSQJLwthS8BVR67e/Aj+8Yrt55vZZalcw= =7VU7 -----END PGP SIGNATURE----- --73wKKDwdHa1pGh8s5FdgbnPmWaHn6MpOj--