Re: [Qemu-devel] [PATCH v4 17/22] libqtest: Add qmp_args() helper

[Eric Blake]

On 08/09/2017 10:57 AM, Markus Armbruster wrote:

> I don't like the qmp_args name.  It's not about arguments, it's about
> sending a command with arguments.
> 
> I'm afraid I'm getting pretty thorougly confused by the evolving
> interface.  So I stop and think how it should look like when done.

At a bare minimum, I like your idea that we want:

FOO_send()     # sends a message, does not wait for a reply
FOO_receive()  # reads one JSON object, returns QObject counterpart
FOO() # combo of FOO_send() and FOO_receive()

Then FOO_discard() is a convenient shorthand for QDECREF(FOO_receive()).

Which FOO do we need? Right now, we have 'qmp' for anything using the
global_qtest, 'qtest_qmp' for anything using an explicit QTestState (but
we're arguing that is overkill), and 'qmp_fd' for anything using a raw
fd (test-qga.c - and where I added 'qga' in 11/22, although that
addition was local rather than in libqtest.h).

I also just had an insight: it is possible to write a macro
qmp_send(...) that will expand to qmp_send_cmd(name) if there is only
one argument, but to qmp_send_cmdf(name, fmt, ...) if there is more than
one argument (and later expose a public qmp_send_cmdv(name, fmt,
va_list) as pair if it is needed, although right now it is not).
Perhaps we can even make the one-arg form expand to qmp_send_cmdf(name,
" "), if that's enough to silence gcc's -Wformat-zero-length, and if our
underlying routines are smart enough to recognize a single-space
argument as not being worthy of calling qobject_from_jsonf() (since
qobject_from_jsonf() aborts rather than returning NULL when presented
just whitespace).

In fact, having a macro qmp_send(cmd, ...) expand to
qmp_send_internal(cmd, " " __VA_ARGS__) might even do the trick without
preprocessor magic of checking whether __VA_ARGS__ contains a comma
(although it has the subtle limitation that if present, a format
argument MUST be a string literal because we now rely on string
concatenation with " " - although given gcc's -Wformat-nonliteral, we
are probably okay with that limitation).

So, with a nice macro, the bulk of the testsuite would just write in
this style:

qmp_send("foo");
qmp_send("foo", "{literal_args...}");
qmp_send("foo", "{'name':%s}", value);

for send without waiting, or:

obj = qmp("foo");
obj = qmp(foo", "{literal_args...}");
obj = qmp("foo", "{'name':%s}", value);

where the result matters.  And the names we choose for internal
implementation are no longer quite as important (although still helpful
to get right).

> 
> We need send and receive.  Convenience functions to do both, and to
> receive and throw away are nice to have.

Do we want both a convenience function to throw away a single read, AND
a function to send a command + throw away a read? Prior to this series,
we have qmp_discard_response() which is send + discard, but nothing that
does plain discard; although plain discard is a better building block
(precisely because then we don't have to duplicate all the different
send forms) - the convenience of send+receive in one call should be
limited to the most common case.

Also, the qmp_eventwait() is a nice convenience function for wait in a
loop until the received object matches a given name; whether we also
need the convenience of qmp_eventwait_ref() is a bit more debatable
(perhaps we could just as easily have qmp_event_wait() always return an
object, and require the caller to QDECREF() it, for one less function to
maintain).

> 
> We need qmp_raw().  We haven't found a need for a raw receive function.

Yes, qmp_raw() (or maybe it should be named qmp_send_raw(), depending on
whether the receive is coupled with it?) is our backdoor for sending
JSON that does not match the normal {'execute':'name','arguments':{}}
paradigm (and pretty much qmp-test.c, or maybe also test-qga.c, would be
the only clients).

[Side node - why can't we pick a consistent naming of our tests?  The
name 'FOO-test' is a bit nicer than 'test-FOO' when it comes to writing
.gitignore entries. Should we do a bulk cleanup rename to get all the
tests into one consistent naming style?]

> 
> Convenience functions to build commands and send are nice to have.  They
> come in pairs, without arguments, to avoid -Wno-format-zero-length
> (aside: that's one of the sillier warnings).

It's possible to alter CFLAGS to shut gcc up on that one while still
getting the rest of -Wformat, if we don't think it adds value to qemu.
My idea above is that you can use a macro to hide the worst of the
paired nature, so that the bulk of the testsuite can supply or omit an
arguments format string as desired.

> 
> We used to have almost everything with and without QTestState, but we're
> refusing to continue that self-flagellation.

And for v5, I think I'll reorder my series to get rid of QTestState
sooner, rather than later.

> 
> For every function taking ..., we want one taking va_list.

Under the hood, probably.  Whether the va_list form has to be exported
is a different question (we might be able to get by with just the ... form).

> 
> Makes sense?
> 
> Can we derive a sensible set of function names from this?
> 

I gave it a shot.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature