From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34710 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PzE8J-0001D5-Pu for qemu-devel@nongnu.org; Mon, 14 Mar 2011 16:12:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PzE8H-0004WX-LT for qemu-devel@nongnu.org; Mon, 14 Mar 2011 16:12:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2945) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PzE8H-0004WO-AB for qemu-devel@nongnu.org; Mon, 14 Mar 2011 16:12:49 -0400 Date: Mon, 14 Mar 2011 17:12:43 -0300 From: Luiz Capitulino Message-ID: <20110314171243.4a44417d@doriath> In-Reply-To: <4D7E6FF4.70503@us.ibm.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Anthony Liguori Cc: Paolo Bonzini , Michael@gnu.org, qemu-devel@nongnu.org, Roth , Markus Armbruster 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? > > > { "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. > > > Although it also accepts broken stuff today, like: > > > > { "execute": ___"query-block" } > > This is really the server, not the parser. The new server doesn't > accept this. This is probably the same as the xxxxx above. > 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"}}}