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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

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

"Necessary" is too strong, as you could use qtest_start(); save
global_qtest; qtest_start(); ... qtest_end(); restore global_qtest;
qtest_end().  But I could buy convenient.

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

Yes, please!

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

This is all quite confusing to me right now.  I trust I'll do better
with v2.

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

Discussed later in the thread.