Re: [Qemu-devel] [PATCH 05/32] qmp: Get rid of x-oob-test command

[Eric Blake <eblake@redhat.com>]

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.

> 
> For 3., open FIFO for writing.
> 
> Drop QMP command x-oob-test.
> 
> Signed-off-by: Markus Armbruster <armbru@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.

Reviewed-by: Eric Blake <eblake@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?


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

With those nits addressed,
Reviewed-by: Eric Blake <eblake@redhat.com>

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