[Qemu-devel] [PATCH 0/6] Various qtest-related patches and a mc146818rtc fix

[Qemu-devel] [PATCH 0/6] Various qtest-related patches and a mc146818rtc fix

[Thomas Huth]

The basic idea for this patch series are the two patches that improve
the device-inrospection test: We now check that the qom-tree and the
qtree do not change during introspection, and the final patch also turns
on testing device introspection with all machines. The other patches are
a bug fix for the mc146818rtc device which has been discovered with the
improved device-introspection test, and two patches to shut up annoying
messages during "make check".

Thomas Huth (6):
  tests/migration-test: Silence the kvm_hv message by default
  tests/device-introspection: Check that the qom-tree and qtree do not
    change
  hw/timer/mc146818rtc: White space clean-up
  hw/timer/mc146818rtc: Fix introspection problem
  net: Silence 'has no peer' messages in testing mode
  tests/device-introspect: Test with all machines, not only with "none"

 hw/ppc/prep.c                  |  3 +++
 hw/timer/mc146818rtc.c         | 20 ++++++---------
 tests/device-introspect-test.c | 55 ++++++++++++++++++++++++++++++++++++------
 tests/migration-test.c         | 20 ++++++++-------
 vl.c                           |  3 +--
 5 files changed, 70 insertions(+), 31 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] tests/migration-test: Silence the kvm_hv message by default

[Thomas Huth]

When running "make check" on a non-POWER host, the output is quite
distorted like this:

  [...]
  GTESTER check-qtest-nios2
  GTESTER check-qtest-or1k
  GTESTER check-qtest-ppc64
Skipping test: kvm_hv not available Skipping test: kvm_hv not available Skipping test: kvm_hv not available Skipping test: kvm_hv not available   GTESTER check-qtest-ppcemb
  GTESTER check-qtest-ppc
  GTESTER check-qtest-riscv32
  GTESTER check-qtest-riscv64
  [...]

Move the check to the beginning of the main function instead, so that
we do not have to test the condition again and again for each test,
and better use g_test_message() instead of g_print() here, like it is
also done in ufd_version_check() already.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/migration-test.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index e079e0b..cb795b2 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -445,15 +445,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                   " -incoming %s",
                                   accel, tmpfs, bootpath, uri);
     } else if (strcmp(arch, "ppc64") == 0) {
-
-        /* On ppc64, the test only works with kvm-hv, but not with kvm-pr
-         * and TCG is touchy due to race conditions on dirty bits
-         * (especially on PPC for some reason)
-         */
-        if (access("/sys/module/kvm_hv", F_OK)) {
-            g_print("Skipping test: kvm_hv not available ");
-            return -1;
-        }
         cmd_src = g_strdup_printf("-machine accel=%s -m 256M"
                                   " -name source,debug-threads=on"
                                   " -serial file:%s/src_serial"
@@ -769,6 +760,17 @@ int main(int argc, char **argv)
         return 0;
     }
 
+    /*
+     * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
+     * is touchy due to race conditions on dirty bits (especially on PPC for
+     * some reason)
+     */
+    if (g_str_equal(qtest_get_arch(), "ppc64") &&
+        access("/sys/module/kvm_hv", F_OK)) {
+        g_test_message("Skipping test: kvm_hv not available");
+        return 0;
+    }
+
     tmpfs = mkdtemp(template);
     if (!tmpfs) {
         g_test_message("mkdtemp on path (%s): %s\n", template, strerror(errno));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/6] tests/migration-test: Silence the kvm_hv message by default

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> When running "make check" on a non-POWER host, the output is quite
> distorted like this:
>
>   [...]
>   GTESTER check-qtest-nios2
>   GTESTER check-qtest-or1k
>   GTESTER check-qtest-ppc64
> Skipping test: kvm_hv not available Skipping test: kvm_hv not available Skipping test: kvm_hv not available Skipping test: kvm_hv not available   GTESTER check-qtest-ppcemb
>   GTESTER check-qtest-ppc
>   GTESTER check-qtest-riscv32
>   GTESTER check-qtest-riscv64
>   [...]
>
> Move the check to the beginning of the main function instead, so that
> we do not have to test the condition again and again for each test,
> and better use g_test_message() instead of g_print() here, like it is
> also done in ufd_version_check() already.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 2/6] tests/device-introspection: Check that the qom-tree and qtree do not change

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH 2/6] tests/device-introspection: Check that the qom-tree and qtree do not change

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 2/6] tests/device-introspection: Check that the qom-tree and qtree do not change

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH 2/6] tests/device-introspection: Check that the qom-tree and qtree do not change

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 2/6] tests/device-introspection: Check that the qom-tree and qtree do not change

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH 2/6] tests/device-introspection: Check that the qom-tree and qtree do not change

