Introspection should not change the qom-tree / qtree, so we should check
this in the device-introspect-test, too. This patch helped to find lots
of instrospection bugs during the QEMU v3.0 soft/hard-freeze period in the
last two months.

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

diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 0b4f221..5b7ec05 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -103,7 +103,14 @@ static QList *device_type_list(bool abstract)
 static void test_one_device(const char *type)
 {
     QDict *resp;
-    char *help, *qom_tree;
+    char *help;
+    char *qom_tree_start, *qom_tree_end;
+    char *qtree_start, *qtree_end;
+
+    g_debug("Testing device '%s'", type);
+
+    qom_tree_start = hmp("info qom-tree");
+    qtree_start = hmp("info qtree");
 
     resp = qmp("{'execute': 'device-list-properties',"
                " 'arguments': {'typename': %s}}",
@@ -115,10 +122,18 @@ static void test_one_device(const char *type)
 
     /*
      * Some devices leave dangling pointers in QOM behind.
-     * "info qom-tree" has a good chance at crashing then
+     * "info qom-tree" or "info qtree" have a good chance at crashing then.
+     * Also make sure that the tree did not change.
      */
-    qom_tree = hmp("info qom-tree");
-    g_free(qom_tree);
+    qom_tree_end = hmp("info qom-tree");
+    g_assert_cmpstr(qom_tree_start, ==, qom_tree_end);
+    g_free(qom_tree_start);
+    g_free(qom_tree_end);
+
+    qtree_end = hmp("info qtree");
+    g_assert_cmpstr(qtree_start, ==, qtree_end);
+    g_free(qtree_start);
+    g_free(qtree_end);
 }
 
 static void test_device_intro_list(void)
-- 
1.8.3.1

Thomas Huth <thuth@redhat.com> writes:

> Introspection should not change the qom-tree / qtree, so we should check
> this in the device-introspect-test, too. This patch helped to find lots
> of instrospection bugs during the QEMU v3.0 soft/hard-freeze period in the
> last two months.

Clever idea.

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/device-introspect-test.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 0b4f221..5b7ec05 100644
> --- a/tests/device-introspect-test.c
> +++ b/tests/device-introspect-test.c
> @@ -103,7 +103,14 @@ static QList *device_type_list(bool abstract)
>  static void test_one_device(const char *type)
>  {
>      QDict *resp;
> -    char *help, *qom_tree;
> +    char *help;
> +    char *qom_tree_start, *qom_tree_end;
> +    char *qtree_start, *qtree_end;
> +
> +    g_debug("Testing device '%s'", type);

This is only the second use of g_debug() in tests/.  What are you trying
to accomplish?

> +
> +    qom_tree_start = hmp("info qom-tree");
> +    qtree_start = hmp("info qtree");
>  
>      resp = qmp("{'execute': 'device-list-properties',"
>                 " 'arguments': {'typename': %s}}",
> @@ -115,10 +122,18 @@ static void test_one_device(const char *type)
>  
>      /*
>       * Some devices leave dangling pointers in QOM behind.
> -     * "info qom-tree" has a good chance at crashing then
> +     * "info qom-tree" or "info qtree" have a good chance at crashing then.
> +     * Also make sure that the tree did not change.
>       */
> -    qom_tree = hmp("info qom-tree");
> -    g_free(qom_tree);
> +    qom_tree_end = hmp("info qom-tree");
> +    g_assert_cmpstr(qom_tree_start, ==, qom_tree_end);
> +    g_free(qom_tree_start);
> +    g_free(qom_tree_end);
> +
> +    qtree_end = hmp("info qtree");
> +    g_assert_cmpstr(qtree_start, ==, qtree_end);
> +    g_free(qtree_start);
> +    g_free(qtree_end);
>  }
>  
>  static void test_device_intro_list(void)

Output of "info qom-tree" depends on hash table iteration order, but
that could almost be considered a feature here.

On 08/14/2018 07:53 PM, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> Introspection should not change the qom-tree / qtree, so we should check
>> this in the device-introspect-test, too. This patch helped to find lots
>> of instrospection bugs during the QEMU v3.0 soft/hard-freeze period in the
>> last two months.
> 
> Clever idea.
> 
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/device-introspect-test.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
>> index 0b4f221..5b7ec05 100644
>> --- a/tests/device-introspect-test.c
>> +++ b/tests/device-introspect-test.c
>> @@ -103,7 +103,14 @@ static QList *device_type_list(bool abstract)
>>  static void test_one_device(const char *type)
>>  {
>>      QDict *resp;
>> -    char *help, *qom_tree;
>> +    char *help;
>> +    char *qom_tree_start, *qom_tree_end;
>> +    char *qtree_start, *qtree_end;
>> +
>> +    g_debug("Testing device '%s'", type);
> 
> This is only the second use of g_debug() in tests/.  What are you trying
> to accomplish?

When the test crashes, I need a way to determine the device which caused
the crash. To avoid that I've then got to insert fprintf statements
manually here and recompile, the g_debug() seems to be a good solution,
since you can enable its output by setting some environment variable (I
use G_MESSAGES_DEBUG=all and G_MESSAGES_PREFIXED=none).

Or do you see a better way to provide a possibility to determine the
device that caused a crash?

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 08/14/2018 07:53 PM, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> Introspection should not change the qom-tree / qtree, so we should check
>>> this in the device-introspect-test, too. This patch helped to find lots
>>> of instrospection bugs during the QEMU v3.0 soft/hard-freeze period in the
>>> last two months.
>> 
>> Clever idea.
>> 
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  tests/device-introspect-test.c | 23 +++++++++++++++++++----
>>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
>>> index 0b4f221..5b7ec05 100644
>>> --- a/tests/device-introspect-test.c
>>> +++ b/tests/device-introspect-test.c
>>> @@ -103,7 +103,14 @@ static QList *device_type_list(bool abstract)
>>>  static void test_one_device(const char *type)
>>>  {
>>>      QDict *resp;
>>> -    char *help, *qom_tree;
>>> +    char *help;
>>> +    char *qom_tree_start, *qom_tree_end;
>>> +    char *qtree_start, *qtree_end;
>>> +
>>> +    g_debug("Testing device '%s'", type);
>> 
>> This is only the second use of g_debug() in tests/.  What are you trying
>> to accomplish?
>
> When the test crashes, I need a way to determine the device which caused
> the crash. To avoid that I've then got to insert fprintf statements
> manually here and recompile, the g_debug() seems to be a good solution,
> since you can enable its output by setting some environment variable (I
> use G_MESSAGES_DEBUG=all and G_MESSAGES_PREFIXED=none).

I see.  However, I'm unlikely to remember these GLib arcana when I run
into test failures.

> Or do you see a better way to provide a possibility to determine the
> device that caused a crash?

g_test_message() and --verbose.

Less arcane, because device-introspect-test --help points to --verbose,
and --verbose does the natural thing, namely printing more about what
it's being done.

Another option would be to split test_device_intro_concrete() into one
test case per device.  main() would have to enumerate devices so it can
register the test cases.  Vaguely similar to how qmp-test enumerates
query commands and registers their tests.  Probably not worth the bother
now.

On 08/14/2018 07:53 PM, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
[...]
>> @@ -115,10 +122,18 @@ static void test_one_device(const char *type)
>>  
>>      /*
>>       * Some devices leave dangling pointers in QOM behind.
>> -     * "info qom-tree" has a good chance at crashing then
>> +     * "info qom-tree" or "info qtree" have a good chance at crashing then.
>> +     * Also make sure that the tree did not change.
>>       */
>> -    qom_tree = hmp("info qom-tree");
>> -    g_free(qom_tree);
>> +    qom_tree_end = hmp("info qom-tree");
>> +    g_assert_cmpstr(qom_tree_start, ==, qom_tree_end);
>> +    g_free(qom_tree_start);
>> +    g_free(qom_tree_end);
>> +
>> +    qtree_end = hmp("info qtree");
>> +    g_assert_cmpstr(qtree_start, ==, qtree_end);
>> +    g_free(qtree_start);
>> +    g_free(qtree_end);
>>  }
>>  
>>  static void test_device_intro_list(void)
> 
> Output of "info qom-tree" depends on hash table iteration order, but
> that could almost be considered a feature here.

Currently, it seems to work fine. If we hit a false positive with
ordering later, we still can add some code for sorting the output, I guess?

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 08/14/2018 07:53 PM, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
> [...]
>>> @@ -115,10 +122,18 @@ static void test_one_device(const char *type)
>>>  
>>>      /*
>>>       * Some devices leave dangling pointers in QOM behind.
>>> -     * "info qom-tree" has a good chance at crashing then
>>> +     * "info qom-tree" or "info qtree" have a good chance at crashing then.
>>> +     * Also make sure that the tree did not change.
>>>       */
>>> -    qom_tree = hmp("info qom-tree");
>>> -    g_free(qom_tree);
>>> +    qom_tree_end = hmp("info qom-tree");
>>> +    g_assert_cmpstr(qom_tree_start, ==, qom_tree_end);
>>> +    g_free(qom_tree_start);
>>> +    g_free(qom_tree_end);
>>> +
>>> +    qtree_end = hmp("info qtree");
>>> +    g_assert_cmpstr(qtree_start, ==, qtree_end);
>>> +    g_free(qtree_start);
>>> +    g_free(qtree_end);
>>>  }
>>>  
>>>  static void test_device_intro_list(void)
>> 
>> Output of "info qom-tree" depends on hash table iteration order, but
>> that could almost be considered a feature here.
>
> Currently, it seems to work fine. If we hit a false positive with
> ordering later, we still can add some code for sorting the output, I guess?

I'm fine with comparing output of info qom-tree and info qtree.

I figure the most likely cause for a change of order would be a series
of QOM tree modifications that cancels out.  There should be *no* QOM
tree modifications.  Thus, "almost a feature".