[Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

'socket_accept' waits for Qemu to init its unix socket.
If Qemu encounters an error during command line parsing,
it can exit before initializing the communication channel.
It gets worse as the make check-qtest-* gets stuck without
notifying which test exactly has problems, so debugging can
be a challenge.

The solution has two parts:
 - Use a timeout for the socket.
 - Expose a qtest_state_valid that checks that the connections
   with Qemu are OK.
Asserting qtest_state_valid in each test after qtest_init
is a must, as we need to trace which test failed.

As a nice side effect, even the nop tests will
check at least that the device/option is still
supported by Qemu.

Note: I encountered this issue while working on 'Machine as QOM object';
because of my changes some tests tried to run qtest with a not supported
machine. The result was 'make check' getting stuck with no clue why is happening.

Marcel Apfelbaum (2):
  tests/libqtest: Fix possible deadlock in qtest initialization
  tests: check that qtest state is valid before starting the test

 tests/acpi-test.c         |  4 +++-
 tests/blockdev-test.c     |  4 +++-
 tests/boot-order-test.c   |  4 +++-
 tests/e1000-test.c        |  5 ++++-
 tests/eepro100-test.c     |  1 +
 tests/endianness-test.c   | 15 ++++++++++++---
 tests/fdc-test.c          |  5 ++++-
 tests/fw_cfg-test.c       |  2 ++
 tests/hd-geo-test.c       | 20 ++++++++++++++++----
 tests/i440fx-test.c       | 10 ++++++++--
 tests/ide-test.c          |  5 ++++-
 tests/ipoctal232-test.c   |  5 ++++-
 tests/libqtest.c          | 26 +++++++++++++++++++++-----
 tests/libqtest.h          |  8 ++++++++
 tests/m48t59-test.c       |  1 +
 tests/ne2000-test.c       |  5 ++++-
 tests/pcnet-test.c        |  5 ++++-
 tests/qdev-monitor-test.c |  4 +++-
 tests/qom-test.c          | 10 ++++++++--
 tests/rtc-test.c          |  4 +++-
 tests/rtl8139-test.c      |  5 ++++-
 tests/tmp105-test.c       |  4 +++-
 tests/tpci200-test.c      |  5 ++++-
 tests/virtio-net-test.c   |  5 ++++-
 tests/vmxnet3-test.c      |  5 ++++-
 25 files changed, 135 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

'socket_accept' waits for Qemu to init its unix socket.
If Qemu encounters an error during command line parsing,
it can exit before initializing the communication channel.
It gets worse as the make check-qtest-* gets stuck without
notifying which test exactly has problems, so debugging can
be a challenge.

The solution has two parts:
 - Use a timeout for the socket.
 - Expose a qtest_state_valid that checks that the connections
   with Qemu are OK.
Asserting qtest_state_valid in each test after qtest_init
is a must, as we need to trace which test failed.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 tests/libqtest.c | 26 +++++++++++++++++++++-----
 tests/libqtest.h |  8 ++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index f587d36..93dfa81 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -34,6 +34,7 @@
 #include "qapi/qmp/json-parser.h"
 
 #define MAX_IRQ 256
+#define SOCKET_TIMEOUT 5
 
 QTestState *global_qtest;
 
@@ -83,7 +84,6 @@ static int socket_accept(int sock)
     do {
         ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
     } while (ret == -1 && errno == EINTR);
-    g_assert_no_errno(ret);
     close(sock);
 
     return ret;
@@ -111,6 +111,8 @@ QTestState *qtest_init(const char *extra_args)
     gchar *command;
     const char *qemu_binary;
     struct sigaction sigact;
+    struct timeval socket_timeout = { .tv_sec = SOCKET_TIMEOUT,
+                                      .tv_usec = 0 };
 
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
@@ -123,6 +125,11 @@ QTestState *qtest_init(const char *extra_args)
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
+    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
+               sizeof(socket_timeout));
+    setsockopt(qmpsock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
+               sizeof(socket_timeout));
+
     /* Catch SIGABRT to clean up on g_assert() failure */
     sigact = (struct sigaction){
         .sa_handler = sigabrt_handler,
@@ -147,7 +154,9 @@ QTestState *qtest_init(const char *extra_args)
     }
 
     s->fd = socket_accept(sock);
-    s->qmp_fd = socket_accept(qmpsock);
+    if (s->fd >= 0) {
+        s->qmp_fd = socket_accept(qmpsock);
+    }
     unlink(socket_path);
     unlink(qmp_socket_path);
     g_free(socket_path);
@@ -158,9 +167,11 @@ QTestState *qtest_init(const char *extra_args)
         s->irq_level[i] = false;
     }
 
-    /* Read the QMP greeting and then do the handshake */
-    qtest_qmp_discard_response(s, "");
-    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
+    if (qtest_state_valid(s)) {
+        /* Read the QMP greeting and then do the handshake */
+        qtest_qmp_discard_response(s, "");
+        qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
+    }
 
     if (getenv("QTEST_STOP")) {
         kill(s->qemu_pid, SIGSTOP);
@@ -169,6 +180,11 @@ QTestState *qtest_init(const char *extra_args)
     return s;
 }
 
