From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57345) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0zoX-0000HF-Ua for qemu-devel@nongnu.org; Fri, 05 Jun 2015 18:10:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0zoV-0006Ad-7m for qemu-devel@nongnu.org; Fri, 05 Jun 2015 18:10:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42761) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0zoV-00069C-0T for qemu-devel@nongnu.org; Fri, 05 Jun 2015 18:10:07 -0400 Message-ID: <55721E38.6030402@redhat.com> Date: Fri, 05 Jun 2015 16:10:00 -0600 From: Eric Blake MIME-Version: 1.0 References: <02aa01d09f9a$5bb198d0$1314ca70$@samsung.com> <20150605143247.GJ11483@redhat.com> In-Reply-To: <20150605143247.GJ11483@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="kLfaocfRDfFv9GBOk1pxTHdGNmoFl0uM1" Subject: Re: [Qemu-devel] [PATCH] Do not fail if id field is present. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , Pavel Fedin Cc: qemu-devel@nongnu.org, 'Markus Armbruster' This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --kLfaocfRDfFv9GBOk1pxTHdGNmoFl0uM1 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/05/2015 08:32 AM, Daniel P. Berrange wrote: > On Fri, Jun 05, 2015 at 05:17:29PM +0300, Pavel Fedin wrote: >> This fixes QMP regression: >> http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.html >> >> Signed-off-by: Pavel Fedin >> --- >> monitor.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/monitor.c b/monitor.c >> index c7baa91..ef21bba 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -4955,6 +4955,8 @@ static QDict *qmp_check_input_obj(QObject *input= _obj, Error **errp) >> "arguments", "object"); >> return NULL; >> } >> + } else if (!strcmp(arg_name, "id")) { >> + /* Ignored, necessary for backwards compatibility */ >> } else { >> error_set(errp, QERR_QMP_EXTRA_MEMBER, arg_name); >> return NULL; >=20 > This should probably be accompanied by an update to docs/qmp/qmp-spec.t= xt > to say this is ignored and remove the bit about it being copied into th= e > replies Not necessary - the "id" string is once again preserved correctly on synchronous commands with this patch applied: $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 3, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"qmp_capabilities","id":1} {"return": {}, "id": 1} Remember, Markus' patch was about removing asynchronous commands, because _those_ commands were where "id" was mishandled (and if we DID want asynch commands, it would be even MORE important that they handle id corerctly). But he accidentally removed 2 lines (the strcmp to "id" and a now stale FIXME comment about wanting to check for duplicate "id"s being tracked across parallel async commands) when it should have only removed one (the stale comment), and I made the mistake of giving R-by based on code review and NOT an actual build-and-run of the applied patch (or I would have instantly spotted the side effect that "id" was broken on synchronous commands). But an empty if clause looks suspicious, so we want a better comment in place of the stale comment, which is why I suggest: /* Specially handled elsewhere to be included in reply to user */ At any rate, with the if statement restored, I can now state: Tested-by: Eric Blake and since we now have no less than three threads pointing out the issue, I hope we can settle on a solution with a nice comment and get it in the tree soon. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --kLfaocfRDfFv9GBOk1pxTHdGNmoFl0uM1 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/ iQEcBAEBCAAGBQJVch48AAoJEKeha0olJ0NqjyUH/2G/2Ng9yNtIbLTLFTTOjVb1 ME28vWvM3u1mORW/XpmJE+oJ8F5n035IA23FUCi3sl0hxGrD1TBU5dNS6BHfVNd0 kIF6g8arZgnlaGCTIf+cCpalamMj/wFxFNe9nbNfFToBEP6kPceP/DY8G0As/A3s Oa0qQkrBeggphxz9Kt5A3688UJNZxowpw7CK75lHsXHMH3VAy97un+pU4IPEfjNw 6I6nj1t7zBVp5B67GFxtobT/TTdyCjd9NqFuN1d3iDFYVyFc0gxYeR6tfJlN/ZVF bKQ+o2waALHrKWKzpa3jIjepGV+9BzrzlCRKRRvjZpuAlYG7z9+APlHQYvTlh+o= =Dmdj -----END PGP SIGNATURE----- --kLfaocfRDfFv9GBOk1pxTHdGNmoFl0uM1--