From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1s6u-0001iK-W5 for qemu-devel@nongnu.org; Mon, 08 Jun 2015 04:08:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1s6n-0006O1-Mm for qemu-devel@nongnu.org; Mon, 08 Jun 2015 04:08:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49621) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1s6n-0006Nk-G1 for qemu-devel@nongnu.org; Mon, 08 Jun 2015 04:08:37 -0400 From: Markus Armbruster References: <02aa01d09f9a$5bb198d0$1314ca70$@samsung.com> <20150605143247.GJ11483@redhat.com> <55721E38.6030402@redhat.com> Date: Mon, 08 Jun 2015 10:08:34 +0200 In-Reply-To: <55721E38.6030402@redhat.com> (Eric Blake's message of "Fri, 05 Jun 2015 16:10:00 -0600") Message-ID: <87wpzelj0d.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: Pavel Fedin , qemu-devel@nongnu.org Eric Blake writes: > 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; >> >> This should probably be accompanied by an update to docs/qmp/qmp-spec.txt >> to say this is ignored and remove the bit about it being copied into the >> 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), Exactly. > 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. Working on it.