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)) {
         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)
-- 
2.46.0

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>