[Markus Armbruster]

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".

[Qemu-devel] [PATCH 3/6] hw/timer/mc146818rtc: White space clean-up

[Thomas Huth]

mc146818rtc.c still contains some TABs. Replace them with spaces.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/timer/mc146818rtc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6f1f723..3a14075 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -120,7 +120,7 @@ static void rtc_coalesced_timer_update(RTCState *s)
         timer_del(s->coalesced_timer);
     } else {
         /* divide each RTC interval to 2 - 8 smaller intervals */
-        int c = MIN(s->irq_coalesced, 7) + 1; 
+        int c = MIN(s->irq_coalesced, 7) + 1;
         int64_t next_clock = qemu_clock_get_ns(rtc_clock) +
             periodic_clock_to_ns(s->period / c);
         timer_mod(s->coalesced_timer, next_clock);
@@ -485,7 +485,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
             s->cmos_data[s->cmos_index] = data;
             check_update_timer(s);
             break;
-	case RTC_IBM_PS2_CENTURY_BYTE:
+        case RTC_IBM_PS2_CENTURY_BYTE:
             s->cmos_index = RTC_CENTURY;
             /* fall through */
         case RTC_CENTURY:
@@ -713,7 +713,7 @@ static uint64_t cmos_ioport_read(void *opaque, hwaddr addr,
         return 0xff;
     } else {
         switch(s->cmos_index) {
-	case RTC_IBM_PS2_CENTURY_BYTE:
+        case RTC_IBM_PS2_CENTURY_BYTE:
             s->cmos_index = RTC_CENTURY;
             /* fall through */
         case RTC_CENTURY:
@@ -915,7 +915,7 @@ static void rtc_reset(void *opaque)
 
     if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
         s->irq_coalesced = 0;
-        s->irq_reinject_on_ack_count = 0;		
+        s->irq_reinject_on_ack_count = 0;
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/6] hw/timer/mc146818rtc: White space clean-up

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> mc146818rtc.c still contains some TABs. Replace them with spaces.

Also delete trailing whitespace.

>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

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

[Qemu-devel] [PATCH 4/6] hw/timer/mc146818rtc: Fix introspection problem

[Thomas Huth]

There is currently a funny problem with the "mc146818rtc" device:
1) Start QEMU like this:
   qemu-system-ppc64 -M pseries -S
2) At the HMP monitor, enter "info qom-tree". Note that there is an
   entry for "/rtc (spapr-rtc)".
3) Introspect the mc146818rtc device like this:
   device_add mc146818rtc,help
4) Run "info qom-tree" again. The "/rtc" entry is gone now!

The rtc_finalize() function of the mc146818rtc device has two bugs: First,
it tries to remove a "rtc" property, while the rtc_realizefn() added a
"rtc-time" property instead. And second, it should have been done in an
unrealize function, not in a finalize function, to avoid that this causes
problems during introspection.

But since adding aliases to the global machine state should not be done
from a device's realize function anyway, let's rather fix this issue
by moving the creation of the alias to the code that creates the device
(and thus is run from the machine init functions instead). We can then
remove the object_property_del() completely. In prep.c, the code for
setting up the alias is added to the function which deals already with
the rtc device for the "40p" machine. The "prep" machine is ignored
here since it is scheduled for removal anyway.

Fixes: 654a36d857ff949e0d1989904b76f53fded9dc83
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/prep.c          |  3 +++
 hw/timer/mc146818rtc.c | 12 +++---------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 3401570..91a8f42 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -696,6 +696,9 @@ static int prep_set_cmos_checksum(DeviceState *dev, void *opaque)
         rtc_set_memory(rtc, 0x3e, checksum & 0xff);
         rtc_set_memory(rtc, 0x2f, checksum >> 8);
         rtc_set_memory(rtc, 0x3f, checksum >> 8);
+
+        object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(rtc),
+                                  "date", NULL);
     }
     return 0;
 }
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 3a14075..a504f03 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -995,9 +995,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 
     object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);
 
-    object_property_add_alias(qdev_get_machine(), "rtc-time",
-                              OBJECT(s), "date", NULL);
-
     qdev_init_gpio_out(dev, &s->irq, 1);
 }
 
