Re: [Qemu-devel] [PATCH 05/56] qmp-test: Cover syntax and lexical errors

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/10/2018 08:52 AM, Markus Armbruster wrote:
>
>>>> +    /* lexical error: impossible byte outside string */
>>>> +    qtest_qmp_send_raw(qts, "{\xFF");
>>>
>>> \xff is an impossible byte inside a string as well; plus it has
>>> special meaning to at least QMP for commanding a parser reset. Is a
>>> better byte more appropriate (maybe \x7f), either in replacement to
>>> \xff or as an additional test?
>>
>> \xFF is documented to have special meaning for QGA, but as far as the
>> code's concerned, it's a lexical error like any other.  I'm fixing the
>> documentation in PATCH 56.  Want me to move that patch to the front of
>> the series?
>
> Might not hurt. We also have a potential design decision to make: for
> most lexical errors, we report the error (with QGA, the user then
> requests that the first valid command after the client's induced
> lexical error also include an 0xff reply byte so that the client can
> easily skip over all the line noise, including said error reports).
> Thus, we COULD decide to make our parser specifically accept 0xff as a
> new token, different from the lexical error token, so that it inhibits
> wasted error messages to the client on the grounds that the client
> sent it on purpose, differently from all other ways the client can use
> a lexical error to cause a reset.

I don't think that's worthwhile.  Let me explain.

I see one and a half use cases.

The full use case is of course QGA synchronization.  Why is that even
necessary?  I believe it's a work-around for transports that fail to
provide proper connection semantics, such as virtio-serial.

When a transport provides it (sockets do), every connection starts with
a clean slate.  The only way it can get de-synchronized is either peer
getting confused enough to send garbage, and then it's probably best to
close the session and start over.  Evidence: we don't bother with
synchronization for QMP, which commonly uses socket transports.

I guess the problem for QGA is reconnecting with virtio-serial.  The
slate isn't clean then.  If the previous connection left some of the
peer's output unread, you'll get that before your own, and it may not be
valid JSON.  Same for the other direction.

To reset the peer's parser, we provoke a lexical error by sending \xFF.

We still have to read and ignore stale peer output.  We do that with the
help of command guest-sync-delimited.  Whether lexical error caused by
\xFF are suppressed or not makes no appreciable difference.

I therefore think suppressing it is not worth the bother.  What we have
seems good enough.

The half use case is human interactive use.  It's fairly easy to
miscount parenthesis and get the peer into some unresponsive state.
Provoking a lexical error lets me start over.  However, I'd rather not
use \xFF to provoke it, because how do you type that?  Sending a
suitable ASCII control character is easier, and will do the trick after
PATCH 17 fixes our JSON parser.