[6/6] tests/device-introspect: Test with all machines, not only with "none"

Certain device introspection crashes used to only happen if you were
using a certain machine, e.g. if the machine was using serial_hd() or
nd_table[], and a device was trying to use these in its instance_init
function, too.

To be able to catch these problems, let's extend the device-introspect
test to check the devices on all machine types. Since this is a rather
slow operation, and most of the problems are already handled by testing
with the "none" machine only, the test with all machines is only run
in the "make check SPEED=slow" mode.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/device-introspect-test.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

On 14/08/2018 16:46, Thomas Huth wrote:
> To be able to catch these problems, let's extend the device-introspect
> test to check the devices on all machine types. Since this is a rather
> slow operation, and most of the problems are already handled by testing
> with the "none" machine only, the test with all machines is only run
> in the "make check SPEED=slow" mode.

For a separate series: we should probably do this also for qom-test and
also, somehow (regex?), detect versioned machine types and do the test
with all non-versioned machines unless SPEED=slow.

Paolo

Thomas Huth <thuth@redhat.com> writes:

> Certain device introspection crashes used to only happen if you were
> using a certain machine, e.g. if the machine was using serial_hd() or
> nd_table[], and a device was trying to use these in its instance_init
> function, too.
>
> To be able to catch these problems, let's extend the device-introspect
> test to check the devices on all machine types. Since this is a rather
> slow operation, and most of the problems are already handled by testing
> with the "none" machine only, the test with all machines is only run
> in the "make check SPEED=slow" mode.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/device-introspect-test.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 5b7ec05..902153c 100644
> --- a/tests/device-introspect-test.c
> +++ b/tests/device-introspect-test.c
> @@ -221,13 +221,13 @@ static void test_device_intro_abstract(void)
>      qtest_end();
>  }
>  
> -static void test_device_intro_concrete(void)
> +static void test_device_intro_concrete(const void *args)
>  {
>      QList *types;
>      QListEntry *entry;
>      const char *type;
>  
> -    qtest_start(common_args);
> +    qtest_start(args);
>      types = device_type_list(false);
>  
>      QLIST_FOREACH_ENTRY(types, entry) {
> @@ -239,6 +239,7 @@ static void test_device_intro_concrete(void)
>  
>      qobject_unref(types);
>      qtest_end();
> +    g_free((void *)args);
>  }
>  
>  static void test_abstract_interfaces(void)
> @@ -275,6 +276,26 @@ static void test_abstract_interfaces(void)
>      qtest_end();
>  }
>  
> +static void add_machine_test_case(const char *mname)
> +{
> +    char *path, *args;
> +
> +    /* Ignore blacklisted machines */
> +    if (g_str_equal("xenfv", mname) || g_str_equal("xenpv", mname)) {
> +        return;
> +    }
> +
> +    path = g_strdup_printf("device/introspect/concrete/defaults/%s", mname);
> +    args = g_strdup_printf("-M %s", mname);
> +    qtest_add_data_func(path, args, test_device_intro_concrete);
> +    g_free(path);
> +
> +    path = g_strdup_printf("device/introspect/concrete/nodefaults/%s", mname);
> +    args = g_strdup_printf("-nodefaults -M %s", mname);
> +    qtest_add_data_func(path, args, test_device_intro_concrete);
> +    g_free(path);
> +}

If !g_test_quick(), we test each machine type (mentioned in commit
message) with and without -nodefaults (not mentioned).  Please explain
that more completely in your commit message.

You allocate @path and @args here, and free @path here, but @args in
test_device_intro_concrete() is slightly awkward.  I'd be tempted to try
using fixtures.  Not a demand; what you got works.

Hmm, can we pass just @mname to the GTestDataFunc, then allocate and
free @args there?  Nope, @mname doesn't stay alive until that function
runs.

> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -283,8 +304,13 @@ int main(int argc, char **argv)
>      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/concrete", test_device_intro_concrete);
>      qtest_add_func("device/introspect/abstract-interfaces", test_abstract_interfaces);
> +    if (g_test_quick()) {
> +        qtest_add_data_func("device/introspect/concrete/defaults/none",
> +                            g_strdup(common_args), test_device_intro_concrete);

Suggest to break this line at the comma.

> +    } else {
> +        qtest_cb_for_every_machine(add_machine_test_case);
> +    }
>  
>      return g_test_run();
>  }

With at least the commit message improved:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

On 08/14/2018 05:32 PM, Paolo Bonzini wrote:
> On 14/08/2018 16:46, Thomas Huth wrote:
>> To be able to catch these problems, let's extend the device-introspect
>> test to check the devices on all machine types. Since this is a rather
>> slow operation, and most of the problems are already handled by testing
>> with the "none" machine only, the test with all machines is only run
>> in the "make check SPEED=slow" mode.
> 
> For a separate series: we should probably do this also for qom-test and
> also, somehow (regex?), detect versioned machine types and do the test
> with all non-versioned machines unless SPEED=slow.

I'll include a patch for this in v2 of my patch series.

 Thomas