From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53538 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PzEPY-0000N7-7U for qemu-devel@nongnu.org; Mon, 14 Mar 2011 16:30:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PzEPW-0008K2-QL for qemu-devel@nongnu.org; Mon, 14 Mar 2011 16:30:40 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:44382) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PzEPW-0008Jt-MY for qemu-devel@nongnu.org; Mon, 14 Mar 2011 16:30:38 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e8.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2EGBRSh012721 for ; Mon, 14 Mar 2011 12:11:27 -0400 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id A781A38C8038 for ; Mon, 14 Mar 2011 16:30:34 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2EKUY542347066 for ; Mon, 14 Mar 2011 16:30:35 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2EKUYFa009663 for ; Mon, 14 Mar 2011 16:30:34 -0400 Message-ID: <4D7E7AE9.3080506@us.ibm.com> Date: Mon, 14 Mar 2011 15:30:33 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1299877249-13433-1-git-send-email-aliguori@us.ibm.com> <1299877249-13433-9-git-send-email-aliguori@us.ibm.com> <20110314162229.013da3b3@doriath> <4D7E6FF4.70503@us.ibm.com> <20110314171243.4a44417d@doriath> In-Reply-To: <20110314171243.4a44417d@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 08/11] json-lexer: reset the lexer state on an invalid token List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Paolo Bonzini , qemu-devel@nongnu.org, Michael D Roth , Markus Armbruster On 03/14/2011 03:12 PM, Luiz Capitulino wrote: > On Mon, 14 Mar 2011 14:43:48 -0500 > Anthony Liguori wrote: > >> On 03/14/2011 02:22 PM, Luiz Capitulino wrote: >>> On Fri, 11 Mar 2011 15:00:46 -0600 >>> Anthony Liguori wrote: >>> >>>> Not everything handles errors from json parsing gracefully. By at least >>>> resetting the lexer, we'll start generating valid tokens again and hopefully >>>> recover the stream. >>>> >>>> Signed-off-by: Anthony Liguori >>>> >>>> diff --git a/json-lexer.c b/json-lexer.c >>>> index c736f42..834d7af 100644 >>>> --- a/json-lexer.c >>>> +++ b/json-lexer.c >>>> @@ -303,6 +303,9 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch) >>>> new_state = IN_START; >>>> break; >>>> case ERROR: >>>> + QDECREF(lexer->token); >>>> + lexer->token = qstring_new(); >>>> + new_state = IN_START; >>>> return -EINVAL; >>> This makes the parser accept broken input like: >>> >>> { "execute": xxxxx } >>> {"return": {}} >> This is a bug in the current QMP server. Here's how my new QMP server >> responds: >> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 13, "major": 0}, >> "package": ""}, "capabilities": []}} >> {"error": {"class": "JSONParseError", "data": {"message": "Missing value >> in dict"}}} > How do you handle it? Do you check the return of json_message_parser_feed()? > > If that's the case, then the real problem in the current server is that we > use qemu's chardev interface and its read handler doesn't allow for > signaling errors. I did not consider not using it. > > By looking at your branch I have the impression you wrote your own stuff, > am I right? If yes, doesn't it duplicate the chardev implementation? No, that test was with the chardev interface. There is both a chardev server and a unix domain socket server. I'm not really sure why the current server isn't working correctly. I'd have to investigate. >>> { "execute": _ } >>> {"return": {}} >> Likewise, the new QMP server does not respond to this at all (which >> confuses me TBH). >> >>> Today, it handles this kind of input correctly: >>> >>> { "execute": xxxxx } >>> {"error": {"class": "JSONParsing", "desc": "Invalid JSON syntax", "data": {}}} >> The parser rejects this verses trying to get what it can out of it and >> passing that to QMP. The idea here is to be more graceful in dealing >> with bad input and trying to recover. > I'm all for trying to recover, but we can't have varied responses for > bad input. It seems easier to just fail. I think we need to make sure that we don't ever succeed in the face of bad input, right? So far, none of the test cases (against the new QMP server) succeed given bad input. >> I guess QMP today just ignores the incoming QObject in capabilities mode >> and always returns {}. You'll see the same thing with: >> >> { "execute": "not-a-valid-command" } >> {"return": {}} >> >> But once you're in command mode, it does the right thing. > I can't reproduce it w/o this series applied: > > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 14, "major": 0}, "package": ""}, "capabilities": []}} > { "execute": "not-a-valid-command" } > {"error": {"class": "CommandNotFound", "desc": "The command not-a-valid-command has not been found", "data": {"name": "not-a-valid-command"}}} Curious, maybe I'm remembering this wrong then. Let me dig in a big. Regards, Anthony Liguori