[Qemu-devel] [PATCH 0/3] qtest: add migration testing

[Qemu-devel] [PATCH 0/3] qtest: add migration testing

[Jason Baron]

Hi,

Add a basic qtest for migration testing. Currently, it just tests a migrate of
machine 'pc' on the same host. Would be nice to extend to multiple machine
versions, but that requires multiple binaries, which could be done, but is
perhaps a bit awkward from qtest? Testing different machine versions within
the same binary doesn't seem like a real world test case to me. Currently,
the test aborts, if the migrate takes more than 2 minutes.

In any case, the test currently fails for q35, since ahci migration suport
isn't in place. Thus, I intend to add q35 testing here, once those ahci
migration patches are accepted.

Thanks,

-Jason


Jason Baron (3):
  qtest: Enable creation of multiple qemu instances
  qtest: extend qtest_qmp() to fill in the reply
  qtest: add migrate-test

 tests/Makefile       |    8 ++-
 tests/libqtest.c     |   48 ++++++++++--------
 tests/libqtest.h     |    4 +-
 tests/migrate-test.c |  140 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+), 25 deletions(-)
 create mode 100644 tests/migrate-test.c

[Qemu-devel] [PATCH 1/3] qtest: Enable creation of multiple qemu instances

[Jason Baron]

From: Jason Baron <jbaron@redhat.com>

Currently, the qtest harness can only spawn 1 qemu instance at a time because
the parent pid is used to create the socket files. Use the child pid instead,
so we can remove that limitation.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 tests/libqtest.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 71b84c1..f3dd4e4 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -101,6 +101,10 @@ static pid_t qtest_qemu_pid(QTestState *s)
     return pid;
 }
 
+#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
+#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
+#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
+
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
@@ -113,25 +117,16 @@ QTestState *qtest_init(const char *extra_args)
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
 
-    s = g_malloc(sizeof(*s));
-
-    s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
-    s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
-    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
-
-    sock = init_socket(s->socket_path);
-    qmpsock = init_socket(s->qmp_socket_path);
-
     pid = fork();
     if (pid == 0) {
         command = g_strdup_printf("%s "
-                                  "-qtest unix:%s,nowait "
+                                  "-qtest unix:" QTEST_FILE_TEMP ",nowait "
                                   "-qtest-log /dev/null "
-                                  "-qmp unix:%s,nowait "
-                                  "-pidfile %s "
+                                  "-qmp unix:" QTEST_QMP_FILE_TEMP ",nowait "
+                                  "-pidfile " QTEST_PID_FILE_TEMP " "
                                   "-machine accel=qtest "
-                                  "%s", qemu_binary, s->socket_path,
-                                  s->qmp_socket_path, pid_file,
+                                  "%s", qemu_binary, getpid(),
+                                  getpid(), getpid(),
                                   extra_args ?: "");
 
         ret = system(command);
@@ -139,6 +134,14 @@ QTestState *qtest_init(const char *extra_args)
         g_free(command);
     }
 
+    s = g_malloc(sizeof(*s));
+    s->socket_path = g_strdup_printf(QTEST_FILE_TEMP, pid);
+    s->qmp_socket_path = g_strdup_printf(QTEST_QMP_FILE_TEMP, pid);
+    pid_file = g_strdup_printf(QTEST_PID_FILE_TEMP, pid);
+
+    sock = init_socket(s->socket_path);
+    qmpsock = init_socket(s->qmp_socket_path);
+
     s->fd = socket_accept(sock);
     s->qmp_fd = socket_accept(qmpsock);
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/3] qtest: extend qtest_qmp() to fill in the reply

[Jason Baron]

From: Jason Baron <jbaron@redhat.com>

Introduce:

Add void qtest_qmp_resp(QTestState *s, QString *resp, const char *fmt, ...)

which allows a response string to be filled in.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 tests/Makefile   |    6 +++---
 tests/libqtest.c |   17 ++++++++++-------
 tests/libqtest.h |    4 +++-
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index b60f0fb..30a101d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -74,10 +74,10 @@ tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-
 tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
 tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
 
-tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
+tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y) qstring.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
-tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
-tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
+tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y) qstring.o
+tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y) qstring.o
 
 # QTest rules
 
