| Message ID | 20190910063724.28470-8-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Schema language cleanups & doc improvements | expand |
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. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > --- 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)? > +++ 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 <eblake@redhat.com>
Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com> >> --- > > >> --- 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 <eblake@redhat.com> Thanks!
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index c3dfad024f..f5583d3eac 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -523,27 +523,7 @@ class QAPISchemaParser(object): if ch == '\n': raise QAPIParseError(self, 'Missing terminating "\'"') if esc: - if ch == 'u': - value = 0 - for _ in range(0, 4): - ch = self.src[self.cursor] - self.cursor += 1 - if ch not in '0123456789abcdefABCDEF': - raise QAPIParseError(self, - '\\u escape needs 4 ' - 'hex digits') - value = (value << 4) + int(ch, 16) - # If Python 2 and 3 didn't disagree so much on - # how to handle Unicode, then we could allow - # Unicode string defaults. But most of QAPI is - # ASCII-only, so we aren't losing much for now. - if not value or value > 0x7f: - raise QAPIParseError(self, - 'For now, \\u escape ' - 'only supports non-zero ' - 'values up to \\u007f') - ch = chr(value) - elif ch not in '\\/\'"': + if ch != '\\': raise QAPIParseError(self, "Unknown escape \\%s" % ch) esc = False diff --git a/tests/Makefile.include b/tests/Makefile.include index 403748579f..3723496959 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -375,9 +375,6 @@ qapi-schema += enum-int-member.json qapi-schema += enum-member-case.json qapi-schema += enum-missing-data.json qapi-schema += enum-wrong-data.json -qapi-schema += escape-outside-string.json -qapi-schema += escape-too-big.json -qapi-schema += escape-too-short.json qapi-schema += event-boxed-empty.json qapi-schema += event-case.json qapi-schema += event-member-invalid-dict.json diff --git a/tests/qapi-schema/escape-outside-string.err b/tests/qapi-schema/escape-outside-string.err deleted file mode 100644 index b9b8837fd2..0000000000 --- a/tests/qapi-schema/escape-outside-string.err +++ /dev/null @@ -1 +0,0 @@ -tests/qapi-schema/escape-outside-string.json:3:27: Stray "\" diff --git a/tests/qapi-schema/escape-outside-string.exit b/tests/qapi-schema/escape-outside-string.exit deleted file mode 100644 index d00491fd7e..0000000000 --- a/tests/qapi-schema/escape-outside-string.exit +++ /dev/null @@ -1 +0,0 @@ -1 diff --git a/tests/qapi-schema/escape-outside-string.json b/tests/qapi-schema/escape-outside-string.json deleted file mode 100644 index 482f79554b..0000000000 --- a/tests/qapi-schema/escape-outside-string.json +++ /dev/null @@ -1,3 +0,0 @@ -# escape sequences are permitted only inside strings -# { 'command': 'foo', 'data': {} } -{ 'command': 'foo', 'data'\u003a{} } diff --git a/tests/qapi-schema/escape-outside-string.out b/tests/qapi-schema/escape-outside-string.out deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/tests/qapi-schema/escape-too-big.err b/tests/qapi-schema/escape-too-big.err deleted file mode 100644 index d9aeb5dc38..0000000000 --- a/tests/qapi-schema/escape-too-big.err +++ /dev/null @@ -1 +0,0 @@ -tests/qapi-schema/escape-too-big.json:3:14: For now, \u escape only supports non-zero values up to \u007f diff --git a/tests/qapi-schema/escape-too-big.exit b/tests/qapi-schema/escape-too-big.exit deleted file mode 100644 index d00491fd7e..0000000000 --- a/tests/qapi-schema/escape-too-big.exit +++ /dev/null @@ -1 +0,0 @@ -1 diff --git a/tests/qapi-schema/escape-too-big.json b/tests/qapi-schema/escape-too-big.json deleted file mode 100644 index 62bcecd557..0000000000 --- a/tests/qapi-schema/escape-too-big.json +++ /dev/null @@ -1,3 +0,0 @@ -# we don't support full Unicode strings, yet -# { 'command': 'é' } -{ 'command': '\u00e9' } diff --git a/tests/qapi-schema/escape-too-big.out b/tests/qapi-schema/escape-too-big.out deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/tests/qapi-schema/escape-too-short.err b/tests/qapi-schema/escape-too-short.err deleted file mode 100644 index 934de598ee..0000000000 --- a/tests/qapi-schema/escape-too-short.err +++ /dev/null @@ -1 +0,0 @@ -tests/qapi-schema/escape-too-short.json:3:14: \u escape needs 4 hex digits diff --git a/tests/qapi-schema/escape-too-short.exit b/tests/qapi-schema/escape-too-short.exit deleted file mode 100644 index d00491fd7e..0000000000 --- a/tests/qapi-schema/escape-too-short.exit +++ /dev/null @@ -1 +0,0 @@ -1 diff --git a/tests/qapi-schema/escape-too-short.json b/tests/qapi-schema/escape-too-short.json deleted file mode 100644 index 6cb1dec8f7..0000000000 --- a/tests/qapi-schema/escape-too-short.json +++ /dev/null @@ -1,3 +0,0 @@ -# the \u escape requires 4 hex digits -# { 'command': 'a' } -{ 'command': '\u61' } diff --git a/tests/qapi-schema/escape-too-short.out b/tests/qapi-schema/escape-too-short.out deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/tests/qapi-schema/ident-with-escape.err b/tests/qapi-schema/ident-with-escape.err index e69de29bb2..5517dcb4b1 100644 --- a/tests/qapi-schema/ident-with-escape.err +++ b/tests/qapi-schema/ident-with-escape.err @@ -0,0 +1 @@ +tests/qapi-schema/ident-with-escape.json:3:3: Unknown escape \u diff --git a/tests/qapi-schema/ident-with-escape.exit b/tests/qapi-schema/ident-with-escape.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/ident-with-escape.exit +++ b/tests/qapi-schema/ident-with-escape.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/ident-with-escape.json b/tests/qapi-schema/ident-with-escape.json index 56617501e7..76b4503d95 100644 --- a/tests/qapi-schema/ident-with-escape.json +++ b/tests/qapi-schema/ident-with-escape.json @@ -1,4 +1,4 @@ -# we allow escape sequences in strings, if they map back to ASCII +# we don't recognize any \ escapes other than \\ (tested elsewhere) # { 'command': 'fooA', 'data': { 'bar1': 'str' } } { 'c\u006fmmand': '\u0066\u006f\u006FA', 'd\u0061ta': { '\u0062\u0061\u00721': '\u0073\u0074\u0072' } } diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out index 39754eba8c..e69de29bb2 100644 --- a/tests/qapi-schema/ident-with-escape.out +++ b/tests/qapi-schema/ident-with-escape.out @@ -1,16 +0,0 @@ -module None -object q_empty -enum QType - prefix QTYPE - member none - member qnull - member qnum - member qstring - member qdict - member qlist - member qbool -module ident-with-escape.json -object q_obj_fooA-arg - member bar1: str optional=False -command fooA q_obj_fooA-arg -> None - gen=True success_response=True boxed=False oob=False preconfig=False diff --git a/tests/qapi-schema/unknown-escape.json b/tests/qapi-schema/unknown-escape.json index 8e6891e52a..8372e8024f 100644 --- a/tests/qapi-schema/unknown-escape.json +++ 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':{} }
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. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/common.py | 22 +------------------- tests/Makefile.include | 3 --- tests/qapi-schema/escape-outside-string.err | 1 - tests/qapi-schema/escape-outside-string.exit | 1 - tests/qapi-schema/escape-outside-string.json | 3 --- tests/qapi-schema/escape-outside-string.out | 0 tests/qapi-schema/escape-too-big.err | 1 - tests/qapi-schema/escape-too-big.exit | 1 - tests/qapi-schema/escape-too-big.json | 3 --- tests/qapi-schema/escape-too-big.out | 0 tests/qapi-schema/escape-too-short.err | 1 - tests/qapi-schema/escape-too-short.exit | 1 - tests/qapi-schema/escape-too-short.json | 3 --- tests/qapi-schema/escape-too-short.out | 0 tests/qapi-schema/ident-with-escape.err | 1 + tests/qapi-schema/ident-with-escape.exit | 2 +- tests/qapi-schema/ident-with-escape.json | 2 +- tests/qapi-schema/ident-with-escape.out | 16 -------------- tests/qapi-schema/unknown-escape.json | 2 +- 19 files changed, 5 insertions(+), 58 deletions(-) delete mode 100644 tests/qapi-schema/escape-outside-string.err delete mode 100644 tests/qapi-schema/escape-outside-string.exit delete mode 100644 tests/qapi-schema/escape-outside-string.json delete mode 100644 tests/qapi-schema/escape-outside-string.out delete mode 100644 tests/qapi-schema/escape-too-big.err delete mode 100644 tests/qapi-schema/escape-too-big.exit delete mode 100644 tests/qapi-schema/escape-too-big.json delete mode 100644 tests/qapi-schema/escape-too-big.out delete mode 100644 tests/qapi-schema/escape-too-short.err delete mode 100644 tests/qapi-schema/escape-too-short.exit delete mode 100644 tests/qapi-schema/escape-too-short.json delete mode 100644 tests/qapi-schema/escape-too-short.out