| Message ID | 20180808120334.10970-6-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | json: Fixes, error reporting improvements, cleanups | expand |
On 08/08/2018 07:02 AM, Markus Armbruster wrote: > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/libqtest.c | 17 +++++++++++++++++ > tests/libqtest.h | 11 +++++++++++ > tests/qmp-test.c | 39 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 67 insertions(+) > > + /* 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? > + resp = qtest_qmp_receive(qts); > + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); > + qobject_unref(resp); > + g_assert(recovered(qts)); > + > + /* lexical error: impossible byte in string */ > + qtest_qmp_send_raw(qts, "{'bad \xFF"); Same question about \xff being special as the parser reset command, so should we test a different byte instead/as well? > + resp = qtest_qmp_receive(qts); > + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); > + qobject_unref(resp); > + g_assert(recovered(qts)); > + > + /* lexical error: interpolation */ > + qtest_qmp_send_raw(qts, "%%p\n"); > + resp = qtest_qmp_receive(qts); > + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); > + qobject_unref(resp); > + g_assert(recovered(qts)); > + > /* Not even a dictionary */ > resp = qtest_qmp(qts, "null"); > g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); >
Eric Blake <eblake@redhat.com> writes: > On 08/08/2018 07:02 AM, Markus Armbruster wrote: >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> tests/libqtest.c | 17 +++++++++++++++++ >> tests/libqtest.h | 11 +++++++++++ >> tests/qmp-test.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 67 insertions(+) >> > >> + /* 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? >> + resp = qtest_qmp_receive(qts); >> + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); >> + qobject_unref(resp); >> + g_assert(recovered(qts)); >> + >> + /* lexical error: impossible byte in string */ >> + qtest_qmp_send_raw(qts, "{'bad \xFF"); > > Same question about \xff being special as the parser reset command, so > should we test a different byte instead/as well? > >> + resp = qtest_qmp_receive(qts); >> + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); >> + qobject_unref(resp); >> + g_assert(recovered(qts)); >> + >> + /* lexical error: interpolation */ >> + qtest_qmp_send_raw(qts, "%%p\n"); >> + resp = qtest_qmp_receive(qts); >> + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); >> + qobject_unref(resp); >> + g_assert(recovered(qts)); >> + >> /* Not even a dictionary */ >> resp = qtest_qmp(qts, "null"); >> g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); >>
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.
Eric Blake <eblake@redhat.com> 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.
diff --git a/tests/libqtest.c b/tests/libqtest.c index 3706f30aa2..c02fc91b37 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -586,6 +586,23 @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...) va_end(ap); } +void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...) +{ + bool log = getenv("QTEST_LOG") != NULL; + va_list ap; + char *str; + + va_start(ap, fmt); + str = g_strdup_vprintf(fmt, ap); + va_end(ap); + + if (log) { + fprintf(stderr, "%s", str); + } + socket_send(s->qmp_fd, str, strlen(str)); + g_free(str); +} + QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event) { QDict *response; diff --git a/tests/libqtest.h b/tests/libqtest.h index def1edaafa..1e831973ff 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -96,6 +96,17 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...) void qtest_qmp_send(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3); +/** + * qtest_qmp_send_raw: + * @s: #QTestState instance to operate on. + * @fmt...: text to send, formatted like sprintf() + * + * Sends text to the QMP monitor verbatim. Need not be valid JSON; + * this is useful for negative tests. + */ +void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...) + GCC_FMT_ATTR(2, 3); + /** * qtest_qmpv: * @s: #QTestState instance to operate on. diff --git a/tests/qmp-test.c b/tests/qmp-test.c index b6eff4fe97..5e56be105e 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -42,10 +42,49 @@ static void test_version(QObject *version) visit_free(v); } +static bool recovered(QTestState *qts) +{ + QDict *resp; + bool ret; + + resp = qtest_qmp(qts, "{ 'execute': 'no-such-cmd' }"); + ret = !strcmp(get_error_class(resp), "CommandNotFound"); + qobject_unref(resp); + return ret; +} + static void test_malformed(QTestState *qts) { QDict *resp; + /* syntax error */ + qtest_qmp_send_raw(qts, "{]\n"); + resp = qtest_qmp_receive(qts); + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); + qobject_unref(resp); + g_assert(recovered(qts)); + + /* lexical error: impossible byte outside string */ + qtest_qmp_send_raw(qts, "{\xFF"); + resp = qtest_qmp_receive(qts); + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); + qobject_unref(resp); + g_assert(recovered(qts)); + + /* lexical error: impossible byte in string */ + qtest_qmp_send_raw(qts, "{'bad \xFF"); + resp = qtest_qmp_receive(qts); + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); + qobject_unref(resp); + g_assert(recovered(qts)); + + /* lexical error: interpolation */ + qtest_qmp_send_raw(qts, "%%p\n"); + resp = qtest_qmp_receive(qts); + g_assert_cmpstr(get_error_class(resp), ==, "GenericError"); + qobject_unref(resp); + g_assert(recovered(qts)); + /* Not even a dictionary */ resp = qtest_qmp(qts, "null"); g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/libqtest.c | 17 +++++++++++++++++ tests/libqtest.h | 11 +++++++++++ tests/qmp-test.c | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+)