Re: [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/18/2015 06:00 AM, Markus Armbruster wrote:
>> The test doesn't check the output makes any sense, only that QEMU
>
> Reads slightly better as:
> s/check/check that/

Okay.

>> survives.  Useful since we've had an astounding number of crash bugs
>> around there.
>> 
>> In fact, we have a bunch of them right now: several devices crash or
>> hang, and all CPUs leave a dangling pointer behind.  Blacklist them in
>> the test.  The next commits will fix.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  tests/Makefile                 |  11 ++-
>>  tests/device-introspect-test.c | 149 
>> +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 157 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/device-introspect-test.c
>> 
>
>> @@ -478,8 +483,8 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): 
>> check-qtest-%: $(check-qtest-y)
>>      $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>>              QTEST_QEMU_IMG=qemu-img$(EXESUF) \
>>              MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
>> -            gtester $(GTESTER_OPTIONS) -m=$(SPEED) 
>> $(check-qtest-$*-y),"GTESTER $@")
>> -    $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
>> +            gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) 
>> $(check-qtest-$*-y),"GTESTER $@")
>
> Long line; worth adding a \ line-wrap?

I'll look into it.

>> +++ b/tests/device-introspect-test.c
>> @@ -0,0 +1,149 @@
>
>> +/*
>> + * Covers QMP device-list-properties and HMP device_add help.  We
>> + * currently don't check their output makes sense, only that QEMU
>
> again, might be better as:
> s/check/check that/

Okay.

>> +static void test_one_device(const char *type)
>> +{
>> +    QDict *resp;
>> +    char *help;
>> +
>> +    /* FIXME device-list-properties crashes for abstract device, skip  */
>> +    if (strcmp(type, "device")) {
>> +        resp = qmp("{'execute': 'device-list-properties',"
>> +                   " 'arguments': {'typename': %s}}",
>> +                   type);
>> +        QDECREF(resp);
>> +    }
>> +
>> +    help = hmp("device_add \"%s,help\"", type);
>> +    g_free(help);
>
> The comment mentions a skip, and the commit message header mentioned a
> blacklist, but I'm not seeing a skip here. Am I missing something?

The conditional skips part of the test when type is "device", because
that part crashes.

>> +
>> +static bool blacklisted(const char *type)
>> +{
>> +    static const char *blacklist[] = {
>> +        /* crash in object_unref(): */
>> +        "pxa2xx-pcmcia",
>> +        /* hang in object_unref(): */
>> +        "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp",
>> +        /* create a CPU, thus use after free (see below): */
>> +        "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp",
>> +    };
>
> Okay, here's the blacklist the commit message mentioned, but maybe that
> means the earlier comment is stale?

The commit message is slightly inaccurate: unlike the other devices,
"device" isn't blacklisted completely.  Good enough?