Re: [Qemu-devel] [PATCH v2 07/16] qapi: Drop support for escape sequences other than \\

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 9/10/19 1:37 AM, Markus Armbruster wrote:
>> Since the previous commit restricted strings to printable ASCII,
>> \uXXXX's only use is obfuscation.  Drop it.
>> 
>> This leaves \\, \/, \', and \".  Since QAPI schema strings are all
>> names, and names are restricted to ASCII letters, digits, hyphen, and
>> underscore, none of them is useful.
>> 
>> The latter three have no test coverage.  Drop them.
>> 
>> Keep \\ to avoid (more) gratuitous incompatibility with JSON.
>
> We have to parse it (to get a sane error message for someone writing
> "a\b" and thinking they were getting a backspace), but we can still
> reject "a\\b" as being a non-QAPI-name.  An alternative might be to
> reject QAPI strings the moment \ is encountered (as the only possible
> use is to introduce a character that is not valid as a name), to the
> point that we could check for name validity at the time we parse strings
> rather than later on.  Up to you.

That works, too.  I chose to parse \\ to keep the language a
sub-language of JSON's (and Python's) at the lexical level: we reject
some valid JSON (and Python) such as 'a\b'.  Not treating \ special at
all would accept it, but with a different meaning.  Will be rejected at
a higher level, so it doesn't really matter.  But the reasoning involves
more than just the lexical level then.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>
>> --- a/tests/qapi-schema/escape-outside-string.err
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -tests/qapi-schema/escape-outside-string.json:3:27: Stray "\"
>
> Do we still want to test that \ is an invalid character outside of
> strings (whether or not \ is also made invalid inside strings)?

funny-char.json checks ';'.  '\' is no different.

>> +++ b/tests/qapi-schema/unknown-escape.json
>> @@ -1,3 +1,3 @@
>> -# we only recognize JSON escape sequences, plus our \' extension (no \x)
>> +# we only recognize \\
>>  # { 'command': 'foo', 'data': {} }
>>  { 'command': 'foo', 'dat\x61':{} }
>> 
>
> At any rate, this change seems reasonable.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!