diff mbox series

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

Message ID 20230119145838.41835-4-philmd@linaro.org
State Superseded
Headers show
Series tests/qtest: Allow running boot-serial / migration with TCG disabled | expand

Commit Message

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

Comments

Richard Henderson Jan. 19, 2023, 9:03 p.m. UTC | #1
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~
Thomas Huth Jan. 20, 2023, 8:59 a.m. UTC | #2
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
Philippe Mathieu-Daudé Jan. 20, 2023, 11:04 a.m. UTC | #3
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.
diff mbox series

Patch

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);
     }