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

[Alex Bennée]

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?

  /*
   * 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


-- 
Alex Bennée