[PATCH v2 0/9] tests/migration-test: Allow testing older machine types

[PATCH v2 0/9] tests/migration-test: Allow testing older machine types

[Fabiano Rosas]

This adds support for running migration-test with two different QEMU
versions to test migration compatibility. The tests automatically
choose the latest machine type supported by both QEMU versions.

changes:

- introduce *_with_env variants of the relevant functions [Daniel, Juan]

- keep the requirement for having the QTEST_QEMU_BINARY always
  present. qtest_get_arch() is used extensively in the qtest_add*
  functions. It would be too much churn to pass a different binary
  into it.

- with this^ we also need to keep the requirement for using only one
  of SRC|DST. Otherwise it would be confusing to have three binaries
  listed.

- query the alias to find out the machine types [Daniel]

I haven't looked into the docker part for now. I think Daniel's
suggestion of QTEST_QEMU_BINARY_SRC='podman run ... qemu-system-foo'
looks interesting. Do we have the latest release already built in the
registry at any given point?

Thanks

v1:
https://lore.kernel.org/r/20231003141932.2367-1-farosas@suse.de

Hi, I had this WIP patch laying around that seems to fit Juan's vision
about testing older machine types. It is a very rough draft for now,
but it may be useful for kickstarting the discussion.

With this we can give the tests two different QEMU versions. The test
picks the older machine type between the two and runs the whole
migration-test suite.

We'd just need a way to provide the older build. Currently I'm doing
this by hand.

sample output:
 # Using two different QEMU binaries. Common machine type: pc-i440fx-8.1
 ...
 # Using ./qemu-system-x86_64 (v8.1.0-952-g8a940312a2-dirty) as migration source
 ...
 # Using ../build-8.1.0/qemu-system-x86_64 (v8.1.0-dirty) as migration destination

Let me know what you think.

Fabiano Rosas (9):
  tests/qtest: Allow qtest_qemu_binary to use a custom environment
    variable
  tests/qtest: Introduce qtest_init_with_env
  tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary
  tests/qtest: Introduce qtest_has_machine_with_env
  tests/qtest: Introduce qtest_resolve_machine_alias
  tests/qtest/migration: Introduce find_common_machine_version
  tests/qtest/migration: Define a machine for all architectures
  tests/qtest/migration: Support more than one QEMU binary
  tests/qtest: Don't print messages from query instances

 tests/qtest/libqtest.c          | 86 +++++++++++++++++++++++++++------
 tests/qtest/libqtest.h          | 32 ++++++++++++
 tests/qtest/migration-helpers.c | 24 +++++++++
 tests/qtest/migration-helpers.h |  2 +
 tests/qtest/migration-test.c    | 36 ++++++++++++--
 5 files changed, 162 insertions(+), 18 deletions(-)

-- 
2.35.3

[PATCH v2 1/9] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable

[Fabiano Rosas]

We're adding support for testing migration using two different QEMU
binaries. We'll provide the second binary in a new environment
variable.

Allow qtest_qemu_binary() to receive the name of the new variable. If
the new environment variable is not set, that's not an error, we use
QTEST_QEMU_BINARY as a fallback.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index b1eba71ffe..1f971b98e0 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -336,10 +336,17 @@ void qtest_remove_abrt_handler(void *data)
     }
 }
 
