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

[Thomas Huth <thuth@redhat.com>]

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