[PATCH v2 00/11] tests/qtest: Allow running boot-serial / migration with TCG disabled

[PATCH v2 00/11] tests/qtest: Allow running boot-serial / migration with TCG disabled

[Philippe Mathieu-Daudé]

Two test were failing on Darwin when testing Fabiano's series
which allows building ARM targets without TCG accelerator:
https://lore.kernel.org/qemu-devel/20230118193518.26433-1-farosas@suse.de/

These patches allow boot-serial / migration tests to run without
TCG / KVM.

Since v1:
- Split GString API conversion in multiple trivial patches (David)
- Use KVM first in migration test (David)
- Dropped HVF changes for now.

Philippe Mathieu-Daudé (11):
  tests/qtest/boot-serial-test: Constify tests[] array
  tests/qtest/boot-serial-test: Simplify test_machine() a bit
  tests/qtest/boot-serial-test: Build command line using GString API
  tests/qtest/boot-serial-test: Only use available accelerators
  tests/qtest/migration-test: Inverse #ifdef'ry ladders
  tests/qtest/migration-test: Reduce 'cmd_source' string scope
  tests/qtest/migration-test: Build command line using GString API (1/4)
  tests/qtest/migration-test: Build command line using GString API (2/4)
  tests/qtest/migration-test: Build command line using GString API (3/4)
  tests/qtest/migration-test: Build command line using GString API (4/4)
  tests/qtest/migration-test: Only use available accelerators

 tests/qtest/boot-serial-test.c | 46 +++++++++-------
 tests/qtest/migration-test.c   | 95 ++++++++++++++++++----------------
 2 files changed, 79 insertions(+), 62 deletions(-)

-- 
2.38.1

[PATCH v2 01/11] tests/qtest/boot-serial-test: Constify tests[] array

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index b216519b62..3aef3a97a9 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -139,7 +139,7 @@ typedef struct testdef {
     const uint8_t *bios;    /* Set in case we use our own mini bios */
 } testdef_t;
 