+bool qtest_state_valid(QTestState *s)
+{
+    return (s->fd >= 0) && (s->qmp_fd >= 0);
+}
+
 void qtest_quit(QTestState *s)
 {
     sigaction(SIGABRT, &s->sigact_old, NULL);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 9deebdc..39a37b1 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -45,6 +45,14 @@ QTestState *qtest_init(const char *extra_args);
 void qtest_quit(QTestState *s);
 
 /**
+ * qtest_state_valid:
+ * @state: #QTestState instance to check
+ *
+ * Returns: True if qtest was initialized successfully
+ */
+bool qtest_state_valid(QTestState *s);
+
+/**
  * qtest_qmp_discard_response:
  * @s: #QTestState instance to operate on.
  * @fmt...: QMP message to send to qemu
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] tests: check that qtest state is valid before starting the test

[Marcel Apfelbaum]

It is possible that the argument parsing went wrong,
check that qstate is valid after qtest_init.
As a nice side effect, even the nop tests will
check at least that the device/option is still
supported by Qemu.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 tests/acpi-test.c         |  4 +++-
 tests/blockdev-test.c     |  4 +++-
 tests/boot-order-test.c   |  4 +++-
 tests/e1000-test.c        |  5 ++++-
 tests/eepro100-test.c     |  1 +
 tests/endianness-test.c   | 15 ++++++++++++---
 tests/fdc-test.c          |  5 ++++-
 tests/fw_cfg-test.c       |  2 ++
 tests/hd-geo-test.c       | 20 ++++++++++++++++----
 tests/i440fx-test.c       | 10 ++++++++--
 tests/ide-test.c          |  5 ++++-
 tests/ipoctal232-test.c   |  5 ++++-
 tests/m48t59-test.c       |  1 +
 tests/ne2000-test.c       |  5 ++++-
 tests/pcnet-test.c        |  5 ++++-
 tests/qdev-monitor-test.c |  4 +++-
 tests/qom-test.c          | 10 ++++++++--
 tests/rtc-test.c          |  4 +++-
 tests/rtl8139-test.c      |  5 ++++-
 tests/tmp105-test.c       |  4 +++-
 tests/tpci200-test.c      |  5 ++++-
 tests/virtio-net-test.c   |  5 ++++-
 tests/vmxnet3-test.c      |  5 ++++-
 23 files changed, 106 insertions(+), 27 deletions(-)

diff --git a/tests/acpi-test.c b/tests/acpi-test.c
index 31f5359..cbcc849 100644
--- a/tests/acpi-test.c
+++ b/tests/acpi-test.c
@@ -551,6 +551,7 @@ static void test_acpi_asl(test_data *data)
 
 static void test_acpi_one(const char *params, test_data *data)
 {
+    QTestState *s;
     char *args;
     uint8_t signature_low;
     uint8_t signature_high;
@@ -564,7 +565,8 @@ static void test_acpi_one(const char *params, test_data *data)
 
     args = g_strdup_printf("-net none -display none %s -drive file=%s%s,",
                            params ? params : "", disk, device);
-    qtest_start(args);
+    s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
 
    /* Wait at most 1 minute */
 #define TEST_DELAY (1 * G_USEC_PER_SEC / 10)
diff --git a/tests/blockdev-test.c b/tests/blockdev-test.c
index c940e00..d0a8bbf 100644
--- a/tests/blockdev-test.c
+++ b/tests/blockdev-test.c
@@ -16,11 +16,13 @@
 
 static void test_drive_add_empty(void)
 {
+    QTestState *s;
     QDict *response;
     const char *response_return;
 
     /* Start with an empty drive */
-    qtest_start("-drive if=none,id=drive0");
+    s = qtest_start("-drive if=none,id=drive0");
+    g_assert(qtest_state_valid(s));
 
     /* Delete the drive */
     response = qmp("{\"execute\": \"human-monitor-command\","
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 360a691..10f995d 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -31,6 +31,7 @@ static void test_a_boot_order(const char *machine,
                               uint64_t expected_boot,
                               uint64_t expected_reboot)
 {
+    QTestState *s;
     char *args;
     uint64_t actual;
 
@@ -38,7 +39,8 @@ static void test_a_boot_order(const char *machine,
                            machine ? " -M " : "",
                            machine ?: "",
                            test_args);
-    qtest_start(args);
+    s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
     actual = read_boot_order();
     g_assert_cmphex(actual, ==, expected_boot);
     qmp_discard_response("{ 'execute': 'system_reset' }");
diff --git a/tests/e1000-test.c b/tests/e1000-test.c
index a8ba2fc..2dfc18f 100644
--- a/tests/e1000-test.c
+++ b/tests/e1000-test.c
@@ -19,12 +19,15 @@ static void nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/e1000/nop", nop);
 
-    qtest_start("-device e1000");
+    s = qtest_start("-device e1000");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
index bf82526..19623c0 100644
--- a/tests/eepro100-test.c
+++ b/tests/eepro100-test.c
@@ -20,6 +20,7 @@ static void test_device(gconstpointer data)
 
     args = g_strdup_printf("-device %s", model);
     s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
 
     /* Tests only initialization so far. TODO: Implement functional tests */
 
diff --git a/tests/endianness-test.c b/tests/endianness-test.c
index 92e17d2..6ffe6a2 100644
--- a/tests/endianness-test.c
+++ b/tests/endianness-test.c
@@ -118,6 +118,7 @@ static void isa_outl(const TestCase *test, uint16_t addr, uint32_t value)
 
 static void test_endianness(gconstpointer data)
 {
+    QTestState *s;
     const TestCase *test = data;
     char *args;
 
@@ -125,7 +126,9 @@ static void test_endianness(gconstpointer data)
                            test->machine,
                            test->superio ? " -device " : "",
                            test->superio ?: "");
-    qtest_start(args);
+    s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
+
     isa_outl(test, 0xe0, 0x87654321);
     g_assert_cmphex(isa_inl(test, 0xe0), ==, 0x87654321);
     g_assert_cmphex(isa_inw(test, 0xe2), ==, 0x8765);
@@ -193,6 +196,7 @@ static void test_endianness(gconstpointer data)
 
 static void test_endianness_split(gconstpointer data)
 {
+    QTestState *s;
     const TestCase *test = data;
     char *args;
 
@@ -200,7 +204,9 @@ static void test_endianness_split(gconstpointer data)
                            test->machine,
                            test->superio ? " -device " : "",
                            test->superio ?: "");
-    qtest_start(args);
+    s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
+
     isa_outl(test, 0xe8, 0x87654321);
     g_assert_cmphex(isa_inl(test, 0xe0), ==, 0x87654321);
     g_assert_cmphex(isa_inw(test, 0xe2), ==, 0x8765);
@@ -240,6 +246,7 @@ static void test_endianness_split(gconstpointer data)
 
 static void test_endianness_combine(gconstpointer data)
 {
+    QTestState *s;
     const TestCase *test = data;
     char *args;
 
@@ -247,7 +254,9 @@ static void test_endianness_combine(gconstpointer data)
                            test->machine,
                            test->superio ? " -device " : "",
                            test->superio ?: "");
-    qtest_start(args);
+    s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
+
     isa_outl(test, 0xe0, 0x87654321);
     g_assert_cmphex(isa_inl(test, 0xe8), ==, 0x87654321);
     g_assert_cmphex(isa_inw(test, 0xea), ==, 0x8765);
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 37096dc..cadada8 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -517,6 +517,7 @@ static void fuzz_registers(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     const char *arch = qtest_get_arch();
     int fd;
     int ret;
@@ -537,7 +538,9 @@ int main(int argc, char **argv)
     /* Run the tests */
     g_test_init(&argc, &argv, NULL);
 
-    qtest_start(NULL);
+    s = qtest_start(NULL);
+    g_assert(qtest_state_valid(s));
+
     qtest_irq_intercept_in(global_qtest, "ioapic");
     qtest_add_func("/fdc/cmos", test_cmos);
     qtest_add_func("/fdc/no_media_on_start", test_no_media_on_start);
diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 5c8f8d6..da03789 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -127,6 +127,8 @@ int main(int argc, char **argv)
 
     cmdline = g_strdup_printf("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8 ");
     s = qtest_start(cmdline);
+    g_assert(qtest_state_valid(s));
+
     g_free(cmdline);
 
     ret = g_test_run();
diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
index c84d1e7..636e4e2 100644
--- a/tests/hd-geo-test.c
+++ b/tests/hd-geo-test.c
@@ -239,16 +239,20 @@ static int setup_ide(int argc, char *argv[], int argv_sz,
  */
 static void test_ide_none(void)
 {
+    QTestState *s;
     char *argv[256];
 
     setup_common(argv, ARRAY_SIZE(argv));
-    qtest_start(g_strjoinv(" ", argv));
+    s = qtest_start(g_strjoinv(" ", argv));
+    g_assert(qtest_state_valid(s));
+
     test_cmos();
     qtest_end();
 }
 
 static void test_ide_mbr(bool use_device, MBRcontents mbr)
 {
+    QTestState *s;
     char *argv[256];
     int argc;
     Backend i;
@@ -260,7 +264,9 @@ static void test_ide_mbr(bool use_device, MBRcontents mbr)
         dev = use_device ? (is_hd(cur_ide[i]) ? "ide-hd" : "ide-cd") : NULL;
         argc = setup_ide(argc, argv, ARRAY_SIZE(argv), i, dev, i, mbr, "");
     }
-    qtest_start(g_strjoinv(" ", argv));
+    s = qtest_start(g_strjoinv(" ", argv));
+    g_assert(qtest_state_valid(s));
+
     test_cmos();
     qtest_end();
 }
@@ -315,6 +321,7 @@ static void test_ide_device_mbr_chs(void)
 
 static void test_ide_drive_user(const char *dev, bool trans)
 {
+    QTestState *s;
     char *argv[256], *opts;
     int argc;
     int secs = img_secs[backend_small];
@@ -332,7 +339,9 @@ static void test_ide_drive_user(const char *dev, bool trans)
                      0, dev ? opts : NULL, backend_small, mbr_chs,
                      dev ? "" : opts);
     g_free(opts);
-    qtest_start(g_strjoinv(" ", argv));
+    s = qtest_start(g_strjoinv(" ", argv));
+    g_assert(qtest_state_valid(s));
+
     test_cmos();
     qtest_end();
 }
@@ -374,6 +383,7 @@ static void test_ide_device_user_chst(void)
  */
 static void test_ide_drive_cd_0(void)
 {
+    QTestState *s;
     char *argv[256];
     int argc, ide_idx;
     Backend i;
@@ -385,7 +395,9 @@ static void test_ide_drive_cd_0(void)
         argc = setup_ide(argc, argv, ARRAY_SIZE(argv),
                          ide_idx, NULL, i, mbr_blank, "");
     }
-    qtest_start(g_strjoinv(" ", argv));
+    s = qtest_start(g_strjoinv(" ", argv));
+    g_assert(qtest_state_valid(s));
+
     test_cmos();
     qtest_end();
 }
diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index ad232b5..b6f1add 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -41,10 +41,13 @@ typedef struct FirmwareTestFixture {
 
 static QPCIBus *test_start_get_bus(const TestData *s)
 {
+    QTestState *state;
     char *cmdline;
 
     cmdline = g_strdup_printf("-smp %d", s->num_cpus);
-    qtest_start(cmdline);
+    state = qtest_start(cmdline);
+    g_assert(qtest_state_valid(state));
+
     g_free(cmdline);
     return qpci_init_pc();
 }
@@ -334,6 +337,7 @@ static char *create_blob_file(void)
 static void test_i440fx_firmware(FirmwareTestFixture *fixture,
                                  gconstpointer user_data)
 {
+    QTestState *s;
     char *fw_pathname, *cmdline;
     uint8_t *buf;
     size_t i, isa_bios_size;
@@ -346,7 +350,9 @@ static void test_i440fx_firmware(FirmwareTestFixture *fixture,
                               fixture->is_bios ? "-bios" : "-pflash",
                               fw_pathname);
     g_test_message("qemu cmdline: %s", cmdline);
-    qtest_start(cmdline);
+    s = qtest_start(cmdline);
+    g_assert(qtest_state_valid(s));
+
     g_free(cmdline);
 
     /* QEMU has loaded the firmware (because qtest_start() only returns after
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 4a0d97f..94c4a38 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -109,6 +109,7 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
 
 static void ide_test_start(const char *cmdline_fmt, ...)
 {
+    QTestState *s;
     va_list ap;
     char *cmdline;
 
@@ -116,7 +117,9 @@ static void ide_test_start(const char *cmdline_fmt, ...)
     cmdline = g_strdup_vprintf(cmdline_fmt, ap);
     va_end(ap);
 
-    qtest_start(cmdline);
+    s = qtest_start(cmdline);
+    g_assert(qtest_state_valid(s));
+
     qtest_irq_intercept_in(global_qtest, "ioapic");
     guest_malloc = pc_alloc_init();
 }
diff --git a/tests/ipoctal232-test.c b/tests/ipoctal232-test.c
index 3ac1714..6d7323c 100644
--- a/tests/ipoctal232-test.c
+++ b/tests/ipoctal232-test.c
@@ -19,12 +19,15 @@ static void nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/ipoctal232/tpci200/nop", nop);
 
-    qtest_start("-device tpci200,id=ipack0 -device ipoctal232,bus=ipack0.0");
+    s = qtest_start("-device tpci200,id=ipack0 -device ipoctal232,bus=ipack0.0");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/m48t59-test.c b/tests/m48t59-test.c
index 71b4f28..bda53e8 100644
--- a/tests/m48t59-test.c
+++ b/tests/m48t59-test.c
@@ -251,6 +251,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     s = qtest_start("-rtc clock=vm");
+    g_assert(qtest_state_valid(s));
 
     qtest_add_func("/rtc/bcd/check-time", bcd_check_time);
     qtest_add_func("/rtc/fuzz-registers", fuzz_registers);
diff --git a/tests/ne2000-test.c b/tests/ne2000-test.c
index 61a678a..85671d9 100644
--- a/tests/ne2000-test.c
+++ b/tests/ne2000-test.c
@@ -19,12 +19,15 @@ static void pci_nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/ne2000/pci/nop", pci_nop);
 
-    qtest_start("-device ne2k_pci");
+    s = qtest_start("-device ne2k_pci");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/pcnet-test.c b/tests/pcnet-test.c
index 84af4f3..0c9db5b 100644
--- a/tests/pcnet-test.c
+++ b/tests/pcnet-test.c
@@ -19,12 +19,15 @@ static void pci_nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/pcnet/pci/nop", pci_nop);
 
-    qtest_start("-device pcnet");
+    s = qtest_start("-device pcnet");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/qdev-monitor-test.c b/tests/qdev-monitor-test.c
index ba7f9cc..5f1ba62 100644
--- a/tests/qdev-monitor-test.c
+++ b/tests/qdev-monitor-test.c
@@ -17,10 +17,12 @@
 
 static void test_device_add(void)
 {
+    QTestState *s;
     QDict *response;
     QDict *error;
 
-    qtest_start("-drive if=none,id=drive0");
+    s = qtest_start("-drive if=none,id=drive0");
+    g_assert(qtest_state_valid(s));
 
     /* Make device_add fail.  If this leaks the virtio-blk-pci device then a
      * reference to drive0 will also be held (via qdev properties).
diff --git a/tests/qom-test.c b/tests/qom-test.c
index b6671fb..3b1a30e 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -45,12 +45,15 @@ static bool is_blacklisted(const char *arch, const char *mach)
 
 static void test_machine(gconstpointer data)
 {
+    QTestState *s;
     const char *machine = data;
     char *args;
     QDict *response;
 
     args = g_strdup_printf("-machine %s", machine);
-    qtest_start(args);
+    s = qtest_start(args);
+    g_assert(qtest_state_valid(s));
+
     response = qmp("{ 'execute': 'quit' }");
     g_assert(qdict_haskey(response, "return"));
     qtest_end();
@@ -59,6 +62,7 @@ static void test_machine(gconstpointer data)
 
 static void add_machine_test_cases(void)
 {
+    QTestState *s;
     const char *arch = qtest_get_arch();
     QDict *response, *minfo;
     QList *list;
@@ -67,7 +71,9 @@ static void add_machine_test_cases(void)
     QString *qstr;
     const char *mname, *path;
 
-    qtest_start("-machine none");
+    s = qtest_start("-machine none");
+    g_assert(qtest_state_valid(s));
+
     response = qmp("{ 'execute': 'query-machines' }");
     g_assert(response);
     list = qdict_get_qlist(response, "return");
diff --git a/tests/rtc-test.c b/tests/rtc-test.c
index 4243624..9668c4a 100644
--- a/tests/rtc-test.c
+++ b/tests/rtc-test.c
@@ -548,12 +548,14 @@ static void register_b_set_flag(void)
 
 int main(int argc, char **argv)
 {
-    QTestState *s = NULL;
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
     s = qtest_start("-rtc clock=vm");
+    g_assert(qtest_state_valid(s));
+
     qtest_irq_intercept_in(s, "ioapic");
 
     qtest_add_func("/rtc/check-time/bcd", bcd_check_time);
diff --git a/tests/rtl8139-test.c b/tests/rtl8139-test.c
index f6a1be3..cfb9feb 100644
--- a/tests/rtl8139-test.c
+++ b/tests/rtl8139-test.c
@@ -19,12 +19,15 @@ static void nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/rtl8139/nop", nop);
 
-    qtest_start("-device rtl8139");
+    s = qtest_start("-device rtl8139");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/tmp105-test.c b/tests/tmp105-test.c
index 0834219..565a225 100644
--- a/tests/tmp105-test.c
+++ b/tests/tmp105-test.c
@@ -55,12 +55,14 @@ static void send_and_receive(void)
 
 int main(int argc, char **argv)
 {
-    QTestState *s = NULL;
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
     s = qtest_start("-machine n800");
+    g_assert(qtest_state_valid(s));
+
     i2c = omap_i2c_create(OMAP2_I2C_1_BASE);
     addr = N8X0_ADDR;
 
diff --git a/tests/tpci200-test.c b/tests/tpci200-test.c
index 9ae0127..dd4e248 100644
--- a/tests/tpci200-test.c
+++ b/tests/tpci200-test.c
@@ -19,12 +19,15 @@ static void nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/tpci200/nop", nop);
 
-    qtest_start("-device tpci200");
+    s = qtest_start("-device tpci200");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index df99343..e0b0bc5 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -19,12 +19,15 @@ static void pci_nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/net/pci/nop", pci_nop);
 
-    qtest_start("-device virtio-net-pci");
+    s = qtest_start("-device virtio-net-pci");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/vmxnet3-test.c b/tests/vmxnet3-test.c
index a2ebed3..9546f24 100644
--- a/tests/vmxnet3-test.c
+++ b/tests/vmxnet3-test.c
@@ -19,12 +19,15 @@ static void nop(void)
 
 int main(int argc, char **argv)
 {
+    QTestState *s;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/vmxnet3/nop", nop);
 
-    qtest_start("-device vmxnet3");
+    s = qtest_start("-device vmxnet3");
+    g_assert(qtest_state_valid(s));
+
     ret = g_test_run();
 
     qtest_end();
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

On Mon, Mar 10, 2014 at 02:12:12PM +0200, Marcel Apfelbaum wrote:
> 'socket_accept' waits for Qemu to init its unix socket.
> If Qemu encounters an error during command line parsing,
> it can exit before initializing the communication channel.
> It gets worse as the make check-qtest-* gets stuck without
> notifying which test exactly has problems, so debugging can
> be a challenge.
> 
> The solution has two parts:
>  - Use a timeout for the socket.
>  - Expose a qtest_state_valid that checks that the connections
>    with Qemu are OK.

See below why I think qtest_state_valid() is unnecessary as a libqtest.h
API.

> Asserting qtest_state_valid in each test after qtest_init
> is a must, as we need to trace which test failed.

Inability to tell which qtest failed is a Makefile problem.  The
solution is not to move all asserts to the outer-most level just so the
error message includes the test name.

Either we need to invoke gtester separately for each test - that way the
Makefile can print "TEST <name>" for each binary.  Or maybe gtester has
options for formatting output better.

Stefan

Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

On Mon, 2014-03-10 at 16:02 +0100, Stefan Hajnoczi wrote:
> On Mon, Mar 10, 2014 at 02:12:12PM +0200, Marcel Apfelbaum wrote:
> > 'socket_accept' waits for Qemu to init its unix socket.
> > If Qemu encounters an error during command line parsing,
> > it can exit before initializing the communication channel.
> > It gets worse as the make check-qtest-* gets stuck without
> > notifying which test exactly has problems, so debugging can
> > be a challenge.
> > 
> > The solution has two parts:
> >  - Use a timeout for the socket.
> >  - Expose a qtest_state_valid that checks that the connections
> >    with Qemu are OK.
> 
> See below why I think qtest_state_valid() is unnecessary as a libqtest.h
> API.
> 
> > Asserting qtest_state_valid in each test after qtest_init
> > is a must, as we need to trace which test failed.
> 
> Inability to tell which qtest failed is a Makefile problem.  The
> solution is not to move all asserts to the outer-most level just so the
> error message includes the test name.
> 
> Either we need to invoke gtester separately for each test - that way the
> Makefile can print "TEST <name>" for each binary.  Or maybe gtester has
> options for formatting output better.
Hi Stefan,
Thanks for the review.

I am more concerned of PATCH 1/2, because it is a blocker for another series I am working on.
I can resend only the first one which adds socket timeout and leaves the original assert.
Would you be OK with this?

Now regarding the issue you brought up (less important):
- Tweaking the Makefile to run each qtest separately and not all tests per arch
  is a viable solution, however I am not familiar with the makefile magic and
  it will take me a lot of time to get into it.
- I am not sure how gtester formatting options can helps us here, because
  we *will* get an assert (after the first patch), but it would be in the qtestlib
  which is not the desired place. (it is not a qtestlib bug)


Thanks,
Marcel
> 
> Stefan
> 

Re: [Qemu-devel] [PATCH 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

On Mon, Mar 10, 2014 at 02:12:13PM +0200, Marcel Apfelbaum wrote:
> @@ -123,6 +125,11 @@ QTestState *qtest_init(const char *extra_args)
>      sock = init_socket(socket_path);
>      qmpsock = init_socket(qmp_socket_path);
>  
> +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> +               sizeof(socket_timeout));
> +    setsockopt(qmpsock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> +               sizeof(socket_timeout));
> +

Please move this into socket_accept() so qtest_init() doesn't need to
know about setsockopt() details.  It should just get an fd or -errno
return value from socket_accept()...

>      s->fd = socket_accept(sock);
> -    s->qmp_fd = socket_accept(qmpsock);
> +    if (s->fd >= 0) {
> +        s->qmp_fd = socket_accept(qmpsock);
> +    }
>      unlink(socket_path);
>      unlink(qmp_socket_path);

...then you can ensure that we unlink the sockets even on failure.

And then something like g_assert(s->fd >= 0 && s->qmp_fd >= 0) would be
fine.  The process can abort() at this point.

Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

On Mon, Mar 10, 2014 at 05:21:17PM +0200, Marcel Apfelbaum wrote:
> On Mon, 2014-03-10 at 16:02 +0100, Stefan Hajnoczi wrote:
> > On Mon, Mar 10, 2014 at 02:12:12PM +0200, Marcel Apfelbaum wrote:
> > > 'socket_accept' waits for Qemu to init its unix socket.
> > > If Qemu encounters an error during command line parsing,
> > > it can exit before initializing the communication channel.
> > > It gets worse as the make check-qtest-* gets stuck without
> > > notifying which test exactly has problems, so debugging can
> > > be a challenge.
> > > 
> > > The solution has two parts:
> > >  - Use a timeout for the socket.
> > >  - Expose a qtest_state_valid that checks that the connections
> > >    with Qemu are OK.
> > 
> > See below why I think qtest_state_valid() is unnecessary as a libqtest.h
> > API.
> > 
> > > Asserting qtest_state_valid in each test after qtest_init
> > > is a must, as we need to trace which test failed.
> > 
> > Inability to tell which qtest failed is a Makefile problem.  The
> > solution is not to move all asserts to the outer-most level just so the
> > error message includes the test name.
> > 
> > Either we need to invoke gtester separately for each test - that way the
> > Makefile can print "TEST <name>" for each binary.  Or maybe gtester has
> > options for formatting output better.
> Hi Stefan,
> Thanks for the review.
> 
> I am more concerned of PATCH 1/2, because it is a blocker for another series I am working on.
> I can resend only the first one which adds socket timeout and leaves the original assert.
> Would you be OK with this?

Yes, that sounds fine.  I posted comments on Patch 1 about ensuring the sockets
are unlinked before we fail (to prevent leaking the UNIX domain socket nodes in
the file system) - it does involve moving the assert to qtest_init().

> Now regarding the issue you brought up (less important):
> - Tweaking the Makefile to run each qtest separately and not all tests per arch
>   is a viable solution, however I am not familiar with the makefile magic and
>   it will take me a lot of time to get into it.

The following produces per-test output.  This means you'll know which test failed:

diff --git a/tests/Makefile b/tests/Makefile
index b17d41e..a8405c8 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -273,7 +273,7 @@ check-help:
        @echo "changed with variable GTESTER_OPTIONS."
 
 SPEED = quick
-GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
+GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
 GCOV_OPTIONS = -n $(if $(V),-f,)
 
 # gtester tests, possibly with verbose output

Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

On Mon, 2014-03-10 at 20:13 +0100, Stefan Hajnoczi wrote:
> On Mon, Mar 10, 2014 at 05:21:17PM +0200, Marcel Apfelbaum wrote:
> > On Mon, 2014-03-10 at 16:02 +0100, Stefan Hajnoczi wrote:
> > > On Mon, Mar 10, 2014 at 02:12:12PM +0200, Marcel Apfelbaum wrote:
> > > > 'socket_accept' waits for Qemu to init its unix socket.
> > > > If Qemu encounters an error during command line parsing,
> > > > it can exit before initializing the communication channel.
> > > > It gets worse as the make check-qtest-* gets stuck without
> > > > notifying which test exactly has problems, so debugging can
> > > > be a challenge.
> > > > 
> > > > The solution has two parts:
> > > >  - Use a timeout for the socket.
> > > >  - Expose a qtest_state_valid that checks that the connections
> > > >    with Qemu are OK.
> > > 
> > > See below why I think qtest_state_valid() is unnecessary as a libqtest.h
> > > API.
> > > 
> > > > Asserting qtest_state_valid in each test after qtest_init
> > > > is a must, as we need to trace which test failed.
> > > 
> > > Inability to tell which qtest failed is a Makefile problem.  The
> > > solution is not to move all asserts to the outer-most level just so the
> > > error message includes the test name.
> > > 
> > > Either we need to invoke gtester separately for each test - that way the
> > > Makefile can print "TEST <name>" for each binary.  Or maybe gtester has
> > > options for formatting output better.
> > Hi Stefan,
> > Thanks for the review.
> > 
> > I am more concerned of PATCH 1/2, because it is a blocker for another series I am working on.
> > I can resend only the first one which adds socket timeout and leaves the original assert.
> > Would you be OK with this?
> 
> Yes, that sounds fine.  I posted comments on Patch 1 about ensuring the sockets
> are unlinked before we fail (to prevent leaking the UNIX domain socket nodes in
> the file system) - it does involve moving the assert to qtest_init().
Hi Stefan,
Thanks a lot for your help! I already sent V2.

> 
> > Now regarding the issue you brought up (less important):
> > - Tweaking the Makefile to run each qtest separately and not all tests per arch
> >   is a viable solution, however I am not familiar with the makefile magic and
> >   it will take me a lot of time to get into it.
> 
> The following produces per-test output.  This means you'll know which test failed:
Yes, it works like a charm! I added it :)

Thanks,
Marcel
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index b17d41e..a8405c8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -273,7 +273,7 @@ check-help:
>         @echo "changed with variable GTESTER_OPTIONS."
>  
>  SPEED = quick
> -GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
> +GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
>  GCOV_OPTIONS = -n $(if $(V),-f,)
>  
>  # gtester tests, possibly with verbose output

Re: [Qemu-devel] [PATCH 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Markus Armbruster]

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> 'socket_accept' waits for Qemu to init its unix socket.
> If Qemu encounters an error during command line parsing,
> it can exit before initializing the communication channel.
> It gets worse as the make check-qtest-* gets stuck without
> notifying which test exactly has problems, so debugging can
> be a challenge.
>
> The solution has two parts:
>  - Use a timeout for the socket.
>  - Expose a qtest_state_valid that checks that the connections
>    with Qemu are OK.
> Asserting qtest_state_valid in each test after qtest_init
> is a must, as we need to trace which test failed.

Is that assert in the next patch?

>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  tests/libqtest.c | 26 +++++++++++++++++++++-----
>  tests/libqtest.h |  8 ++++++++
>  2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index f587d36..93dfa81 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -34,6 +34,7 @@
>  #include "qapi/qmp/json-parser.h"
>  
>  #define MAX_IRQ 256
> +#define SOCKET_TIMEOUT 5
>  
>  QTestState *global_qtest;
>  
> @@ -83,7 +84,6 @@ static int socket_accept(int sock)
>      do {
>          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
>      } while (ret == -1 && errno == EINTR);
> -    g_assert_no_errno(ret);
>      close(sock);
>  
>      return ret;
> @@ -111,6 +111,8 @@ QTestState *qtest_init(const char *extra_args)
>      gchar *command;
>      const char *qemu_binary;
>      struct sigaction sigact;
> +    struct timeval socket_timeout = { .tv_sec = SOCKET_TIMEOUT,
> +                                      .tv_usec = 0 };
>  
>      qemu_binary = getenv("QTEST_QEMU_BINARY");
>      g_assert(qemu_binary != NULL);
> @@ -123,6 +125,11 @@ QTestState *qtest_init(const char *extra_args)
>      sock = init_socket(socket_path);
>      qmpsock = init_socket(qmp_socket_path);
>  
> +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> +               sizeof(socket_timeout));
> +    setsockopt(qmpsock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> +               sizeof(socket_timeout));
> +
>      /* Catch SIGABRT to clean up on g_assert() failure */
>      sigact = (struct sigaction){
>          .sa_handler = sigabrt_handler,
> @@ -147,7 +154,9 @@ QTestState *qtest_init(const char *extra_args)
>      }
>  
>      s->fd = socket_accept(sock);
> -    s->qmp_fd = socket_accept(qmpsock);
> +    if (s->fd >= 0) {
> +        s->qmp_fd = socket_accept(qmpsock);
> +    }
>      unlink(socket_path);
>      unlink(qmp_socket_path);
>      g_free(socket_path);

The conditional looks odd.  But without it, we could wait for timeout
two times.

If s->fd < 0, then s->qmp_fd remains 0, and should not be used.  Are you
sure that's the case?  qtest_quit() and qtest_qmpv() use it.  Reachable?

Perhaps s->qmp_fd = -1 would be safer.

Could you explain to me again why we want to continue after
socket_accept() fails, regardless of whether it fails due to timeout or
something else?

> @@ -158,9 +167,11 @@ QTestState *qtest_init(const char *extra_args)
>          s->irq_level[i] = false;
>      }
>  
> -    /* Read the QMP greeting and then do the handshake */
> -    qtest_qmp_discard_response(s, "");
> -    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> +    if (qtest_state_valid(s)) {
> +        /* Read the QMP greeting and then do the handshake */
> +        qtest_qmp_discard_response(s, "");
> +        qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> +    }
>  
>      if (getenv("QTEST_STOP")) {
>          kill(s->qemu_pid, SIGSTOP);
> @@ -169,6 +180,11 @@ QTestState *qtest_init(const char *extra_args)
>      return s;
>  }
>  
> +bool qtest_state_valid(QTestState *s)
> +{
> +    return (s->fd >= 0) && (s->qmp_fd >= 0);
> +}
> +
>  void qtest_quit(QTestState *s)
>  {
>      sigaction(SIGABRT, &s->sigact_old, NULL);
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 9deebdc..39a37b1 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -45,6 +45,14 @@ QTestState *qtest_init(const char *extra_args);
>  void qtest_quit(QTestState *s);
>  
>  /**
> + * qtest_state_valid:
> + * @state: #QTestState instance to check
> + *
> + * Returns: True if qtest was initialized successfully

If you mean the macro defined by stdbool.h, that one's spelled with a
lower case 't'.

> + */
> +bool qtest_state_valid(QTestState *s);
> +
> +/**
>   * qtest_qmp_discard_response:
>   * @s: #QTestState instance to operate on.
>   * @fmt...: QMP message to send to qemu

Re: [Qemu-devel] [PATCH 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

On Wed, 2014-03-12 at 10:42 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > 'socket_accept' waits for Qemu to init its unix socket.
> > If Qemu encounters an error during command line parsing,
> > it can exit before initializing the communication channel.
> > It gets worse as the make check-qtest-* gets stuck without
> > notifying which test exactly has problems, so debugging can
> > be a challenge.
> >
> > The solution has two parts:
> >  - Use a timeout for the socket.
> >  - Expose a qtest_state_valid that checks that the connections
> >    with Qemu are OK.
> > Asserting qtest_state_valid in each test after qtest_init
> > is a must, as we need to trace which test failed.
> 
> Is that assert in the next patch?
Yes, for every qtest test, but Stefan NACKED it :(.
The reason would be that he didn't want to have a manual assertion
on each test just to see the test name. The alternative is to
output each test using gtest options.

I have my doubts because even that we see the test name,
we still receive the assert in qtestlib, but it is not
the qtestlib's fault, it is the test's supplied command line fault,
this why I think the test should assert.

But this is too philosophical for me :), the other pacth
that prevents the gtest getting stuck was accepted, I am now
looking for a maintainer to put it into his tree.

Thanks,
Marcel
 
> 
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/libqtest.c | 26 +++++++++++++++++++++-----
> >  tests/libqtest.h |  8 ++++++++
> >  2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/libqtest.c b/tests/libqtest.c
> > index f587d36..93dfa81 100644
> > --- a/tests/libqtest.c
> > +++ b/tests/libqtest.c
> > @@ -34,6 +34,7 @@
> >  #include "qapi/qmp/json-parser.h"
> >  
> >  #define MAX_IRQ 256
> > +#define SOCKET_TIMEOUT 5
> >  
> >  QTestState *global_qtest;
> >  
> > @@ -83,7 +84,6 @@ static int socket_accept(int sock)
> >      do {
> >          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> >      } while (ret == -1 && errno == EINTR);
> > -    g_assert_no_errno(ret);
> >      close(sock);
> >  
> >      return ret;
> > @@ -111,6 +111,8 @@ QTestState *qtest_init(const char *extra_args)
> >      gchar *command;
> >      const char *qemu_binary;
> >      struct sigaction sigact;
> > +    struct timeval socket_timeout = { .tv_sec = SOCKET_TIMEOUT,
> > +                                      .tv_usec = 0 };
> >  
> >      qemu_binary = getenv("QTEST_QEMU_BINARY");
> >      g_assert(qemu_binary != NULL);
> > @@ -123,6 +125,11 @@ QTestState *qtest_init(const char *extra_args)
> >      sock = init_socket(socket_path);
> >      qmpsock = init_socket(qmp_socket_path);
> >  
> > +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> > +               sizeof(socket_timeout));
> > +    setsockopt(qmpsock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> > +               sizeof(socket_timeout));
> > +
> >      /* Catch SIGABRT to clean up on g_assert() failure */
> >      sigact = (struct sigaction){
> >          .sa_handler = sigabrt_handler,
> > @@ -147,7 +154,9 @@ QTestState *qtest_init(const char *extra_args)
> >      }
> >  
> >      s->fd = socket_accept(sock);
> > -    s->qmp_fd = socket_accept(qmpsock);
> > +    if (s->fd >= 0) {
> > +        s->qmp_fd = socket_accept(qmpsock);
> > +    }
> >      unlink(socket_path);
> >      unlink(qmp_socket_path);
> >      g_free(socket_path);
> 
> The conditional looks odd.  But without it, we could wait for timeout
> two times.
> 
> If s->fd < 0, then s->qmp_fd remains 0, and should not be used.  Are you
> sure that's the case?  qtest_quit() and qtest_qmpv() use it.  Reachable?
> 
> Perhaps s->qmp_fd = -1 would be safer.
> 
> Could you explain to me again why we want to continue after
> socket_accept() fails, regardless of whether it fails due to timeout or
> something else?
> 
> > @@ -158,9 +167,11 @@ QTestState *qtest_init(const char *extra_args)
> >          s->irq_level[i] = false;
> >      }
> >  
> > -    /* Read the QMP greeting and then do the handshake */
> > -    qtest_qmp_discard_response(s, "");
> > -    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> > +    if (qtest_state_valid(s)) {
> > +        /* Read the QMP greeting and then do the handshake */
> > +        qtest_qmp_discard_response(s, "");
> > +        qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> > +    }
> >  
> >      if (getenv("QTEST_STOP")) {
> >          kill(s->qemu_pid, SIGSTOP);
> > @@ -169,6 +180,11 @@ QTestState *qtest_init(const char *extra_args)
> >      return s;
> >  }
> >  
> > +bool qtest_state_valid(QTestState *s)
> > +{
> > +    return (s->fd >= 0) && (s->qmp_fd >= 0);
> > +}
> > +
> >  void qtest_quit(QTestState *s)
> >  {
> >      sigaction(SIGABRT, &s->sigact_old, NULL);
> > diff --git a/tests/libqtest.h b/tests/libqtest.h
> > index 9deebdc..39a37b1 100644
> > --- a/tests/libqtest.h
> > +++ b/tests/libqtest.h
> > @@ -45,6 +45,14 @@ QTestState *qtest_init(const char *extra_args);
> >  void qtest_quit(QTestState *s);
> >  
> >  /**
> > + * qtest_state_valid:
> > + * @state: #QTestState instance to check
> > + *
> > + * Returns: True if qtest was initialized successfully
> 
> If you mean the macro defined by stdbool.h, that one's spelled with a
> lower case 't'.
> 
> > + */
> > +bool qtest_state_valid(QTestState *s);
> > +
> > +/**
> >   * qtest_qmp_discard_response:
> >   * @s: #QTestState instance to operate on.
> >   * @fmt...: QMP message to send to qemu