| Message ID | 20240905191434.694440-4-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow check-qtest with "--without-default-devices" | expand |
Hi Thomas, On 5/9/24 21:14, Thomas Huth wrote: > Let's make sure that we always pass a machine name to the test_boot_orders() > function, so we can check whether the machine is available in the binary > and skip the test in case it is not included in the build. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/qtest/boot-order-test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/qtest/boot-order-test.c b/tests/qtest/boot-order-test.c > index 8f2b6ef05a..c67b8cfe16 100644 > --- a/tests/qtest/boot-order-test.c > +++ b/tests/qtest/boot-order-test.c > @@ -31,7 +31,7 @@ static void test_a_boot_order(const char *machine, > uint64_t actual; > QTestState *qts; > > - if (machine && !qtest_has_machine(machine)) { > + if (!qtest_has_machine(machine)) { Should we defer the NULL check to qtest_has_machine_with_env()? It uses g_str_equal() which is described as: Note that this function is primarily meant as a hash table comparison function. For a general-purpose, NULL-safe string comparison function, see g_strcmp0(). Better switch to g_strcmp0() in qtest_has_machine_with_env()? > g_test_skip("Machine is not available"); > return; > } > @@ -107,7 +107,7 @@ static const boot_order_test test_cases_pc[] = { > > static void test_pc_boot_order(void) > { > - test_boot_orders(NULL, read_boot_order_pc, test_cases_pc); > + test_boot_orders("pc", read_boot_order_pc, test_cases_pc); > } > > static uint64_t read_boot_order_pmac(QTestState *qts)
On 06/09/2024 09.59, Philippe Mathieu-Daudé wrote: > Hi Thomas, > > On 5/9/24 21:14, Thomas Huth wrote: >> Let's make sure that we always pass a machine name to the test_boot_orders() >> function, so we can check whether the machine is available in the binary >> and skip the test in case it is not included in the build. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> tests/qtest/boot-order-test.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tests/qtest/boot-order-test.c b/tests/qtest/boot-order-test.c >> index 8f2b6ef05a..c67b8cfe16 100644 >> --- a/tests/qtest/boot-order-test.c >> +++ b/tests/qtest/boot-order-test.c >> @@ -31,7 +31,7 @@ static void test_a_boot_order(const char *machine, >> uint64_t actual; >> QTestState *qts; >> - if (machine && !qtest_has_machine(machine)) { >> + if (!qtest_has_machine(machine)) { > > Should we defer the NULL check to qtest_has_machine_with_env()? > It uses g_str_equal() which is described as: > > Note that this function is primarily meant as a hash table > comparison function. For a general-purpose, NULL-safe string > comparison function, see g_strcmp0(). > > Better switch to g_strcmp0() in qtest_has_machine_with_env()? What would be the intended meaning of the function when it is called with "NULL" ? Use the default machine? Skip the test? ... I think it is rather an error to call it with NULL, so it's OK if the test crashes here since it should never happen? Thomas
On 6/9/24 10:04, Thomas Huth wrote: > On 06/09/2024 09.59, Philippe Mathieu-Daudé wrote: >> Hi Thomas, >> >> On 5/9/24 21:14, Thomas Huth wrote: >>> Let's make sure that we always pass a machine name to the >>> test_boot_orders() >>> function, so we can check whether the machine is available in the binary >>> and skip the test in case it is not included in the build. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> tests/qtest/boot-order-test.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/tests/qtest/boot-order-test.c >>> b/tests/qtest/boot-order-test.c >>> index 8f2b6ef05a..c67b8cfe16 100644 >>> --- a/tests/qtest/boot-order-test.c >>> +++ b/tests/qtest/boot-order-test.c >>> @@ -31,7 +31,7 @@ static void test_a_boot_order(const char *machine, >>> uint64_t actual; >>> QTestState *qts; >>> - if (machine && !qtest_has_machine(machine)) { >>> + if (!qtest_has_machine(machine)) { >> >> Should we defer the NULL check to qtest_has_machine_with_env()? >> It uses g_str_equal() which is described as: >> >> Note that this function is primarily meant as a hash table >> comparison function. For a general-purpose, NULL-safe string >> comparison function, see g_strcmp0(). >> >> Better switch to g_strcmp0() in qtest_has_machine_with_env()? > > What would be the intended meaning of the function when it is called > with "NULL" ? Use the default machine? Skip the test? ... I think it is > rather an error to call it with NULL, so it's OK if the test crashes > here since it should never happen? Right. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/tests/qtest/boot-order-test.c b/tests/qtest/boot-order-test.c index 8f2b6ef05a..c67b8cfe16 100644 --- a/tests/qtest/boot-order-test.c +++ b/tests/qtest/boot-order-test.c @@ -31,7 +31,7 @@ static void test_a_boot_order(const char *machine, uint64_t actual; QTestState *qts; - if (machine && !qtest_has_machine(machine)) { + if (!qtest_has_machine(machine)) { g_test_skip("Machine is not available"); return; } @@ -107,7 +107,7 @@ static const boot_order_test test_cases_pc[] = { static void test_pc_boot_order(void) { - test_boot_orders(NULL, read_boot_order_pc, test_cases_pc); + test_boot_orders("pc", read_boot_order_pc, test_cases_pc); } static uint64_t read_boot_order_pmac(QTestState *qts)
Let's make sure that we always pass a machine name to the test_boot_orders() function, so we can check whether the machine is available in the binary and skip the test in case it is not included in the build. Signed-off-by: Thomas Huth <thuth@redhat.com> --- tests/qtest/boot-order-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)