diff --git a/tests/libqtest.c b/tests/libqtest.c
index f3dd4e4..71c9eb4 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -288,7 +288,7 @@ redo:
     return words;
 }
 
-void qtest_qmp(QTestState *s, const char *fmt, ...)
+void qtest_qmp_resp(QTestState *s, QString *resp, const char *fmt, ...)
 {
     va_list ap;
     bool has_reply = false;
@@ -313,16 +313,19 @@ void qtest_qmp(QTestState *s, const char *fmt, ...)
             fprintf(stderr, "Broken pipe\n");
             exit(1);
         }
-
-        switch (c) {
-        case '{':
+        if (c == '{') {
             nesting++;
             has_reply = true;
-            break;
-        case '}':
+        }
+        if (c == '}') {
             nesting--;
-            break;
         }
+        if (has_reply && resp) {
+            qstring_append_chr(resp, c);
+        }
+    }
+    if (has_reply && resp) {
+        qstring_append_chr(resp, '\0');
     }
 }
 
diff --git a/tests/libqtest.h b/tests/libqtest.h
index c8ade85..6441e50 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -18,6 +18,7 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include <sys/types.h>
+#include "qstring.h"
 
 typedef struct QTestState QTestState;
 
@@ -44,7 +45,8 @@ void qtest_quit(QTestState *s);
  *
  * Sends a QMP message to QEMU
  */
