On 08/09/2017 10:34 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> None of our tests were directly using qtest_qmp() and friends; >> even tests like postcopy-test.c that manage multiple connections >> get along just fine changing global_qtest as needed (other than >> one callsite where it forgot to use the inlined form). It's >> also annoying that we have qmp_async() but qtest_async_qmp(), >> with inconsistent naming for tracing through the wrappers. >>
> What about all the other functions taking a QTestState? Aren't they
> just as silly?
What's left after this patch:
- qtest_init()
qtest_init_without_qmp_handshake()
qtest_quit()
necessary for setting up parallel state.
and then a lot of functions that have static inline wrappers (for
example, qmp_receive(), inb(), ...).
So yes, I could additionally get rid of more wrappers and have even more
functions implicitly depend on global_qtest.
>
> Having two of every function is tiresome, but consistent.
>
> Having just one is easier to maintain, so if it serves our needs,
> possibly with the occasional state switch, I like it.
>
> What I don't like is a mix of the two.
Okay, I'll prune even harder in the next revision. Deleting cruft is fun!
>> +++ b/tests/libqtest.c
>> @@ -233,9 +233,10 @@ QTestState *qtest_init(const char *extra_args)
>> QDict *greeting;
>>
>> /* Read the QMP greeting and then do the handshake */
>> - greeting = qtest_qmp_receive(s);
>> + greeting = qmp_fd_receive(s->qmp_fd);
>
> Why doesn't this become qmp_receive()?
Because in THIS version of the patch, qmp_receive() is still a static
inline wrapper that calls qtest_qmp_receive(global_qtest) - but
global_qtest is not set here. (If I delete qtest_qmp_receive() and have
qmp_receive() not be static inline, then we STILL want to operate
directly on the just-created s->qmp_fd rather than assuming that
global_qtest == s, when in the body of qtest_init).
>> @@ -446,7 +447,7 @@ QDict *qtest_qmp_receive(QTestState *s)
>> * Internal code that converts from interpolated JSON into a message
>> * to send over the wire, without waiting for a reply.
>> */
>> -static void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>> +static void qtest_qmp_sendv(QTestState *s, const char *fmt, va_list ap)
>
> Why this move in the other direction?
Because it fixes the disparity you pointed out in 12/22 about
qmp_fd_sendv() no longer being a sane pairing to qmp_fd_send(), AND
because I needed the .../va_list pairing to work in order for...
>> -char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
>> +static char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
>> {
>> char *cmd;
>> QDict *resp;
>> char *ret;
>>
>> cmd = g_strdup_vprintf(fmt, ap);
>> - resp = qtest_qmp(s, "{'execute': 'human-monitor-command',"
>> - " 'arguments': {'command-line': %s}}",
>> - cmd);
>> + qtest_qmp_send(s, "{'execute': 'human-monitor-command',"
>> + " 'arguments': {'command-line': %s}}", cmd);
>> + resp = qtest_qmp_receive(s);
...this to work. So now my sane pairing is
qtest_qmp_send()/qtest_qmp_sendv() - although your argument that
qtest_qmp_sendf() might have been a nicer name for the ... form may
still have merit - at least any time the sendv() form is in a public
header. Then again, by the end of the series, I managed to get rid of
all va_list in libqtest.h, needing it only in libqtest.c.
>> @@ -889,7 +854,7 @@ void qmp_async(const char *fmt, ...)
>> va_list ap;
>>
>> va_start(ap, fmt);
>> - qtest_async_qmpv(global_qtest, fmt, ap);
>> + qtest_qmp_sendv(global_qtest, fmt, ap);
>> va_end(ap);
>> }
>
> Hmm. Before this patch, qmp_async() is the ... buddy of va_list
> qmp_fd_sendv(). If we keep qmp_fd_sendv(), they should be named
> accordingly.
What name to use, though? By the end of the series, we have
qmp_async(...) but no public va_list form.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
