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

[Juan Quintela]

Thomas Huth <address@hidden> wrote:
> 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) ...

[...]

> 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

I do that on later patches.  But the _final_ better name that I could
get with was "cmd_source".  cmd_src ends being arch_source.

About using qtest_initf():

- I didn't knew it's existence (O:-)
- I was considering about merning the command parts of cmd_source/target
  But arrived to the conclusion that it was more complicated to have it
  share it, that to repeat it.  But you need to look at the last patch
  to arrive to one conclusion.

Thanks, Juan.