Now than we can probe if the TCG accelerator is available
at runtime with a QMP command, do it once at the beginning
and only register the tests we can run.
We can then replace the #ifdef'ry by a runtime check.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d0339..8902d2f169f 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -21,19 +21,24 @@ const char common_args[] = "-nodefaults -machine none";
 
 /* Query smoke tests */
 
+static bool tcg_accel_available;
+
 static int query_error_class(const char *cmd)
 {
-    static struct {
+    static const struct {
         const char *cmd;
         int err_class;
+        /*
+         * Pointer to boolean.
+         * If non-NULL and value is %true, the error class is skipped.
+         */
+        bool *skip_err_class;
     } fails[] = {
         /* Success depends on build configuration: */
 #ifndef CONFIG_SPICE
         { "query-spice", ERROR_CLASS_COMMAND_NOT_FOUND },
 #endif
-#ifndef CONFIG_TCG
-        { "query-replay", ERROR_CLASS_COMMAND_NOT_FOUND },
-#endif
+        { "query-replay", ERROR_CLASS_COMMAND_NOT_FOUND, &tcg_accel_available },
 #ifndef CONFIG_VNC
         { "query-vnc", ERROR_CLASS_GENERIC_ERROR },
         { "query-vnc-servers", ERROR_CLASS_GENERIC_ERROR },
@@ -51,6 +56,9 @@ static int query_error_class(const char *cmd)
     int i;
 
     for (i = 0; fails[i].cmd; i++) {
+        if (fails[i].skip_err_class && *fails[i].skip_err_class) {
+            continue;
+        }
         if (!strcmp(cmd, fails[i].cmd)) {
             return fails[i].err_class;
         }
@@ -334,6 +342,8 @@ int main(int argc, char *argv[])
     QmpSchema schema;
     int ret;
 
+    tcg_accel_available = qtest_has_accel("tcg");
+
     g_test_init(&argc, &argv, NULL);
 
     qmp_schema_init(&schema);
-- 
2.26.3

Hi Thomas, Markus, do you mind reviewing this one please?

On 4/15/21 6:33 PM, Philippe Mathieu-Daudé wrote:
> Now than we can probe if the TCG accelerator is available
> at runtime with a QMP command, do it once at the beginning
> and only register the tests we can run.
> We can then replace the #ifdef'ry by a runtime check.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
> index c98b78d0339..8902d2f169f 100644
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -21,19 +21,24 @@ const char common_args[] = "-nodefaults -machine none";
>  
>  /* Query smoke tests */
>  
> +static bool tcg_accel_available;
> +
>  static int query_error_class(const char *cmd)
>  {
> -    static struct {
> +    static const struct {
>          const char *cmd;
>          int err_class;
> +        /*
> +         * Pointer to boolean.
> +         * If non-NULL and value is %true, the error class is skipped.
> +         */
> +        bool *skip_err_class;
>      } fails[] = {
>          /* Success depends on build configuration: */
>  #ifndef CONFIG_SPICE
>          { "query-spice", ERROR_CLASS_COMMAND_NOT_FOUND },
>  #endif
> -#ifndef CONFIG_TCG
> -        { "query-replay", ERROR_CLASS_COMMAND_NOT_FOUND },
> -#endif
> +        { "query-replay", ERROR_CLASS_COMMAND_NOT_FOUND, &tcg_accel_available },
>  #ifndef CONFIG_VNC
>          { "query-vnc", ERROR_CLASS_GENERIC_ERROR },
>          { "query-vnc-servers", ERROR_CLASS_GENERIC_ERROR },
> @@ -51,6 +56,9 @@ static int query_error_class(const char *cmd)
>      int i;
>  
>      for (i = 0; fails[i].cmd; i++) {
> +        if (fails[i].skip_err_class && *fails[i].skip_err_class) {
> +            continue;
> +        }
>          if (!strcmp(cmd, fails[i].cmd)) {
>              return fails[i].err_class;
>          }
> @@ -334,6 +342,8 @@ int main(int argc, char *argv[])
>      QmpSchema schema;
>      int ret;
>  
> +    tcg_accel_available = qtest_has_accel("tcg");
> +
>      g_test_init(&argc, &argv, NULL);
>  
>      qmp_schema_init(&schema);
> 

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Now than we can probe if the TCG accelerator is available
> at runtime with a QMP command, do it once at the beginning
> and only register the tests we can run.
> We can then replace the #ifdef'ry by a runtime check.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Please read the last remark first.  The other ones are detail; feel free
to skip them until we're done with the last one.

> ---
>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
> index c98b78d0339..8902d2f169f 100644
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -21,19 +21,24 @@ const char common_args[] = "-nodefaults -machine none";
>  
>  /* Query smoke tests */
>  
> +static bool tcg_accel_available;
> +
>  static int query_error_class(const char *cmd)
>  {
> -    static struct {
> +    static const struct {
>          const char *cmd;
>          int err_class;
> +        /*
> +         * Pointer to boolean.

Let's not paraphrase code in comments.

> +         * If non-NULL and value is %true, the error class is skipped.

Suggest "When it points to true, the test case is skipped", and ...

> +         */
> +        bool *skip_err_class;

... name this just @skip.  Or maybe @skip_ptr, because fails[i].skip
reads as if the test case was to be skipped.

>      } fails[] = {
>          /* Success depends on build configuration: */

Note the headline ^^^

>  #ifndef CONFIG_SPICE
>          { "query-spice", ERROR_CLASS_COMMAND_NOT_FOUND },
>  #endif
> -#ifndef CONFIG_TCG
> -        { "query-replay", ERROR_CLASS_COMMAND_NOT_FOUND },
> -#endif
> +        { "query-replay", ERROR_CLASS_COMMAND_NOT_FOUND, &tcg_accel_available },

No longer fits under the headline.  Move it under its own headline,
perhaps

           /* Success depends on dynamic state as reported by other queries */

>  #ifndef CONFIG_VNC
>          { "query-vnc", ERROR_CLASS_GENERIC_ERROR },
>          { "query-vnc-servers", ERROR_CLASS_GENERIC_ERROR },
> @@ -51,6 +56,9 @@ static int query_error_class(const char *cmd)
>      int i;
>  
>      for (i = 0; fails[i].cmd; i++) {
> +        if (fails[i].skip_err_class && *fails[i].skip_err_class) {
> +            continue;
> +        }
>          if (!strcmp(cmd, fails[i].cmd)) {
>              return fails[i].err_class;
>          }
> @@ -334,6 +342,8 @@ int main(int argc, char *argv[])
>      QmpSchema schema;
>      int ret;
>  
> +    tcg_accel_available = qtest_has_accel("tcg");
> +

When does tcg_accel_available differ from defined(CONFIG_TCG)?

>      g_test_init(&argc, &argv, NULL);
>  
>      qmp_schema_init(&schema);

On 4/29/21 7:43 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> Now than we can probe if the TCG accelerator is available
>> at runtime with a QMP command, do it once at the beginning
>> and only register the tests we can run.
>> We can then replace the #ifdef'ry by a runtime check.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Please read the last remark first.  The other ones are detail; feel free
> to skip them until we're done with the last one.
> 
>> ---
>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)

>> +    tcg_accel_available = qtest_has_accel("tcg");
>> +
> 
> When does tcg_accel_available differ from defined(CONFIG_TCG)?

qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
is build-time.

Having tests doing runtime checking allow to:

- have easier reviews, having less #ifdef'ry
- build them once for all targets
- build them once for all ./configure options
  (thinking about CI, the a single job could build the tests, then
  we run them using the QEMU binaries from other jobs)
- use the same binaries to test the built binary and the distribution
  installed one
- remove the dependencies between tests and binaries

> 
>>      g_test_init(&argc, &argv, NULL);
>>  
>>      qmp_schema_init(&schema);
> 

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/29/21 7:43 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> Now than we can probe if the TCG accelerator is available
>>> at runtime with a QMP command, do it once at the beginning
>>> and only register the tests we can run.
>>> We can then replace the #ifdef'ry by a runtime check.
>>>
>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> 
>> Please read the last remark first.  The other ones are detail; feel free
>> to skip them until we're done with the last one.
>> 
>>> ---
>>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>
>>> +    tcg_accel_available = qtest_has_accel("tcg");
>>> +
>> 
>> When does tcg_accel_available differ from defined(CONFIG_TCG)?
>
> qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
> is build-time.

Let me rephrase my question: under what conditions can the values of
qtest_has_accel("tcg") and defined(CONFIG_TCG) differ?

> Having tests doing runtime checking allow to:
>
> - have easier reviews, having less #ifdef'ry
> - build them once for all targets
> - build them once for all ./configure options
>   (thinking about CI, the a single job could build the tests, then
>   we run them using the QEMU binaries from other jobs)
> - use the same binaries to test the built binary and the distribution
>   installed one
> - remove the dependencies between tests and binaries
>
>> 
>>>      g_test_init(&argc, &argv, NULL);
>>>  
>>>      qmp_schema_init(&schema);
>> 

On 4/29/21 3:22 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>> Now than we can probe if the TCG accelerator is available
>>>> at runtime with a QMP command, do it once at the beginning
>>>> and only register the tests we can run.
>>>> We can then replace the #ifdef'ry by a runtime check.
>>>>
>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>> Please read the last remark first.  The other ones are detail; feel free
>>> to skip them until we're done with the last one.
>>>
>>>> ---
>>>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>>>> +    tcg_accel_available = qtest_has_accel("tcg");
>>>> +
>>>
>>> When does tcg_accel_available differ from defined(CONFIG_TCG)?
>>
>> qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
>> is build-time.
> 
> Let me rephrase my question: under what conditions can the values of
> qtest_has_accel("tcg") and defined(CONFIG_TCG) differ?

They are currently the same, so this patch is a no-op. *But* it
allows us to remove the global dependence on CONFIG_TCG in the
Meson machinery (see the last commit in this series).

Is that missing part of the commit description?

"This will allow us to remove the global CONFIG_TCG dependency
in our Meson machinery in a pair of commits."?

>> Having tests doing runtime checking allow to:
>>
>> - have easier reviews, having less #ifdef'ry
>> - build them once for all targets
>> - build them once for all ./configure options
>>   (thinking about CI, the a single job could build the tests, then
>>   we run them using the QEMU binaries from other jobs)
>> - use the same binaries to test the built binary and the distribution
>>   installed one
>> - remove the dependencies between tests and binaries
>>
>>>
>>>>      g_test_init(&argc, &argv, NULL);
>>>>  
>>>>      qmp_schema_init(&schema);
>>>
> 

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/29/21 3:22 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>>> Now than we can probe if the TCG accelerator is available
>>>>> at runtime with a QMP command, do it once at the beginning
>>>>> and only register the tests we can run.
>>>>> We can then replace the #ifdef'ry by a runtime check.
>>>>>
>>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>
>>>> Please read the last remark first.  The other ones are detail; feel free
>>>> to skip them until we're done with the last one.
>>>>
>>>>> ---
>>>>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>>>> +    tcg_accel_available = qtest_has_accel("tcg");
>>>>> +
>>>>
>>>> When does tcg_accel_available differ from defined(CONFIG_TCG)?
>>>
>>> qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
>>> is build-time.
>> 
>> Let me rephrase my question: under what conditions can the values of
>> qtest_has_accel("tcg") and defined(CONFIG_TCG) differ?
>
> They are currently the same, so this patch is a no-op. *But* it
> allows us to remove the global dependence on CONFIG_TCG in the
> Meson machinery (see the last commit in this series).
>
> Is that missing part of the commit description?
>
> "This will allow us to remove the global CONFIG_TCG dependency
> in our Meson machinery in a pair of commits."?

Do you mean "in the next two commits"?

Please also note that the probing at run-time always gives the same
result as the compile-time check it replaces.

I don't understand what exactly the conversion to probing enables and
how, but I believe I don't have to.

[...]

On 4/30/21 8:13 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> On 4/29/21 3:22 PM, Markus Armbruster wrote:
>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>>>> Now than we can probe if the TCG accelerator is available
>>>>>> at runtime with a QMP command, do it once at the beginning
>>>>>> and only register the tests we can run.
>>>>>> We can then replace the #ifdef'ry by a runtime check.
>>>>>>
>>>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>
>>>>> Please read the last remark first.  The other ones are detail; feel free
>>>>> to skip them until we're done with the last one.
>>>>>
>>>>>> ---
>>>>>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>>>> +    tcg_accel_available = qtest_has_accel("tcg");
>>>>>> +
>>>>>
>>>>> When does tcg_accel_available differ from defined(CONFIG_TCG)?
>>>>
>>>> qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
>>>> is build-time.
>>>
>>> Let me rephrase my question: under what conditions can the values of
>>> qtest_has_accel("tcg") and defined(CONFIG_TCG) differ?
>>
>> They are currently the same, so this patch is a no-op. *But* it
>> allows us to remove the global dependence on CONFIG_TCG in the
>> Meson machinery (see the last commit in this series).
>>
>> Is that missing part of the commit description?
>>
>> "This will allow us to remove the global CONFIG_TCG dependency
>> in our Meson machinery in a pair of commits."?
> 
> Do you mean "in the next two commits"?

Yes.

> Please also note that the probing at run-time always gives the same
> result as the compile-time check it replaces.
> 
> I don't understand what exactly the conversion to probing enables and
> how, but I believe I don't have to.

This series is removing some limitations in qtests to help Claudio work:

x86: https://www.mail-archive.com/qemu-devel@nongnu.org/msg793885.html
arm: https://www.mail-archive.com/qemu-devel@nongnu.org/msg799328.html
s390x: https://www.mail-archive.com/qemu-devel@nongnu.org/msg800254.html

which allow building with different sets of accelerators (and more).

Claudio/Paolo could better explain :)


What I like from feature tested at runtime is we can run qtests using
older binaries, binaries built with different configure flags, or the
binaries installed by the distribution.

Thanks,

Phil.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/30/21 8:13 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>> On 4/29/21 3:22 PM, Markus Armbruster wrote:
>>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>>>>> Now than we can probe if the TCG accelerator is available
>>>>>>> at runtime with a QMP command, do it once at the beginning
>>>>>>> and only register the tests we can run.
>>>>>>> We can then replace the #ifdef'ry by a runtime check.
>>>>>>>
>>>>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>>
>>>>>> Please read the last remark first.  The other ones are detail; feel free
>>>>>> to skip them until we're done with the last one.
>>>>>>
>>>>>>> ---
>>>>>>>  tests/qtest/qmp-cmd-test.c | 18 ++++++++++++++----
>>>>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>
>>>>>>> +    tcg_accel_available = qtest_has_accel("tcg");
>>>>>>> +
>>>>>>
>>>>>> When does tcg_accel_available differ from defined(CONFIG_TCG)?
>>>>>
>>>>> qtest_has_accel("tcg") is a runtime check, while defined(CONFIG_TCG)
>>>>> is build-time.
>>>>
>>>> Let me rephrase my question: under what conditions can the values of
>>>> qtest_has_accel("tcg") and defined(CONFIG_TCG) differ?
>>>
>>> They are currently the same, so this patch is a no-op. *But* it
>>> allows us to remove the global dependence on CONFIG_TCG in the
>>> Meson machinery (see the last commit in this series).
>>>
>>> Is that missing part of the commit description?
>>>
>>> "This will allow us to remove the global CONFIG_TCG dependency
>>> in our Meson machinery in a pair of commits."?
>> 
>> Do you mean "in the next two commits"?
>
> Yes.
>
>> Please also note that the probing at run-time always gives the same
>> result as the compile-time check it replaces.
>> 
>> I don't understand what exactly the conversion to probing enables and
>> how, but I believe I don't have to.
>
> This series is removing some limitations in qtests to help Claudio work:
>
> x86: https://www.mail-archive.com/qemu-devel@nongnu.org/msg793885.html
> arm: https://www.mail-archive.com/qemu-devel@nongnu.org/msg799328.html
> s390x: https://www.mail-archive.com/qemu-devel@nongnu.org/msg800254.html
>
> which allow building with different sets of accelerators (and more).
>
> Claudio/Paolo could better explain :)

Will this lead to different values of qtest_has_accel("tcg") and
defined(CONFIG_TCG) within a single build tree?

> What I like from feature tested at runtime is we can run qtests using
> older binaries, binaries built with different configure flags, or the
> binaries installed by the distribution.

Running with binaries build from a different HEAD seems unlikely to be
useful.  Any failures are just as likely to be caused bu the version
mismatch as by actual bugs.  Binaries for the same HEAD built for
another configuration can be made to work (this patch doesn't yet
suffice), but why should I care?

What I don't like is yet another moving part.

I'm not sure this is a good trade.  Quite possibly because I still don't
fully understand what we're trying to accomplish here :)