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.