-void qtest_qmp(QTestState *s, const char *fmt, ...);
+void qtest_qmp_resp(QTestState *s, QString *resp, const char *fmt, ...);
+#define qtest_qmp(s, fmt, ...) qtest_qmp_resp(s, NULL, fmt, ## __VA_ARGS__)
 
 /**
  * qtest_get_irq:
-- 
1.7.1

[Qemu-devel] [PATCH 3/3] qtest: add migrate-test

[Jason Baron]

From: Jason Baron <jbaron@redhat.com>

Tests a single 'pc' machine migration on the same host. Currently, the test
fail for q35 since the ahci controller doesn't yet migrate. Will add support
for q35 once the ahci support is accepted.

Would be nice to extend the test matrix to various machine versions, but that
requires building multiple qemu binaries, which is a bit awkward in the
context of qtest. Testing migration between different machine versions with the
same binary doesn't seem too useful.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 tests/Makefile       |    2 +
 tests/migrate-test.c |  140 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+), 0 deletions(-)
 create mode 100644 tests/migrate-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 30a101d..d50dff0 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -25,6 +25,7 @@ check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 check-qtest-i386-y = tests/fdc-test$(EXESUF)
 check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
 check-qtest-i386-y += tests/rtc-test$(EXESUF)
+check-qtest-i386-y += tests/migrate-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
 check-qtest-sparc64-y = tests/m48t59-test$(EXESUF)
@@ -78,6 +79,7 @@ tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y) qstring.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
 tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y) qstring.o
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y) qstring.o
+tests/migrate-test$(EXESUF): tests/migrate-test.o $(test-qapi-obj-y) $(qom-obj-y)
 
 # QTest rules
 
diff --git a/tests/migrate-test.c b/tests/migrate-test.c
new file mode 100644
index 0000000..c62d5af
--- /dev/null
+++ b/tests/migrate-test.c
@@ -0,0 +1,140 @@
+/*
+ * Migration tests
+ *
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Authors:
+ *  Jason Baron   <jbaron@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "libqtest.h"
+
+#include <glib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "qjson.h"
+#include "error.h"
+#include "qemu/object.h"
+#include "qdict.h"
+#include "qbool.h"
+
+#define migrate_assert(cond) \
+    if (!(cond)) {           \
+        migrate_cleanup();   \
+        fprintf(stderr, "%s:%d %s\n", __FILE__, __LINE__, #cond); \
+        abort();             \
+    }                        \
+
+static QTestState *mach_a;
+static QTestState *mach_b;
+
+static void migrate_cleanup(void)
+{
+    if (mach_a) {
+        qtest_quit(mach_a);
+    }
+    if (mach_b) {
+        qtest_quit(mach_b);
+    }
+}
+
+static int expected_qobject(QObject *obj, qtype_code type)
+{
+    if (!obj) {
+        return 0;
+    }
+    return (qobject_type(obj) == type);
+}
+
+/*
+ * Return vals:
+ * 1: yes
+ * 0: no
+ * -1: retry
+ */
+static int is_running(QTestState *mch)
+{
+    QString *resp = qstring_new();
+    QObject *resp_obj;
+    QObject *ret_obj;
+    QObject *run_obj;
+    int ret;
+
+    resp = qstring_new();
+    qtest_qmp_resp(mch, resp, "{ 'execute': 'query-status' }", NULL);
+
+    resp_obj = qobject_from_json(qstring_get_str(resp));
+    if (!expected_qobject(resp_obj, QTYPE_QDICT)) {
+        ret = -1;
+        goto out;
+    }
+
+    ret_obj = qdict_get(qobject_to_qdict(resp_obj), "return");
+    if (!expected_qobject(ret_obj, QTYPE_QDICT)) {
+        ret = -1;
+        goto out;
+    }
+
+    run_obj = qdict_get(qobject_to_qdict(ret_obj), "running");
+    if (!expected_qobject(run_obj, QTYPE_QBOOL)) {
+        ret = -1;
+        goto out;
+    }
+    ret = qbool_get_int(qobject_to_qbool(run_obj));
+
+out:
+    qobject_decref(resp_obj);
+    QDECREF(resp);
+    return ret;
+}
+
+#define SLEEP_INTERVAL 2
+/* Abort after 2 minutes */
+#define SLEEP_MAX (60 * 2)
+
+static void migrate_a_to_b(void)
+{
+    int a_run = 0;
+    int b_run = 0;
+    int iter = 0;
+
+    /* is running on A ? */
+    migrate_assert(is_running(mach_a));
+
+    /* do migrate */
+    qtest_qmp(mach_a, "{ 'execute': 'migrate',"
+              "'arguments': { 'uri': 'tcp:0:4444' } }", NULL);
+
+    while (iter < SLEEP_MAX) {
+        a_run = is_running(mach_a);
+        b_run = is_running(mach_b);
+        if ((a_run == 0) && (b_run == 1)) {
+            break;
+        }
+        sleep(SLEEP_INTERVAL);
+        iter += SLEEP_INTERVAL;
+    }
+    migrate_assert((a_run == 0) && (b_run == 1));
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    mach_a = qtest_start("-display none -machine pc");
+    mach_b = qtest_start("-display none -machine pc -incoming tcp:0:4444");
+
+    qtest_add_func("/migrate/a-to-b", migrate_a_to_b);
+    ret = g_test_run();
+
+    migrate_cleanup();
+    return ret;
+}
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] qtest: extend qtest_qmp() to fill in the reply

[Andreas Färber]

Am 13.12.2012 23:02, schrieb Jason Baron:
> diff --git a/tests/Makefile b/tests/Makefile
> index b60f0fb..30a101d 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -74,10 +74,10 @@ tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-
>  tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
>  tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
>  
> -tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
> +tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y) qstring.o
>  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
> -tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
> -tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
> +tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y) qstring.o
> +tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y) qstring.o