-static const char *qtest_qemu_binary(void)
+static const char *qtest_qemu_binary(const char *var)
 {
     const char *qemu_bin;
 
+    if (var) {
+        qemu_bin = getenv(var);
+        if (qemu_bin) {
+            return qemu_bin;
+        }
+    }
+
     qemu_bin = getenv("QTEST_QEMU_BINARY");
     if (!qemu_bin) {
         fprintf(stderr, "Environment variable QTEST_QEMU_BINARY required\n");
@@ -392,7 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
 
     va_start(ap, fmt);
     g_string_append_printf(command, CMD_EXEC "%s %s",
-                           qtest_qemu_binary(), tracearg);
+                           qtest_qemu_binary(NULL), tracearg);
     g_string_append_vprintf(command, fmt, ap);
     va_end(ap);
 
@@ -907,7 +914,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...)
 
 const char *qtest_get_arch(void)
 {
-    const char *qemu = qtest_qemu_binary();
+    const char *qemu = qtest_qemu_binary(NULL);
     const char *end = strrchr(qemu, '-');
 
     if (!end) {
-- 
2.35.3

[PATCH v2 2/9] tests/qtest: Introduce qtest_init_with_env

[Fabiano Rosas]

Add a version of qtest_init() that takes an environment variable
containing the path of the QEMU binary. This allows tests to use more
than one QEMU binary.

If no variable is provided or the environment variable does not exist,
that is not an error. Fallback to using QTEST_QEMU_BINARY.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 26 +++++++++++++++++++-------
 tests/qtest/libqtest.h | 13 +++++++++++++
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 1f971b98e0..88b79cb477 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -388,7 +388,8 @@ static pid_t qtest_create_process(char *cmd)
 }
 #endif /* _WIN32 */
 
-static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
+static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin,
+                                                        const char *fmt, ...)
 {
     va_list ap;
     QTestState *s = g_new0(QTestState, 1);
@@ -398,8 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
     g_autoptr(GString) command = g_string_new("");
 
     va_start(ap, fmt);
-    g_string_append_printf(command, CMD_EXEC "%s %s",
-                           qtest_qemu_binary(NULL), tracearg);
+    g_string_append_printf(command, CMD_EXEC "%s %s", qemu_bin, tracearg);
     g_string_append_vprintf(command, fmt, ap);
     va_end(ap);
 
@@ -441,7 +441,8 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
     return s;
 }
 
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+static QTestState *qtest_init_internal(const char *qemu_bin,
+                                       const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
@@ -466,7 +467,8 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
-    s = qtest_spawn_qemu("-qtest unix:%s "
+    s = qtest_spawn_qemu(qemu_bin,
+                         "-qtest unix:%s "
                          "-qtest-log %s "
                          "-chardev socket,path=%s,id=char0 "
                          "-mon chardev=char0,mode=control "
@@ -518,9 +520,14 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     return s;
 }
 
-QTestState *qtest_init(const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 {
-    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+    return qtest_init_internal(qtest_qemu_binary(NULL), extra_args);
+}
+
+QTestState *qtest_init_with_env(const char *var, const char *extra_args)
+{
+    QTestState *s = qtest_init_internal(qtest_qemu_binary(var), extra_args);
     QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
@@ -531,6 +538,11 @@ QTestState *qtest_init(const char *extra_args)
     return s;
 }
 
+QTestState *qtest_init(const char *extra_args)
+{
+    return qtest_init_with_env(NULL, extra_args);
+}
+
 QTestState *qtest_vinitf(const char *fmt, va_list ap)
 {
     char *args = g_strdup_vprintf(fmt, ap);
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index e53e350e3a..eef3a11ca4 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -55,6 +55,19 @@ QTestState *qtest_vinitf(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
  */
 QTestState *qtest_init(const char *extra_args);
 
+/**
+ * qtest_init_with_env:
+ * @var: Environment variable from where to take the QEMU binary
+ * @extra_args: Other arguments to pass to QEMU.  CAUTION: these
+ * arguments are subject to word splitting and shell evaluation.
+ *
+ * Like qtest_init(), but use a different environment variable for the
+ * QEMU binary.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_with_env(const char *var, const char *extra_args);
+
 /**
  * qtest_init_without_qmp_handshake:
  * @extra_args: other arguments to pass to QEMU.  CAUTION: these
-- 
2.35.3

[PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Fabiano Rosas]

We're adding support for using more than one QEMU binary in
tests. Modify qtest_get_machines() to take an environment variable
that contains the QEMU binary path.

Since the function keeps a cache of the machines list in the form of a
static variable, refresh it any time the environment variable changes.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 88b79cb477..47c8b6d46f 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1441,9 +1441,10 @@ struct MachInfo {
  * Returns an array with pointers to the available machine names.
  * The terminating entry has the name set to NULL.
  */
-static struct MachInfo *qtest_get_machines(void)
+static struct MachInfo *qtest_get_machines(const char *var)
 {
     static struct MachInfo *machines;
+    static char *qemu_var;
     QDict *response, *minfo;
     QList *list;
     const QListEntry *p;
@@ -1452,11 +1453,19 @@ static struct MachInfo *qtest_get_machines(void)
     QTestState *qts;
     int idx;
 
+    if (g_strcmp0(qemu_var, var)) {
+        qemu_var = g_strdup(var);
+
+        /* new qemu, clear the cache */
+        g_free(machines);
+        machines = NULL;
+    }
+
     if (machines) {
         return machines;
     }
 
-    qts = qtest_init("-machine none");
+    qts = qtest_init_with_env(qemu_var, "-machine none");
     response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
     g_assert(response);
     list = qdict_get_qlist(response, "return");
@@ -1497,7 +1506,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
     struct MachInfo *machines;
     int i;
 
-    machines = qtest_get_machines();
+    machines = qtest_get_machines(NULL);
 
     for (i = 0; machines[i].name != NULL; i++) {
         /* Ignore machines that cannot be used for qtests */
@@ -1518,7 +1527,7 @@ bool qtest_has_machine(const char *machine)
     struct MachInfo *machines;
     int i;
 
-    machines = qtest_get_machines();
+    machines = qtest_get_machines(NULL);
 
     for (i = 0; machines[i].name != NULL; i++) {
         if (g_str_equal(machine, machines[i].name) ||
-- 
2.35.3

[PATCH v2 4/9] tests/qtest: Introduce qtest_has_machine_with_env

[Fabiano Rosas]

Add a variant of qtest_has_machine() that receives an environment
variable containing an alternate QEMU binary path.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 9 +++++++--
 tests/qtest/libqtest.h | 9 +++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 47c8b6d46f..c0e4f05979 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1522,12 +1522,12 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
     }
 }
 
-bool qtest_has_machine(const char *machine)
+bool qtest_has_machine_with_env(const char *var, const char *machine)
 {
     struct MachInfo *machines;
     int i;
 
-    machines = qtest_get_machines(NULL);
+    machines = qtest_get_machines(var);
 
     for (i = 0; machines[i].name != NULL; i++) {
         if (g_str_equal(machine, machines[i].name) ||
@@ -1539,6 +1539,11 @@ bool qtest_has_machine(const char *machine)
     return false;
 }
 
+bool qtest_has_machine(const char *machine)
+{
+    return qtest_has_machine_with_env(NULL, machine);
+}
+
 bool qtest_has_device(const char *device)
 {
     static QList *list;
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index eef3a11ca4..083b2a6520 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -902,6 +902,15 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
  */
 bool qtest_has_machine(const char *machine);
 
+/**
+ * qtest_has_machine_with_env:
+ * @var: Environment variable from where to take the QEMU binary
+ * @machine: The machine to look for
+ *
+ * Returns: true if the machine is available in the specified binary.
+ */
+bool qtest_has_machine_with_env(const char *var, const char *machine);
+
 /**
  * qtest_has_device:
  * @device: The device to look for
-- 
2.35.3

[PATCH v2 5/9] tests/qtest: Introduce qtest_resolve_machine_alias

[Fabiano Rosas]

The migration tests are being enhanced to test migration between
different QEMU versions. A requirement of migration is that the
machine type between source and destination matches, including the
version.

We cannot hardcode machine types in the tests because those change
with each release. QEMU provides a machine type alias that has a fixed
name, but points to the latest machine type at each release.

Add a helper to resolve the alias into the exact machine
type. E.g. "-machine pc" resolves to "pc-i440fx-8.2"

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 16 ++++++++++++++++
 tests/qtest/libqtest.h | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index c0e4f05979..f2f1756de1 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1522,6 +1522,22 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
     }
 }
 
+char *qtest_resolve_machine_alias(const char *var, const char *alias)
+{
+    struct MachInfo *machines;
+    int i;
+
+    machines = qtest_get_machines(var);
+
+    for (i = 0; machines[i].name != NULL; i++) {
+        if (machines[i].alias && g_str_equal(alias, machines[i].alias)) {
+            return g_strdup(machines[i].name);
+        }
+    }
+
+    return NULL;
+}
+
 bool qtest_has_machine_with_env(const char *var, const char *machine)
 {
     struct MachInfo *machines;
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 083b2a6520..75f9310bb0 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -894,6 +894,16 @@ void qtest_qmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
 void qtest_cb_for_every_machine(void (*cb)(const char *machine),
                                 bool skip_old_versioned);
 
+/**
+ * qtest_resolve_machine_alias:
+ * @var: Environment variable from where to take the QEMU binary
+ * @alias: The alias to resolve
+ *
+ * Returns: the machine type corresponding to the alias if any,
+ * otherwise NULL.
+ */
+char *qtest_resolve_machine_alias(const char *var, const char *alias);
+
 /**
  * qtest_has_machine:
  * @machine: The machine to look for
-- 
2.35.3

[PATCH v2 6/9] tests/qtest/migration: Introduce find_common_machine_version

[Fabiano Rosas]

When using two different QEMU binaries for migration testing, we'll
need to find what is the machine version that will work with both
binaries. Add a helper for that.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 24 ++++++++++++++++++++++++
 tests/qtest/migration-helpers.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index be00c52d00..8512134b92 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -180,3 +180,27 @@ void wait_for_migration_fail(QTestState *from, bool allow_active)
     g_assert(qdict_get_bool(rsp_return, "running"));
     qobject_unref(rsp_return);
 }
+
+char *find_common_machine_version(const char *mtype, const char *var1,
+                                  const char *var2)
+{
+    g_autofree char *type1 = qtest_resolve_machine_alias(var1, mtype);
+    g_autofree char *type2 = qtest_resolve_machine_alias(var2, mtype);
+
+    g_assert(type1 && type2);
+
+    if (g_str_equal(type1, type2)) {
+        /* either can be used */
+        return g_strdup(type1);
+    }
+
+    if (qtest_has_machine_with_env(var2, type1)) {
+        return g_strdup(type1);
+    }
+
+    if (qtest_has_machine_with_env(var1, type2)) {
+        return g_strdup(type2);
+    }
+
+    g_assert_not_reached();
+}
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 009e250e90..fc7f693fb6 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,4 +33,6 @@ void wait_for_migration_complete(QTestState *who);
 
 void wait_for_migration_fail(QTestState *from, bool allow_active);
 
+char *find_common_machine_version(const char *mtype, const char *var1,
+                                  const char *var2);
 #endif /* MIGRATION_HELPERS_H */
-- 
2.35.3

[PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Fabiano Rosas]

Stop relying on defaults and select a machine explicitly for every
architecture.

This is a prerequisite for being able to select machine types for
migration using different QEMU binaries for source and destination.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 46f1c275a2..7c10ac925b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *kvm_opts = NULL;
     const char *arch = qtest_get_arch();
     const char *memory_size;
+    const char *machine;
 
     if (args->use_shmem) {
         if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
@@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     got_dst_resume = false;
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         memory_size = "150M";
+        machine = "pc";
         arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
         start_address = X86_TEST_MEM_START;
         end_address = X86_TEST_MEM_END;
     } else if (g_str_equal(arch, "s390x")) {
         memory_size = "128M";
+        machine = "s390-ccw-virtio";
         arch_opts = g_strdup_printf("-bios %s", bootpath);
         start_address = S390_TEST_MEM_START;
         end_address = S390_TEST_MEM_END;
@@ -774,10 +777,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                       "'nvramrc=hex .\" _\" begin %x %x "
                                       "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
                                       "until'", end_address, start_address);
+        machine = "pseries";
         arch_opts = g_strdup("-nodefaults -machine vsmt=8");
     } else if (strcmp(arch, "aarch64") == 0) {
         memory_size = "150M";
-        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
+        machine = "virt";
+        arch_opts = g_strdup_printf("-machine gic-version=max -cpu max "
                                     "-kernel %s", bootpath);
         start_address = ARM_TEST_MEM_START;
         end_address = ARM_TEST_MEM_END;
@@ -813,11 +818,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
+                                 "-machine %s "
                                  "-name source,debug-threads=on "
                                  "-m %s "
                                  "-serial file:%s/src_serial "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
+                                 machine,
                                  memory_size, tmpfs,
                                  arch_opts ? arch_opts : "",
                                  arch_source ? arch_source : "",
@@ -832,12 +839,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
+                                 "-machine %s "
                                  "-name target,debug-threads=on "
                                  "-m %s "
                                  "-serial file:%s/dest_serial "
                                  "-incoming %s "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
+                                 machine,
                                  memory_size, tmpfs, uri,
                                  arch_opts ? arch_opts : "",
                                  arch_target ? arch_target : "",
-- 
2.35.3

[PATCH v2 8/9] tests/qtest/migration: Support more than one QEMU binary

[Fabiano Rosas]

We have strict rules around migration compatibility between different
QEMU versions but no test to validate the migration state between
different binaries.

Add infrastructure to allow running the migration tests with two
different QEMU binaries as migration source and destination.

The code now recognizes two new environment variables
QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
either of them, the test will use the QTEST_QEMU_BINARY variable. If
both are missing then the tests are run with single binary as
previously.

The machine type is selected automatically as the latest machine type
version that works with both binaries.

Usage:
QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
QTEST_QEMU_BINARY_DST=../build-8.1.0/qemu-system-x86_64 \
./tests/qtest/migration-test

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7c10ac925b..984f52becd 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -66,6 +66,9 @@ static bool got_dst_resume;
  */
 #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
 
+#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
+#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
+
 #if defined(__linux__)
 #include <sys/syscall.h>
 #include <sys/vfs.h>
@@ -747,6 +750,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *arch = qtest_get_arch();
     const char *memory_size;
     const char *machine;
+    g_autofree char *machine_type = NULL;
 
     if (args->use_shmem) {
         if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
@@ -817,6 +821,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         kvm_opts = ",dirty-ring-size=4096";
     }
 
+    machine_type = find_common_machine_version(machine, QEMU_ENV_SRC,
+                                               QEMU_ENV_DST);
+    g_test_message("Using machine type: %s", machine_type);
+
     cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
                                  "-machine %s "
                                  "-name source,debug-threads=on "
@@ -824,7 +832,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  "-serial file:%s/src_serial "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
-                                 machine,
+                                 machine_type,
                                  memory_size, tmpfs,
                                  arch_opts ? arch_opts : "",
                                  arch_source ? arch_source : "",
@@ -832,7 +840,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  args->opts_source ? args->opts_source : "",
                                  ignore_stderr);
     if (!args->only_target) {
-        *from = qtest_init(cmd_source);
+        *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source);
         qtest_qmp_set_event_callback(*from,
                                      migrate_watch_for_stop,
                                      &got_src_stop);
@@ -846,14 +854,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  "-incoming %s "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
-                                 machine,
+                                 machine_type,
                                  memory_size, tmpfs, uri,
                                  arch_opts ? arch_opts : "",
                                  arch_target ? arch_target : "",
                                  shmem_opts ? shmem_opts : "",
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
-    *to = qtest_init(cmd_target);
+    *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target);
     qtest_qmp_set_event_callback(*to,
                                  migrate_watch_for_resume,
                                  &got_dst_resume);
@@ -2792,10 +2800,23 @@ int main(int argc, char **argv)
     bool has_uffd;
     const char *arch;
     g_autoptr(GError) err = NULL;
+    const char *qemu_src = getenv(QEMU_ENV_SRC);
+    const char *qemu_dst = getenv(QEMU_ENV_DST);
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
+    /*
+     * The default QTEST_QEMU_BINARY must always be provided because
+     * that is what helpers use to query the accel type and
+     * architecture.
+     */
+    if (qemu_src && qemu_dst) {
+        g_test_message("Only one of %s, %s is allowed",
+                       QEMU_ENV_SRC, QEMU_ENV_DST);
+        exit(1);
+    }
+
     has_kvm = qtest_has_accel("kvm");
     has_tcg = qtest_has_accel("tcg");
 
-- 
2.35.3

[PATCH v2 9/9] tests/qtest: Don't print messages from query instances

[Fabiano Rosas]

Now that we can query more than one binary, the "starting QEMU..."
message can get a little noisy. Mute those messages unless we're
running with --verbose.

Only affects qtest_init() calls from within libqtest. The tests
continue to output as usual.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index f2f1756de1..ea844ad7bc 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -91,6 +91,7 @@ struct QTestState
 
 static GHookList abrt_hooks;
 static void (*sighandler_old)(int);
+static bool silence_spawn_log;
 
 static int qtest_query_target_endianness(QTestState *s);
 
@@ -405,7 +406,9 @@ static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin,
 
     qtest_add_abrt_handler(kill_qemu_hook_func, s);
 
-    g_test_message("starting QEMU: %s", command->str);
+    if (!silence_spawn_log) {
+        g_test_message("starting QEMU: %s", command->str);
+    }
 
 #ifndef _WIN32
     s->qemu_pid = fork();
@@ -1465,6 +1468,8 @@ static struct MachInfo *qtest_get_machines(const char *var)
         return machines;
     }
 
+    silence_spawn_log = !g_test_verbose();
+
     qts = qtest_init_with_env(qemu_var, "-machine none");
     response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
     g_assert(response);
@@ -1496,6 +1501,8 @@ static struct MachInfo *qtest_get_machines(const char *var)
     qtest_quit(qts);
     qobject_unref(response);
 
+    silence_spawn_log = false;
+
     memset(&machines[idx], 0, sizeof(struct MachInfo)); /* Terminating entry */
     return machines;
 }
-- 
2.35.3

Re: [PATCH v2 1/9] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We're adding support for testing migration using two different QEMU
> binaries. We'll provide the second binary in a new environment
> variable.
>
> Allow qtest_qemu_binary() to receive the name of the new variable. If
> the new environment variable is not set, that's not an error, we use
> QTEST_QEMU_BINARY as a fallback.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Thomas, do you want me to get this through the migration tree?

I will wait for you ack anyways.

Re: [PATCH v2 2/9] tests/qtest: Introduce qtest_init_with_env

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Add a version of qtest_init() that takes an environment variable
> containing the path of the QEMU binary. This allows tests to use more
> than one QEMU binary.
>
> If no variable is provided or the environment variable does not exist,
> that is not an error. Fallback to using QTEST_QEMU_BINARY.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We're adding support for using more than one QEMU binary in
> tests. Modify qtest_get_machines() to take an environment variable
> that contains the QEMU binary path.
>
> Since the function keeps a cache of the machines list in the form of a
> static variable, refresh it any time the environment variable changes.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v2 4/9] tests/qtest: Introduce qtest_has_machine_with_env

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Add a variant of qtest_has_machine() that receives an environment
> variable containing an alternate QEMU binary path.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v2 5/9] tests/qtest: Introduce qtest_resolve_machine_alias

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> The migration tests are being enhanced to test migration between
> different QEMU versions. A requirement of migration is that the
> machine type between source and destination matches, including the
> version.
>
> We cannot hardcode machine types in the tests because those change
> with each release. QEMU provides a machine type alias that has a fixed
> name, but points to the latest machine type at each release.
>
> Add a helper to resolve the alias into the exact machine
> type. E.g. "-machine pc" resolves to "pc-i440fx-8.2"
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v2 6/9] tests/qtest/migration: Introduce find_common_machine_version

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> When using two different QEMU binaries for migration testing, we'll
> need to find what is the machine version that will work with both
> binaries. Add a helper for that.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

As it should never fails.

> +    if (qtest_has_machine_with_env(var1, type2)) {
> +        return g_strdup(type2);
> +    }

But if it ever fails, printing here an error message with the contents
of mtype, var1 and var2 sounds like a good idea.

> +    g_assert_not_reached();

Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Stop relying on defaults and select a machine explicitly for every
> architecture.
>
> This is a prerequisite for being able to select machine types for
> migration using different QEMU binaries for source and destination.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration-test.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 46f1c275a2..7c10ac925b 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>      const char *kvm_opts = NULL;
>      const char *arch = qtest_get_arch();
>      const char *memory_size;
> +    const char *machine;
>  
>      if (args->use_shmem) {
>          if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> @@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>      got_dst_resume = false;
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          memory_size = "150M";
> +        machine = "pc";

I would suggest:

      if (strcmp(arch, "i386")) {
          machine = "pc";
      } else {
          machine = "q35";
      }

New development is happening in q35, so I think this should be the more tested.

> @@ -774,10 +777,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>                                        "'nvramrc=hex .\" _\" begin %x %x "
>                                        "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
>                                        "until'", end_address, start_address);
> +        machine = "pseries";
>          arch_opts = g_strdup("-nodefaults -machine vsmt=8");
>      } else if (strcmp(arch, "aarch64") == 0) {
>          memory_size = "150M";
> -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
> +        machine = "virt";
> +        arch_opts = g_strdup_printf("-machine gic-version=max -cpu max "

Does this double -machine command line works?

I expect yes, but who knows.

Later, Juan.

Re: [PATCH v2 1/9] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable

[Thomas Huth]

On 11/10/2023 16.17, Juan Quintela wrote:
> Fabiano Rosas <farosas@suse.de> wrote:
>> We're adding support for testing migration using two different QEMU
>> binaries. We'll provide the second binary in a new environment
>> variable.
>>
>> Allow qtest_qemu_binary() to receive the name of the new variable. If
>> the new environment variable is not set, that's not an error, we use
>> QTEST_QEMU_BINARY as a fallback.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> Thomas, do you want me to get this through the migration tree?

I'm fine either way, but please give me one or two more days for review first.

  Thomas

Re: [PATCH v2 8/9] tests/qtest/migration: Support more than one QEMU binary

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We have strict rules around migration compatibility between different
> QEMU versions but no test to validate the migration state between
> different binaries.
>
> Add infrastructure to allow running the migration tests with two
> different QEMU binaries as migration source and destination.
>
> The code now recognizes two new environment variables
> QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
> either of them, the test will use the QTEST_QEMU_BINARY variable. If
> both are missing then the tests are run with single binary as
> previously.
>
> The machine type is selected automatically as the latest machine type
> version that works with both binaries.
>
> Usage:
> QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
> QTEST_QEMU_BINARY_DST=../build-8.1.0/qemu-system-x86_64 \
> ./tests/qtest/migration-test
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

> ---
>  tests/qtest/migration-test.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 7c10ac925b..984f52becd 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -66,6 +66,9 @@ static bool got_dst_resume;
>   */
>  #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>  
> +#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
> +#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
> +
>  #if defined(__linux__)
>  #include <sys/syscall.h>
>  #include <sys/vfs.h>
> @@ -747,6 +750,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>      const char *arch = qtest_get_arch();
>      const char *memory_size;
>      const char *machine;
> +    g_autofree char *machine_type = NULL;

machine_name?

Re: [PATCH v2 9/9] tests/qtest: Don't print messages from query instances

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Now that we can query more than one binary, the "starting QEMU..."
> message can get a little noisy. Mute those messages unless we're
> running with --verbose.
>
> Only affects qtest_init() calls from within libqtest. The tests
> continue to output as usual.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v2 1/9] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 11/10/2023 16.17, Juan Quintela wrote:
>> Fabiano Rosas <farosas@suse.de> wrote:
>>> We're adding support for testing migration using two different QEMU
>>> binaries. We'll provide the second binary in a new environment
>>> variable.
>>>
>>> Allow qtest_qemu_binary() to receive the name of the new variable. If
>>> the new environment variable is not set, that's not an error, we use
>>> QTEST_QEMU_BINARY as a fallback.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Thomas, do you want me to get this through the migration tree?
>
> I'm fine either way, but please give me one or two more days for review first.

Sure.

Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> Stop relying on defaults and select a machine explicitly for every
>> architecture.
>>
>> This is a prerequisite for being able to select machine types for
>> migration using different QEMU binaries for source and destination.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-test.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 46f1c275a2..7c10ac925b 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>      const char *kvm_opts = NULL;
>>      const char *arch = qtest_get_arch();
>>      const char *memory_size;
>> +    const char *machine;
>>  
>>      if (args->use_shmem) {
>>          if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
>> @@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>      got_dst_resume = false;
>>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>          memory_size = "150M";
>> +        machine = "pc";
>
> I would suggest:
>
>       if (strcmp(arch, "i386")) {
>           machine = "pc";
>       } else {
>           machine = "q35";
>       }
>
> New development is happening in q35, so I think this should be the more tested.
>

Ok, I'll change it.

>> @@ -774,10 +777,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>                                        "'nvramrc=hex .\" _\" begin %x %x "
>>                                        "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
>>                                        "until'", end_address, start_address);
>> +        machine = "pseries";
>>          arch_opts = g_strdup("-nodefaults -machine vsmt=8");
>>      } else if (strcmp(arch, "aarch64") == 0) {
>>          memory_size = "150M";
>> -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
>> +        machine = "virt";
>> +        arch_opts = g_strdup_printf("-machine gic-version=max -cpu max "
>
> Does this double -machine command line works?
>
> I expect yes, but who knows.

I remember it did. But I'll double check just in case.

Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Daniel P. Berrangé]

On Wed, Oct 11, 2023 at 04:28:41PM +0200, Juan Quintela wrote:
> Fabiano Rosas <farosas@suse.de> wrote:
> > Stop relying on defaults and select a machine explicitly for every
> > architecture.
> >
> > This is a prerequisite for being able to select machine types for
> > migration using different QEMU binaries for source and destination.
> >
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  tests/qtest/migration-test.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 46f1c275a2..7c10ac925b 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >      const char *kvm_opts = NULL;
> >      const char *arch = qtest_get_arch();
> >      const char *memory_size;
> > +    const char *machine;
> >  
> >      if (args->use_shmem) {
> >          if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> > @@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >      got_dst_resume = false;
> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> >          memory_size = "150M";
> > +        machine = "pc";
> 
> I would suggest:
> 
>       if (strcmp(arch, "i386")) {
>           machine = "pc";
>       } else {
>           machine = "q35";
>       }
> 
> New development is happening in q35, so I think this should be the more tested.
> 
> > @@ -774,10 +777,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >                                        "'nvramrc=hex .\" _\" begin %x %x "
> >                                        "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
> >                                        "until'", end_address, start_address);
> > +        machine = "pseries";
> >          arch_opts = g_strdup("-nodefaults -machine vsmt=8");
> >      } else if (strcmp(arch, "aarch64") == 0) {
> >          memory_size = "150M";
> > -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
> > +        machine = "virt";
> > +        arch_opts = g_strdup_printf("-machine gic-version=max -cpu max "
> 
> Does this double -machine command line works?

Why not just call the variable 'machine_opts' and here you can
do

 -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
 +        machine_opts = "virt,gic-version=max";
 +        arch_opts = g_strdup_printf("-cpu max "


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/9] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> We're adding support for testing migration using two different QEMU
> binaries. We'll provide the second binary in a new environment
> variable.
> 
> Allow qtest_qemu_binary() to receive the name of the new variable. If
> the new environment variable is not set, that's not an error, we use
> QTEST_QEMU_BINARY as a fallback.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 2/9] tests/qtest: Introduce qtest_init_with_env

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> Add a version of qtest_init() that takes an environment variable
> containing the path of the QEMU binary. This allows tests to use more
> than one QEMU binary.
> 
> If no variable is provided or the environment variable does not exist,
> that is not an error. Fallback to using QTEST_QEMU_BINARY.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 26 +++++++++++++++++++-------
>   tests/qtest/libqtest.h | 13 +++++++++++++
>   2 files changed, 32 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 11, 2023 at 04:28:41PM +0200, Juan Quintela wrote:
>> Fabiano Rosas <farosas@suse.de> wrote:
>> > Stop relying on defaults and select a machine explicitly for every
>> > architecture.
>> >
>> > This is a prerequisite for being able to select machine types for
>> > migration using different QEMU binaries for source and destination.
>> >
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> >  tests/qtest/migration-test.c | 11 ++++++++++-
>> >  1 file changed, 10 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> > index 46f1c275a2..7c10ac925b 100644
>> > --- a/tests/qtest/migration-test.c
>> > +++ b/tests/qtest/migration-test.c
>> > @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>> >      const char *kvm_opts = NULL;
>> >      const char *arch = qtest_get_arch();
>> >      const char *memory_size;
>> > +    const char *machine;
>> >  
>> >      if (args->use_shmem) {
>> >          if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
>> > @@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>> >      got_dst_resume = false;
>> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> >          memory_size = "150M";
>> > +        machine = "pc";
>> 
>> I would suggest:
>> 
>>       if (strcmp(arch, "i386")) {
>>           machine = "pc";
>>       } else {
>>           machine = "q35";
>>       }
>> 
>> New development is happening in q35, so I think this should be the more tested.
>> 
>> > @@ -774,10 +777,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>> >                                        "'nvramrc=hex .\" _\" begin %x %x "
>> >                                        "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
>> >                                        "until'", end_address, start_address);
>> > +        machine = "pseries";
>> >          arch_opts = g_strdup("-nodefaults -machine vsmt=8");
>> >      } else if (strcmp(arch, "aarch64") == 0) {
>> >          memory_size = "150M";
>> > -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
>> > +        machine = "virt";
>> > +        arch_opts = g_strdup_printf("-machine gic-version=max -cpu max "
>> 
>> Does this double -machine command line works?
>
> Why not just call the variable 'machine_opts' and here you can
> do
>
>  -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
>  +        machine_opts = "virt,gic-version=max";
>  +        arch_opts = g_strdup_printf("-cpu max "

The machine name needs to be standalone so it can be overridden in the
next patch when we compute the common machine type.

Maybe I could add the machine_opts anyway just to make it more explicit.

Re: [PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> We're adding support for using more than one QEMU binary in
> tests. Modify qtest_get_machines() to take an environment variable
> that contains the QEMU binary path.
> 
> Since the function keeps a cache of the machines list in the form of a
> static variable, refresh it any time the environment variable changes.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 4/9] tests/qtest: Introduce qtest_has_machine_with_env

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> Add a variant of qtest_has_machine() that receives an environment
> variable containing an alternate QEMU binary path.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 9 +++++++--
>   tests/qtest/libqtest.h | 9 +++++++++
>   2 files changed, 16 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 5/9] tests/qtest: Introduce qtest_resolve_machine_alias

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> The migration tests are being enhanced to test migration between
> different QEMU versions. A requirement of migration is that the
> machine type between source and destination matches, including the
> version.
> 
> We cannot hardcode machine types in the tests because those change
> with each release. QEMU provides a machine type alias that has a fixed
> name, but points to the latest machine type at each release.
> 
> Add a helper to resolve the alias into the exact machine
> type. E.g. "-machine pc" resolves to "pc-i440fx-8.2"
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 16 ++++++++++++++++
>   tests/qtest/libqtest.h | 10 ++++++++++
>   2 files changed, 26 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 5/9] tests/qtest: Introduce qtest_resolve_machine_alias

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> The migration tests are being enhanced to test migration between
> different QEMU versions. A requirement of migration is that the
> machine type between source and destination matches, including the
> version.
> 
> We cannot hardcode machine types in the tests because those change
> with each release. QEMU provides a machine type alias that has a fixed
> name, but points to the latest machine type at each release.
> 
> Add a helper to resolve the alias into the exact machine
> type. E.g. "-machine pc" resolves to "pc-i440fx-8.2"
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 16 ++++++++++++++++
>   tests/qtest/libqtest.h | 10 ++++++++++
>   2 files changed, 26 insertions(+)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index c0e4f05979..f2f1756de1 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1522,6 +1522,22 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
>       }
>   }
>   
> +char *qtest_resolve_machine_alias(const char *var, const char *alias)
> +{
> +    struct MachInfo *machines;
> +    int i;
> +
> +    machines = qtest_get_machines(var);
> +
> +    for (i = 0; machines[i].name != NULL; i++) {
> +        if (machines[i].alias && g_str_equal(alias, machines[i].alias)) {
> +            return g_strdup(machines[i].name);

Looking at this twice, I think there is no need for the g_strdup() here, 
since the name strings won't go away. You could then maybe also change the 
return type to "const char *".

  Thomas


> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>   bool qtest_has_machine_with_env(const char *var, const char *machine)
>   {
>       struct MachInfo *machines;
> diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
> index 083b2a6520..75f9310bb0 100644
> --- a/tests/qtest/libqtest.h
> +++ b/tests/qtest/libqtest.h
> @@ -894,6 +894,16 @@ void qtest_qmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
>   void qtest_cb_for_every_machine(void (*cb)(const char *machine),
>                                   bool skip_old_versioned);
>   
> +/**
> + * qtest_resolve_machine_alias:
> + * @var: Environment variable from where to take the QEMU binary
> + * @alias: The alias to resolve
> + *
> + * Returns: the machine type corresponding to the alias if any,
> + * otherwise NULL.
> + */
> +char *qtest_resolve_machine_alias(const char *var, const char *alias);
> +
>   /**
>    * qtest_has_machine:
>    * @machine: The machine to look for

Re: [PATCH v2 6/9] tests/qtest/migration: Introduce find_common_machine_version

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> When using two different QEMU binaries for migration testing, we'll
> need to find what is the machine version that will work with both
> binaries. Add a helper for that.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-helpers.c | 24 ++++++++++++++++++++++++
>   tests/qtest/migration-helpers.h |  2 ++
>   2 files changed, 26 insertions(+)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index be00c52d00..8512134b92 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -180,3 +180,27 @@ void wait_for_migration_fail(QTestState *from, bool allow_active)
>       g_assert(qdict_get_bool(rsp_return, "running"));
>       qobject_unref(rsp_return);
>   }
> +
> +char *find_common_machine_version(const char *mtype, const char *var1,
> +                                  const char *var2)
> +{
> +    g_autofree char *type1 = qtest_resolve_machine_alias(var1, mtype);
> +    g_autofree char *type2 = qtest_resolve_machine_alias(var2, mtype);
> +
> +    g_assert(type1 && type2);
> +
> +    if (g_str_equal(type1, type2)) {
> +        /* either can be used */
> +        return g_strdup(type1);

If you remove the g_strdup() in the previous patch, you can also get rid of 
the g_strdup() and g_autofree in this function here (and change the return 
type to "const char *".

  Thomas


> +    }
> +
> +    if (qtest_has_machine_with_env(var2, type1)) {
> +        return g_strdup(type1);
> +    }
> +
> +    if (qtest_has_machine_with_env(var1, type2)) {
> +        return g_strdup(type2);
> +    }
> +
> +    g_assert_not_reached();
> +}
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 009e250e90..fc7f693fb6 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -33,4 +33,6 @@ void wait_for_migration_complete(QTestState *who);
>   
>   void wait_for_migration_fail(QTestState *from, bool allow_active);
>   
> +char *find_common_machine_version(const char *mtype, const char *var1,
> +                                  const char *var2);
>   #endif /* MIGRATION_HELPERS_H */

Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> Stop relying on defaults and select a machine explicitly for every
> architecture.
> 
> This is a prerequisite for being able to select machine types for
> migration using different QEMU binaries for source and destination.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-test.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 46f1c275a2..7c10ac925b 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       const char *kvm_opts = NULL;
>       const char *arch = qtest_get_arch();
>       const char *memory_size;
> +    const char *machine;

I'd maybe call this machine_alias to avoid confusion with the variable that 
is added in the next patch.

  Thomas

Re: [PATCH v2 9/9] tests/qtest: Don't print messages from query instances

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> Now that we can query more than one binary, the "starting QEMU..."
> message can get a little noisy. Mute those messages unless we're
> running with --verbose.
> 
> Only affects qtest_init() calls from within libqtest. The tests
> continue to output as usual.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Thomas Huth]

On 06/10/2023 14.39, Fabiano Rosas wrote:
> We're adding support for using more than one QEMU binary in
> tests. Modify qtest_get_machines() to take an environment variable
> that contains the QEMU binary path.
> 
> Since the function keeps a cache of the machines list in the form of a
> static variable, refresh it any time the environment variable changes.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/libqtest.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 88b79cb477..47c8b6d46f 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1441,9 +1441,10 @@ struct MachInfo {
>    * Returns an array with pointers to the available machine names.
>    * The terminating entry has the name set to NULL.
>    */
> -static struct MachInfo *qtest_get_machines(void)
> +static struct MachInfo *qtest_get_machines(const char *var)
>   {
>       static struct MachInfo *machines;
> +    static char *qemu_var;
>       QDict *response, *minfo;
>       QList *list;
>       const QListEntry *p;
> @@ -1452,11 +1453,19 @@ static struct MachInfo *qtest_get_machines(void)
>       QTestState *qts;
>       int idx;
>   
> +    if (g_strcmp0(qemu_var, var)) {
> +        qemu_var = g_strdup(var);
> +
> +        /* new qemu, clear the cache */
> +        g_free(machines);
> +        machines = NULL;
> +    }
> +
>       if (machines) {
>           return machines;
>       }

After sleeping on the topic of the string handling in this patch series a 
little bit  I think it was maybe a bad idea to suggest to remove the 
g_strdups in the other patches. If you actually clear the cache here, the 
strings that previously were guaranteed to stay around until the end of the 
program might now vanish. So instead of returning the pointer to the cache 
here, it might be better to create a copy of the whole structure here and 
let the callers decide whether they want to keep it around or free it at the 
end?

  Thomas

Re: [PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 06/10/2023 14.39, Fabiano Rosas wrote:
>> We're adding support for using more than one QEMU binary in
>> tests. Modify qtest_get_machines() to take an environment variable
>> that contains the QEMU binary path.
>> 
>> Since the function keeps a cache of the machines list in the form of a
>> static variable, refresh it any time the environment variable changes.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/libqtest.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>> index 88b79cb477..47c8b6d46f 100644
>> --- a/tests/qtest/libqtest.c
>> +++ b/tests/qtest/libqtest.c
>> @@ -1441,9 +1441,10 @@ struct MachInfo {
>>    * Returns an array with pointers to the available machine names.
>>    * The terminating entry has the name set to NULL.
>>    */
>> -static struct MachInfo *qtest_get_machines(void)
>> +static struct MachInfo *qtest_get_machines(const char *var)
>>   {
>>       static struct MachInfo *machines;
>> +    static char *qemu_var;
>>       QDict *response, *minfo;
>>       QList *list;
>>       const QListEntry *p;
>> @@ -1452,11 +1453,19 @@ static struct MachInfo *qtest_get_machines(void)
>>       QTestState *qts;
>>       int idx;
>>   
>> +    if (g_strcmp0(qemu_var, var)) {
>> +        qemu_var = g_strdup(var);
>> +
>> +        /* new qemu, clear the cache */
>> +        g_free(machines);
>> +        machines = NULL;
>> +    }
>> +
>>       if (machines) {
>>           return machines;
>>       }
>
> After sleeping on the topic of the string handling in this patch series a 
> little bit  I think it was maybe a bad idea to suggest to remove the 
> g_strdups in the other patches. If you actually clear the cache here, the 
> strings that previously were guaranteed to stay around until the end of the 
> program might now vanish. So instead of returning the pointer to the cache 
> here, it might be better to create a copy of the whole structure here and 
> let the callers decide whether they want to keep it around or free it at the 
> end?

Hm, let me try that out. We could have a 'bool refresh' parameter in the
top level API then, which would be a clearer interface perhaps.

Thanks

Re: [PATCH v2 3/9] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Thomas Huth <thuth@redhat.com> writes:
>
>> On 06/10/2023 14.39, Fabiano Rosas wrote:
>>> We're adding support for using more than one QEMU binary in
>>> tests. Modify qtest_get_machines() to take an environment variable
>>> that contains the QEMU binary path.
>>> 
>>> Since the function keeps a cache of the machines list in the form of a
>>> static variable, refresh it any time the environment variable changes.
>>> 
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>   tests/qtest/libqtest.c | 17 +++++++++++++----
>>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>>> index 88b79cb477..47c8b6d46f 100644
>>> --- a/tests/qtest/libqtest.c
>>> +++ b/tests/qtest/libqtest.c
>>> @@ -1441,9 +1441,10 @@ struct MachInfo {
>>>    * Returns an array with pointers to the available machine names.
>>>    * The terminating entry has the name set to NULL.
>>>    */
>>> -static struct MachInfo *qtest_get_machines(void)
>>> +static struct MachInfo *qtest_get_machines(const char *var)
>>>   {
>>>       static struct MachInfo *machines;
>>> +    static char *qemu_var;
>>>       QDict *response, *minfo;
>>>       QList *list;
>>>       const QListEntry *p;
>>> @@ -1452,11 +1453,19 @@ static struct MachInfo *qtest_get_machines(void)
>>>       QTestState *qts;
>>>       int idx;
>>>   
>>> +    if (g_strcmp0(qemu_var, var)) {
>>> +        qemu_var = g_strdup(var);
>>> +
>>> +        /* new qemu, clear the cache */
>>> +        g_free(machines);
>>> +        machines = NULL;
>>> +    }
>>> +
>>>       if (machines) {
>>>           return machines;
>>>       }
>>
>> After sleeping on the topic of the string handling in this patch series a 
>> little bit  I think it was maybe a bad idea to suggest to remove the 
>> g_strdups in the other patches. If you actually clear the cache here, the 
>> strings that previously were guaranteed to stay around until the end of the 
>> program might now vanish. So instead of returning the pointer to the cache 
>> here, it might be better to create a copy of the whole structure here and 
>> let the callers decide whether they want to keep it around or free it at the 
>> end?
>
> Hm, let me try that out. We could have a 'bool refresh' parameter in the
> top level API then, which would be a clearer interface perhaps.

I'm looking into this right now. I don't think callers ever want to keep
the machines list around. We'd have to cache the list and the binary
name a second time in the callers just to avoid having to copy/free a
few strings.

The caching needs to be centralized at qtest_get_machines(), otherwise
we'd be better off having doing setenv around the function calls, which
is what my hacked first version did.

If you're ok with that I'll just add a cleanup function to free all
strings when clearing the cache and keep strdup'ing where appropriate.

Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> Stop relying on defaults and select a machine explicitly for every
>> architecture.
>>
>> This is a prerequisite for being able to select machine types for
>> migration using different QEMU binaries for source and destination.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration-test.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 46f1c275a2..7c10ac925b 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>      const char *kvm_opts = NULL;
>>      const char *arch = qtest_get_arch();
>>      const char *memory_size;
>> +    const char *machine;
>>  
>>      if (args->use_shmem) {
>>          if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
>> @@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>      got_dst_resume = false;
>>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>          memory_size = "150M";
>> +        machine = "pc";
>
> I would suggest:
>
>       if (strcmp(arch, "i386")) {
>           machine = "pc";
>       } else {
>           machine = "q35";
>       }

Turns out we cannot run the tests with the q35 currently. It seems the
bootsector we use to print the A and Bs is not recognized by seabios on
that machine. I'm investigating.