Re: [PATCH v3 06/11] tests/qtest/migration-test: Reduce 'cmd_source' string scope

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 30/01/2023 05.44, Juan Quintela wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> I am assuming that you will pull this patches through tests tree,
>> not
>> migration tree.
>> Otherwise, let me know.
>
> I had some remarks (in v2 of the series), so I'm not going to pick
> this up (yet). If you want to take the migration part, feel free to do
> so.
>
> I still think it's quite a lot of code churn for just supporting
> multiple "-accel" statements here.
>
> What about introducing a new lib functions like this:
>
> char *qtest_get_accel_params(bool use_tcg_first)
> {
>     bool tcg = qtest_has_accel("tcg");
>
>     return g_strdup_printf("%s %s %s %s",
>             use_tcg_first && tcg   ? "-accel tcg" : "",
>             qtest_has_accel("kvm") ? "-accel kvm" : "",
>             qtest_has_accel("hvf") ? "-accel hvf" : "",
>             !use_tcg_first && tcg  ? "-accel tcg" : "");

I know that it is me, but I don't find the ? operator especially
readable.

What about:

GString *s = g_string_new("");
bool tcg = qtest_has_accel("tcg");

if (use_tcg_first && tcg)
   g_string_append(s, "-accel tcg ");
if (qtest_has_accel("kvm"))
   g_string_append(s, "-accel kvm ");
if (qtest_has_accel("hvf"))
   g_string_append(s, "-accel hvf ");
if (!use_tcg_first && tcg)
   g_string_append(s, "-accel tcg");

return s;

Yes, in the function that Phillipe is changing now, each time that I
have to change it, I have to count to see what and where I need to put
the format/change/whatever.


> }
>
> ... then all tests that want to run real code could simply call this
> function instead of having to deal with the list of supported
> accelerators again and again?

It is ok with me.

Later, Juan.