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

2018-07-03 Thread Eric Blake

On 07/03/2018 03:53 AM, Markus Armbruster wrote:

tests/qmp-test tests an out-of-band command overtaking a slow in-band
command.  To do that, it needs:





Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 


R-b stands, although:


---



+static void setup_blocking_cmd(void)
+{
+int rc;
+
+if (!mkdtemp(tmpdir)) {
+g_error("mkdtemp: %s", strerror(errno));
+}
+fifo_name = g_strdup_printf("%s/fifo", tmpdir);
+rc = mkfifo(fifo_name, 0666);
+g_assert(!rc);


It's weird seeing two error handling styles in close proximity; we could 
have done:


if (!mkdtemp(tmpdir)) {
g_error(...)
}
fifo_name = ...
if (mkfifo(fifo_name, 0666)) {
g_error(...)
}



+
+static void send_oob_cmd_that_fails(QTestState *s, const char *id)


Nice change of name compared to v1.

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



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

2018-07-03 Thread Markus Armbruster
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"}}}

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; 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 
Reviewed-by: Eric Blake 
---
 qapi/misc.json   | 18 -
 qmp.c| 16 
 tests/qmp-test.c | 96 +---
 3 files changed, 66 insertions(+), 64 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 74cd97f237..f1860418e8 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3472,24 +3472,6 @@
 { 'event': 'COMMAND_DROPPED' ,
   'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
 
-##
-# @x-oob-test:
-#
-# Test OOB functionality.  When sending this command with lock=true,
-# it'll try to hang the dispatcher.  When sending it with lock=false,
-# it'll try to notify the locked thread to continue.  Note: it should
-# only be used by QMP test program rather than anything else.
-#
-# Since: 2.12
-#
-# Example:
-#
-# { "execute": "x-oob-test",
-#   "arguments": { "lock": true } }
-##
-{ 'command': 'x-oob-test', 'data' : { 'lock': 'bool' },
-  'allow-oob': true }
-
 ##
 # @set-numa-node:
 #
diff --git a/qmp.c b/qmp.c
index 73e46d795f..62325ac6aa 100644
--- a/qmp.c
+++ b/qmp.c
@@ -775,19 +775,3 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
 
 return mem_info;
 }
-
-static QemuSemaphore x_oob_test_sem;
-
-static void __attribute__((constructor)) x_oob_test_init(void)
-{
-qemu_sem_init(_oob_test_sem, 0);
-}
-
-void qmp_x_oob_test(bool lock, Error **errp)
-{
-if (lock) {
-qemu_sem_wait(_oob_test_sem);
-} else {
-qemu_sem_post(_oob_test_sem);
-}
-}
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 5206b14ca6..6dd331fcdd 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -135,16 +135,67 @@ static void test_qmp_protocol(void)
 qtest_quit(qts);
 }
 
-/* Tests for out-of-band support. */
+/* Out-of-band tests */
+
+char tmpdir[] = "/tmp/qmp-test-XX";
+char *fifo_name;
+
+static void setup_blocking_cmd(void)
+{
+int rc;
+
+if (!mkdtemp(tmpdir)) {
+g_error("mkdtemp: %s", strerror(errno));
+}
+fifo_name = g_strdup_printf("%s/fifo", tmpdir);
+rc = mkfifo(fifo_name, 0666);
+g_assert(!rc);
+}
+
+static void cleanup_blocking_cmd(void)
+{
+unlink(fifo_name);
+rmdir(tmpdir);
+}
+
+static void send_cmd_that_blocks(QTestState *s, const char *id)
+{
+qtest_async_qmp(s, "{ 'execute': 'blockdev-add',  'id': %s,"
+" 'arguments': {"
+" 'driver': 'blkdebug', 'node-name': %s,"
+" 'config': %s,"
+" 'image': { 'driver': 'null-co' } } }",
+id, id, fifo_name);
+}
+
+static void unblock_blocked_cmd(void)
+{
+int fd = open(fifo_name, O_WRONLY);
+g_assert(fd >= 0);
+close(fd);
+}
+
+static void send_oob_cmd_that_fails(QTestState *s, const char *id)
+{
+qtest_async_qmp(s, "{ 'execute': 'migrate-pause', 'id': %s,"
+"  'control': { 'run-oob': true } }", id);
+}
+
+static void recv_cmd_id(QTestState *s, const char *id)
+{
+QDict *resp = qtest_qmp_receive(s);
+
+g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, id);
+qobject_unref(resp);
+}
+
 static void test_qmp_oob(void)
 {
 QTestState *qts;
 QDict *resp, *q;
-int acks = 0;