Re: [PATCH v2 01/10] migration-test: Create cmd_soure and cmd_target

[Thomas Huth]

On 18/12/2019 02.55, Juan Quintela wrote:
> We are repeating almost everything for each machine while creating the
> command line for migration.  And once for source and another for
> destination.  We start putting there opts_src and opts_dst.
> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  tests/migration-test.c | 44 ++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index a5343fdc66..fbddcf2317 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -557,6 +557,7 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>                                 const char *opts_dst)
>  {
>      gchar *cmd_src, *cmd_dst;
> +    gchar *cmd_source, *cmd_target;

The naming looks quite unfortunate to me ... "cmd_src" can easily be
mixed up with "cmd_source" ... but maybe you could do it without these
additional variables (see below) ...

[...]
> @@ -671,11 +671,17 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>          cmd_dst = tmp;
>      }
>  
> -    *from = qtest_init(cmd_src);
> +    cmd_source = g_strdup_printf("%s %s",
> +                                 cmd_src, opts_src);
>      g_free(cmd_src);
> +    *from = qtest_init(cmd_source);
> +    g_free(cmd_source);
>  
> -    *to = qtest_init(cmd_dst);
> +    cmd_target = g_strdup_printf("%s %s",
> +                                 cmd_dst, opts_dst);
>      g_free(cmd_dst);
> +    *to = qtest_init(cmd_target);
> +    g_free(cmd_target);

May I suggest to qtest_initf() here instead:

  *from = qtest_initf("%s %s", cmd_src, opts_src);

  *to = qtest_initf("%s %s", cmd_dst, opts_dst);


And maybe you could even move the extra_opts here, too? e.g.:

  *from = qtest_initf("%s %s %s", cmd_src, extra_opts ?: "", opts_src);

  *to = qtest_initf("%s %s %s", cmd_dst,  extra_opts ?: "", opts_dst);

 Thomas