@@ -1019,6 +1016,9 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
     }
     QLIST_INSERT_HEAD(&rtc_devices, s, link);
 
+    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(s),
+                              "date", NULL);
+
     return isadev;
 }
 
@@ -1052,17 +1052,11 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
     dc->user_creatable = false;
 }
 
-static void rtc_finalize(Object *obj)
-{
-    object_property_del(qdev_get_machine(), "rtc", NULL);
-}
-
 static const TypeInfo mc146818rtc_info = {
     .name          = TYPE_MC146818_RTC,
     .parent        = TYPE_ISA_DEVICE,
     .instance_size = sizeof(RTCState),
     .class_init    = rtc_class_initfn,
-    .instance_finalize = rtc_finalize,
 };
 
 static void mc146818rtc_register_types(void)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/6] hw/timer/mc146818rtc: Fix introspection problem

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> There is currently a funny problem with the "mc146818rtc" device:
> 1) Start QEMU like this:
>    qemu-system-ppc64 -M pseries -S
> 2) At the HMP monitor, enter "info qom-tree". Note that there is an
>    entry for "/rtc (spapr-rtc)".
> 3) Introspect the mc146818rtc device like this:
>    device_add mc146818rtc,help
> 4) Run "info qom-tree" again. The "/rtc" entry is gone now!
>
> The rtc_finalize() function of the mc146818rtc device has two bugs: First,
> it tries to remove a "rtc" property, while the rtc_realizefn() added a
> "rtc-time" property instead. And second, it should have been done in an
> unrealize function, not in a finalize function, to avoid that this causes
> problems during introspection.
>
> But since adding aliases to the global machine state should not be done
> from a device's realize function anyway, let's rather fix this issue
> by moving the creation of the alias to the code that creates the device
> (and thus is run from the machine init functions instead). We can then
> remove the object_property_del() completely. In prep.c, the code for
> setting up the alias is added to the function which deals already with
> the rtc device for the "40p" machine. The "prep" machine is ignored
> here since it is scheduled for removal anyway.

That's okay as long as this patch goes in after the removal.  And then
you don't need this sentence.

> Fixes: 654a36d857ff949e0d1989904b76f53fded9dc83
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/prep.c          |  3 +++
>  hw/timer/mc146818rtc.c | 12 +++---------
>  2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570..91a8f42 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -696,6 +696,9 @@ static int prep_set_cmos_checksum(DeviceState *dev, void *opaque)
>          rtc_set_memory(rtc, 0x3e, checksum & 0xff);
>          rtc_set_memory(rtc, 0x2f, checksum >> 8);
>          rtc_set_memory(rtc, 0x3f, checksum >> 8);
> +
> +        object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(rtc),
> +                                  "date", NULL);
>      }
>      return 0;
>  }
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 3a14075..a504f03 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -995,9 +995,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  
>      object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);
>  
> -    object_property_add_alias(qdev_get_machine(), "rtc-time",
> -                              OBJECT(s), "date", NULL);
> -
>      qdev_init_gpio_out(dev, &s->irq, 1);
>  }
>  
> @@ -1019,6 +1016,9 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
>      }
>      QLIST_INSERT_HEAD(&rtc_devices, s, link);
>  
> +    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(s),
> +                              "date", NULL);
> +
>      return isadev;
>  }
>  
> @@ -1052,17 +1052,11 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
>      dc->user_creatable = false;
>  }
>  
> -static void rtc_finalize(Object *obj)
> -{
> -    object_property_del(qdev_get_machine(), "rtc", NULL);
> -}
> -
>  static const TypeInfo mc146818rtc_info = {
>      .name          = TYPE_MC146818_RTC,
>      .parent        = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(RTCState),
>      .class_init    = rtc_class_initfn,
> -    .instance_finalize = rtc_finalize,
>  };
>  
>  static void mc146818rtc_register_types(void)

Let's see...  the device is created in two places, mc146818_rtc_init()
and i82378_realize().  You add the alias in the former, but not the
latter.  Instead, you add it where the i82378 device is created: in
ibm_40p_init().  ppc_prep_init() also creates it, but you intentionally
ignore that one.  Net effect: you add the alias a little later.  Looks
good to me.

Preferably with the superfluous sentence deleted from the commit
message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

