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

["Andreas Färber" <afaerber@suse.de>]

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 <aik@ozlabs.ru>
> ---
>  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.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg