Eric Blake <ebl...@redhat.com> writes: > 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.
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.