Adding a dependency to every qtest is calling for a $(qtest-obj-y) to
administer that list in a central location. I am expecting the number of
tests to grow significantly over time.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/3] qtest: add migrate-test

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Jason Baron" <jbaron@redhat.com>
> A: qemu-devel@nongnu.org
> Cc: aliguori@us.ibm.com, kwolf@redhat.com, pbonzini@redhat.com, quintela@redhat.com
> Inviato: Giovedì, 13 dicembre 2012 23:02:22
> Oggetto: [PATCH 3/3] qtest: add migrate-test
> 
> From: Jason Baron <jbaron@redhat.com>
> 
> Tests a single 'pc' machine migration on the same host. Currently,
> the test
> fail for q35 since the ahci controller doesn't yet migrate. Will add
> support
> for q35 once the ahci support is accepted.
> 
> Would be nice to extend the test matrix to various machine versions,
> but that
> requires building multiple qemu binaries, which is a bit awkward in
> the
> context of qtest. Testing migration between different machine
> versions with the
> same binary doesn't seem too useful.
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  tests/Makefile       |    2 +
>  tests/migrate-test.c |  140
>  ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 142 insertions(+), 0 deletions(-)
>  create mode 100644 tests/migrate-test.c
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 30a101d..d50dff0 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -25,6 +25,7 @@ check-block-$(CONFIG_POSIX) +=
> tests/qemu-iotests-quick.sh
>  check-qtest-i386-y = tests/fdc-test$(EXESUF)
>  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
> +check-qtest-i386-y += tests/migrate-test$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>  check-qtest-sparc64-y = tests/m48t59-test$(EXESUF)
> @@ -78,6 +79,7 @@ tests/rtc-test$(EXESUF): tests/rtc-test.o
> $(trace-obj-y) qstring.o
>  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
>  tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o
>  $(trace-obj-y) qstring.o
>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o
>  $(trace-obj-y) qstring.o
> +tests/migrate-test$(EXESUF): tests/migrate-test.o $(test-qapi-obj-y)
> $(qom-obj-y)
>  
>  # QTest rules
>  
> diff --git a/tests/migrate-test.c b/tests/migrate-test.c
> new file mode 100644
> index 0000000..c62d5af
> --- /dev/null
> +++ b/tests/migrate-test.c
> @@ -0,0 +1,140 @@
> +/*
> + * Migration tests
> + *
> + * Copyright Red Hat, Inc. 2012
> + *
> + * Authors:
> + *  Jason Baron   <jbaron@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +#include "libqtest.h"
> +
> +#include <glib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include "qjson.h"
> +#include "error.h"
> +#include "qemu/object.h"
> +#include "qdict.h"
> +#include "qbool.h"
> +
> +#define migrate_assert(cond) \
> +    if (!(cond)) {           \
> +        migrate_cleanup();   \
> +        fprintf(stderr, "%s:%d %s\n", __FILE__, __LINE__, #cond); \
> +        abort();             \
> +    }                        \
> +
> +static QTestState *mach_a;
> +static QTestState *mach_b;
> +
> +static void migrate_cleanup(void)
> +{
> +    if (mach_a) {
> +        qtest_quit(mach_a);
> +    }
> +    if (mach_b) {
> +        qtest_quit(mach_b);
> +    }
> +}
> +
> +static int expected_qobject(QObject *obj, qtype_code type)
> +{
> +    if (!obj) {
> +        return 0;
> +    }
> +    return (qobject_type(obj) == type);
> +}
> +
> +/*
> + * Return vals:
> + * 1: yes
> + * 0: no
> + * -1: retry
> + */
> +static int is_running(QTestState *mch)
> +{
> +    QString *resp = qstring_new();
> +    QObject *resp_obj;
> +    QObject *ret_obj;
> +    QObject *run_obj;
> +    int ret;
> +
> +    resp = qstring_new();
> +    qtest_qmp_resp(mch, resp, "{ 'execute': 'query-status' }",
> NULL);
> +
> +    resp_obj = qobject_from_json(qstring_get_str(resp));
> +    if (!expected_qobject(resp_obj, QTYPE_QDICT)) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    ret_obj = qdict_get(qobject_to_qdict(resp_obj), "return");
> +    if (!expected_qobject(ret_obj, QTYPE_QDICT)) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    run_obj = qdict_get(qobject_to_qdict(ret_obj), "running");
> +    if (!expected_qobject(run_obj, QTYPE_QBOOL)) {
> +        ret = -1;
> +        goto out;
> +    }
> +    ret = qbool_get_int(qobject_to_qbool(run_obj));
> +
> +out:
> +    qobject_decref(resp_obj);
> +    QDECREF(resp);
> +    return ret;
> +}
> +
> +#define SLEEP_INTERVAL 2
> +/* Abort after 2 minutes */
> +#define SLEEP_MAX (60 * 2)
> +
> +static void migrate_a_to_b(void)

Do you think this function could be turned into a libqtest call?
It would take mach_a as an argument, add -incoming tcp:localhost:4444
to the command line of mach_a, use that to spawn mach_b, and
return mach_b as the return value (or perhaps change mach_a to
refer to the new machine).