When running qtests with -nodefaults, we are not interested in
these 'XYZ has no peer' messages.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 vl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 16b913f..7055df3 100644
--- a/vl.c
+++ b/vl.c
@@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
      * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
      * sets up a nic that isn't connected to anything.
      */
-    if (!default_net) {
+    if (!default_net && (!qtest_enabled() || has_defaults)) {
         net_check_clients();
     }
 
-
     if (boot_once) {
         qemu_boot_set(boot_once, &error_fatal);
         qemu_register_reset(restore_boot_order, g_strdup(boot_order));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Paolo Bonzini]

On 14/08/2018 16:46, Thomas Huth wrote:
> When running qtests with -nodefaults, we are not interested in
> these 'XYZ has no peer' messages.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  vl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 16b913f..7055df3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
>       * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
>       * sets up a nic that isn't connected to anything.
>       */
> -    if (!default_net) {
> +    if (!default_net && (!qtest_enabled() || has_defaults)) {
>          net_check_clients();
>      }
>  

Why does it have no peer?  Not a nack, just curiosity.

Paolo

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 08/14/2018 05:33 PM, Paolo Bonzini wrote:
> On 14/08/2018 16:46, Thomas Huth wrote:
>> When running qtests with -nodefaults, we are not interested in
>> these 'XYZ has no peer' messages.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  vl.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 16b913f..7055df3 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
>>       * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
>>       * sets up a nic that isn't connected to anything.
>>       */
>> -    if (!default_net) {
>> +    if (!default_net && (!qtest_enabled() || has_defaults)) {
>>          net_check_clients();
>>      }
>>  
> 
> Why does it have no peer?  Not a nack, just curiosity.

The machines which emulate an embedded system often always create a NIC
(since it is hard-wired on the board, not optional). But since there is
no back-end on the host side with "-nodefaults", the net_check_clients()
function complains in this case.

For example:

$ microblaze-softmmu/qemu-system-microblaze -nodefaults -S
qemu-system-microblaze: warning: nic xlnx.xps-ethernetlite.0 has no peer

 Thomas

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Paolo Bonzini]

On 14/08/2018 17:43, Thomas Huth wrote:
> On 08/14/2018 05:33 PM, Paolo Bonzini wrote:
>> On 14/08/2018 16:46, Thomas Huth wrote:
>>> When running qtests with -nodefaults, we are not interested in
>>> these 'XYZ has no peer' messages.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  vl.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 16b913f..7055df3 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
>>>       * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
>>>       * sets up a nic that isn't connected to anything.
>>>       */
>>> -    if (!default_net) {
>>> +    if (!default_net && (!qtest_enabled() || has_defaults)) {
>>>          net_check_clients();
>>>      }
>>>  
>>
>> Why does it have no peer?  Not a nack, just curiosity.
> 
> The machines which emulate an embedded system often always create a NIC
> (since it is hard-wired on the board, not optional). But since there is
> no back-end on the host side with "-nodefaults", the net_check_clients()
> function complains in this case.

Ok, the has_defaults test then makes sense.  Is the qtest_enabled() part
still needed, or is the message unnecessary even in normal operation?

Paolo

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 08/14/2018 05:57 PM, Paolo Bonzini wrote:
> On 14/08/2018 17:43, Thomas Huth wrote:
>> On 08/14/2018 05:33 PM, Paolo Bonzini wrote:
>>> On 14/08/2018 16:46, Thomas Huth wrote:
>>>> When running qtests with -nodefaults, we are not interested in
>>>> these 'XYZ has no peer' messages.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  vl.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index 16b913f..7055df3 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
>>>>       * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
>>>>       * sets up a nic that isn't connected to anything.
>>>>       */
>>>> -    if (!default_net) {
>>>> +    if (!default_net && (!qtest_enabled() || has_defaults)) {
>>>>          net_check_clients();
>>>>      }
>>>>  
>>>
>>> Why does it have no peer?  Not a nack, just curiosity.
>>
>> The machines which emulate an embedded system often always create a NIC
>> (since it is hard-wired on the board, not optional). But since there is
>> no back-end on the host side with "-nodefaults", the net_check_clients()
>> function complains in this case.
> 
> Ok, the has_defaults test then makes sense.  Is the qtest_enabled() part
> still needed, or is the message unnecessary even in normal operation?

I think it is still needed, since you could also screw up your command
line parameters after specifying -nodefaults (e.g. "-nodefaults -net
nic" without giving an additional "-net user" or something similar).

 Thomas

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Paolo Bonzini]

On 14/08/2018 18:01, Thomas Huth wrote:
> On 08/14/2018 05:57 PM, Paolo Bonzini wrote:
>> On 14/08/2018 17:43, Thomas Huth wrote:
>>> On 08/14/2018 05:33 PM, Paolo Bonzini wrote:
>>>> On 14/08/2018 16:46, Thomas Huth wrote:
>>>>> When running qtests with -nodefaults, we are not interested in
>>>>> these 'XYZ has no peer' messages.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  vl.c | 3 +--
>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/vl.c b/vl.c
>>>>> index 16b913f..7055df3 100644
>>>>> --- a/vl.c
>>>>> +++ b/vl.c
>>>>> @@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
>>>>>       * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
>>>>>       * sets up a nic that isn't connected to anything.
>>>>>       */
>>>>> -    if (!default_net) {
>>>>> +    if (!default_net && (!qtest_enabled() || has_defaults)) {
>>>>>          net_check_clients();
>>>>>      }
>>>>>  
>>>>
>>>> Why does it have no peer?  Not a nack, just curiosity.
>>>
>>> The machines which emulate an embedded system often always create a NIC
>>> (since it is hard-wired on the board, not optional). But since there is
>>> no back-end on the host side with "-nodefaults", the net_check_clients()
>>> function complains in this case.
>>
>> Ok, the has_defaults test then makes sense.  Is the qtest_enabled() part
>> still needed, or is the message unnecessary even in normal operation?
> 
> I think it is still needed, since you could also screw up your command
> line parameters after specifying -nodefaults (e.g. "-nodefaults -net
> nic" without giving an additional "-net user" or something similar).

True.  Though it cannot happen with -nic, so another possibility is to
give it only if -net was used?

Paolo

Paolo

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 08/14/2018 06:03 PM, Paolo Bonzini wrote:
> On 14/08/2018 18:01, Thomas Huth wrote:
>> On 08/14/2018 05:57 PM, Paolo Bonzini wrote:
>>> On 14/08/2018 17:43, Thomas Huth wrote:
>>>> On 08/14/2018 05:33 PM, Paolo Bonzini wrote:
>>>>> On 14/08/2018 16:46, Thomas Huth wrote:
>>>>>> When running qtests with -nodefaults, we are not interested in
>>>>>> these 'XYZ has no peer' messages.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>  vl.c | 3 +--
>>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/vl.c b/vl.c
>>>>>> index 16b913f..7055df3 100644
>>>>>> --- a/vl.c
>>>>>> +++ b/vl.c
>>>>>> @@ -4559,11 +4559,10 @@ int main(int argc, char **argv, char **envp)
>>>>>>       * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
>>>>>>       * sets up a nic that isn't connected to anything.
>>>>>>       */
>>>>>> -    if (!default_net) {
>>>>>> +    if (!default_net && (!qtest_enabled() || has_defaults)) {
>>>>>>          net_check_clients();
>>>>>>      }
>>>>>>  
>>>>>
>>>>> Why does it have no peer?  Not a nack, just curiosity.
>>>>
>>>> The machines which emulate an embedded system often always create a NIC
>>>> (since it is hard-wired on the board, not optional). But since there is
>>>> no back-end on the host side with "-nodefaults", the net_check_clients()
>>>> function complains in this case.
>>>
>>> Ok, the has_defaults test then makes sense.  Is the qtest_enabled() part
>>> still needed, or is the message unnecessary even in normal operation?
>>
>> I think it is still needed, since you could also screw up your command
>> line parameters after specifying -nodefaults (e.g. "-nodefaults -net
>> nic" without giving an additional "-net user" or something similar).
> 
> True.  Though it cannot happen with -nic, so another possibility is to
> give it only if -net was used?

Sorry, I don't quite get you ... this is a generic check in vl.c, how
should this code know whether the NICs have been specified with -nic or
-net?

 Thomas

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Paolo Bonzini]

On 14/08/2018 18:11, Thomas Huth wrote:
> On 08/14/2018 06:03 PM, Paolo Bonzini wrote:
>> On 14/08/2018 18:01, Thomas Huth wrote:
>>> On 08/14/2018 05:57 PM, Paolo Bonzini wrote:
>>>> On 14/08/2018 17:43, Thomas Huth wrote:
>>>>> The machines which emulate an embedded system often always create a NIC
>>>>> (since it is hard-wired on the board, not optional). But since there is
>>>>> no back-end on the host side with "-nodefaults", the net_check_clients()
>>>>> function complains in this case.
>>>>
>>>> [...] is the message unnecessary even in normal operation?
>>>
>>> I think it is still needed, since you could also screw up your command
>>> line parameters after specifying -nodefaults (e.g. "-nodefaults -net
>>> nic" without giving an additional "-net user" or something similar).
>>
>> True.  Though it cannot happen with -nic, so another possibility is to
>> give it only if -net was used?
> 
> Sorry, I don't quite get you ... this is a generic check in vl.c, how
> should this code know whether the NICs have been specified with -nic or
> -net?

	case QEMU_OPTION_net:
	    had_net = true;
	    ...

	if (!default_net && had_net) {
	    net_check_clients();
	}

:)

Paolo

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 08/14/2018 07:10 PM, Paolo Bonzini wrote:
> On 14/08/2018 18:11, Thomas Huth wrote:
>> On 08/14/2018 06:03 PM, Paolo Bonzini wrote:
>>> On 14/08/2018 18:01, Thomas Huth wrote:
>>>> On 08/14/2018 05:57 PM, Paolo Bonzini wrote:
>>>>> On 14/08/2018 17:43, Thomas Huth wrote:
>>>>>> The machines which emulate an embedded system often always create a NIC
>>>>>> (since it is hard-wired on the board, not optional). But since there is
>>>>>> no back-end on the host side with "-nodefaults", the net_check_clients()
>>>>>> function complains in this case.
>>>>>
>>>>> [...] is the message unnecessary even in normal operation?
>>>>
>>>> I think it is still needed, since you could also screw up your command
>>>> line parameters after specifying -nodefaults (e.g. "-nodefaults -net
>>>> nic" without giving an additional "-net user" or something similar).
>>>
>>> True.  Though it cannot happen with -nic, so another possibility is to
>>> give it only if -net was used?
>>
>> Sorry, I don't quite get you ... this is a generic check in vl.c, how
>> should this code know whether the NICs have been specified with -nic or
>> -net?
> 
> 	case QEMU_OPTION_net:
> 	    had_net = true;
> 	    ...
> 
> 	if (!default_net && had_net) {
> 	    net_check_clients();
> 	}
> 
> :)

Ah, ok, thanks, I somehow only had that nd_table stuff in mind... That
change looks reasonable to me, I'll give it a try!

 Thomas

Re: [Qemu-devel] [PATCH 5/6] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 08/14/2018 07:10 PM, Paolo Bonzini wrote:
> On 14/08/2018 18:11, Thomas Huth wrote:
>> On 08/14/2018 06:03 PM, Paolo Bonzini wrote:
>>> On 14/08/2018 18:01, Thomas Huth wrote:
>>>> On 08/14/2018 05:57 PM, Paolo Bonzini wrote:
>>>>> On 14/08/2018 17:43, Thomas Huth wrote:
>>>>>> The machines which emulate an embedded system often always create a NIC
>>>>>> (since it is hard-wired on the board, not optional). But since there is
>>>>>> no back-end on the host side with "-nodefaults", the net_check_clients()
>>>>>> function complains in this case.
>>>>>
>>>>> [...] is the message unnecessary even in normal operation?
>>>>
>>>> I think it is still needed, since you could also screw up your command
>>>> line parameters after specifying -nodefaults (e.g. "-nodefaults -net
>>>> nic" without giving an additional "-net user" or something similar).
>>>
>>> True.  Though it cannot happen with -nic, so another possibility is to
>>> give it only if -net was used?
>>
>> Sorry, I don't quite get you ... this is a generic check in vl.c, how
>> should this code know whether the NICs have been specified with -nic or
>> -net?
> 
> 	case QEMU_OPTION_net:
> 	    had_net = true;
> 	    ...
> 
> 	if (!default_net && had_net) {
> 	    net_check_clients();
> 	}

Looking at net_check_clients(), I think we're also checking -netdev
devices there, so we likely should call net_check_clients()
independently of whether -net has been specified by the user or not... I
think I'll best keep the patch in its current shape.

 Thomas

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

[Thomas Huth]

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);
+}
+
 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);
+    } else {
+        qtest_cb_for_every_machine(add_machine_test_case);
+    }
 
     return g_test_run();
 }
-- 
1.8.3.1

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

[Paolo Bonzini]

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

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

[Thomas Huth]

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

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

[Markus Armbruster]

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>