Re: [Qemu-devel] [PATCH 2/2] tests-ppc64: test for -device spapr-pci-host-bridge

[Alexey Kardashevskiy]

On 03/06/2014 07:42 AM, Andreas Färber wrote:
> Am 14.02.2014 05:05, schrieb Alexey Kardashevskiy:
>> On 02/11/2014 02:48 AM, Andreas Färber wrote:
>>> Am 10.02.2014 04:52, schrieb Alexey Kardashevskiy:
>>>> This adds a test if SPAPR PHB can be added via the command line.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>> ---
>>>>  tests/Makefile         |  2 ++
>>>>  tests/spapr-phb-test.c | 28 ++++++++++++++++++++++++++++
>>>>  2 files changed, 30 insertions(+)
>>>>  create mode 100644 tests/spapr-phb-test.c
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index 3a00ea7..303225e 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -117,6 +117,7 @@ check-qtest-s390x-y += tests/qom-test$(EXESUF)
>>>>  check-qtest-unicore32-y += tests/qom-test$(EXESUF)
>>>>  check-qtest-xtensa-y += tests/qom-test$(EXESUF)
>>>>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>>> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>>>>  
>>>>  check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>>>>          comments.json empty.json funny-char.json indented-expr.json \
>>>> @@ -203,6 +204,7 @@ libqos-omap-obj-y = $(libqos-obj-y) 
>>>> tests/libqos/i2c-omap.o
>>>>  tests/rtc-test$(EXESUF): tests/rtc-test.o
>>>>  tests/m48t59-test$(EXESUF): tests/m48t59-test.o
>>>>  tests/endianness-test$(EXESUF): tests/endianness-test.o
>>>> +tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
>>>>  tests/fdc-test$(EXESUF): tests/fdc-test.o
>>>>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
>>>>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>>> diff --git a/tests/spapr-phb-test.c b/tests/spapr-phb-test.c
>>>> new file mode 100644
>>>> index 0000000..8cad354
>>>> --- /dev/null
>>>> +++ b/tests/spapr-phb-test.c
>>>> @@ -0,0 +1,28 @@
>>>> +/*
>>>> + * QTest testcase for SPAPR PHB
>>>> + *
>>>
>>> Missing some IBM or aik copyright?
>>>
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>>> later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +#include <glib.h>
>>>> +
>>>> +#include "libqtest.h"
>>>> +
>>>> +#define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
>>>> +
>>>> +static void test_phb_device(void)
>>>> +{
>>>> +    qtest_start("-device " TYPE_SPAPR_PCI_HOST_BRIDGE ",index=100");
>>>> +
>>>> +    qtest_end();
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>> +{
>>>> +    g_test_init(&argc, &argv, NULL);
>>>> +
>>>> +    qtest_add_func("/qmp/phb_device", test_phb_device);
>>>> +
>>>> +    return g_test_run();
>>>> +}
>>>> +
>>>
>>> Trailing white line FWIW.
>>>
>>> This is looking good, thanks for your work. Apart from the device
>>> there's nothing ppc-specific in here, so I might take it through
>>> qom-next. One nit here is the test name, which I think should rather be
>>> /spapr-phb/device (nothing QMP-specific in there) - I could tweak that
>>> for you.
>>>
>>> Another question is, do you plan to test variations of the device?
>>> Otherwise I would suggest to place the qtest_start() and qtest_end()
>>> into main(), so that individual future tests can test certain
>>> functionalities on the instantiated device rather than repeatedly
>>> launching QEMU processes.
>>
>> Is this better? It does the job.
> 
> Hmm... ;)
> 
>> I am not quite sure what qtest_add_func() is for. Is there any good example
>> of calling such a function via json or whatever it is? Thanks.
> 
> There are lots of other tests/*-test.c files, including the series you
> replied to. I've tweaked your original patch so that it outputs:
> 
> $ make check-qtest-ppc64 V=1
> [...]
> TEST: tests/spapr-phb-test... (pid=3080)
>   /ppc64/spapr-phb/device:                                            OK
> PASS: tests/spapr-phb-test
> 
> "/spapr-phb/device" is the name of the unit test supplied to
> qtest_add_func(), which then prepends the architecture for uniqueness.
> Without calling it, there is no indication that the test was successful.
> 
> Individual tests reading/writing registers etc. will need the
> qemu-system-ppc64 process running, so g_test_run() needs to go before
> qtest_end(). Or if you want to launch it multiple times (e.g. with
> different parameters) place it into the test function like originally.
> 
> Your test did not pass on qemu.git or qom-next - I picked up the
> corresponding fix from ppc-next to reallow -device spapr-pci-host-bridge.

Sure it did not pass, this is why I made the test ;)

But I wonder why this fix not gone to upstream immediately - it was breakage.


> Further the patch was missing a line for gcov to pick up the .o file:



> Gcov report for ppc64-softmmu/hw/ppc/spapr_pci.c:
> File '/home/andreas/QEMU/qemu/hw/ppc/spapr_pci.c'
> Lines executed:42.98% of 342



What did you do to get this output? None from "make check-help" seems to do
this.

btw it would nice if people updated "make check-help" when they add new
tests as without documentation this is all we have to know what tests are
there. For example, it was not very obvious how
check-tests/qemu-iotests-quick.sh is called and how it works.

Or there is some howto and I am just disturbingly blind? Thanks.


Also thanks for comments and tweaks but I still do not understand if I
still have to do anything regarding these 2 patches.


> 
> File '../trace/generated-tracers.h'
> Lines executed:0.00% of 12
> 
> File '/usr/include/libfdt.h'
> Lines executed:100.00% of 5
> 
> File '/usr/include/libfdt_env.h'
> Lines executed:100.00% of 2
> 
> File '/home/andreas/QEMU/qemu/include/hw/pci-host/spapr.h'
> Lines executed:0.00% of 2
> 
> File '/home/andreas/QEMU/qemu/include/hw/ppc/spapr.h'
> Lines executed:22.22% of 9
> 
> File '/home/andreas/QEMU/qemu/include/hw/pci/pci.h'
> Lines executed:0.00% of 4
> 
> File '/home/andreas/QEMU/qemu/include/hw/irq.h'
> Lines executed:0.00% of 4
> 
> File '/home/andreas/QEMU/qemu/include/qemu/bitops.h'
> Lines executed:100.00% of 5
> 
> File '/home/andreas/QEMU/qemu/include/qemu/bswap.h'
> Lines executed:100.00% of 6
> 
> File '/usr/include/bits/byteswap.h'
> Lines executed:100.00% of 4
> 
> 
> Regards,
> Andreas
> 
>> diff --git a/tests/spapr-phb-test.c b/tests/spapr-phb-test.c
>> new file mode 100644
>> index 0000000..11f186c
>> --- /dev/null
>> +++ b/tests/spapr-phb-test.c
>> @@ -0,0 +1,24 @@
>> +/*
>> + * QTest testcase for SPAPR PHB
>> + *
>> + * Authors:
>> + *  Alexey Kardashevskiy <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#include <glib.h>
>> +
>> +#include "libqtest.h"
>> +
>> +#define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    qtest_start("-device " TYPE_SPAPR_PCI_HOST_BRIDGE ",index=100");
>> +    qtest_end();
>> +
>> +    return g_test_run();
>> +}
> 


-- 
Alexey