Re: [Qemu-devel] [PATCH v4 15/22] libqtest: Delete qtest_qmp() wrappers

[Eric Blake]

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