On Mon, 14 Mar 2011 14:43:48 -0500
Anthony Liguori <aligu...@us.ibm.com> wrote:
> On 03/14/2011 02:22 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:00:46 -0600
> > Anthony Liguori<aligu...@us.ibm.com> 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<aligu...@us.ibm.com>
> >>
> >> 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"}}}
