Re: [PATCH 4/8] tests/qtest/migration-test: Build command line using GString API

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Philippe Mathieu-Daudé (philmd@linaro.org) wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  tests/qtest/migration-test.c | 85 ++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 43 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index dbde726adf..36e6074653 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -582,13 +582,13 @@ typedef struct {
>  static int test_migrate_start(QTestState **from, QTestState **to,
>                                const char *uri, MigrateStart *args)
>  {

bit of a big change with lots of things moving around, I think it's
mostly OK but...

> +    g_autoptr(GString) cmd_common = NULL;
>      g_autofree gchar *arch_source = NULL;
> +    g_autoptr(GString) cmd_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_autoptr(GString) cmd_target = NULL;
> +    const gchar *ignore_stderr = NULL;
>      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;
> @@ -602,6 +602,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>      }
>  
>      got_stop = false;
> +
> +    cmd_common = g_string_new("");
> +    g_string_append(cmd_common, "-accel tcg ");
> +    g_string_append_printf(cmd_common, "-accel kvm%s ",
> +                           args->use_dirty_ring ? ",dirty-ring-size=4096" : "");
> +

Isn't that swapping the order of -accel tcg and -accel kvm ?
In the original it's
                    g_strdup_printf("-accel kvm%s -accel tcg%s%s "

I think you're ending up with tcg first?

Dave

>      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 */
> @@ -645,65 +651,58 @@ 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) {
> -#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 = "";
>      }
>  
>      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("");
>      }
>  
> -    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) {
> -        *from = qtest_init(cmd_source);
> +        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);
> +        }
> +        *from = qtest_init(cmd_source->str);
>      }
>  
> -    cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s%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,
> -                                 arch_target, shmem_opts,
> -                                 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_source, ignore_stderr);
> +    }
> +    *to = qtest_init(cmd_target->str);
>  
>      /*
>       * Remove shmem file immediately to avoid memory leak in test failed case.
> -- 
> 2.38.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK