Eric Blake <ebl...@redhat.com> writes: > On 07/02/2018 11:21 AM, Markus Armbruster wrote: >> tests/qmp-test tests an out-of-band command overtaking a slow in-band >> command. To do that, it needs: >> >> 1. An in-band command that *reliably* takes long enough to be >> overtaken. >> >> 2. An out-of-band command to do the overtaking. >> >> 3. To avoid delays, a way to make the in-band command complete quickly >> after it was overtaken. >> >> To satisfy these needs, commit 469638f9cb3 provides the rather >> peculiar oob-capable QMP command x-oob-test: >> >> * With "lock": true, it waits for a global semaphore. >> >> * With "lock": false, it signals the global semaphore. >> >> To satisfy 1., the test runs x-oob-test in-band with "lock": true. >> To satisfy 2. and 3., it runs x-oob-test out-of-band with "lock": false. >> >> Note that waiting for a semaphore violates the rules for oob-capable >> commands. Running x-oob-test with "lock": true hangs the monitor >> until you run x-oob-test with "lock": false on another monitor (which >> you might not have set up). >> >> Having an externally visible QMP command that may hang the monitor is >> not nice. Let's apply a little more ingenuity to the problem. Idea: >> have an existing command block on reading a FIFO special file, unblock >> it by opening the FIFO for writing. >> >> For 1., use >> >> {"execute": "blockdev-add", "id": ID1, >> "arguments": { >> "driver": "blkdebug", "node-name": ID1, "config": FIFO, >> "image": { "driver": "null-co"}}} > > I like it! > >> >> where ID1 is an arbitrary string, and FIFO is the name of the FIFO. >> >> For 2., use >> >> {"execute": "migrate-pause", "id": ID2, "control": {"run-oob": true}} >> >> where ID2 is a different arbitrary string. Since there's no migration >> to pause, the command will fail, but that's fine. > > Maybe add: > > that's fine, since instant failure is still a test of out-of-band > responses overtaking in-band commands.
Sold. >> >> For 3., open FIFO for writing. >> >> Drop QMP command x-oob-test. >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> qapi/misc.json | 18 ---------- >> qmp.c | 16 --------- >> tests/qmp-test.c | 93 ++++++++++++++++++++++++++++++++---------------- >> 3 files changed, 63 insertions(+), 64 deletions(-) > > And in about the same lines of code. /me bows :) > Reviewed-by: Eric Blake <ebl...@redhat.com> > >> +++ b/tests/qmp-test.c >> @@ -135,16 +135,64 @@ static void test_qmp_protocol(void) >> qtest_quit(qts); >> } >> -/* Tests for out-of-band support. */ >> +/* Out-of-band tests */ >> + >> +char tmpdir[] = "/tmp/qmp-test-XXXXXX"; >> +char *fifo_name; >> + >> +static void setup_blocking_cmd(void) >> +{ >> + if (!mkdtemp(tmpdir)) { >> + g_error("mkdtemp: %s", strerror(errno)); >> + } >> + fifo_name = g_strdup_printf("%s/fifo", tmpdir); >> + g_assert(!mkfifo(fifo_name, 0666)); > > g_assert() can be compiled out; should the side-effect mkfifo() call > be done separately from asserting that its return code is expected? We don't support compiling them out (see the note in osdep.h), but you're right, we should do this properly. >> + >> +static void send_oob_cmd(QTestState *s, const char *id) >> +{ >> + qtest_async_qmp(s, "{ 'execute': 'migrate-pause', 'id': %s," >> + " 'control': { 'run-oob': true } }", id); >> +} > > Worth a comment here that we expect this to fail, and really only care > that the response overtakes the in-band message? Yes. > With those nits addressed, > Reviewed-by: Eric Blake <ebl...@redhat.com> Thanks!