Re: [PATCH v4 10/12] qtest/qmp-cmd-test: Make test build-independent from accelerator

[Markus Armbruster <armbru@redhat.com>]

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);