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

[Claudio Fontana <cfontana@suse.de>]

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.

Thanks,

Claudio