-static testdef_t tests[] = {
+static const testdef_t tests[] = {
     { "alpha", "clipper", "", "PCI:" },
     { "avr", "arduino-duemilanove", "", "T", sizeof(bios_avr), NULL, bios_avr },
     { "avr", "arduino-mega-2560-v3", "", "T", sizeof(bios_avr), NULL, bios_avr},
-- 
2.38.1

[PATCH v2 02/11] tests/qtest/boot-serial-test: Simplify test_machine() a bit

[Philippe Mathieu-Daudé]

Slighly modify test_machine() to simplify next commit review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 3aef3a97a9..3a854b0174 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -227,7 +227,6 @@ static void test_machine(const void *data)
     g_autofree char *serialtmp = NULL;
     g_autofree char *codetmp = NULL;
     const char *codeparam = "";
-    const uint8_t *code = NULL;
     QTestState *qts;
     int ser_fd;
 
@@ -235,21 +234,13 @@ static void test_machine(const void *data)
     g_assert(ser_fd != -1);
     close(ser_fd);
 
-    if (test->kernel) {
-        code = test->kernel;
-        codeparam = "-kernel";
-    } else if (test->bios) {
-        code = test->bios;
-        codeparam = "-bios";
-    }
-
-    if (code) {
+    if (test->kernel || test->bios) {
         ssize_t wlen;
         int code_fd;
 
         code_fd = g_file_open_tmp("qtest-boot-serial-cXXXXXX", &codetmp, NULL);
         g_assert(code_fd != -1);
-        wlen = write(code_fd, code, test->codesize);
+        wlen = write(code_fd, test->kernel ? : test->bios, test->codesize);
         g_assert(wlen == test->codesize);
         close(code_fd);
     }
@@ -258,12 +249,14 @@ static void test_machine(const void *data)
      * Make sure that this test uses tcg if available: It is used as a
      * fast-enough smoketest for that.
      */
-    qts = qtest_initf("%s %s -M %s -no-shutdown "
+    qts = qtest_initf("%s %s %s -M %s -no-shutdown "
                       "-chardev file,id=serial0,path=%s "
                       "-serial chardev:serial0 -accel tcg -accel kvm %s",
-                      codeparam, code ? codetmp : "", test->machine,
+                      codeparam,
+                      test->kernel ? "-kernel " : test->bios ? "-bios " : "",
+                      codetmp ? : "", test->machine,
                       serialtmp, test->extra);
-    if (code) {
+    if (codetmp) {
         unlink(codetmp);
     }
 
-- 
2.38.1

[PATCH v2 03/11] tests/qtest/boot-serial-test: Build command line using GString API

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 3a854b0174..92890b409d 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -226,14 +226,17 @@ static void test_machine(const void *data)
     const testdef_t *test = data;
     g_autofree char *serialtmp = NULL;
     g_autofree char *codetmp = NULL;
-    const char *codeparam = "";
     QTestState *qts;
     int ser_fd;
+    g_autoptr(GString) cmd = g_string_new("");
 
     ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL);
     g_assert(ser_fd != -1);
     close(ser_fd);
 
+    g_string_append_printf(cmd, "-M %s ", test->machine);
+    g_string_append(cmd, "-no-shutdown ");
+
     if (test->kernel || test->bios) {
         ssize_t wlen;
         int code_fd;
@@ -243,19 +246,23 @@ static void test_machine(const void *data)
         wlen = write(code_fd, test->kernel ? : test->bios, test->codesize);
         g_assert(wlen == test->codesize);
         close(code_fd);
+        g_string_append_printf(cmd, "%s %s ",
+                               test->kernel ? "-kernel " : "-bios ", codetmp);
     }
 
+    g_string_append_printf(cmd, "-chardev file,id=serial0,path=%s "
+                                "-serial chardev:serial0 ", serialtmp);
+
     /*
      * Make sure that this test uses tcg if available: It is used as a
      * fast-enough smoketest for that.
      */
-    qts = qtest_initf("%s %s %s -M %s -no-shutdown "
-                      "-chardev file,id=serial0,path=%s "
-                      "-serial chardev:serial0 -accel tcg -accel kvm %s",
-                      codeparam,
-                      test->kernel ? "-kernel " : test->bios ? "-bios " : "",
-                      codetmp ? : "", test->machine,
-                      serialtmp, test->extra);
+    g_string_append(cmd, "-accel tcg ");
+    g_string_append(cmd, "-accel kvm ");
+    g_string_append(cmd, test->extra);
+
+    qts = qtest_init(cmd->str);
+
     if (codetmp) {
         unlink(codetmp);
     }
-- 
2.38.1

[PATCH v2 04/11] tests/qtest/boot-serial-test: Only use available accelerators

[Philippe Mathieu-Daudé]

For example, avoid when TCG is disabled:

  $ make check-qtest-aarch64
  ...
  18/20 qemu:qtest+qtest-aarch64 / qtest-aarch64/boot-serial-test
  qemu-system-aarch64: -accel tcg: invalid accelerator tcg

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 92890b409d..f375b16dae 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -17,6 +17,9 @@
 #include "libqtest.h"
 #include "libqos/libqos-spapr.h"
 
+static bool has_tcg;
+static bool has_kvm;
+
 static const uint8_t bios_avr[] = {
     0x88, 0xe0,             /* ldi r24, 0x08   */
     0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */
@@ -257,8 +260,12 @@ static void test_machine(const void *data)
      * Make sure that this test uses tcg if available: It is used as a
      * fast-enough smoketest for that.
      */
-    g_string_append(cmd, "-accel tcg ");
-    g_string_append(cmd, "-accel kvm ");
+    if (has_tcg) {
+        g_string_append(cmd, "-accel tcg ");
+    }
+    if (has_kvm) {
+        g_string_append(cmd, "-accel kvm ");
+    }
     g_string_append(cmd, test->extra);
 
     qts = qtest_init(cmd->str);
@@ -285,6 +292,9 @@ int main(int argc, char *argv[])
     const char *arch = qtest_get_arch();
     int i;
 
+    has_tcg = qtest_has_accel("tcg");
+    has_kvm = qtest_has_accel("kvm");
+
     g_test_init(&argc, &argv, NULL);
 
     for (i = 0; tests[i].arch != NULL; i++) {
-- 
2.38.1

[PATCH v2 05/11] tests/qtest/migration-test: Inverse #ifdef'ry ladders

[Philippe Mathieu-Daudé]

This slighly simplify the logic, and eases the following conversion.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index dbde726adf..8beeda4686 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -647,15 +647,15 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     if (!getenv("QTEST_LOG") && args->hide_stderr) {
-#ifndef _WIN32
-        ignore_stderr = "2>/dev/null";
-#else
+#ifdef _WIN32
         /*
          * On Windows the QEMU executable is created via CreateProcess() and
          * IO redirection does not work, so don't bother adding IO redirection
          * to the command line.
          */
         ignore_stderr = "";
+#else
+        ignore_stderr = "2>/dev/null";
 #endif
     } else {
         ignore_stderr = "";
-- 
2.38.1

[PATCH v2 06/11] tests/qtest/migration-test: Reduce 'cmd_source' string scope

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 8beeda4686..6c3db95113 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -584,7 +584,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
 {
     g_autofree gchar *arch_source = NULL;
     g_autofree gchar *arch_target = NULL;
-    g_autofree gchar *cmd_source = NULL;
     g_autofree gchar *cmd_target = NULL;
     const gchar *ignore_stderr;
     g_autofree char *bootpath = NULL;
@@ -672,20 +671,22 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         shmem_opts = g_strdup("");
     }
 
-    cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
-                                 "-name source,debug-threads=on "
-                                 "-m %s "
-                                 "-serial file:%s/src_serial "
-                                 "%s %s %s %s",
-                                 args->use_dirty_ring ?
-                                 ",dirty-ring-size=4096" : "",
-                                 machine_opts ? " -machine " : "",
-                                 machine_opts ? machine_opts : "",
-                                 memory_size, tmpfs,
-                                 arch_source, shmem_opts,
-                                 args->opts_source ? args->opts_source : "",
-                                 ignore_stderr);
     if (!args->only_target) {
+        g_autofree gchar *cmd_source = NULL;
+
+        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
+                                     "-name source,debug-threads=on "
+                                     "-m %s "
+                                     "-serial file:%s/src_serial "
+                                     "%s %s %s %s",
+                                     args->use_dirty_ring ?
+                                     ",dirty-ring-size=4096" : "",
+                                     machine_opts ? " -machine " : "",
+                                     machine_opts ? machine_opts : "",
+                                     memory_size, tmpfs,
+                                     arch_source, shmem_opts,
+                                     args->opts_source ? args->opts_source : "",
+                                     ignore_stderr);
         *from = qtest_init(cmd_source);
     }
 
-- 
2.38.1

[PATCH v2 07/11] tests/qtest/migration-test: Build command line using GString API (1/4)

[Philippe Mathieu-Daudé]

Part 1/4: Convert memory & machine options.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 6c3db95113..7aa323a7a7 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -582,6 +582,7 @@ typedef struct {
 static int test_migrate_start(QTestState **from, QTestState **to,
                               const char *uri, MigrateStart *args)
 {
+    g_autoptr(GString) cmd_common = NULL;
     g_autofree gchar *arch_source = NULL;
     g_autofree gchar *arch_target = NULL;
     g_autofree gchar *cmd_target = NULL;
@@ -601,6 +602,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     got_stop = false;
+
+    cmd_common = g_string_new("");
+
     bootpath = g_strdup_printf("%s/bootsect", tmpfs);
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         /* the assembled x86 boot sector should be exactly one sector large */
@@ -644,6 +648,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     } else {
         g_assert_not_reached();
     }
+    if (machine_opts) {
+        g_string_append_printf(cmd_common, " -machine %s ", machine_opts);
+    }
+    g_string_append_printf(cmd_common, "-m %s ", memory_size);
 
     if (!getenv("QTEST_LOG") && args->hide_stderr) {
 #ifdef _WIN32
@@ -674,33 +682,27 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     if (!args->only_target) {
         g_autofree gchar *cmd_source = NULL;
 
-        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
+        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s "
                                      "-name source,debug-threads=on "
-                                     "-m %s "
                                      "-serial file:%s/src_serial "
                                      "%s %s %s %s",
                                      args->use_dirty_ring ?
                                      ",dirty-ring-size=4096" : "",
-                                     machine_opts ? " -machine " : "",
-                                     machine_opts ? machine_opts : "",
-                                     memory_size, tmpfs,
+                                     cmd_common->str, tmpfs,
                                      arch_source, shmem_opts,
                                      args->opts_source ? args->opts_source : "",
                                      ignore_stderr);
         *from = qtest_init(cmd_source);
     }
 
-    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
+    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s "
                                  "-name target,debug-threads=on "
-                                 "-m %s "
                                  "-serial file:%s/dest_serial "
                                  "-incoming %s "
                                  "%s %s %s %s",
                                  args->use_dirty_ring ?
                                  ",dirty-ring-size=4096" : "",
-                                 machine_opts ? " -machine " : "",
-                                 machine_opts ? machine_opts : "",
-                                 memory_size, tmpfs, uri,
+                                 cmd_common->str, tmpfs, uri,
                                  arch_target, shmem_opts,
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
-- 
2.38.1

[PATCH v2 08/11] tests/qtest/migration-test: Build command line using GString API (2/4)

[Philippe Mathieu-Daudé]

Part 2/4: Convert shmem option.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7aa323a7a7..8377b3976a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -588,7 +588,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     g_autofree gchar *cmd_target = NULL;
     const gchar *ignore_stderr;
     g_autofree char *bootpath = NULL;
-    g_autofree char *shmem_opts = NULL;
     g_autofree char *shmem_path = NULL;
     const char *arch = qtest_get_arch();
     const char *machine_opts = NULL;
@@ -670,13 +669,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
 
     if (args->use_shmem) {
         shmem_path = g_strdup_printf("/dev/shm/qemu-%d", getpid());
-        shmem_opts = g_strdup_printf(
+        g_string_append_printf(cmd_common,
             "-object memory-backend-file,id=mem0,size=%s"
             ",mem-path=%s,share=on -numa node,memdev=mem0",
             memory_size, shmem_path);
-    } else {
-        shmem_path = NULL;
-        shmem_opts = g_strdup("");
     }
 
     if (!args->only_target) {
@@ -685,11 +681,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s "
                                      "-name source,debug-threads=on "
                                      "-serial file:%s/src_serial "
-                                     "%s %s %s %s",
+                                     "%s %s %s",
                                      args->use_dirty_ring ?
                                      ",dirty-ring-size=4096" : "",
-                                     cmd_common->str, tmpfs,
-                                     arch_source, shmem_opts,
+                                     cmd_common->str, tmpfs, arch_source,
                                      args->opts_source ? args->opts_source : "",
                                      ignore_stderr);
         *from = qtest_init(cmd_source);
@@ -699,11 +694,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  "-name target,debug-threads=on "
                                  "-serial file:%s/dest_serial "
                                  "-incoming %s "
-                                 "%s %s %s %s",
+                                 "%s %s %s",
                                  args->use_dirty_ring ?
                                  ",dirty-ring-size=4096" : "",
-                                 cmd_common->str, tmpfs, uri,
-                                 arch_target, shmem_opts,
+                                 cmd_common->str, tmpfs, uri, arch_target,
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
     *to = qtest_init(cmd_target);
-- 
2.38.1

[PATCH v2 09/11] tests/qtest/migration-test: Build command line using GString API (3/4)

[Philippe Mathieu-Daudé]

Part 3/4: Convert accelerator options.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 8377b3976a..015b774a9e 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -603,6 +603,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     got_stop = false;
 
     cmd_common = g_string_new("");
+    g_string_append_printf(cmd_common, "-accel kvm%s ",
+                           args->use_dirty_ring ? ",dirty-ring-size=4096" : "");
+    g_string_append(cmd_common, "-accel tcg ");
 
     bootpath = g_strdup_printf("%s/bootsect", tmpfs);
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
@@ -678,25 +681,21 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     if (!args->only_target) {
         g_autofree gchar *cmd_source = NULL;
 
-        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s "
+        cmd_source = g_strdup_printf("%s "
                                      "-name source,debug-threads=on "
                                      "-serial file:%s/src_serial "
                                      "%s %s %s",
-                                     args->use_dirty_ring ?
-                                     ",dirty-ring-size=4096" : "",
                                      cmd_common->str, tmpfs, arch_source,
                                      args->opts_source ? args->opts_source : "",
                                      ignore_stderr);
         *from = qtest_init(cmd_source);
     }
 
-    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s "
+    cmd_target = g_strdup_printf("%s "
                                  "-name target,debug-threads=on "
                                  "-serial file:%s/dest_serial "
                                  "-incoming %s "
                                  "%s %s %s",
-                                 args->use_dirty_ring ?
-                                 ",dirty-ring-size=4096" : "",
                                  cmd_common->str, tmpfs, uri, arch_target,
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
-- 
2.38.1

[PATCH v2 10/11] tests/qtest/migration-test: Build command line using GString API (4/4)

[Philippe Mathieu-Daudé]

Part 4/4: Convert rest of options.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 50 +++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 015b774a9e..a930964268 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -585,8 +585,8 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     g_autoptr(GString) cmd_common = NULL;
     g_autofree gchar *arch_source = NULL;
     g_autofree gchar *arch_target = NULL;
-    g_autofree gchar *cmd_target = NULL;
-    const gchar *ignore_stderr;
+    g_autoptr(GString) cmd_target = NULL;
+    const gchar *ignore_stderr = NULL;
     g_autofree char *bootpath = NULL;
     g_autofree char *shmem_path = NULL;
     const char *arch = qtest_get_arch();
@@ -662,12 +662,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
          * IO redirection does not work, so don't bother adding IO redirection
          * to the command line.
          */
-        ignore_stderr = "";
 #else
         ignore_stderr = "2>/dev/null";
 #endif
-    } else {
-        ignore_stderr = "";
     }
 
     if (args->use_shmem) {
@@ -679,27 +676,32 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     if (!args->only_target) {
-        g_autofree gchar *cmd_source = NULL;
-
-        cmd_source = g_strdup_printf("%s "
-                                     "-name source,debug-threads=on "
-                                     "-serial file:%s/src_serial "
-                                     "%s %s %s",
-                                     cmd_common->str, tmpfs, arch_source,
-                                     args->opts_source ? args->opts_source : "",
-                                     ignore_stderr);
-        *from = qtest_init(cmd_source);
+        g_autoptr(GString) cmd_source = g_string_new(cmd_common->str);
+        g_string_append(cmd_source, "-name source,debug-threads=on ");
+        g_string_append_printf(cmd_source, "-serial file:%s/src_serial ",
+                               tmpfs);
+        g_string_append_printf(cmd_source, "%s ", arch_source);
+        if (args->opts_source) {
+            g_string_append_printf(cmd_source, "%s ", args->opts_source);
+        }
+        if (ignore_stderr) {
+            g_string_append(cmd_source, ignore_stderr); /* last string */
+        }
+        *from = qtest_init(cmd_source->str);
     }
 
-    cmd_target = g_strdup_printf("%s "
-                                 "-name target,debug-threads=on "
-                                 "-serial file:%s/dest_serial "
-                                 "-incoming %s "
-                                 "%s %s %s",
-                                 cmd_common->str, tmpfs, uri, arch_target,
-                                 args->opts_target ? args->opts_target : "",
-                                 ignore_stderr);
-    *to = qtest_init(cmd_target);
+    cmd_target = g_string_new(cmd_common->str);
+    g_string_append(cmd_target, "-name target,debug-threads=on ");
+    g_string_append_printf(cmd_target, "-serial file:%s/dest_serial ", tmpfs);
+    g_string_append_printf(cmd_target, "-incoming %s ", uri);
+    g_string_append_printf(cmd_target, "%s ", arch_target);
+    if (args->opts_target) {
+        g_string_append_printf(cmd_target, "%s ", args->opts_target);
+    }
+    if (ignore_stderr) {
+        g_string_append(cmd_target, ignore_stderr); /* last string */
+    }
+    *to = qtest_init(cmd_target->str);
 
     /*
      * Remove shmem file immediately to avoid memory leak in test failed case.
-- 
2.38.1

[PATCH v2 11/11] tests/qtest/migration-test: Only use available accelerators

[Philippe Mathieu-Daudé]

For example, avoid when TCG is disabled:

  $ make check-qtest-aarch64
  ...
  20/20 qemu:qtest+qtest-aarch64 / qtest-aarch64/migration-test
  qemu-system-aarch64: -accel tcg: invalid accelerator tcg

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index a930964268..17783d7334 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -45,6 +45,8 @@
 
 unsigned start_address;
 unsigned end_address;
+static bool has_tcg;
+static bool has_kvm;
 static bool uffd_feature_thread_id;
 
 /*
@@ -603,9 +605,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     got_stop = false;
 
     cmd_common = g_string_new("");
-    g_string_append_printf(cmd_common, "-accel kvm%s ",
-                           args->use_dirty_ring ? ",dirty-ring-size=4096" : "");
-    g_string_append(cmd_common, "-accel tcg ");
+    if (has_kvm) { /* KVM first */
+        g_string_append_printf(cmd_common, "-accel kvm%s ",
+                               args->use_dirty_ring
+                               ? ",dirty-ring-size=4096" : "");
+    }
+    if (has_tcg) {
+        g_string_append(cmd_common, "-accel tcg ");
+    }
 
     bootpath = g_strdup_printf("%s/bootsect", tmpfs);
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
@@ -2457,12 +2464,14 @@ static bool kvm_dirty_ring_supported(void)
 
 int main(int argc, char **argv)
 {
-    const bool has_kvm = qtest_has_accel("kvm");
     const bool has_uffd = ufd_version_check();
     const char *arch = qtest_get_arch();
     g_autoptr(GError) err = NULL;
     int ret;
 
+    has_tcg = qtest_has_accel("tcg");
+    has_kvm = qtest_has_accel("kvm");
+
     g_test_init(&argc, &argv, NULL);
 
     /*
-- 
2.38.1

Re: [PATCH v2 04/11] tests/qtest/boot-serial-test: Only use available accelerators

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> For example, avoid when TCG is disabled:
>
>   $ make check-qtest-aarch64
>   ...
>   18/20 qemu:qtest+qtest-aarch64 / qtest-aarch64/boot-serial-test
>   qemu-system-aarch64: -accel tcg: invalid accelerator tcg
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 01/11] tests/qtest/boot-serial-test: Constify tests[] array

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/boot-serial-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 02/11] tests/qtest/boot-serial-test: Simplify test_machine() a bit

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Slighly modify test_machine() to simplify next commit review.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/boot-serial-test.c | 21 +++++++--------------
>   1 file changed, 7 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 03/11] tests/qtest/boot-serial-test: Build command line using GString API

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/boot-serial-test.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 04/11] tests/qtest/boot-serial-test: Only use available accelerators

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> For example, avoid when TCG is disabled:
> 
>    $ make check-qtest-aarch64
>    ...
>    18/20 qemu:qtest+qtest-aarch64 / qtest-aarch64/boot-serial-test
>    qemu-system-aarch64: -accel tcg: invalid accelerator tcg
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/boot-serial-test.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 05/11] tests/qtest/migration-test: Inverse #ifdef'ry ladders

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> This slighly simplify the logic, and eases the following conversion.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 06/11] tests/qtest/migration-test: Reduce 'cmd_source' string scope

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 07/11] tests/qtest/migration-test: Build command line using GString API (1/4)

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Part 1/4: Convert memory & machine options.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 6c3db95113..7aa323a7a7 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -582,6 +582,7 @@ typedef struct {
>   static int test_migrate_start(QTestState **from, QTestState **to,
>                                 const char *uri, MigrateStart *args)
>   {
> +    g_autoptr(GString) cmd_common = NULL;
>       g_autofree gchar *arch_source = NULL;
>       g_autofree gchar *arch_target = NULL;
>       g_autofree gchar *cmd_target = NULL;
> @@ -601,6 +602,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       }
>   
>       got_stop = false;
> +
> +    cmd_common = g_string_new("");
> +
>       bootpath = g_strdup_printf("%s/bootsect", tmpfs);
>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>           /* the assembled x86 boot sector should be exactly one sector large */
> @@ -644,6 +648,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       } else {
>           g_assert_not_reached();
>       }
> +    if (machine_opts) {
> +        g_string_append_printf(cmd_common, " -machine %s ", machine_opts);
> +    }
> +    g_string_append_printf(cmd_common, "-m %s ", memory_size);
>   
>       if (!getenv("QTEST_LOG") && args->hide_stderr) {
>   #ifdef _WIN32
> @@ -674,33 +682,27 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       if (!args->only_target) {
>           g_autofree gchar *cmd_source = NULL;
>   
> -        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
> +        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s "
>                                        "-name source,debug-threads=on "
> -                                     "-m %s "
>                                        "-serial file:%s/src_serial "
>                                        "%s %s %s %s",
>                                        args->use_dirty_ring ?
>                                        ",dirty-ring-size=4096" : "",
> -                                     machine_opts ? " -machine " : "",
> -                                     machine_opts ? machine_opts : "",

You removed two strings here, but only one %s above.


r~

> -                                     memory_size, tmpfs,
> +                                     cmd_common->str, tmpfs,
>                                        arch_source, shmem_opts,
>                                        args->opts_source ? args->opts_source : "",
>                                        ignore_stderr);
>           *from = qtest_init(cmd_source);
>       }
>   
> -    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
> +    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s "
>                                    "-name target,debug-threads=on "
> -                                 "-m %s "
>                                    "-serial file:%s/dest_serial "
>                                    "-incoming %s "
>                                    "%s %s %s %s",
>                                    args->use_dirty_ring ?
>                                    ",dirty-ring-size=4096" : "",
> -                                 machine_opts ? " -machine " : "",
> -                                 machine_opts ? machine_opts : "",
> -                                 memory_size, tmpfs, uri,
> +                                 cmd_common->str, tmpfs, uri,
>                                    arch_target, shmem_opts,
>                                    args->opts_target ? args->opts_target : "",
>                                    ignore_stderr);

Re: [PATCH v2 08/11] tests/qtest/migration-test: Build command line using GString API (2/4)

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Part 2/4: Convert shmem option.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 09/11] tests/qtest/migration-test: Build command line using GString API (3/4)

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Part 3/4: Convert accelerator options.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 8377b3976a..015b774a9e 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -603,6 +603,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       got_stop = false;
>   
>       cmd_common = g_string_new("");
> +    g_string_append_printf(cmd_common, "-accel kvm%s ",
> +                           args->use_dirty_ring ? ",dirty-ring-size=4096" : "");
> +    g_string_append(cmd_common, "-accel tcg ");

Maybe clearer as

   if (args->use_dirty_ring) {
       g_string_append(s, "-accel kvm,dirty-ring-size=4096 ");
   } else {
       g_string_append(s, "-accel kvm ");
   }

but what you have works, so,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 10/11] tests/qtest/migration-test: Build command line using GString API (4/4)

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> Part 4/4: Convert rest of options.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 50 +++++++++++++++++++-----------------
>   1 file changed, 26 insertions(+), 24 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 11/11] tests/qtest/migration-test: Only use available accelerators

[Richard Henderson]

On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
> For example, avoid when TCG is disabled:
> 
>    $ make check-qtest-aarch64
>    ...
>    20/20 qemu:qtest+qtest-aarch64 / qtest-aarch64/migration-test
>    qemu-system-aarch64: -accel tcg: invalid accelerator tcg
> 
> Reviewed-by: Dr. David Alan Gilbert<dgilbert@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 09/11] tests/qtest/migration-test: Build command line using GString API (3/4)

[Philippe Mathieu-Daudé]

On 19/1/23 22:17, Richard Henderson wrote:
> On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
>> Part 3/4: Convert accelerator options.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/migration-test.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 8377b3976a..015b774a9e 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -603,6 +603,9 @@ static int test_migrate_start(QTestState **from, 
>> QTestState **to,
>>       got_stop = false;
>>       cmd_common = g_string_new("");
>> +    g_string_append_printf(cmd_common, "-accel kvm%s ",
>> +                           args->use_dirty_ring ? 
>> ",dirty-ring-size=4096" : "");
>> +    g_string_append(cmd_common, "-accel tcg ");
> 
> Maybe clearer as
> 
>    if (args->use_dirty_ring) {
>        g_string_append(s, "-accel kvm,dirty-ring-size=4096 ");
>    } else {
>        g_string_append(s, "-accel kvm ");
>    }

Agreed, I first did that change, then went back to have simpler "one
big patch" in v1. Now since v2 splits the changes I'll do that.

Re: [PATCH v2 07/11] tests/qtest/migration-test: Build command line using GString API (1/4)

[Philippe Mathieu-Daudé]

On 19/1/23 22:10, Richard Henderson wrote:
> On 1/19/23 04:58, Philippe Mathieu-Daudé wrote:
>> Part 1/4: Convert memory & machine options.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/migration-test.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 6c3db95113..7aa323a7a7 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -582,6 +582,7 @@ typedef struct {
>>   static int test_migrate_start(QTestState **from, QTestState **to,
>>                                 const char *uri, MigrateStart *args)
>>   {
>> +    g_autoptr(GString) cmd_common = NULL;
>>       g_autofree gchar *arch_source = NULL;
>>       g_autofree gchar *arch_target = NULL;
>>       g_autofree gchar *cmd_target = NULL;
>> @@ -601,6 +602,9 @@ static int test_migrate_start(QTestState **from, 
>> QTestState **to,
>>       }
>>       got_stop = false;
>> +
>> +    cmd_common = g_string_new("");
>> +
>>       bootpath = g_strdup_printf("%s/bootsect", tmpfs);
>>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>           /* the assembled x86 boot sector should be exactly one 
>> sector large */
>> @@ -644,6 +648,10 @@ static int test_migrate_start(QTestState **from, 
>> QTestState **to,
>>       } else {
>>           g_assert_not_reached();
>>       }
>> +    if (machine_opts) {
>> +        g_string_append_printf(cmd_common, " -machine %s ", 
>> machine_opts);
>> +    }
>> +    g_string_append_printf(cmd_common, "-m %s ", memory_size);
>>       if (!getenv("QTEST_LOG") && args->hide_stderr) {
>>   #ifdef _WIN32
>> @@ -674,33 +682,27 @@ static int test_migrate_start(QTestState **from, 
>> QTestState **to,
>>       if (!args->only_target) {
>>           g_autofree gchar *cmd_source = NULL;
>> -        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
>> +        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s "
>>                                        "-name source,debug-threads=on "
>> -                                     "-m %s "
>>                                        "-serial file:%s/src_serial "
>>                                        "%s %s %s %s",
>>                                        args->use_dirty_ring ?
>>                                        ",dirty-ring-size=4096" : "",
>> -                                     machine_opts ? " -machine " : "",
>> -                                     machine_opts ? machine_opts : "",
> 
> You removed two strings here, but only one %s above.

>> -                                     memory_size, tmpfs,
>> +                                     cmd_common->str, tmpfs,

One new string is added here             ^^^^^^^^^^^^^^^.

>>                                        arch_source, shmem_opts,
>>                                        args->opts_source ? 
>> args->opts_source : "",
>>                                        ignore_stderr);
>>           *from = qtest_init(cmd_source);
>>       }
>> -    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s%s "
>> +    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s "
>>                                    "-name target,debug-threads=on "
>> -                                 "-m %s "
>>                                    "-serial file:%s/dest_serial "
>>                                    "-incoming %s "
>>                                    "%s %s %s %s",
>>                                    args->use_dirty_ring ?
>>                                    ",dirty-ring-size=4096" : "",
>> -                                 machine_opts ? " -machine " : "",
>> -                                 machine_opts ? machine_opts : "",
>> -                                 memory_size, tmpfs, uri,
>> +                                 cmd_common->str, tmpfs, uri,
>>                                    arch_target, shmem_opts,
>>                                    args->opts_target ? 
>> args->opts_target : "",
>>                                    ignore_stderr);
> 

Re: [PATCH v2 02/11] tests/qtest/boot-serial-test: Simplify test_machine() a bit

[Thomas Huth]

On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote:
> Slighly modify test_machine() to simplify next commit review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/boot-serial-test.c | 21 +++++++--------------
>   1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index 3aef3a97a9..3a854b0174 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -227,7 +227,6 @@ static void test_machine(const void *data)
>       g_autofree char *serialtmp = NULL;
>       g_autofree char *codetmp = NULL;
>       const char *codeparam = "";
> -    const uint8_t *code = NULL;
>       QTestState *qts;
>       int ser_fd;
>   
> @@ -235,21 +234,13 @@ static void test_machine(const void *data)
>       g_assert(ser_fd != -1);
>       close(ser_fd);
>   
> -    if (test->kernel) {
> -        code = test->kernel;
> -        codeparam = "-kernel";
> -    } else if (test->bios) {
> -        code = test->bios;
> -        codeparam = "-bios";
> -    }
> -
> -    if (code) {
> +    if (test->kernel || test->bios) {
>           ssize_t wlen;
>           int code_fd;
>   
>           code_fd = g_file_open_tmp("qtest-boot-serial-cXXXXXX", &codetmp, NULL);
>           g_assert(code_fd != -1);
> -        wlen = write(code_fd, code, test->codesize);
> +        wlen = write(code_fd, test->kernel ? : test->bios, test->codesize);

Just a matter of taste, but I prefer the Elvis operator without space in 
between.

>           g_assert(wlen == test->codesize);
>           close(code_fd);
>       }
> @@ -258,12 +249,14 @@ static void test_machine(const void *data)
>        * Make sure that this test uses tcg if available: It is used as a
>        * fast-enough smoketest for that.
>        */
> -    qts = qtest_initf("%s %s -M %s -no-shutdown "
> +    qts = qtest_initf("%s %s %s -M %s -no-shutdown "
>                         "-chardev file,id=serial0,path=%s "
>                         "-serial chardev:serial0 -accel tcg -accel kvm %s",
> -                      codeparam, code ? codetmp : "", test->machine,
> +                      codeparam,

You removed the initialization of codeparam, so this is now always the empty 
string, isn't it? ... please just remove it completely now.

> +                      test->kernel ? "-kernel " : test->bios ? "-bios " : "",
> +                      codetmp ? : "", test->machine,
>                         serialtmp, test->extra);
> -    if (code) {
> +    if (codetmp) {
>           unlink(codetmp);
>       }
>   

  Thomas

Re: [PATCH v2 03/11] tests/qtest/boot-serial-test: Build command line using GString API

[Thomas Huth]

On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/boot-serial-test.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index 3a854b0174..92890b409d 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -226,14 +226,17 @@ static void test_machine(const void *data)
>       const testdef_t *test = data;
>       g_autofree char *serialtmp = NULL;
>       g_autofree char *codetmp = NULL;
> -    const char *codeparam = "";
>       QTestState *qts;
>       int ser_fd;
> +    g_autoptr(GString) cmd = g_string_new("");

You could already start with the "-no-shutdown" here.

>       ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL);
>       g_assert(ser_fd != -1);
>       close(ser_fd);
>   
> +    g_string_append_printf(cmd, "-M %s ", test->machine);
> +    g_string_append(cmd, "-no-shutdown ");
> +
>       if (test->kernel || test->bios) {
>           ssize_t wlen;
>           int code_fd;
> @@ -243,19 +246,23 @@ static void test_machine(const void *data)
>           wlen = write(code_fd, test->kernel ? : test->bios, test->codesize);
>           g_assert(wlen == test->codesize);
>           close(code_fd);
> +        g_string_append_printf(cmd, "%s %s ",
> +                               test->kernel ? "-kernel " : "-bios ", codetmp);
>       }
>   
> +    g_string_append_printf(cmd, "-chardev file,id=serial0,path=%s "
> +                                "-serial chardev:serial0 ", serialtmp);

Why not include this in the initial g_string_append_printf statement already?

>       /*
>        * Make sure that this test uses tcg if available: It is used as a
>        * fast-enough smoketest for that.
>        */
> -    qts = qtest_initf("%s %s %s -M %s -no-shutdown "
> -                      "-chardev file,id=serial0,path=%s "
> -                      "-serial chardev:serial0 -accel tcg -accel kvm %s",
> -                      codeparam,
> -                      test->kernel ? "-kernel " : test->bios ? "-bios " : "",
> -                      codetmp ? : "", test->machine,
> -                      serialtmp, test->extra);
> +    g_string_append(cmd, "-accel tcg ");
> +    g_string_append(cmd, "-accel kvm ");
> +    g_string_append(cmd, test->extra);
> +
> +    qts = qtest_init(cmd->str);

... and I have to say that this is already quite a lot of code churn, just 
to get the -accel parameters tweaked in the end.

Why don't you simply do a single small patch that just replaces the "-accel 
tcg -accel" part with "%s %s" and add two parameters like this:

   has_tcg ? "-accel tcg" : "", has_kvm ? "-accel kvm" : ""

?

  Thomas

Re: [PATCH v2 06/11] tests/qtest/migration-test: Reduce 'cmd_source' string scope

[Thomas Huth]

On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/migration-test.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)

Missing explanation in the commit description. What's the benefit of doing this?

  Thomas

Re: [PATCH v2 06/11] tests/qtest/migration-test: Reduce 'cmd_source' string scope

[Philippe Mathieu-Daudé]

On 20/1/23 10:02, Thomas Huth wrote:
> On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/migration-test.c | 29 +++++++++++++++--------------
>>   1 file changed, 15 insertions(+), 14 deletions(-)
> 
> Missing explanation in the commit description. What's the benefit of 
> doing this?

Reduce the changes in the next patches to ease David's review.

Re: [PATCH v2 03/11] tests/qtest/boot-serial-test: Build command line using GString API

[Philippe Mathieu-Daudé]

On 20/1/23 09:59, Thomas Huth wrote:
> On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/boot-serial-test.c | 23 +++++++++++++++--------
>>   1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/qtest/boot-serial-test.c 
>> b/tests/qtest/boot-serial-test.c
>> index 3a854b0174..92890b409d 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -226,14 +226,17 @@ static void test_machine(const void *data)
>>       const testdef_t *test = data;
>>       g_autofree char *serialtmp = NULL;
>>       g_autofree char *codetmp = NULL;
>> -    const char *codeparam = "";
>>       QTestState *qts;
>>       int ser_fd;
>> +    g_autoptr(GString) cmd = g_string_new("");
> 
> You could already start with the "-no-shutdown" here.

It is just the matter of knowing the style of the maintainer :)
IIRC Alex prefers starting with an empty "" to avoid future churn
when adding new params (just add new line instead of modify).

I'll use your suggestion if I respin.

> 
>>       ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", 
>> &serialtmp, NULL);
>>       g_assert(ser_fd != -1);
>>       close(ser_fd);
>> +    g_string_append_printf(cmd, "-M %s ", test->machine);
>> +    g_string_append(cmd, "-no-shutdown ");
>> +
>>       if (test->kernel || test->bios) {
>>           ssize_t wlen;
>>           int code_fd;
>> @@ -243,19 +246,23 @@ static void test_machine(const void *data)
>>           wlen = write(code_fd, test->kernel ? : test->bios, 
>> test->codesize);
>>           g_assert(wlen == test->codesize);
>>           close(code_fd);
>> +        g_string_append_printf(cmd, "%s %s ",
>> +                               test->kernel ? "-kernel " : "-bios ", 
>> codetmp);
>>       }
>> +    g_string_append_printf(cmd, "-chardev file,id=serial0,path=%s "
>> +                                "-serial chardev:serial0 ", serialtmp);
> 
> Why not include this in the initial g_string_append_printf statement 
> already?
> 
>>       /*
>>        * Make sure that this test uses tcg if available: It is used as a
>>        * fast-enough smoketest for that.
>>        */
>> -    qts = qtest_initf("%s %s %s -M %s -no-shutdown "
>> -                      "-chardev file,id=serial0,path=%s "
>> -                      "-serial chardev:serial0 -accel tcg -accel kvm 
>> %s",
>> -                      codeparam,
>> -                      test->kernel ? "-kernel " : test->bios ? "-bios 
>> " : "",
>> -                      codetmp ? : "", test->machine,
>> -                      serialtmp, test->extra);
>> +    g_string_append(cmd, "-accel tcg ");
>> +    g_string_append(cmd, "-accel kvm ");
>> +    g_string_append(cmd, test->extra);
>> +
>> +    qts = qtest_init(cmd->str);
> 
> ... and I have to say that this is already quite a lot of code churn, 
> just to get the -accel parameters tweaked in the end.
> 
> Why don't you simply do a single small patch that just replaces the 
> "-accel tcg -accel" part with "%s %s" and add two parameters like this:
> 
>    has_tcg ? "-accel tcg" : "", has_kvm ? "-accel kvm" : ""

I thought it would be simpler when adding HVF support later, KISS,
but I don't mind to change.

Thanks,

Phil.