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