[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 05/32] qmp: Get rid of x-oob-test command
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 05/32] qmp: Get rid of x-oob-test command |
Date: |
Tue, 03 Jul 2018 08:05:11 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>> 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 <address@hidden>
>
>> +++ 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 <address@hidden>
Thanks!
- Re: [Qemu-devel] [PATCH 07/32] qmp: Make "id" optional again even in "oob" monitors, (continued)
[Qemu-devel] [PATCH 04/32] qmp: Document COMMAND_DROPPED design flaw, Markus Armbruster, 2018/07/02
[Qemu-devel] [PATCH 05/32] qmp: Get rid of x-oob-test command, Markus Armbruster, 2018/07/02
[Qemu-devel] [PATCH 21/32] qobject: New qdict_from_jsonf_nofail(), Markus Armbruster, 2018/07/02
[Qemu-devel] [PATCH 15/32] qmp: Simplify code around monitor_qmp_dispatch_one(), Markus Armbruster, 2018/07/02
[Qemu-devel] [PATCH 19/32] monitor: Rename use_io_thr to use_io_thread, Markus Armbruster, 2018/07/02
[Qemu-devel] [PATCH 08/32] tests/test-qga: Demonstrate the guest-agent ignores "id", Markus Armbruster, 2018/07/02
[Qemu-devel] [PATCH 14/32] qmp: Always free QMPRequest with qmp_request_free(), Markus Armbruster, 2018/07/02