qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2 03/11] tests/qtest/boot-serial-test: Build command line using GString API
Date: Fri, 20 Jan 2023 12:04:15 +0100
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.6.1

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]