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


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