The reason is that I can anticipate having many migration qtests,
at least one for every subsection we ever had to add.

Paolo

> +{
> +    int a_run = 0;
> +    int b_run = 0;
> +    int iter = 0;
> +
> +    /* is running on A ? */
> +    migrate_assert(is_running(mach_a));
> +
> +    /* do migrate */
> +    qtest_qmp(mach_a, "{ 'execute': 'migrate',"
> +              "'arguments': { 'uri': 'tcp:0:4444' } }", NULL);
> +
> +    while (iter < SLEEP_MAX) {
> +        a_run = is_running(mach_a);
> +        b_run = is_running(mach_b);
> +        if ((a_run == 0) && (b_run == 1)) {
> +            break;
> +        }
> +        sleep(SLEEP_INTERVAL);
> +        iter += SLEEP_INTERVAL;
> +    }
> +    migrate_assert((a_run == 0) && (b_run == 1));
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    mach_a = qtest_start("-display none -machine pc");
> +    mach_b = qtest_start("-display none -machine pc -incoming
> tcp:0:4444");
> +
> +    qtest_add_func("/migrate/a-to-b", migrate_a_to_b);
> +    ret = g_test_run();
> +
> +    migrate_cleanup();
> +    return ret;
> +}
> --
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH 2/3] qtest: extend qtest_qmp() to fill in the reply

[Jason Baron]

On Fri, Dec 14, 2012 at 01:07:24AM +0100, Andreas Färber wrote:
> Am 13.12.2012 23:02, schrieb Jason Baron:
> > diff --git a/tests/Makefile b/tests/Makefile
> > index b60f0fb..30a101d 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -74,10 +74,10 @@ tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-
> >  tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
> >  tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
> >  
> > -tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
> > +tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y) qstring.o
> >  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
> > -tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
> > -tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
> > +tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y) qstring.o
> > +tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y) qstring.o
> 
> Adding a dependency to every qtest is calling for a $(qtest-obj-y) to
> administer that list in a central location. I am expecting the number of
> tests to grow significantly over time.
> 
> Andreas
> 

ok, seems as though we can add $(test-qapi-obj-y) $(qom-obj-y), etc. to the
qtest-obj-y target, to simplify things a bit. I'll re-post that as a
separate patch.

Thanks,

-Jason

Re: [Qemu-devel] [PATCH 3/3] qtest: add migrate-test

[Jason Baron]

