Re: [Qemu-devel] [PATCH 37/56] json: Treat unwanted interpolation as lexical error

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/08/2018 07:03 AM, Markus Armbruster wrote:
>> The JSON parser optionally supports interpolation.  The lexer
>> recognizes interpolation tokens unconditionally.  The parser rejects
>> them when interpolation is disabled, in parse_interpolation().
>> However, it neglects to set an error then, which can make
>> json_parser_parse() fail without setting an error.
>>
>> Move the check for unwanted interpolation from the parser's
>> parse_interpolation() into the lexer's finite state machine.  When
>> interpolation is disabled, '%' is now handled like any other
>> unexpected character.
>>
>> The next commit will improve how such lexical errors are handled.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   include/qapi/qmp/json-lexer.h |  4 ++--
>>   qobject/json-lexer.c          | 42 ++++++++++++++++++++++++++---------
>>   qobject/json-parser.c         |  4 ----
>>   qobject/json-streamer.c       |  2 +-
>>   tests/qmp-test.c              |  4 ++++
>>   5 files changed, 39 insertions(+), 17 deletions(-)
>>
>
>
>> @@ -271,17 +272,38 @@ static const uint8_t json_lexer[][256] =  {
>>           [','] = JSON_COMMA,
>>           [':'] = JSON_COLON,
>>           ['a' ... 'z'] = IN_KEYWORD,
>> +        [' '] = IN_WHITESPACE,
>> +        ['\t'] = IN_WHITESPACE,
>> +        ['\r'] = IN_WHITESPACE,
>> +        ['\n'] = IN_WHITESPACE,
>> +    },
>> +
>> +    [IN_START_INTERPOL] = {
>> +        ['"'] = IN_DQ_STRING,
> ...
>
>> +        ['\n'] = IN_WHITESPACE,
>> +        /* matches IN_START up to here */
>>           ['%'] = IN_INTERPOL,
>
> You could compress this as:
>
> [IN_START_INTERPOL ... IN_START] = {
>    ['"'] = ...
>    ['\n'] = ...
> },
> [IN_START_INTERPOL]['%'] = IN_INTERPOL,
>
> rather than duplicating the common list twice. (We already exploit
> gcc's range initialization, and the fact that you can initialize a
> broader range and then re-initialize a more specific subset later)

Neat!

It'll lose some of its charm in PATCH 52, but enough remains for me to
like it.

>> +++ b/tests/qmp-test.c
>> @@ -94,6 +94,10 @@ static void test_malformed(QTestState *qts)
>>         /* lexical error: interpolation */
>>       qtest_qmp_send_raw(qts, "%%p\n");
>> +    /* two errors, one for "%", one for "p" */
>> +    resp = qtest_qmp_receive(qts);
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    qobject_unref(resp);
>>       resp = qtest_qmp_receive(qts);
>>       g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>>       qobject_unref(resp);
>
> I'm impressed at how easily you got the lexer to parse two different
> token grammars, and agree that doing it in the lexer when we don't
> want interpolation is a nicer place.
>
> Reviewed-by: Eric Blake <address@hidden>

Well, what you see here isn't my first attempt to make the lexer do it,
it's the one I finally liked :)

Thanks!