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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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