On Fri, Dec 14, 2012 at 03:08:21AM -0500, Paolo Bonzini wrote:
> > Tests a single 'pc' machine migration on the same host. Currently,
> > the test
> > fail for q35 since the ahci controller doesn't yet migrate. Will add
> > support
> > for q35 once the ahci support is accepted.
> > 
> > Would be nice to extend the test matrix to various machine versions,
> > but that
> > requires building multiple qemu binaries, which is a bit awkward in
> > the
> > context of qtest. Testing migration between different machine
> > versions with the
> > same binary doesn't seem too useful.
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  tests/Makefile       |    2 +
> >  tests/migrate-test.c |  140
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 142 insertions(+), 0 deletions(-)
> >  create mode 100644 tests/migrate-test.c
> > 
> > diff --git a/tests/Makefile b/tests/Makefile
> > index 30a101d..d50dff0 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -25,6 +25,7 @@ check-block-$(CONFIG_POSIX) +=
> > tests/qemu-iotests-quick.sh
> >  check-qtest-i386-y = tests/fdc-test$(EXESUF)
> >  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
> >  check-qtest-i386-y += tests/rtc-test$(EXESUF)
> > +check-qtest-i386-y += tests/migrate-test$(EXESUF)
> >  check-qtest-x86_64-y = $(check-qtest-i386-y)
> >  check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
> >  check-qtest-sparc64-y = tests/m48t59-test$(EXESUF)
> > @@ -78,6 +79,7 @@ tests/rtc-test$(EXESUF): tests/rtc-test.o
> > $(trace-obj-y) qstring.o
> >  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
> >  tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o
> >  $(trace-obj-y) qstring.o
> >  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o
> >  $(trace-obj-y) qstring.o
> > +tests/migrate-test$(EXESUF): tests/migrate-test.o $(test-qapi-obj-y)
> > $(qom-obj-y)
> >  
> >  # QTest rules
> >  
> > diff --git a/tests/migrate-test.c b/tests/migrate-test.c
> > new file mode 100644
> > index 0000000..c62d5af
> > --- /dev/null
> > +++ b/tests/migrate-test.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * Migration tests
> > + *
> > + * Copyright Red Hat, Inc. 2012
> > + *
> > + * Authors:
> > + *  Jason Baron   <jbaron@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2
> > or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +#include "libqtest.h"
> > +
> > +#include <glib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +
> > +#include "qjson.h"
> > +#include "error.h"
> > +#include "qemu/object.h"
> > +#include "qdict.h"
> > +#include "qbool.h"
> > +
> > +#define migrate_assert(cond) \
> > +    if (!(cond)) {           \
> > +        migrate_cleanup();   \
> > +        fprintf(stderr, "%s:%d %s\n", __FILE__, __LINE__, #cond); \
> > +        abort();             \
> > +    }                        \
> > +
> > +static QTestState *mach_a;
> > +static QTestState *mach_b;
> > +
> > +static void migrate_cleanup(void)
> > +{
> > +    if (mach_a) {
> > +        qtest_quit(mach_a);
> > +    }
> > +    if (mach_b) {
> > +        qtest_quit(mach_b);
> > +    }
> > +}
> > +
> > +static int expected_qobject(QObject *obj, qtype_code type)
> > +{
> > +    if (!obj) {
> > +        return 0;
> > +    }
> > +    return (qobject_type(obj) == type);
> > +}
> > +
> > +/*
> > + * Return vals:
> > + * 1: yes
> > + * 0: no
> > + * -1: retry
> > + */
> > +static int is_running(QTestState *mch)
> > +{
> > +    QString *resp = qstring_new();
> > +    QObject *resp_obj;
> > +    QObject *ret_obj;
> > +    QObject *run_obj;
> > +    int ret;
> > +
> > +    resp = qstring_new();
> > +    qtest_qmp_resp(mch, resp, "{ 'execute': 'query-status' }",
> > NULL);
> > +
> > +    resp_obj = qobject_from_json(qstring_get_str(resp));
> > +    if (!expected_qobject(resp_obj, QTYPE_QDICT)) {
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    ret_obj = qdict_get(qobject_to_qdict(resp_obj), "return");
> > +    if (!expected_qobject(ret_obj, QTYPE_QDICT)) {
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    run_obj = qdict_get(qobject_to_qdict(ret_obj), "running");
> > +    if (!expected_qobject(run_obj, QTYPE_QBOOL)) {
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +    ret = qbool_get_int(qobject_to_qbool(run_obj));
> > +
> > +out:
> > +    qobject_decref(resp_obj);
> > +    QDECREF(resp);
> > +    return ret;
> > +}
> > +
> > +#define SLEEP_INTERVAL 2
> > +/* Abort after 2 minutes */
> > +#define SLEEP_MAX (60 * 2)
> > +
> > +static void migrate_a_to_b(void)
> 
> Do you think this function could be turned into a libqtest call?

Seems like a good idea.

> It would take mach_a as an argument, add -incoming tcp:localhost:4444
> to the command line of mach_a, use that to spawn mach_b, and

why add to mach_a? I thought -incoming is just for the destination.

> return mach_b as the return value (or perhaps change mach_a to
> refer to the new machine).

I think it makes sense for the caller to create and pass the machines
and then just call a library function to do the migrate. That way the
caller 'owns' the machines. But maybe I'm missing something.

> 
> The reason is that I can anticipate having many migration qtests,
> at least one for every subsection we ever had to add.
> 
> Paolo
> 




> > +{
> > +    int a_run = 0;
> > +    int b_run = 0;
> > +    int iter = 0;
> > +
> > +    /* is running on A ? */
> > +    migrate_assert(is_running(mach_a));
> > +
> > +    /* do migrate */
> > +    qtest_qmp(mach_a, "{ 'execute': 'migrate',"
> > +              "'arguments': { 'uri': 'tcp:0:4444' } }", NULL);
> > +
> > +    while (iter < SLEEP_MAX) {
> > +        a_run = is_running(mach_a);
> > +        b_run = is_running(mach_b);
> > +        if ((a_run == 0) && (b_run == 1)) {
> > +            break;
> > +        }
> > +        sleep(SLEEP_INTERVAL);
> > +        iter += SLEEP_INTERVAL;
> > +    }
> > +    migrate_assert((a_run == 0) && (b_run == 1));
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    int ret;
> > +
> > +    g_test_init(&argc, &argv, NULL);
> > +
> > +    mach_a = qtest_start("-display none -machine pc");
> > +    mach_b = qtest_start("-display none -machine pc -incoming
> > tcp:0:4444");
> > +
> > +    qtest_add_func("/migrate/a-to-b", migrate_a_to_b);
> > +    ret = g_test_run();
> > +
> > +    migrate_cleanup();
> > +    return ret;
> > +}
> > --
> > 1.7.1
> > 
> > 

Re: [Qemu-devel] [PATCH 3/3] qtest: add migrate-test

[Paolo Bonzini]

> > Do you think this function could be turned into a libqtest call?
> 
> Seems like a good idea.
> 
> > It would take mach_a as an argument, add -incoming
> > tcp:localhost:4444
> > to the command line of mach_a, use that to spawn mach_b, and
> 
> why add to mach_a? I thought -incoming is just for the destination.

Yep.  Tack it at the end of mach_a's command line (actually at the end
of the parameter of qtest_start) and use the result to start mach_b.  The
command-lines for the two machines must match (apart from -incoming of
course), it's not necessary to pass it twice.

> > return mach_b as the return value (or perhaps change mach_a to
> > refer to the new machine).
> 
> I think it makes sense for the caller to create and pass the machines
> and then just call a library function to do the migrate. That way the
> caller 'owns' the machines. But maybe I'm missing something.

Yeah, owning the machines makes sense.  In this case migration would
just be a constructor for QTestState.

The alternative is to kill mach_a during migration and only proceed
with mach_b, since mach_a is effectively not going to be used.

Paolo

Re: [Qemu-devel] [PATCH 1/3] qtest: Enable creation of multiple qemu instances

[Blue Swirl]

On Thu, Dec 13, 2012 at 10:02 PM, Jason Baron <jbaron@redhat.com> wrote:
> From: Jason Baron <jbaron@redhat.com>
>
> Currently, the qtest harness can only spawn 1 qemu instance at a time because
> the parent pid is used to create the socket files. Use the child pid instead,
> so we can remove that limitation.
>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  tests/libqtest.c |   31 +++++++++++++++++--------------
>  1 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 71b84c1..f3dd4e4 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -101,6 +101,10 @@ static pid_t qtest_qemu_pid(QTestState *s)
>      return pid;
>  }
>
> +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
> +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
> +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"

These filenames are too predictable from security point of view,
please change the code to use mkstemp() instead. That would also solve
the original file name conflict problem.

> +
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s;
> @@ -113,25 +117,16 @@ QTestState *qtest_init(const char *extra_args)
>      qemu_binary = getenv("QTEST_QEMU_BINARY");
>      g_assert(qemu_binary != NULL);
>
> -    s = g_malloc(sizeof(*s));
> -
> -    s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> -    s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> -    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
> -
> -    sock = init_socket(s->socket_path);
> -    qmpsock = init_socket(s->qmp_socket_path);
> -
>      pid = fork();
>      if (pid == 0) {
>          command = g_strdup_printf("%s "
> -                                  "-qtest unix:%s,nowait "
> +                                  "-qtest unix:" QTEST_FILE_TEMP ",nowait "
>                                    "-qtest-log /dev/null "
> -                                  "-qmp unix:%s,nowait "
> -                                  "-pidfile %s "
> +                                  "-qmp unix:" QTEST_QMP_FILE_TEMP ",nowait "
> +                                  "-pidfile " QTEST_PID_FILE_TEMP " "
>                                    "-machine accel=qtest "
> -                                  "%s", qemu_binary, s->socket_path,
> -                                  s->qmp_socket_path, pid_file,
> +                                  "%s", qemu_binary, getpid(),
> +                                  getpid(), getpid(),
>                                    extra_args ?: "");
>
>          ret = system(command);
> @@ -139,6 +134,14 @@ QTestState *qtest_init(const char *extra_args)
>          g_free(command);
>      }
>
> +    s = g_malloc(sizeof(*s));
> +    s->socket_path = g_strdup_printf(QTEST_FILE_TEMP, pid);
> +    s->qmp_socket_path = g_strdup_printf(QTEST_QMP_FILE_TEMP, pid);
> +    pid_file = g_strdup_printf(QTEST_PID_FILE_TEMP, pid);
> +
> +    sock = init_socket(s->socket_path);
> +    qmpsock = init_socket(s->qmp_socket_path);
> +
>      s->fd = socket_accept(sock);
>      s->qmp_fd = socket_accept(qmpsock);
>
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH 1/3] qtest: Enable creation of multiple qemu instances

[Paolo Bonzini]

> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
> 
> These filenames are too predictable from security point of view,

This need not be secure as long as the file is created with 0600
permissions.  In fact, inspecting the pid file from the shell can
be useful.

However, using mkstemp() on a prefix that includes the parent pid
can indeed be the best of both worlds.

Paolo

Re: [Qemu-devel] [PATCH 1/3] qtest: Enable creation of multiple qemu instances

[Blue Swirl]

On Sat, Dec 15, 2012 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
>> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
>> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
>>
>> These filenames are too predictable from security point of view,
>
> This need not be secure as long as the file is created with 0600
> permissions.  In fact, inspecting the pid file from the shell can
> be useful.

Permissions do not help at all because the attacker could for example
target overwriting of a critical file.

>
> However, using mkstemp() on a prefix that includes the parent pid
> can indeed be the best of both worlds.

Yes.

>
> Paolo

Re: [Qemu-devel] [PATCH 1/3] qtest: Enable creation of multiple qemu instances

[Jason Baron]

On Sat, Dec 15, 2012 at 09:20:13AM +0000, Blue Swirl wrote:
> On Sat, Dec 15, 2012 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
> >> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
> >> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
> >>
> >> These filenames are too predictable from security point of view,
> >
> > This need not be secure as long as the file is created with 0600
> > permissions.  In fact, inspecting the pid file from the shell can
> > be useful.
> 
> Permissions do not help at all because the attacker could for example
> target overwriting of a critical file.
> 
> >
> > However, using mkstemp() on a prefix that includes the parent pid
> > can indeed be the best of both worlds.
> 
> Yes.
> 
> >
> > Paolo
> 

Yes, but mkstemp() creates the file, and bind() returns EADDRINUSE, if the file
already exists.

Using mktemp() in this case, with bind() should be ok, since bind() checks if
the file exists and then creates it, if not, all within the bind() system call
(so its atomic).

Thanks,

-Jason

Re: [Qemu-devel] [PATCH 1/3] qtest: Enable creation of multiple qemu instances

[Blue Swirl]

On Mon, Dec 17, 2012 at 5:13 PM, Jason Baron <jbaron@redhat.com> wrote:
> On Sat, Dec 15, 2012 at 09:20:13AM +0000, Blue Swirl wrote:
>> On Sat, Dec 15, 2012 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
>> >> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
>> >> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
>> >>
>> >> These filenames are too predictable from security point of view,
>> >
>> > This need not be secure as long as the file is created with 0600
>> > permissions.  In fact, inspecting the pid file from the shell can
>> > be useful.
>>
>> Permissions do not help at all because the attacker could for example
>> target overwriting of a critical file.
>>
>> >
>> > However, using mkstemp() on a prefix that includes the parent pid
>> > can indeed be the best of both worlds.
>>
>> Yes.
>>
>> >
>> > Paolo
>>
>
> Yes, but mkstemp() creates the file, and bind() returns EADDRINUSE, if the file
> already exists.
>
> Using mktemp() in this case, with bind() should be ok, since bind() checks if
> the file exists and then creates it, if not, all within the bind() system call
> (so its atomic).

mktemp() manual page warns against using it, tempnam() looks like a
better choice.

>
> Thanks,
>
> -Jason