| Message ID | 20230119145838.41835-4-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests/qtest: Allow running boot-serial / migration with TCG disabled | expand |
On 1/19/23 04:58, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > tests/qtest/boot-serial-test.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > tests/qtest/boot-serial-test.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c > index 3a854b0174..92890b409d 100644 > --- a/tests/qtest/boot-serial-test.c > +++ b/tests/qtest/boot-serial-test.c > @@ -226,14 +226,17 @@ static void test_machine(const void *data) > const testdef_t *test = data; > g_autofree char *serialtmp = NULL; > g_autofree char *codetmp = NULL; > - const char *codeparam = ""; > QTestState *qts; > int ser_fd; > + g_autoptr(GString) cmd = g_string_new(""); You could already start with the "-no-shutdown" here. > ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL); > g_assert(ser_fd != -1); > close(ser_fd); > > + g_string_append_printf(cmd, "-M %s ", test->machine); > + g_string_append(cmd, "-no-shutdown "); > + > if (test->kernel || test->bios) { > ssize_t wlen; > int code_fd; > @@ -243,19 +246,23 @@ static void test_machine(const void *data) > wlen = write(code_fd, test->kernel ? : test->bios, test->codesize); > g_assert(wlen == test->codesize); > close(code_fd); > + g_string_append_printf(cmd, "%s %s ", > + test->kernel ? "-kernel " : "-bios ", codetmp); > } > > + g_string_append_printf(cmd, "-chardev file,id=serial0,path=%s " > + "-serial chardev:serial0 ", serialtmp); Why not include this in the initial g_string_append_printf statement already? > /* > * Make sure that this test uses tcg if available: It is used as a > * fast-enough smoketest for that. > */ > - qts = qtest_initf("%s %s %s -M %s -no-shutdown " > - "-chardev file,id=serial0,path=%s " > - "-serial chardev:serial0 -accel tcg -accel kvm %s", > - codeparam, > - test->kernel ? "-kernel " : test->bios ? "-bios " : "", > - codetmp ? : "", test->machine, > - serialtmp, test->extra); > + g_string_append(cmd, "-accel tcg "); > + g_string_append(cmd, "-accel kvm "); > + g_string_append(cmd, test->extra); > + > + qts = qtest_init(cmd->str); ... and I have to say that this is already quite a lot of code churn, just to get the -accel parameters tweaked in the end. Why don't you simply do a single small patch that just replaces the "-accel tcg -accel" part with "%s %s" and add two parameters like this: has_tcg ? "-accel tcg" : "", has_kvm ? "-accel kvm" : "" ? Thomas
On 20/1/23 09:59, Thomas Huth wrote: > On 19/01/2023 15.58, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> tests/qtest/boot-serial-test.c | 23 +++++++++++++++-------- >> 1 file changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/tests/qtest/boot-serial-test.c >> b/tests/qtest/boot-serial-test.c >> index 3a854b0174..92890b409d 100644 >> --- a/tests/qtest/boot-serial-test.c >> +++ b/tests/qtest/boot-serial-test.c >> @@ -226,14 +226,17 @@ static void test_machine(const void *data) >> const testdef_t *test = data; >> g_autofree char *serialtmp = NULL; >> g_autofree char *codetmp = NULL; >> - const char *codeparam = ""; >> QTestState *qts; >> int ser_fd; >> + g_autoptr(GString) cmd = g_string_new(""); > > You could already start with the "-no-shutdown" here. It is just the matter of knowing the style of the maintainer :) IIRC Alex prefers starting with an empty "" to avoid future churn when adding new params (just add new line instead of modify). I'll use your suggestion if I respin. > >> ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", >> &serialtmp, NULL); >> g_assert(ser_fd != -1); >> close(ser_fd); >> + g_string_append_printf(cmd, "-M %s ", test->machine); >> + g_string_append(cmd, "-no-shutdown "); >> + >> if (test->kernel || test->bios) { >> ssize_t wlen; >> int code_fd; >> @@ -243,19 +246,23 @@ static void test_machine(const void *data) >> wlen = write(code_fd, test->kernel ? : test->bios, >> test->codesize); >> g_assert(wlen == test->codesize); >> close(code_fd); >> + g_string_append_printf(cmd, "%s %s ", >> + test->kernel ? "-kernel " : "-bios ", >> codetmp); >> } >> + g_string_append_printf(cmd, "-chardev file,id=serial0,path=%s " >> + "-serial chardev:serial0 ", serialtmp); > > Why not include this in the initial g_string_append_printf statement > already? > >> /* >> * Make sure that this test uses tcg if available: It is used as a >> * fast-enough smoketest for that. >> */ >> - qts = qtest_initf("%s %s %s -M %s -no-shutdown " >> - "-chardev file,id=serial0,path=%s " >> - "-serial chardev:serial0 -accel tcg -accel kvm >> %s", >> - codeparam, >> - test->kernel ? "-kernel " : test->bios ? "-bios >> " : "", >> - codetmp ? : "", test->machine, >> - serialtmp, test->extra); >> + g_string_append(cmd, "-accel tcg "); >> + g_string_append(cmd, "-accel kvm "); >> + g_string_append(cmd, test->extra); >> + >> + qts = qtest_init(cmd->str); > > ... and I have to say that this is already quite a lot of code churn, > just to get the -accel parameters tweaked in the end. > > Why don't you simply do a single small patch that just replaces the > "-accel tcg -accel" part with "%s %s" and add two parameters like this: > > has_tcg ? "-accel tcg" : "", has_kvm ? "-accel kvm" : "" I thought it would be simpler when adding HVF support later, KISS, but I don't mind to change. Thanks, Phil.
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c index 3a854b0174..92890b409d 100644 --- a/tests/qtest/boot-serial-test.c +++ b/tests/qtest/boot-serial-test.c @@ -226,14 +226,17 @@ static void test_machine(const void *data) const testdef_t *test = data; g_autofree char *serialtmp = NULL; g_autofree char *codetmp = NULL; - const char *codeparam = ""; QTestState *qts; int ser_fd; + g_autoptr(GString) cmd = g_string_new(""); ser_fd = g_file_open_tmp("qtest-boot-serial-sXXXXXX", &serialtmp, NULL); g_assert(ser_fd != -1); close(ser_fd); + g_string_append_printf(cmd, "-M %s ", test->machine); + g_string_append(cmd, "-no-shutdown "); + if (test->kernel || test->bios) { ssize_t wlen; int code_fd; @@ -243,19 +246,23 @@ static void test_machine(const void *data) wlen = write(code_fd, test->kernel ? : test->bios, test->codesize); g_assert(wlen == test->codesize); close(code_fd); + g_string_append_printf(cmd, "%s %s ", + test->kernel ? "-kernel " : "-bios ", codetmp); } + g_string_append_printf(cmd, "-chardev file,id=serial0,path=%s " + "-serial chardev:serial0 ", serialtmp); + /* * Make sure that this test uses tcg if available: It is used as a * fast-enough smoketest for that. */ - qts = qtest_initf("%s %s %s -M %s -no-shutdown " - "-chardev file,id=serial0,path=%s " - "-serial chardev:serial0 -accel tcg -accel kvm %s", - codeparam, - test->kernel ? "-kernel " : test->bios ? "-bios " : "", - codetmp ? : "", test->machine, - serialtmp, test->extra); + g_string_append(cmd, "-accel tcg "); + g_string_append(cmd, "-accel kvm "); + g_string_append(cmd, test->extra); + + qts = qtest_init(cmd->str); + if (codetmp) { unlink(codetmp); }
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- tests/qtest/boot-serial-test.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)