Re: [Qemu-devel] [PATCH v4 13/22] libqtest: Add qmp_raw()

[Markus Armbruster]

Eric Blake  writes:

> On 08/09/2017 09:54 AM, Markus Armbruster wrote:
>> Eric Blake  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.
>>> +++ 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.
>
> Indeed, and it disappears later in the series.  But it was useful in the
> interim, to prove that ALL callers through this function are passing a
> command name (and therefore my later patches to rewrite qmp() to take a
> command name aren't overlooking any callers).
>
>> 
>> 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.
>
> I'm fine omitting the assertions in the next spin, even if they proved
> useful in this revision for making sure I converted everything.

Omitting them is fine.

Keeping them temporarily with a comment why would also be fine, but more
work.

>>>
>>>  /* 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.
>
> Maybe you'll change your mind by the end of the series, once you see the
> changes to make qmp() shorter (and where those changes necessitate a
> qmp_raw() as the backdoor for anything that is not a normal
> command+arguments).

It's a big series.  I may not see the forest for the trees right now.  A
v2 taking care of the uncontroversial improvements should improve my
view some.

Re: [Qemu-devel] [PATCH v4 13/22] libqtest: Add qmp_raw()

[Eric Blake]

On 08/09/2017 09:54 AM, Markus Armbruster wrote:
> Eric Blake  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.
>> +++ 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.

Indeed, and it disappears later in the series.  But it was useful in the
interim, to prove that ALL callers through this function are passing a
command name (and therefore my later patches to rewrite qmp() to take a
command name aren't overlooking any callers).

> 
> 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.

I'm fine omitting the assertions in the next spin, even if they proved
useful in this revision for making sure I converted everything.


>>
>>  /* 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.

Maybe you'll change your mind by the end of the series, once you see the
changes to make qmp() shorter (and where those changes necessitate a
qmp_raw() as the backdoor for anything that is not a normal
command+arguments).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 13/22] libqtest: Add qmp_raw()

[Markus Armbruster]

Eric Blake  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 
> ---
>  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");
>  

[Qemu-devel] [PATCH v4 13/22] libqtest: Add qmp_raw()

[Eric Blake]

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 
---
 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"));

 /*
  * 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);
-- 
2.13.3