diff mbox series

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

Message ID 20230120082341.59913-7-philmd@linaro.org
State New
Headers show
Series tests/qtest: Allow running boot-serial / migration with TCG disabled | expand

Commit Message

Philippe Mathieu-Daudé Jan. 20, 2023, 8:23 a.m. UTC 
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/migration-test.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Juan Quintela Jan. 30, 2023, 4:44 a.m. UTC | #1 
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.
Thomas Huth Jan. 31, 2023, 2:23 p.m. UTC | #2 
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" : "");
}

... 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?

  Thomas
Juan Quintela Feb. 1, 2023, 1:07 p.m. UTC | #3 
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.
diff mbox series

Patch

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 5271ddb868..f96c73f552 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -584,7 +584,6 @@  static int test_migrate_start(QTestState **from, QTestState **to,
 {
     g_autofree gchar *arch_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_autofree char *bootpath = NULL;
@@ -672,20 +671,22 @@  static int test_migrate_start(QTestState **from, QTestState **to,
         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) {
+        g_autofree gchar *cmd_source = NULL;
+
+        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);
         *from = qtest_init(cmd_source);
     }