Eric Blake <ebl...@redhat.com> writes:
> The majority of calls into libqtest's qmp() and friends are passing
> a JSON object that includes a command name; we can prove this by
> adding an assertion. The only outlier is qmp-test, which is testing
> appropriate error responses to protocol violations and id support,
> by sending raw strings, where those raw strings don't need
> interpolation anyways.
>
> Adding a new entry-point makes a clean separation of which input
> needs interpolation, so that upcoming patches can refactor qmp()
> to be more like the recent additions to test-qga in taking the
> command name separately from the arguments for an overall
> reduction in testsuite boilerplate.
>
> This change also lets us move the workaround for the QMP parser
> limitation of not ending a parse until } or newline: all calls
> through qmp() are passing an object ending in }, so only the
> few callers of qmp_raw() have to worry about a trailing newline.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> tests/libqtest.h | 8 ++++++++
> tests/libqtest.c | 13 +++++++------
> tests/qmp-test.c | 19 ++++++++++++-------
> 3 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 33af3ae8ff..917ec5cf92 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -550,6 +550,14 @@ static inline void qtest_end(void)
> QDict *qmp(const char *fmt, ...);
>
> /**
> + * qmp_raw:
> + * @msg: Raw QMP message to send to qemu.
> + *
> + * Sends a QMP message to QEMU and returns the response.
> + */
> +QDict *qmp_raw(const char *msg);
> +
> +/**
> * qmp_async:
> * @fmt...: QMP message to send to qemu; formats arguments through
> * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 0fa32928c8..3071be2efb 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -451,7 +451,7 @@ static void qmp_fd_sendv(int fd, const char *fmt, va_list
> ap)
> QString *qstr;
> const char *str;
>
> - assert(*fmt);
> + assert(strstr(fmt, "execute"));
I doubt this assertion is worthwhile.
One , qmp_fd_sendv() works just fine whether you include an 'execute' or
not. Two, there are zillions of other ways to send nonsense with
qmp_fd_sendv(). If you do, your test doesn't work, so you fix it.
Rejecting nonsensical QMP input is QEMU's job, not libqtest's.
>
> /*
> * A round trip through QObject is only needed if % interpolation
> @@ -496,11 +496,6 @@ void qmp_fd_send(int fd, const char *msg)
> }
> /* Send QMP request */
> socket_send(fd, msg, strlen(msg));
> - /*
> - * BUG: QMP doesn't react to input until it sees a newline, an
> - * object, or an array. Work-around: give it a newline.
> - */
> - socket_send(fd, "\n", 1);
> }
>
> QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
> @@ -899,6 +894,12 @@ QDict *qmp(const char *fmt, ...)
> return response;
> }
>
> +QDict *qmp_raw(const char *msg)
> +{
> + qmp_fd_send(global_qtest->qmp_fd, msg);
> + return qtest_qmp_receive(global_qtest);
> +}
> +
> void qmp_async(const char *fmt, ...)
> {
> va_list ap;
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index 5d0260b2be..905fb4b3e5 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -44,28 +44,33 @@ static void test_malformed(void)
> {
> QDict *resp;
>
> + /*
> + * BUG: QMP doesn't react to input until it sees a newline, an
> + * object, or an array. Work-around: give it a newline.
> + */
> +
> /* Not even a dictionary */
> - resp = qmp("null");
> + resp = qmp_raw("null\n");
> g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> QDECREF(resp);
>
> /* No "execute" key */
> - resp = qmp("{}");
> + resp = qmp_raw("{}");
> g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> QDECREF(resp);
>
> /* "execute" isn't a string */
> - resp = qmp("{ 'execute': true }");
> + resp = qmp_raw("{ 'execute': true }");
> g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> QDECREF(resp);
>
> /* "arguments" isn't a dictionary */
> - resp = qmp("{ 'execute': 'no-such-cmd', 'arguments': [] }");
> + resp = qmp_raw("{ 'execute': 'no-such-cmd', 'arguments': [] }");
> g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> QDECREF(resp);
>
> /* extra key */
> - resp = qmp("{ 'execute': 'no-such-cmd', 'extra': true }");
> + resp = qmp_raw("{ 'execute': 'no-such-cmd', 'extra': true }");
> g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> QDECREF(resp);
> }
> @@ -114,14 +119,14 @@ static void test_qmp_protocol(void)
> test_malformed();
>
> /* Test 'id' */
> - resp = qmp("{ 'execute': 'query-name', 'id': 'cookie#1' }");
> + resp = qmp_raw("{ 'execute': 'query-name', 'id': 'cookie#1' }");
> ret = qdict_get_qdict(resp, "return");
> g_assert(ret);
> g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, "cookie#1");
> QDECREF(resp);
>
> /* Test command failure with 'id' */
> - resp = qmp("{ 'execute': 'human-monitor-command', 'id': 2 }");
> + resp = qmp_raw("{ 'execute': 'human-monitor-command', 'id': 2 }");
> g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
> QDECREF(resp);
I'm afraid I don't like this patch. All the extra function buys us is
an assertion that isn't even tight, and the lifting of a newline out of
a place where it shouldn't be.
