Re: [RFC v14 52/80] tests: device-introspect-test: cope with ARM TCG-only devices

[Claudio Fontana]

On 4/20/21 11:34 AM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 4/19/21 12:29 PM, Thomas Huth wrote:
>>> On 19/04/2021 12.24, Claudio Fontana wrote:
>>>> On 4/19/21 12:22 PM, Thomas Huth wrote:
>>>>> On 16/04/2021 18.27, Claudio Fontana wrote:
>>>>>> Skip the test_device_intro_concrete for now for ARM KVM-only build,
>>>>>> as on ARM we currently build devices for ARM that are not
>>>>>> compatible with a KVM-only build.
>>>>>>
>>>>>> We can remove this workaround when we fix this in KConfig etc,
>>>>>> and we only list and build machines that are compatible with KVM
>>>>>> for KVM-only builds.
>>>>>>
>>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> ---
>>>>>>    tests/qtest/device-introspect-test.c | 18 ++++++++++++++++++
>>>>>>    1 file changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/tests/qtest/device-introspect-test.c 
>>>>>> b/tests/qtest/device-introspect-test.c
>>>>>> index bbec166dbc..1ff15e2247 100644
>>>>>> --- a/tests/qtest/device-introspect-test.c
>>>>>> +++ b/tests/qtest/device-introspect-test.c
>>>>>> @@ -329,12 +329,30 @@ int main(int argc, char **argv)
>>>>>>        qtest_add_func("device/introspect/none", test_device_intro_none);
>>>>>>        qtest_add_func("device/introspect/abstract", 
>>>>>> test_device_intro_abstract);
>>>>>>        qtest_add_func("device/introspect/abstract-interfaces", 
>>>>>> test_abstract_interfaces);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * XXX currently we build also boards for ARM that are incompatible 
>>>>>> with KVM.
>>>>>> +     * We therefore need to check this explicitly, and only test virt 
>>>>>> for kvm-only
>>>>>> +     * arm builds.
>>>>>> +     * After we do the work of Kconfig etc to ensure that only 
>>>>>> KVM-compatible boards
>>>>>> +     * are built for the kvm-only build, we could remove this.
>>>>>> +     */
>>>>>> +#ifndef CONFIG_TCG
>>>>>> +    {
>>>>>> +        const char *arch = qtest_get_arch();
>>>>>> +        if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) {
>>>>>> +            goto add_machine_test_done;
>>>>>> +        }
>>>>>> +    }
>>>>>> +#endif /* !CONFIG_TCG */
>>>>>>        if (g_test_quick()) {
>>>>>>            
>>>>>> qtest_add_data_func("device/introspect/concrete/defaults/none",
>>>>>>                                g_strdup(common_args), 
>>>>>> test_device_intro_concrete);
>>>>>>        } else {
>>>>>>            qtest_cb_for_every_machine(add_machine_test_case, true);
>>>>>>        }
>>>>>> +    goto add_machine_test_done;
>>>>>
>>>>> That last goto does not make much sense, since the label is right below.
>>>>
>>>> It is necessary to remove the warning that is otherwise produced about the 
>>>> unused label IIRC.
>>>
>>> Ah, ok.
>>>
>>> Alternatively, you could maybe also drop the label completely and simply 
>>> write the #ifndef block above like this (note the "else"):
>>>
>>> #ifndef CONFIG_TCG
>>>      const char *arch = qtest_get_arch();
>>>      if (!strcmp(arch, "arm") || !strcmp(arch, "aarch64")) {
>>>          /* Do nothing */
>>>      }
>>>      else
>>> #endif /* !CONFIG_TCG */
>>>
>>> ... not sure what's nicer, though.
>>>
>>>   Thomas
>>>
>>
>> Indeed, I tried a couple of combinations, in the end to me the less 
>> confusing was the goto one,
>> the #ifdef containing an open else is in my opinion worse, more
>> error-prone, but I am open to additional comments/ideas.
> 
> Surely a helper makes intent clearer?

Yes, thank you :-)

Ciao,

Claudio

> 
>   /*
>    * XXX currently we build also boards for ARM that are incompatible with 
> KVM.
>    * We therefore need to check this explicitly, and only test virt for 
> kvm-only
>    * arm builds.
>    * After we do the work of Kconfig etc to ensure that only KVM-compatible 
> boards
>    * are built for the kvm-only build, we could remove this.
>    */
>   static bool skip_machine_tests(void)
>   {
>   #ifndef CONFIG_TCG
>       const char *arch = qtest_get_arch();
>       if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) {
>           return true;
>       }
>   #endif /* !CONFIG_TCG */
>       return false;
>   }
> 
> 
>   int main(int argc, char **argv)
>   {
>       g_test_init(&argc, &argv, NULL);
> 
>       qtest_add_func("device/introspect/list", test_device_intro_list);
>       qtest_add_func("device/introspect/list-fields", test_qom_list_fields);
>       qtest_add_func("device/introspect/none", test_device_intro_none);
>       qtest_add_func("device/introspect/abstract", 
> test_device_intro_abstract);
>       qtest_add_func("device/introspect/abstract-interfaces", 
> test_abstract_interfaces);
> 
>       if (!skip_machine_tests()) {
>           if (g_test_quick()) {
>               qtest_add_data_func("device/introspect/concrete/defaults/none",
>                                   g_strdup(common_args), 
> test_device_intro_concrete);
>           } else {
>               qtest_cb_for_every_machine(add_machine_test_case, true);
>           }
>       }
> 
>       return g_test_run();
>   }
> 
> 
>>
>> Thanks,
>>
>> Claudio
> 
>