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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@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 <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.

/me bows :)

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

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 <eblake@redhat.com>

Thanks!