[Qemu-devel] [PATCH for-2.11 0/6] Enable more qtests for s390x

[Qemu-devel] [PATCH for-2.11 0/6] Enable more qtests for s390x

[Thomas Huth]

We currently do not have many tests enabled for QEMU on s390x yet,
so this series reworks some of the tests to be also usable on s390x
to get some extended test coverage there.

Along the way, this also cleans up some of the generic test code,
e.g. by introducing generic functions to do hot-plugging with
the device_add QMP command.

Please review!

 Thanks,
  Thomas


Thomas Huth (6):
  tests: Run filter-redirector and -mirror test only on POSIX systems
  tests: Add network filter tests to the check-qtest-s390x list
  tests: Enable the drive_del test also on s390x
  tests: Introduce generic device hot-plug/hot-unplug functions
  tests: Add qvirtio_(un)plug_device_test wrapper functions
  tests: Enable the simple virtio tests on s390x, too

 tests/Makefile.include         | 20 +++++++++-----
 tests/drive_del-test.c         | 13 ++++-----
 tests/libqos/pci.c             | 19 ++-----------
 tests/libqos/usb.c             | 30 +++++----------------
 tests/libqos/virtio.c          | 29 ++++++++++++++++++++
 tests/libqos/virtio.h          |  5 ++++
 tests/libqtest.c               | 60 ++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h               | 19 +++++++++++++
 tests/test-filter-mirror.c     | 14 +++++-----
 tests/test-filter-redirector.c | 32 +++++++++++-----------
 tests/test-netfilter.c         | 11 +++++++-
 tests/usb-hcd-uhci-test.c      | 26 ++----------------
 tests/usb-hcd-xhci-test.c      | 51 +++--------------------------------
 tests/virtio-balloon-test.c    |  6 ++---
 tests/virtio-console-test.c    | 12 ++++-----
 tests/virtio-net-test.c        |  8 ++----
 tests/virtio-rng-test.c        | 17 +++++-------
 tests/virtio-scsi-test.c       | 24 ++---------------
 tests/virtio-serial-test.c     | 33 +++++------------------
 19 files changed, 208 insertions(+), 221 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] tests: Run filter-redirector and -mirror test only on POSIX systems

[Thomas Huth]

This way we can get rid of the ugly #ifdefs in the code which makes
it easier to extend later.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include         |  8 ++++----
 tests/test-filter-mirror.c     |  5 -----
 tests/test-filter-redirector.c | 10 ----------
 3 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 37c1bed..8d5991d 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -283,8 +283,8 @@ ifeq ($(CONFIG_VHOST_USER_NET_TEST_i386),)
 check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
 endif
 check-qtest-i386-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
-check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
-check-qtest-i386-y += tests/test-filter-redirector$(EXESUF)
+check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
+check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/postcopy-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
@@ -325,8 +325,8 @@ check-qtest-ppc64-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-ppc64-y += hw/usb/hcd-xhci.c
 check-qtest-ppc64-y += $(check-qtest-virtio-y)
 check-qtest-ppc64-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
-check-qtest-ppc64-y += tests/test-filter-mirror$(EXESUF)
-check-qtest-ppc64-y += tests/test-filter-redirector$(EXESUF)
+check-qtest-ppc64-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
+check-qtest-ppc64-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
index 9f84402..a1d5865 100644
--- a/tests/test-filter-mirror.c
+++ b/tests/test-filter-mirror.c
@@ -17,9 +17,6 @@
 
 static void test_mirror(void)
 {
-#ifndef _WIN32
-/* socketpair(PF_UNIX) which does not exist on windows */
-
     int send_sock[2], recv_sock;
     char *cmdline;
     uint32_t ret = 0, len = 0;
@@ -74,8 +71,6 @@ static void test_mirror(void)
     g_free(recv_buf);
     close(recv_sock);
     unlink(sock_path);
-
-#endif
 }
 
 int main(int argc, char **argv)
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index 0c4b8d5..69c663b 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -59,9 +59,6 @@
 
 static void test_redirector_tx(void)
 {
-#ifndef _WIN32
-/* socketpair(PF_UNIX) which does not exist on windows */
-
     int backend_sock[2], recv_sock;
     char *cmdline;
     uint32_t ret = 0, len = 0;
@@ -129,15 +126,10 @@ static void test_redirector_tx(void)
     unlink(sock_path0);
     unlink(sock_path1);
     qtest_end();
-
-#endif
 }
 
 static void test_redirector_rx(void)
 {
-#ifndef _WIN32
-/* socketpair(PF_UNIX) which does not exist on windows */
-
     int backend_sock[2], send_sock;
     char *cmdline;
     uint32_t ret = 0, len = 0;
@@ -203,8 +195,6 @@ static void test_redirector_rx(void)
     unlink(sock_path0);
     unlink(sock_path1);
     qtest_end();
-
-#endif
 }
 
 int main(int argc, char **argv)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/6] tests: Run filter-redirector and -mirror test only on POSIX systems

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:08 +0200
Thomas Huth <thuth@redhat.com> wrote:

> This way we can get rid of the ugly #ifdefs in the code which makes
> it easier to extend later.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include         |  8 ++++----
>  tests/test-filter-mirror.c     |  5 -----
>  tests/test-filter-redirector.c | 10 ----------
>  3 files changed, 4 insertions(+), 19 deletions(-)

Yes, that is much nicer.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH 1/6] tests: Run filter-redirector and -mirror test only on POSIX systems

[David Hildenbrand]

On 17.08.2017 08:25, Thomas Huth wrote:
> This way we can get rid of the ugly #ifdefs in the code which makes
> it easier to extend later.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 1/6] tests: Run filter-redirector and -mirror test only on POSIX systems

[Zhang Chen]

On 08/17/2017 02:25 PM, Thomas Huth wrote:
> This way we can get rid of the ugly #ifdefs in the code which makes
> it easier to extend later.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Zhang Chen<zhangchen.fnst@cn.fujitsu.com>

It's better for my code.

Thanks
Zhang Chen

> ---
>   tests/Makefile.include         |  8 ++++----
>   tests/test-filter-mirror.c     |  5 -----
>   tests/test-filter-redirector.c | 10 ----------
>   3 files changed, 4 insertions(+), 19 deletions(-)
>

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH 1/6] tests: Run filter-redirector and -mirror test only on POSIX systems

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 4138 bytes --]



On 08/17/2017 02:25 AM, Thomas Huth wrote:
> This way we can get rid of the ugly #ifdefs in the code which makes
> it easier to extend later.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include         |  8 ++++----
>  tests/test-filter-mirror.c     |  5 -----
>  tests/test-filter-redirector.c | 10 ----------
>  3 files changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 37c1bed..8d5991d 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -283,8 +283,8 @@ ifeq ($(CONFIG_VHOST_USER_NET_TEST_i386),)
>  check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
>  endif
>  check-qtest-i386-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
> -check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
> -check-qtest-i386-y += tests/test-filter-redirector$(EXESUF)
> +check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
> +check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
>  check-qtest-i386-y += tests/postcopy-test$(EXESUF)
>  check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
>  check-qtest-i386-y += tests/numa-test$(EXESUF)
> @@ -325,8 +325,8 @@ check-qtest-ppc64-y += tests/usb-hcd-xhci-test$(EXESUF)
>  gcov-files-ppc64-y += hw/usb/hcd-xhci.c
>  check-qtest-ppc64-y += $(check-qtest-virtio-y)
>  check-qtest-ppc64-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
> -check-qtest-ppc64-y += tests/test-filter-mirror$(EXESUF)
> -check-qtest-ppc64-y += tests/test-filter-redirector$(EXESUF)
> +check-qtest-ppc64-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
> +check-qtest-ppc64-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
>  check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
>  check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>  check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
> index 9f84402..a1d5865 100644
> --- a/tests/test-filter-mirror.c
> +++ b/tests/test-filter-mirror.c
> @@ -17,9 +17,6 @@
>  
>  static void test_mirror(void)
>  {
> -#ifndef _WIN32
> -/* socketpair(PF_UNIX) which does not exist on windows */
> -
>      int send_sock[2], recv_sock;
>      char *cmdline;
>      uint32_t ret = 0, len = 0;
> @@ -74,8 +71,6 @@ static void test_mirror(void)
>      g_free(recv_buf);
>      close(recv_sock);
>      unlink(sock_path);
> -
> -#endif
>  }
>  
>  int main(int argc, char **argv)
> diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
> index 0c4b8d5..69c663b 100644
> --- a/tests/test-filter-redirector.c
> +++ b/tests/test-filter-redirector.c
> @@ -59,9 +59,6 @@
>  
>  static void test_redirector_tx(void)
>  {
> -#ifndef _WIN32
> -/* socketpair(PF_UNIX) which does not exist on windows */
> -
>      int backend_sock[2], recv_sock;
>      char *cmdline;
>      uint32_t ret = 0, len = 0;
> @@ -129,15 +126,10 @@ static void test_redirector_tx(void)
>      unlink(sock_path0);
>      unlink(sock_path1);
>      qtest_end();
> -
> -#endif
>  }
>  
>  static void test_redirector_rx(void)
>  {
> -#ifndef _WIN32
> -/* socketpair(PF_UNIX) which does not exist on windows */
> -
>      int backend_sock[2], send_sock;
>      char *cmdline;
>      uint32_t ret = 0, len = 0;
> @@ -203,8 +195,6 @@ static void test_redirector_rx(void)
>      unlink(sock_path0);
>      unlink(sock_path1);
>      qtest_end();
> -
> -#endif
>  }
>  
>  int main(int argc, char **argv)
> 

Tested attempting to cross-compile those:

 $ ./configure --cross-prefix=i686-w64-mingw32-
 $ make
 $ make tests/test-filter-mirror.exe
 ...
  CC      tests/test-filter-redirector.o
tests/test-filter-redirector.c: In function 'test_redirector_tx':
tests/test-filter-redirector.c:72:11: error: implicit declaration of
function 'socketpair'; did you mean 'socket_init'?
[-Werror=implicit-function-declaration]
 ...

Which is supposed to fail.

Tested-by: Cleber Rosa <crosa@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Thomas Huth]

With some small modifications, we can also use the the netfilter,
the fiter-mirror and the filter-redirector tests on s390x.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include         |  3 +++
 tests/test-filter-mirror.c     |  9 +++++++--
 tests/test-filter-redirector.c | 22 ++++++++++++++++------
 tests/test-netfilter.c         | 11 ++++++++++-
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 8d5991d..0bb18b3 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -360,6 +360,9 @@ check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
 check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
+check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
+check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
+check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
index a1d5865..d569d27 100644
--- a/tests/test-filter-mirror.c
+++ b/tests/test-filter-mirror.c
@@ -25,6 +25,11 @@ static void test_mirror(void)
     char *recv_buf;
     uint32_t size = sizeof(send_buf);
     size = htonl(size);
+    const char *devstr = "e1000";
+
+    if (g_str_equal(qtest_get_arch(), "s390x")) {
+        devstr = "virtio-net-ccw";
+    }
 
     ret = socketpair(PF_UNIX, SOCK_STREAM, 0, send_sock);
     g_assert_cmpint(ret, !=, -1);
@@ -33,10 +38,10 @@ static void test_mirror(void)
     g_assert_cmpint(ret, !=, -1);
 
     cmdline = g_strdup_printf("-netdev socket,id=qtest-bn0,fd=%d "
-                 "-device e1000,netdev=qtest-bn0,id=qtest-e0 "
+                 "-device %s,netdev=qtest-bn0,id=qtest-e0 "
                  "-chardev socket,id=mirror0,path=%s,server,nowait "
                  "-object filter-mirror,id=qtest-f0,netdev=qtest-bn0,queue=tx,outdev=mirror0 "
-                 , send_sock[1], sock_path);
+                 , send_sock[1], devstr, sock_path);
     qtest_start(cmdline);
     g_free(cmdline);
 
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index 69c663b..3afd411 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -57,6 +57,16 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 
+static const char *get_devstr(void)
+{
+    if (g_str_equal(qtest_get_arch(), "s390x")) {
+        return "virtio-net-ccw";
+    }
+
+    return "rtl8139";
+}
+
+
 static void test_redirector_tx(void)
 {
     int backend_sock[2], recv_sock;
@@ -78,7 +88,7 @@ static void test_redirector_tx(void)
     g_assert_cmpint(ret, !=, -1);
 
     cmdline = g_strdup_printf("-netdev socket,id=qtest-bn0,fd=%d "
-                "-device rtl8139,netdev=qtest-bn0,id=qtest-e0 "
+                "-device %s,netdev=qtest-bn0,id=qtest-e0 "
                 "-chardev socket,id=redirector0,path=%s,server,nowait "
                 "-chardev socket,id=redirector1,path=%s,server,nowait "
                 "-chardev socket,id=redirector2,path=%s,nowait "
@@ -87,8 +97,8 @@ static void test_redirector_tx(void)
                 "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
                 "queue=tx,indev=redirector2 "
                 "-object filter-redirector,id=qtest-f2,netdev=qtest-bn0,"
-                "queue=tx,outdev=redirector1 "
-                , backend_sock[1], sock_path0, sock_path1, sock_path0);
+                "queue=tx,outdev=redirector1 ", backend_sock[1], get_devstr(),
+                sock_path0, sock_path1, sock_path0);
     qtest_start(cmdline);
     g_free(cmdline);
 
@@ -149,7 +159,7 @@ static void test_redirector_rx(void)
     g_assert_cmpint(ret, !=, -1);
 
     cmdline = g_strdup_printf("-netdev socket,id=qtest-bn0,fd=%d "
-                "-device rtl8139,netdev=qtest-bn0,id=qtest-e0 "
+                "-device %s,netdev=qtest-bn0,id=qtest-e0 "
                 "-chardev socket,id=redirector0,path=%s,server,nowait "
                 "-chardev socket,id=redirector1,path=%s,server,nowait "
                 "-chardev socket,id=redirector2,path=%s,nowait "
@@ -158,8 +168,8 @@ static void test_redirector_rx(void)
                 "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
                 "queue=rx,outdev=redirector2 "
                 "-object filter-redirector,id=qtest-f2,netdev=qtest-bn0,"
-                "queue=rx,indev=redirector1 "
-                , backend_sock[1], sock_path0, sock_path1, sock_path0);
+                "queue=rx,indev=redirector1 ", backend_sock[1], get_devstr(),
+                sock_path0, sock_path1, sock_path0);
     qtest_start(cmdline);
     g_free(cmdline);
 
diff --git a/tests/test-netfilter.c b/tests/test-netfilter.c
index 8b5a9b2..2506473 100644
--- a/tests/test-netfilter.c
+++ b/tests/test-netfilter.c
@@ -182,6 +182,12 @@ static void remove_netdev_with_multi_netfilter(void)
 int main(int argc, char **argv)
 {
     int ret;
+    char *args;
+    const char *devstr = "e1000";
+
+    if (g_str_equal(qtest_get_arch(), "s390x")) {
+        devstr = "virtio-net-ccw";
+    }
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/netfilter/addremove_one", add_one_netfilter);
@@ -191,10 +197,13 @@ int main(int argc, char **argv)
     qtest_add_func("/netfilter/remove_netdev_multi",
                    remove_netdev_with_multi_netfilter);
 
-    qtest_start("-netdev user,id=qtest-bn0 -device e1000,netdev=qtest-bn0");
+    args = g_strdup_printf("-netdev user,id=qtest-bn0 "
+                           "-device %s,netdev=qtest-bn0", devstr);
+    qtest_start(args);
     ret = g_test_run();
 
     qtest_end();
+    g_free(args);
 
     return ret;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:09 +0200
Thomas Huth <thuth@redhat.com> wrote:

> With some small modifications, we can also use the the netfilter,
> the fiter-mirror and the filter-redirector tests on s390x.

s/fiter/filter/

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include         |  3 +++
>  tests/test-filter-mirror.c     |  9 +++++++--
>  tests/test-filter-redirector.c | 22 ++++++++++++++++------
>  tests/test-netfilter.c         | 11 ++++++++++-
>  4 files changed, 36 insertions(+), 9 deletions(-)
> 

> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
> index a1d5865..d569d27 100644
> --- a/tests/test-filter-mirror.c
> +++ b/tests/test-filter-mirror.c
> @@ -25,6 +25,11 @@ static void test_mirror(void)
>      char *recv_buf;
>      uint32_t size = sizeof(send_buf);
>      size = htonl(size);
> +    const char *devstr = "e1000";
> +
> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
> +        devstr = "virtio-net-ccw";
> +    }

I'm wondering if we could unify selection of the network device
somehow. There's probably two cases:
- Test a specific device. This obviously needs to be decided
  individually.
- Just use a functional network device. For s390x, this will be
  virtio-net-ccw; for other architectures, this test uses e1000, while
  one of the tests below uses rtl8139 (why?). A helper for that may be
  useful.

>  
>      ret = socketpair(PF_UNIX, SOCK_STREAM, 0, send_sock);
>      g_assert_cmpint(ret, !=, -1);

> diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
> index 69c663b..3afd411 100644
> --- a/tests/test-filter-redirector.c
> +++ b/tests/test-filter-redirector.c
> @@ -57,6 +57,16 @@
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
>  
> +static const char *get_devstr(void)
> +{
> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
> +        return "virtio-net-ccw";
> +    }
> +
> +    return "rtl8139";

No problem with your patch, but I'm wondering why this does not use
e1000. Special capabilities of rtl8139?

> +}
> +
> +
>  static void test_redirector_tx(void)
>  {
>      int backend_sock[2], recv_sock;

> diff --git a/tests/test-netfilter.c b/tests/test-netfilter.c
> index 8b5a9b2..2506473 100644
> --- a/tests/test-netfilter.c
> +++ b/tests/test-netfilter.c
> @@ -182,6 +182,12 @@ static void remove_netdev_with_multi_netfilter(void)
>  int main(int argc, char **argv)
>  {
>      int ret;
> +    char *args;
> +    const char *devstr = "e1000";

It's our old friend again :)

> +
> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
> +        devstr = "virtio-net-ccw";
> +    }
>  
>      g_test_init(&argc, &argv, NULL);
>      qtest_add_func("/netfilter/addremove_one", add_one_netfilter);
> @@ -191,10 +197,13 @@ int main(int argc, char **argv)
>      qtest_add_func("/netfilter/remove_netdev_multi",
>                     remove_netdev_with_multi_netfilter);
>  
> -    qtest_start("-netdev user,id=qtest-bn0 -device e1000,netdev=qtest-bn0");
> +    args = g_strdup_printf("-netdev user,id=qtest-bn0 "
> +                           "-device %s,netdev=qtest-bn0", devstr);
> +    qtest_start(args);
>      ret = g_test_run();
>  
>      qtest_end();
> +    g_free(args);
>  
>      return ret;
>  }

Even though I think we should deal with the questions above, having
more tests for s390x is certainly a good idea. Thus,

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Thomas Huth]

On 17.08.2017 10:41, Cornelia Huck wrote:
> On Thu, 17 Aug 2017 08:25:09 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> With some small modifications, we can also use the the netfilter,
>> the fiter-mirror and the filter-redirector tests on s390x.
> 
> s/fiter/filter/

OK ... could you please fix that when picking up the patch (in case I do
not have to resend)?

>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/Makefile.include         |  3 +++
>>  tests/test-filter-mirror.c     |  9 +++++++--
>>  tests/test-filter-redirector.c | 22 ++++++++++++++++------
>>  tests/test-netfilter.c         | 11 ++++++++++-
>>  4 files changed, 36 insertions(+), 9 deletions(-)
>>
> 
>> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
>> index a1d5865..d569d27 100644
>> --- a/tests/test-filter-mirror.c
>> +++ b/tests/test-filter-mirror.c
>> @@ -25,6 +25,11 @@ static void test_mirror(void)
>>      char *recv_buf;
>>      uint32_t size = sizeof(send_buf);
>>      size = htonl(size);
>> +    const char *devstr = "e1000";
>> +
>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>> +        devstr = "virtio-net-ccw";
>> +    }
> 
> I'm wondering if we could unify selection of the network device
> somehow. There's probably two cases:
> - Test a specific device. This obviously needs to be decided
>   individually.
> - Just use a functional network device. For s390x, this will be
>   virtio-net-ccw; for other architectures, this test uses e1000, while
>   one of the tests below uses rtl8139 (why?). A helper for that may be
>   useful.

Maybe ... OTOH, this likely increases also test coverage if we do not
use the same PCI NIC in all the tests...?

>>  
>>      ret = socketpair(PF_UNIX, SOCK_STREAM, 0, send_sock);
>>      g_assert_cmpint(ret, !=, -1);
> 
>> diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
>> index 69c663b..3afd411 100644
>> --- a/tests/test-filter-redirector.c
>> +++ b/tests/test-filter-redirector.c
>> @@ -57,6 +57,16 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/main-loop.h"
>>  
>> +static const char *get_devstr(void)
>> +{
>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>> +        return "virtio-net-ccw";
>> +    }
>> +
>> +    return "rtl8139";
> 
> No problem with your patch, but I'm wondering why this does not use
> e1000. Special capabilities of rtl8139?

Maybe Zhang Chen can answer that question? (Now on CC: - forgot to do
that initially, sorry!)
But I guess it's just an arbitrary NIC that works on most of the
platforms (i.e. a normal PCI NIC)...?

>> +}
>> +
>> +
>>  static void test_redirector_tx(void)
>>  {
>>      int backend_sock[2], recv_sock;
> 
>> diff --git a/tests/test-netfilter.c b/tests/test-netfilter.c
>> index 8b5a9b2..2506473 100644
>> --- a/tests/test-netfilter.c
>> +++ b/tests/test-netfilter.c
>> @@ -182,6 +182,12 @@ static void remove_netdev_with_multi_netfilter(void)
>>  int main(int argc, char **argv)
>>  {
>>      int ret;
>> +    char *args;
>> +    const char *devstr = "e1000";
> 
> It's our old friend again :)
> 
>> +
>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>> +        devstr = "virtio-net-ccw";
>> +    }
>>  
>>      g_test_init(&argc, &argv, NULL);
>>      qtest_add_func("/netfilter/addremove_one", add_one_netfilter);
>> @@ -191,10 +197,13 @@ int main(int argc, char **argv)
>>      qtest_add_func("/netfilter/remove_netdev_multi",
>>                     remove_netdev_with_multi_netfilter);
>>  
>> -    qtest_start("-netdev user,id=qtest-bn0 -device e1000,netdev=qtest-bn0");
>> +    args = g_strdup_printf("-netdev user,id=qtest-bn0 "
>> +                           "-device %s,netdev=qtest-bn0", devstr);
>> +    qtest_start(args);
>>      ret = g_test_run();
>>  
>>      qtest_end();
>> +    g_free(args);
>>  
>>      return ret;
>>  }
> 
> Even though I think we should deal with the questions above, having
> more tests for s390x is certainly a good idea. Thus,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thanks for the review,
 Thomas

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Zhang Chen]

On 08/17/2017 10:02 PM, Thomas Huth wrote:
> On 17.08.2017 10:41, Cornelia Huck wrote:
>> On Thu, 17 Aug 2017 08:25:09 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> With some small modifications, we can also use the the netfilter,
>>> the fiter-mirror and the filter-redirector tests on s390x.
>> s/fiter/filter/
> OK ... could you please fix that when picking up the patch (in case I do
> not have to resend)?
>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   tests/Makefile.include         |  3 +++
>>>   tests/test-filter-mirror.c     |  9 +++++++--
>>>   tests/test-filter-redirector.c | 22 ++++++++++++++++------
>>>   tests/test-netfilter.c         | 11 ++++++++++-
>>>   4 files changed, 36 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
>>> index a1d5865..d569d27 100644
>>> --- a/tests/test-filter-mirror.c
>>> +++ b/tests/test-filter-mirror.c
>>> @@ -25,6 +25,11 @@ static void test_mirror(void)
>>>       char *recv_buf;
>>>       uint32_t size = sizeof(send_buf);
>>>       size = htonl(size);
>>> +    const char *devstr = "e1000";
>>> +
>>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>>> +        devstr = "virtio-net-ccw";
>>> +    }
>> I'm wondering if we could unify selection of the network device
>> somehow. There's probably two cases:
>> - Test a specific device. This obviously needs to be decided
>>    individually.
>> - Just use a functional network device. For s390x, this will be
>>    virtio-net-ccw; for other architectures, this test uses e1000, while
>>    one of the tests below uses rtl8139 (why?). A helper for that may be
>>    useful.
> Maybe ... OTOH, this likely increases also test coverage if we do not
> use the same PCI NIC in all the tests...?
>
>>>   
>>>       ret = socketpair(PF_UNIX, SOCK_STREAM, 0, send_sock);
>>>       g_assert_cmpint(ret, !=, -1);
>>> diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
>>> index 69c663b..3afd411 100644
>>> --- a/tests/test-filter-redirector.c
>>> +++ b/tests/test-filter-redirector.c
>>> @@ -57,6 +57,16 @@
>>>   #include "qemu/error-report.h"
>>>   #include "qemu/main-loop.h"
>>>   
>>> +static const char *get_devstr(void)
>>> +{
>>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>>> +        return "virtio-net-ccw";
>>> +    }
>>> +
>>> +    return "rtl8139";
>> No problem with your patch, but I'm wondering why this does not use
>> e1000. Special capabilities of rtl8139?
> Maybe Zhang Chen can answer that question? (Now on CC: - forgot to do
> that initially, sorry!)
> But I guess it's just an arbitrary NIC that works on most of the
> platforms (i.e. a normal PCI NIC)...?

Yes, you are right, we can use other NIC here(like e1000).

Reviewed-by: Zhang Chen<zhangchen.fnst@cn.fujitsu.com>


Thanks
Zhang Chen

>
>>> +}
>>> +
>>> +
>>>   static void test_redirector_tx(void)
>>>   {
>>>       int backend_sock[2], recv_sock;
>>> diff --git a/tests/test-netfilter.c b/tests/test-netfilter.c
>>> index 8b5a9b2..2506473 100644
>>> --- a/tests/test-netfilter.c
>>> +++ b/tests/test-netfilter.c
>>> @@ -182,6 +182,12 @@ static void remove_netdev_with_multi_netfilter(void)
>>>   int main(int argc, char **argv)
>>>   {
>>>       int ret;
>>> +    char *args;
>>> +    const char *devstr = "e1000";
>> It's our old friend again :)
>>
>>> +
>>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>>> +        devstr = "virtio-net-ccw";
>>> +    }
>>>   
>>>       g_test_init(&argc, &argv, NULL);
>>>       qtest_add_func("/netfilter/addremove_one", add_one_netfilter);
>>> @@ -191,10 +197,13 @@ int main(int argc, char **argv)
>>>       qtest_add_func("/netfilter/remove_netdev_multi",
>>>                      remove_netdev_with_multi_netfilter);
>>>   
>>> -    qtest_start("-netdev user,id=qtest-bn0 -device e1000,netdev=qtest-bn0");
>>> +    args = g_strdup_printf("-netdev user,id=qtest-bn0 "
>>> +                           "-device %s,netdev=qtest-bn0", devstr);
>>> +    qtest_start(args);
>>>       ret = g_test_run();
>>>   
>>>       qtest_end();
>>> +    g_free(args);
>>>   
>>>       return ret;
>>>   }
>> Even though I think we should deal with the questions above, having
>> more tests for s390x is certainly a good idea. Thus,
>>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Thanks for the review,
>   Thomas
>
>
> .
>

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Cornelia Huck]

On Thu, 17 Aug 2017 16:02:00 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17.08.2017 10:41, Cornelia Huck wrote:
> > On Thu, 17 Aug 2017 08:25:09 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> With some small modifications, we can also use the the netfilter,
> >> the fiter-mirror and the filter-redirector tests on s390x.  
> > 
> > s/fiter/filter/  
> 
> OK ... could you please fix that when picking up the patch (in case I do
> not have to resend)?

Sure.

> 
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  tests/Makefile.include         |  3 +++
> >>  tests/test-filter-mirror.c     |  9 +++++++--
> >>  tests/test-filter-redirector.c | 22 ++++++++++++++++------
> >>  tests/test-netfilter.c         | 11 ++++++++++-
> >>  4 files changed, 36 insertions(+), 9 deletions(-)
> >>  
> >   
> >> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
> >> index a1d5865..d569d27 100644
> >> --- a/tests/test-filter-mirror.c
> >> +++ b/tests/test-filter-mirror.c
> >> @@ -25,6 +25,11 @@ static void test_mirror(void)
> >>      char *recv_buf;
> >>      uint32_t size = sizeof(send_buf);
> >>      size = htonl(size);
> >> +    const char *devstr = "e1000";
> >> +
> >> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
> >> +        devstr = "virtio-net-ccw";
> >> +    }  
> > 
> > I'm wondering if we could unify selection of the network device
> > somehow. There's probably two cases:
> > - Test a specific device. This obviously needs to be decided
> >   individually.
> > - Just use a functional network device. For s390x, this will be
> >   virtio-net-ccw; for other architectures, this test uses e1000, while
> >   one of the tests below uses rtl8139 (why?). A helper for that may be
> >   useful.  
> 
> Maybe ... OTOH, this likely increases also test coverage if we do not
> use the same PCI NIC in all the tests...?

It just looks like a bit of unneeded churn to me.

Re coverage: Do we have a very simple test that we can run for all kind
of NICs? This would give some reliable testing for various devices
instead of having to rely on people picking different devices for their
tests...

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Thomas Huth]

On 18.08.2017 09:54, Cornelia Huck wrote:
> On Thu, 17 Aug 2017 16:02:00 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 17.08.2017 10:41, Cornelia Huck wrote:
[...]
>>> I'm wondering if we could unify selection of the network device
>>> somehow. There's probably two cases:
>>> - Test a specific device. This obviously needs to be decided
>>>   individually.
>>> - Just use a functional network device. For s390x, this will be
>>>   virtio-net-ccw; for other architectures, this test uses e1000, while
>>>   one of the tests below uses rtl8139 (why?). A helper for that may be
>>>   useful.  
>>
>> Maybe ... OTOH, this likely increases also test coverage if we do not
>> use the same PCI NIC in all the tests...?
> 
> It just looks like a bit of unneeded churn to me.
> 
> Re coverage: Do we have a very simple test that we can run for all kind
> of NICs? This would give some reliable testing for various devices
> instead of having to rely on people picking different devices for their
> tests...

I think there is only the pxe-tester that comes close to a generic NIC
tester. But there are two issues:
1) You need a firmware that has a driver for the NIC
2) It's not a very fast test, so adding lots of NICs there might slow
   down "make check" quite a bit.

(There are also some dedicated NIC tests available already, e.g.
tests/rtl8139-test.c tests at least some aspects of that NIC.)

Hmm, maybe we could also use a function that returns a different NIC for
the i386 and x86_64 architectures, something like:

char *get_preferred_nic_name(void)
{
    const char *arch = qtest_get_arch();

    if (g_str_equal(arch, "i386")) {
        return "rtl8139";
    } else if (g_str_equal(arch, "s390x")) {
        return "virtio-net-ccw";
    } else if (g_str_equal(arch, "ppc64")) {
        return "spapr-vlan";
    } else {
        return "e1000";
    }
}

That way, we'd also get test coverage for both, e1000 and rtl8139... ?

 Thomas

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]



On 08/18/2017 03:54 AM, Cornelia Huck wrote:
> On Thu, 17 Aug 2017 16:02:00 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 17.08.2017 10:41, Cornelia Huck wrote:
>>> On Thu, 17 Aug 2017 08:25:09 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> With some small modifications, we can also use the the netfilter,
>>>> the fiter-mirror and the filter-redirector tests on s390x.  
>>>
>>> s/fiter/filter/  
>>
>> OK ... could you please fix that when picking up the patch (in case I do
>> not have to resend)?
> 
> Sure.
> 
>>
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  tests/Makefile.include         |  3 +++
>>>>  tests/test-filter-mirror.c     |  9 +++++++--
>>>>  tests/test-filter-redirector.c | 22 ++++++++++++++++------
>>>>  tests/test-netfilter.c         | 11 ++++++++++-
>>>>  4 files changed, 36 insertions(+), 9 deletions(-)
>>>>  
>>>   
>>>> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
>>>> index a1d5865..d569d27 100644
>>>> --- a/tests/test-filter-mirror.c
>>>> +++ b/tests/test-filter-mirror.c
>>>> @@ -25,6 +25,11 @@ static void test_mirror(void)
>>>>      char *recv_buf;
>>>>      uint32_t size = sizeof(send_buf);
>>>>      size = htonl(size);
>>>> +    const char *devstr = "e1000";
>>>> +
>>>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>>>> +        devstr = "virtio-net-ccw";
>>>> +    }  
>>>
>>> I'm wondering if we could unify selection of the network device
>>> somehow. There's probably two cases:
>>> - Test a specific device. This obviously needs to be decided
>>>   individually.
>>> - Just use a functional network device. For s390x, this will be
>>>   virtio-net-ccw; for other architectures, this test uses e1000, while
>>>   one of the tests below uses rtl8139 (why?). A helper for that may be
>>>   useful.  
>>
>> Maybe ... OTOH, this likely increases also test coverage if we do not
>> use the same PCI NIC in all the tests...?
> 
> It just looks like a bit of unneeded churn to me.
> 
> Re coverage: Do we have a very simple test that we can run for all kind
> of NICs? This would give some reliable testing for various devices
> instead of having to rely on people picking different devices for their
> tests...
> 

The ideal approach here would be to have a predictable default network
(or any other kind of) device selection, per-target.  Then, no one would
wonder if rtl8139 was chosen on purpose for this specific test.

Besides the default predictable selection, the framework should provide
a way for test *runners* to switch to another device at test run-time.
This is the kind of thing that can improve the coverage a lot, at a very
low development cost.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/6] tests: Add network filter tests to the check-qtest-s390x list

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]



On 08/17/2017 04:41 AM, Cornelia Huck wrote:
> On Thu, 17 Aug 2017 08:25:09 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> With some small modifications, we can also use the the netfilter,
>> the fiter-mirror and the filter-redirector tests on s390x.
> 
> s/fiter/filter/
> 
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/Makefile.include         |  3 +++
>>  tests/test-filter-mirror.c     |  9 +++++++--
>>  tests/test-filter-redirector.c | 22 ++++++++++++++++------
>>  tests/test-netfilter.c         | 11 ++++++++++-
>>  4 files changed, 36 insertions(+), 9 deletions(-)
>>
> 
>> diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
>> index a1d5865..d569d27 100644
>> --- a/tests/test-filter-mirror.c
>> +++ b/tests/test-filter-mirror.c
>> @@ -25,6 +25,11 @@ static void test_mirror(void)
>>      char *recv_buf;
>>      uint32_t size = sizeof(send_buf);
>>      size = htonl(size);
>> +    const char *devstr = "e1000";
>> +
>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>> +        devstr = "virtio-net-ccw";
>> +    }
> 
> I'm wondering if we could unify selection of the network device
> somehow. There's probably two cases:
> - Test a specific device. This obviously needs to be decided
>   individually.
> - Just use a functional network device. For s390x, this will be
>   virtio-net-ccw; for other architectures, this test uses e1000, while
>   one of the tests below uses rtl8139 (why?). A helper for that may be
>   useful.
> 

+1.

>>  
>>      ret = socketpair(PF_UNIX, SOCK_STREAM, 0, send_sock);
>>      g_assert_cmpint(ret, !=, -1);
> 
>> diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
>> index 69c663b..3afd411 100644
>> --- a/tests/test-filter-redirector.c
>> +++ b/tests/test-filter-redirector.c
>> @@ -57,6 +57,16 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/main-loop.h"
>>  
>> +static const char *get_devstr(void)
>> +{
>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>> +        return "virtio-net-ccw";
>> +    }
>> +
>> +    return "rtl8139";
> 
> No problem with your patch, but I'm wondering why this does not use
> e1000. Special capabilities of rtl8139?
> 

Tested with e1000.  No problems at all.

>> +}
>> +
>> +
>>  static void test_redirector_tx(void)
>>  {
>>      int backend_sock[2], recv_sock;
> 
>> diff --git a/tests/test-netfilter.c b/tests/test-netfilter.c
>> index 8b5a9b2..2506473 100644
>> --- a/tests/test-netfilter.c
>> +++ b/tests/test-netfilter.c
>> @@ -182,6 +182,12 @@ static void remove_netdev_with_multi_netfilter(void)
>>  int main(int argc, char **argv)
>>  {
>>      int ret;
>> +    char *args;
>> +    const char *devstr = "e1000";
> 
> It's our old friend again :)
> 
>> +
>> +    if (g_str_equal(qtest_get_arch(), "s390x")) {
>> +        devstr = "virtio-net-ccw";
>> +    }
>>  
>>      g_test_init(&argc, &argv, NULL);
>>      qtest_add_func("/netfilter/addremove_one", add_one_netfilter);
>> @@ -191,10 +197,13 @@ int main(int argc, char **argv)
>>      qtest_add_func("/netfilter/remove_netdev_multi",
>>                     remove_netdev_with_multi_netfilter);
>>  
>> -    qtest_start("-netdev user,id=qtest-bn0 -device e1000,netdev=qtest-bn0");
>> +    args = g_strdup_printf("-netdev user,id=qtest-bn0 "
>> +                           "-device %s,netdev=qtest-bn0", devstr);
>> +    qtest_start(args);
>>      ret = g_test_run();
>>  
>>      qtest_end();
>> +    g_free(args);
>>  
>>      return ret;
>>  }
> 
> Even though I think we should deal with the questions above, having
> more tests for s390x is certainly a good idea. Thus,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Seconded: the network device selection is a different problem.

Reviewed-by: Cleber Rosa <crosa@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Thomas Huth]

By using the "virtio-xxx" device name aliases instead of the
"virtio-xxx-pci" names, we can use this test on s390x, too,
to check that adding and deleting also works fine with the
virtio-ccw bus.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |  1 +
 tests/drive_del-test.c | 13 +++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 0bb18b3..ff2a551 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -363,6 +363,7 @@ check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
 check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
 check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
 check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
+check-qtest-s390x-y += tests/drive_del-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 2175139..efceb31 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -65,12 +65,12 @@ static void test_after_failed_device_add(void)
 
     qtest_start("-drive if=none,id=drive0");
 
-    /* Make device_add fail.  If this leaks the virtio-blk-pci device then a
+    /* Make device_add fail.  If this leaks the virtio-blk device then a
      * reference to drive0 will also be held (via qdev properties).
      */
     response = qmp("{'execute': 'device_add',"
                    " 'arguments': {"
-                   "   'driver': 'virtio-blk-pci',"
+                   "   'driver': 'virtio-blk',"
                    "   'drive': 'drive0'"
                    "}}");
     g_assert(response);
@@ -82,7 +82,7 @@ static void test_after_failed_device_add(void)
     drive_del();
 
     /* Try to re-add the drive.  This fails with duplicate IDs if a leaked
-     * virtio-blk-pci exists that holds a reference to the old drive0.
+     * virtio-blk exists that holds a reference to the old drive0.
      */
     drive_add();
 
@@ -93,7 +93,7 @@ static void test_drive_del_device_del(void)
 {
     /* Start with a drive used by a device that unplugs instantaneously */
     qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
-                " -device virtio-scsi-pci"
+                " -device virtio-scsi"
                 " -device scsi-hd,drive=drive0,id=dev0");
 
     /*
@@ -114,9 +114,10 @@ int main(int argc, char **argv)
 
     qtest_add_func("/drive_del/without-dev", test_drive_without_dev);
 
-    /* TODO I guess any arch with PCI would do */
+    /* TODO I guess any arch with a hot-pluggable virtio bus would do */
     if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64") ||
-        !strcmp(arch, "ppc") || !strcmp(arch, "ppc64")) {
+        !strcmp(arch, "ppc") || !strcmp(arch, "ppc64") ||
+        !strcmp(arch, "s390x")) {
         qtest_add_func("/drive_del/after_failed_device_add",
                        test_after_failed_device_add);
         qtest_add_func("/blockdev/drive_del_device_del",
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:10 +0200
Thomas Huth <thuth@redhat.com> wrote:

> By using the "virtio-xxx" device name aliases instead of the
> "virtio-xxx-pci" names, we can use this test on s390x, too,
> to check that adding and deleting also works fine with the
> virtio-ccw bus.

I don't think we should leak the aliasing stuff into tests, but rather
specify the transport on a per-architecture basis explicitly. (We might
want to test virtio-pci on s390x in the future as well -- in addition
to virtio-ccw, not replacing it.)

Also, the same question as for the previous patch: Is this supposed to
test virtio explicitly, or do we just want a reasonable device?

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |  1 +
>  tests/drive_del-test.c | 13 +++++++------
>  2 files changed, 8 insertions(+), 6 deletions(-)

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[David Hildenbrand]

On 17.08.2017 10:53, Cornelia Huck wrote:
> On Thu, 17 Aug 2017 08:25:10 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> By using the "virtio-xxx" device name aliases instead of the
>> "virtio-xxx-pci" names, we can use this test on s390x, too,
>> to check that adding and deleting also works fine with the
>> virtio-ccw bus.
> 
> I don't think we should leak the aliasing stuff into tests, but rather
> specify the transport on a per-architecture basis explicitly. (We might
> want to test virtio-pci on s390x in the future as well -- in addition
> to virtio-ccw, not replacing it.)

I also remember that using virtio aliases should be avoided (e.g. we are
not supposed to introduce new ones)

> 
> Also, the same question as for the previous patch: Is this supposed to
> test virtio explicitly, or do we just want a reasonable device?


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Thomas Huth]

On 17.08.2017 11:46, David Hildenbrand wrote:
> On 17.08.2017 10:53, Cornelia Huck wrote:
>> On Thu, 17 Aug 2017 08:25:10 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> By using the "virtio-xxx" device name aliases instead of the
>>> "virtio-xxx-pci" names, we can use this test on s390x, too,
>>> to check that adding and deleting also works fine with the
>>> virtio-ccw bus.
>>
>> I don't think we should leak the aliasing stuff into tests, but rather
>> specify the transport on a per-architecture basis explicitly. (We might
>> want to test virtio-pci on s390x in the future as well -- in addition
>> to virtio-ccw, not replacing it.)
> 
> I also remember that using virtio aliases should be avoided (e.g. we are
> not supposed to introduce new ones)

Hmm, maybe the right way is to use virtio-xxx-device and hook it up to
the preferred virtio bus of the current architecture? ... I'll ponder
about that a little bit ...

>>
>> Also, the same question as for the previous patch: Is this supposed to
>> test virtio explicitly, or do we just want a reasonable device?

I think this just needs a reasonable device - but since we've got the
same problem with the virtio tests later in this series again, it's
maybe best to fix this here in the same way, I guess.

 Thomas

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Cornelia Huck]

On Thu, 17 Aug 2017 15:54:38 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17.08.2017 11:46, David Hildenbrand wrote:
> > On 17.08.2017 10:53, Cornelia Huck wrote:  
> >> On Thu, 17 Aug 2017 08:25:10 +0200
> >> Thomas Huth <thuth@redhat.com> wrote:
> >>  
> >>> By using the "virtio-xxx" device name aliases instead of the
> >>> "virtio-xxx-pci" names, we can use this test on s390x, too,
> >>> to check that adding and deleting also works fine with the
> >>> virtio-ccw bus.  
> >>
> >> I don't think we should leak the aliasing stuff into tests, but rather
> >> specify the transport on a per-architecture basis explicitly. (We might
> >> want to test virtio-pci on s390x in the future as well -- in addition
> >> to virtio-ccw, not replacing it.)  
> > 
> > I also remember that using virtio aliases should be avoided (e.g. we are
> > not supposed to introduce new ones)  
> 
> Hmm, maybe the right way is to use virtio-xxx-device and hook it up to
> the preferred virtio bus of the current architecture? ... I'll ponder
> about that a little bit ...

What about defining a per-arch default virtio transport and pointing to
that? This still allows us to explicitly test virtio-pci on s390x later.

> 
> >>
> >> Also, the same question as for the previous patch: Is this supposed to
> >> test virtio explicitly, or do we just want a reasonable device?  
> 
> I think this just needs a reasonable device - but since we've got the
> same problem with the virtio tests later in this series again, it's
> maybe best to fix this here in the same way, I guess.

Yes. virtio will probably fill the 'reasonable device' place quite
well, in any case.

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 3605 bytes --]



On 08/17/2017 02:25 AM, Thomas Huth wrote:
> By using the "virtio-xxx" device name aliases instead of the
> "virtio-xxx-pci" names, we can use this test on s390x, too,
> to check that adding and deleting also works fine with the
> virtio-ccw bus.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |  1 +
>  tests/drive_del-test.c | 13 +++++++------
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 0bb18b3..ff2a551 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -363,6 +363,7 @@ check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>  check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
>  check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
>  check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
> +check-qtest-s390x-y += tests/drive_del-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
> diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
> index 2175139..efceb31 100644
> --- a/tests/drive_del-test.c
> +++ b/tests/drive_del-test.c
> @@ -65,12 +65,12 @@ static void test_after_failed_device_add(void)
>  
>      qtest_start("-drive if=none,id=drive0");
>  
> -    /* Make device_add fail.  If this leaks the virtio-blk-pci device then a
> +    /* Make device_add fail.  If this leaks the virtio-blk device then a
>       * reference to drive0 will also be held (via qdev properties).
>       */
>      response = qmp("{'execute': 'device_add',"
>                     " 'arguments': {"
> -                   "   'driver': 'virtio-blk-pci',"
> +                   "   'driver': 'virtio-blk',"
>                     "   'drive': 'drive0'"
>                     "}}");
>      g_assert(response);
> @@ -82,7 +82,7 @@ static void test_after_failed_device_add(void)
>      drive_del();
>  
>      /* Try to re-add the drive.  This fails with duplicate IDs if a leaked
> -     * virtio-blk-pci exists that holds a reference to the old drive0.
> +     * virtio-blk exists that holds a reference to the old drive0.
>       */
>      drive_add();
>  
> @@ -93,7 +93,7 @@ static void test_drive_del_device_del(void)
>  {
>      /* Start with a drive used by a device that unplugs instantaneously */
>      qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
> -                " -device virtio-scsi-pci"
> +                " -device virtio-scsi"
>                  " -device scsi-hd,drive=drive0,id=dev0");
>  
>      /*
> @@ -114,9 +114,10 @@ int main(int argc, char **argv)
>  
>      qtest_add_func("/drive_del/without-dev", test_drive_without_dev);
>  
> -    /* TODO I guess any arch with PCI would do */
> +    /* TODO I guess any arch with a hot-pluggable virtio bus would do */
>      if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64") ||
> -        !strcmp(arch, "ppc") || !strcmp(arch, "ppc64")) {
> +        !strcmp(arch, "ppc") || !strcmp(arch, "ppc64") ||
> +        !strcmp(arch, "s390x")) {
>          qtest_add_func("/drive_del/after_failed_device_add",
>                         test_after_failed_device_add);
>          qtest_add_func("/blockdev/drive_del_device_del",
> 

I honestly can't comment on the device to be used on this test (as
others have commented).  Putting that aside, and given the fact  that I
went through the lengths of testing it on the s390x target:

Tested-by: Cleber Rosa <crosa@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/6] tests: Enable the drive_del test also on s390x

[Cornelia Huck]

On Wed, 30 Aug 2017 17:41:01 -0400
Cleber Rosa <crosa@redhat.com> wrote:

> On 08/17/2017 02:25 AM, Thomas Huth wrote:
> > By using the "virtio-xxx" device name aliases instead of the
> > "virtio-xxx-pci" names, we can use this test on s390x, too,
> > to check that adding and deleting also works fine with the
> > virtio-ccw bus.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  tests/Makefile.include |  1 +
> >  tests/drive_del-test.c | 13 +++++++------
> >  2 files changed, 8 insertions(+), 6 deletions(-)

(...)

> I honestly can't comment on the device to be used on this test (as
> others have commented).  Putting that aside, and given the fact  that I
> went through the lengths of testing it on the s390x target:
> 
> Tested-by: Cleber Rosa <crosa@redhat.com>

Do you want to test v2 of this patch as well? There's still time to add
your tag :)

[Qemu-devel] [PATCH 4/6] tests: Introduce generic device hot-plug/hot-unplug functions

[Thomas Huth]

A lot of tests provide code for adding and removing a device via the
device_add and device_del QMP commands. Maintaining this code in so
many places is cumbersome and error-prone (some of the code parts
check the responses in an incorrect way, for example), so let's
provide some proper generic qtest functions for adding and removing a
device instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/libqos/pci.c         | 19 ++-------------
 tests/libqos/usb.c         | 30 +++++------------------
 tests/libqtest.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h           | 19 +++++++++++++++
 tests/usb-hcd-uhci-test.c  | 26 ++------------------
 tests/usb-hcd-xhci-test.c  | 51 ++++-----------------------------------
 tests/virtio-scsi-test.c   | 24 ++-----------------
 tests/virtio-serial-test.c | 25 +++----------------
 8 files changed, 98 insertions(+), 156 deletions(-)

diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdead..aada753 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 void qpci_plug_device_test(const char *driver, const char *id,
                            uint8_t slot, const char *opts)
 {
-    QDict *response;
-    char *cmd;
-
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': '%s',"
-                          "   'addr': '%d',"
-                          "   %s%s"
-                          "   'id': '%s'"
-                          "}}", driver, slot,
-                          opts ? opts : "", opts ? "," : "",
-                          id);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
+                          opts ? ", " : "", opts ? opts : "");
 }
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 0cdfaec..f8d0190 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -40,34 +40,16 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 void usb_test_hotplug(const char *hcd_id, const int port,
                       void (*port_check)(void))
 {
-    QDict *response;
-    char  *cmd;
+    char  *id = g_strdup_printf("usbdev%d", port);
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': 'usb-tablet',"
-                          "   'port': '%d',"
-                          "   'bus': '%s.0',"
-                          "   'id': 'usbdev%d'"
-                          "}}", port, hcd_id, port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
+                          port, hcd_id);
 
     if (port_check) {
         port_check();
     }
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev%d'"
-                           "}}", port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device(id);
+
+    g_free(id);
 }
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f18..4339d97 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -987,3 +987,63 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
     qtest_end();
     QDECREF(response);
 }
+
+/**
+ * Generic hot-plugging test via the device_add QMP command
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...)
+{
+    QDict *response;
+    char *cmd, *opts = NULL;
+    va_list va;
+
+    if (fmt) {
+        va_start(va, fmt);
+        opts = g_strdup_vprintf(fmt, va);
+        va_end(va);
+    }
+
+    cmd = g_strdup_printf("{'execute': 'device_add',"
+                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
+                          driver, id, opts ? ", " : "", opts ? opts : "");
+    g_free(opts);
+
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    while (qdict_haskey(response, "event")) {
+        /* We can get DEVICE_DELETED events in case something went wrong */
+        g_assert_cmpstr(qdict_get_str(response, "event"), !=, "DEVICE_DELETED");
+        QDECREF(response);
+        response = qmp("");
+        g_assert(response);
+    }
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}
+
+/**
+ * Generic hot-unplugging test via the device_del QMP command
+ */
+void qtest_hot_unplug_device(const char *id)
+{
+    QDict *response;
+    char *cmd;
+
+    cmd = g_strdup_printf("{'execute': 'device_del',"
+                          " 'arguments': { 'id': '%s' }}", id);
+
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    while (qdict_haskey(response, "event")) {
+        /* We should get DEVICE_DELETED event first */
+        g_assert_cmpstr(qdict_get_str(response, "event"), ==, "DEVICE_DELETED");
+        QDECREF(response);
+        response = qmp("");
+        g_assert(response);
+    }
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 3ae5709..9c1006f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -927,4 +927,23 @@ QDict *qmp_fd(int fd, const char *fmt, ...);
  */
 void qtest_cb_for_every_machine(void (*cb)(const char *machine));
 
+/**
+ * qtest_hot_plug_device:
+ * @driver: Name of the device that should be added
+ * @id: Identification string
+ * @fmt: printf-like format string for further options to device_add
+ *
+ * Generic hot-plugging test via the device_add QMP command.
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...) GCC_FMT_ATTR(3, 4);
+
+/**
+ * qtest_hot_unplug_device:
+ * @id: Identification string
+ *
+ * Generic hot-unplugging test via the device_del QMP command.
+ */
+void qtest_hot_unplug_device(const char *id);
+
 #endif
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 5b500fe..7465bfd 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -48,31 +48,9 @@ static void test_uhci_hotplug(void)
 
 static void test_usb_storage_hotplug(void)
 {
-    QDict *response;
+    qtest_hot_plug_device("usb-storage", "usbdev0", "'drive': 'drive0'");
 
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-storage',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'usbdev0'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev0'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device("usbdev0");
 }
 
 int main(int argc, char **argv)
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 031764d..d729395 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -23,59 +23,16 @@ static void test_xhci_hotplug(void)
 
 static void test_usb_uas_hotplug(void)
 {
-    QDict *response;
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-uas',"
-                   "   'id': 'uas'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'scsi-hd',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'scsi-hd'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device("usb-uas", "uas", NULL);
+    qtest_hot_plug_device("scsi-hd", "scsihd", "'drive': 'drive0'");
 
     /* TODO:
         UAS HBA driver in libqos, to check that
         added disk is visible after BUS rescan
     */
 
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'scsi-hd'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
-
-
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'uas'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device("scsihd");
+    qtest_hot_unplug_device("uas");
 }
 
 int main(int argc, char **argv)
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 87a3b6e..6e7ba6f 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -192,32 +192,12 @@ static void pci_nop(void)
 
 static void hotplug(void)
 {
-    QDict *response;
     QOSState *qs;
 
     qs = qvirtio_scsi_start(
             "-drive id=drv1,if=none,file=null-co://,format=raw");
-    response = qmp("{\"execute\": \"device_add\","
-                   " \"arguments\": {"
-                   "   \"driver\": \"scsi-hd\","
-                   "   \"id\": \"scsi-hd\","
-                   "   \"drive\": \"drv1\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{\"execute\": \"device_del\","
-                   " \"arguments\": {"
-                   "   \"id\": \"scsi-hd\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_plug_device("scsi-hd", "scsihd", "'drive': 'drv1'");
+    qtest_hot_unplug_device("scsihd");
     qvirtio_scsi_stop(qs);
 }
 
diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c
index b14d943..97f8f52 100644
--- a/tests/virtio-serial-test.c
+++ b/tests/virtio-serial-test.c
@@ -17,28 +17,9 @@ static void pci_nop(void)
 
 static void hotplug(void)
 {
-    QDict *response;
-
-    response = qmp("{\"execute\": \"device_add\","
-                   " \"arguments\": {"
-                   "   \"driver\": \"virtserialport\","
-                   "   \"id\": \"hp-port\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{\"execute\": \"device_del\","
-                   " \"arguments\": {"
-                   "   \"id\": \"hp-port\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_plug_device("virtserialport", "hp-port", NULL);
+
+    qtest_hot_unplug_device("hp-port");
 }
 
 int main(int argc, char **argv)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/6] tests: Introduce generic device hot-plug/hot-unplug functions

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:11 +0200
Thomas Huth <thuth@redhat.com> wrote:

> A lot of tests provide code for adding and removing a device via the
> device_add and device_del QMP commands. Maintaining this code in so
> many places is cumbersome and error-prone (some of the code parts
> check the responses in an incorrect way, for example), so let's
> provide some proper generic qtest functions for adding and removing a
> device instead.

This sounds like a good idea.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/libqos/pci.c         | 19 ++-------------
>  tests/libqos/usb.c         | 30 +++++------------------
>  tests/libqtest.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/libqtest.h           | 19 +++++++++++++++
>  tests/usb-hcd-uhci-test.c  | 26 ++------------------
>  tests/usb-hcd-xhci-test.c  | 51 ++++-----------------------------------
>  tests/virtio-scsi-test.c   | 24 ++-----------------
>  tests/virtio-serial-test.c | 25 +++----------------
>  8 files changed, 98 insertions(+), 156 deletions(-)
> 

> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b9a1f18..4339d97 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -987,3 +987,63 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
>      qtest_end();
>      QDECREF(response);
>  }
> +
> +/**
> + * Generic hot-plugging test via the device_add QMP command
> + */
> +void qtest_hot_plug_device(const char *driver, const char *id,
> +                           const char *fmt, ...)
> +{
> +    QDict *response;
> +    char *cmd, *opts = NULL;
> +    va_list va;
> +
> +    if (fmt) {
> +        va_start(va, fmt);
> +        opts = g_strdup_vprintf(fmt, va);
> +        va_end(va);
> +    }
> +
> +    cmd = g_strdup_printf("{'execute': 'device_add',"
> +                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
> +                          driver, id, opts ? ", " : "", opts ? opts : "");
> +    g_free(opts);
> +
> +    response = qmp(cmd);
> +    g_free(cmd);
> +    g_assert(response);
> +    while (qdict_haskey(response, "event")) {
> +        /* We can get DEVICE_DELETED events in case something went wrong */
> +        g_assert_cmpstr(qdict_get_str(response, "event"), !=, "DEVICE_DELETED");

Is there other stuff we should check for?

> +        QDECREF(response);
> +        response = qmp("");
> +        g_assert(response);
> +    }
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +}
> +
> +/**
> + * Generic hot-unplugging test via the device_del QMP command
> + */
> +void qtest_hot_unplug_device(const char *id)
> +{
> +    QDict *response;
> +    char *cmd;
> +
> +    cmd = g_strdup_printf("{'execute': 'device_del',"
> +                          " 'arguments': { 'id': '%s' }}", id);
> +
> +    response = qmp(cmd);
> +    g_free(cmd);
> +    g_assert(response);
> +    while (qdict_haskey(response, "event")) {
> +        /* We should get DEVICE_DELETED event first */
> +        g_assert_cmpstr(qdict_get_str(response, "event"), ==, "DEVICE_DELETED");

'should' does not sound assert-worthy to me :) Is this an expected event?

> +        QDECREF(response);
> +        response = qmp("");
> +        g_assert(response);
> +    }
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +}

Re: [Qemu-devel] [PATCH 4/6] tests: Introduce generic device hot-plug/hot-unplug functions

[David Hildenbrand]

On 17.08.2017 08:25, Thomas Huth wrote:
> A lot of tests provide code for adding and removing a device via the
> device_add and device_del QMP commands. Maintaining this code in so
> many places is cumbersome and error-prone (some of the code parts
> check the responses in an incorrect way, for example), so let's
> provide some proper generic qtest functions for adding and removing a
> device instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/libqos/pci.c         | 19 ++-------------
>  tests/libqos/usb.c         | 30 +++++------------------
>  tests/libqtest.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/libqtest.h           | 19 +++++++++++++++
>  tests/usb-hcd-uhci-test.c  | 26 ++------------------
>  tests/usb-hcd-xhci-test.c  | 51 ++++-----------------------------------
>  tests/virtio-scsi-test.c   | 24 ++-----------------
>  tests/virtio-serial-test.c | 25 +++----------------
>  8 files changed, 98 insertions(+), 156 deletions(-)
> 
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 2dcdead..aada753 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
>  void qpci_plug_device_test(const char *driver, const char *id,
>                             uint8_t slot, const char *opts)
>  {
> -    QDict *response;
> -    char *cmd;
> -
> -    cmd = g_strdup_printf("{'execute': 'device_add',"
> -                          " 'arguments': {"
> -                          "   'driver': '%s',"
> -                          "   'addr': '%d',"
> -                          "   %s%s"
> -                          "   'id': '%s'"
> -                          "}}", driver, slot,
> -                          opts ? opts : "", opts ? "," : "",
> -                          id);
> -    response = qmp(cmd);
> -    g_free(cmd);
> -    g_assert(response);
> -    g_assert(!qdict_haskey(response, "error"));
> -    QDECREF(response);
> +    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
> +                          opts ? ", " : "", opts ? opts : "");
>  }
> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
> index 0cdfaec..f8d0190 100644
> --- a/tests/libqos/usb.c
> +++ b/tests/libqos/usb.c
> @@ -40,34 +40,16 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>  void usb_test_hotplug(const char *hcd_id, const int port,
>                        void (*port_check)(void))
>  {
> -    QDict *response;
> -    char  *cmd;
> +    char  *id = g_strdup_printf("usbdev%d", port);
>  
> -    cmd = g_strdup_printf("{'execute': 'device_add',"
> -                          " 'arguments': {"
> -                          "   'driver': 'usb-tablet',"
> -                          "   'port': '%d',"
> -                          "   'bus': '%s.0',"
> -                          "   'id': 'usbdev%d'"
> -                          "}}", port, hcd_id, port);
> -    response = qmp(cmd);
> -    g_free(cmd);
> -    g_assert(response);
> -    g_assert(!qdict_haskey(response, "error"));
> -    QDECREF(response);
> +    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
> +                          port, hcd_id);
>  
>      if (port_check) {
>          port_check();
>      }
>  
> -    cmd = g_strdup_printf("{'execute': 'device_del',"
> -                           " 'arguments': {"
> -                           "   'id': 'usbdev%d'"
> -                           "}}", port);
> -    response = qmp(cmd);
> -    g_free(cmd);
> -    g_assert(response);
> -    g_assert(qdict_haskey(response, "event"));
> -    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
> -    QDECREF(response);
> +    qtest_hot_unplug_device(id);
> +
> +    g_free(id);
>  }
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b9a1f18..4339d97 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -987,3 +987,63 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
>      qtest_end();
>      QDECREF(response);
>  }
> +
> +/**
> + * Generic hot-plugging test via the device_add QMP command
> + */
> +void qtest_hot_plug_device(const char *driver, const char *id,
> +                           const char *fmt, ...)
> +{
> +    QDict *response;
> +    char *cmd, *opts = NULL;
> +    va_list va;
> +
> +    if (fmt) {
> +        va_start(va, fmt);
> +        opts = g_strdup_vprintf(fmt, va);
> +        va_end(va);
> +    }
> +
> +    cmd = g_strdup_printf("{'execute': 'device_add',"
> +                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
> +                          driver, id, opts ? ", " : "", opts ? opts : "");
> +    g_free(opts);
> +
> +    response = qmp(cmd);
> +    g_free(cmd);
> +    g_assert(response);
> +    while (qdict_haskey(response, "event")) {
> +        /* We can get DEVICE_DELETED events in case something went wrong */
> +        g_assert_cmpstr(qdict_get_str(response, "event"), !=, "DEVICE_DELETED");
> +        QDECREF(response);
> +        response = qmp("");
> +        g_assert(response);
> +    }
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +}
> +
> +/**
> + * Generic hot-unplugging test via the device_del QMP command
> + */
> +void qtest_hot_unplug_device(const char *id)
> +{
> +    QDict *response;
> +    char *cmd;
> +
> +    cmd = g_strdup_printf("{'execute': 'device_del',"
> +                          " 'arguments': { 'id': '%s' }}", id);
> +
> +    response = qmp(cmd);
> +    g_free(cmd);
> +    g_assert(response);
> +    while (qdict_haskey(response, "event")) {
> +        /* We should get DEVICE_DELETED event first */
> +        g_assert_cmpstr(qdict_get_str(response, "event"), ==, "DEVICE_DELETED");
> +        QDECREF(response);
> +        response = qmp("");
> +        g_assert(response);
> +    }
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +}
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 3ae5709..9c1006f 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -927,4 +927,23 @@ QDict *qmp_fd(int fd, const char *fmt, ...);
>   */
>  void qtest_cb_for_every_machine(void (*cb)(const char *machine));
>  
> +/**
> + * qtest_hot_plug_device:
> + * @driver: Name of the device that should be added
> + * @id: Identification string
> + * @fmt: printf-like format string for further options to device_add
> + *
> + * Generic hot-plugging test via the device_add QMP command.
> + */
> +void qtest_hot_plug_device(const char *driver, const char *id,
> +                           const char *fmt, ...) GCC_FMT_ATTR(3, 4);

Not sure if it would be better to avoid the fmt and provide more
parameters instead. Specifying a parameter as NULL will ifgnore it.

void qtest_hot_plug_device(const char *driver, const char *id,
                           const char *drive, const char *port,
                           const char *bus)

that should cover all cases and callers don't have to build strings in a
special format.


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 4/6] tests: Introduce generic device hot-plug/hot-unplug functions

[Thomas Huth]

On 17.08.2017 11:53, David Hildenbrand wrote:
> On 17.08.2017 08:25, Thomas Huth wrote:
>> A lot of tests provide code for adding and removing a device via the
>> device_add and device_del QMP commands. Maintaining this code in so
>> many places is cumbersome and error-prone (some of the code parts
>> check the responses in an incorrect way, for example), so let's
>> provide some proper generic qtest functions for adding and removing a
>> device instead.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
[...]
>> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
>> index 2dcdead..aada753 100644
>> --- a/tests/libqos/pci.c
>> +++ b/tests/libqos/pci.c
>> @@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
>>  void qpci_plug_device_test(const char *driver, const char *id,
>>                             uint8_t slot, const char *opts)
>>  {
>> -    QDict *response;
>> -    char *cmd;
>> -
>> -    cmd = g_strdup_printf("{'execute': 'device_add',"
>> -                          " 'arguments': {"
>> -                          "   'driver': '%s',"
>> -                          "   'addr': '%d',"
>> -                          "   %s%s"
>> -                          "   'id': '%s'"
>> -                          "}}", driver, slot,
>> -                          opts ? opts : "", opts ? "," : "",
>> -                          id);
>> -    response = qmp(cmd);
>> -    g_free(cmd);
>> -    g_assert(response);
>> -    g_assert(!qdict_haskey(response, "error"));
>> -    QDECREF(response);
>> +    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
>> +                          opts ? ", " : "", opts ? opts : "");

While PCI uses 'addr' as additional parameter here...

[...]
>> +    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
>> +                          port, hcd_id);

... USB uses 'port' and 'bus' ...

[...]
>
> Not sure if it would be better to avoid the fmt and provide more
> parameters instead. Specifying a parameter as NULL will ifgnore it.
> 
> void qtest_hot_plug_device(const char *driver, const char *id,
>                            const char *drive, const char *port,
>                            const char *bus)
> 
> that should cover all cases and callers don't have to build strings in a
> special format.

... so I'm afraid, but no, this does not work. The additional parameters
are specific to the device type, so there is no way to get around the
format string if we want to stay flexible here.

 Thomas

Re: [Qemu-devel] [PATCH 4/6] tests: Introduce generic device hot-plug/hot-unplug functions

[David Hildenbrand]

On 17.08.2017 12:57, Thomas Huth wrote:
> On 17.08.2017 11:53, David Hildenbrand wrote:
>> On 17.08.2017 08:25, Thomas Huth wrote:
>>> A lot of tests provide code for adding and removing a device via the
>>> device_add and device_del QMP commands. Maintaining this code in so
>>> many places is cumbersome and error-prone (some of the code parts
>>> check the responses in an incorrect way, for example), so let's
>>> provide some proper generic qtest functions for adding and removing a
>>> device instead.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
> [...]
>>> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
>>> index 2dcdead..aada753 100644
>>> --- a/tests/libqos/pci.c
>>> +++ b/tests/libqos/pci.c
>>> @@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
>>>  void qpci_plug_device_test(const char *driver, const char *id,
>>>                             uint8_t slot, const char *opts)
>>>  {
>>> -    QDict *response;
>>> -    char *cmd;
>>> -
>>> -    cmd = g_strdup_printf("{'execute': 'device_add',"
>>> -                          " 'arguments': {"
>>> -                          "   'driver': '%s',"
>>> -                          "   'addr': '%d',"
>>> -                          "   %s%s"
>>> -                          "   'id': '%s'"
>>> -                          "}}", driver, slot,
>>> -                          opts ? opts : "", opts ? "," : "",
>>> -                          id);
>>> -    response = qmp(cmd);
>>> -    g_free(cmd);
>>> -    g_assert(response);
>>> -    g_assert(!qdict_haskey(response, "error"));
>>> -    QDECREF(response);
>>> +    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
>>> +                          opts ? ", " : "", opts ? opts : "");
> 
> While PCI uses 'addr' as additional parameter here...

so addr would be needed in addition.

> 
> [...]
>>> +    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
>>> +                          port, hcd_id);
> 
> ... USB uses 'port' and 'bus' ...
> 
> [...]
>>
>> Not sure if it would be better to avoid the fmt and provide more
>> parameters instead. Specifying a parameter as NULL will ifgnore it.
>>
>> void qtest_hot_plug_device(const char *driver, const char *id,
>>                            const char *drive, const char *port,
>>                            const char *bus)
>>
>> that should cover all cases and callers don't have to build strings in a
>> special format.
> 
> ... so I'm afraid, but no, this does not work. The additional parameters
> are specific to the device type, so there is no way to get around the
> format string if we want to stay flexible here.

It will work, but yes it is not flexible. Just an idea because I don't
like having to specify json strings on a function.

> 
>  Thomas
> 


-- 

Thanks,

David

[Qemu-devel] [PATCH 5/6] tests: Add qvirtio_(un)plug_device_test wrapper functions

[Thomas Huth]

To support hot-plugging tests with virtio-ccw later, the current
tests should become independent from PCI specific functions. Thus
let's add some proper wrapper function for virtio device hot-plugging
and -unplugging first.
It also seems like device unplugging works fine on ppc64 when using
the generic qtest_hot_unplug_device() function, so hot-unplugging
is now tested on ppc64, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include  |  4 ++--
 tests/libqos/virtio.c   | 27 +++++++++++++++++++++++++++
 tests/libqos/virtio.h   |  5 +++++
 tests/virtio-net-test.c |  8 ++------
 tests/virtio-rng-test.c |  9 +++------
 5 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index ff2a551..e3acdff 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -751,8 +751,8 @@ tests/wdt_ib700-test$(EXESUF): tests/wdt_ib700-test.o
 tests/tco-test$(EXESUF): tests/tco-test.o $(libqos-pc-obj-y)
 tests/virtio-balloon-test$(EXESUF): tests/virtio-balloon-test.o
 tests/virtio-blk-test$(EXESUF): tests/virtio-blk-test.o $(libqos-virtio-obj-y)
-tests/virtio-net-test$(EXESUF): tests/virtio-net-test.o $(libqos-pc-obj-y) $(libqos-virtio-obj-y)
-tests/virtio-rng-test$(EXESUF): tests/virtio-rng-test.o $(libqos-pc-obj-y)
+tests/virtio-net-test$(EXESUF): tests/virtio-net-test.o $(libqos-virtio-obj-y)
+tests/virtio-rng-test$(EXESUF): tests/virtio-rng-test.o $(libqos-virtio-obj-y)
 tests/virtio-scsi-test$(EXESUF): tests/virtio-scsi-test.o $(libqos-virtio-obj-y)
 tests/virtio-9p-test$(EXESUF): tests/virtio-9p-test.o $(libqos-virtio-obj-y)
 tests/virtio-serial-test$(EXESUF): tests/virtio-serial-test.o
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 9880a69..87d2e65 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 #include "libqos/virtio.h"
+#include "libqos/pci.h"
 #include "standard-headers/linux/virtio_config.h"
 #include "standard-headers/linux/virtio_ring.h"
 
@@ -339,3 +340,29 @@ void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx)
     /* vq->avail->used_event */
     writew(vq->avail + 4 + (2 * vq->size), idx);
 }
+
+void qvirtio_plug_device_test(const char *driver, const char *id,
+                              uint8_t addr, const char *opts)
+{
+    const char *arch = qtest_get_arch();
+
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64") ||
+        g_str_equal(arch, "ppc64")) {
+        qpci_plug_device_test(driver, id, addr, opts);
+    } else {
+        g_assert_not_reached();
+    }
+}
+
+void qvirtio_unplug_device_test(const char *id, uint8_t addr)
+{
+    const char *arch = qtest_get_arch();
+
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
+        qpci_unplug_acpi_device_test(id, addr);
+    } else if (g_str_equal(arch, "ppc64")) {
+        qtest_hot_unplug_device(id);
+    } else {
+        g_assert_not_reached();
+    }
+}
diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index 8fbcd18..03e33d6 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -143,4 +143,9 @@ void qvirtqueue_kick(QVirtioDevice *d, QVirtQueue *vq, uint32_t free_head);
 bool qvirtqueue_get_buf(QVirtQueue *vq, uint32_t *desc_idx);
 
 void qvirtqueue_set_used_event(QVirtQueue *vq, uint16_t idx);
+
+void qvirtio_plug_device_test(const char *driver, const char *id,
+                              uint8_t addr, const char *opts);
+void qvirtio_unplug_device_test(const char *id, uint8_t addr);
+
 #endif
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 635b942..49e733d 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -241,15 +241,11 @@ static void pci_basic(gconstpointer data)
 
 static void hotplug(void)
 {
-    const char *arch = qtest_get_arch();
-
     qtest_start("-device virtio-net-pci");
 
-    qpci_plug_device_test("virtio-net-pci", "net1", PCI_SLOT_HP, NULL);
+    qvirtio_plug_device_test("virtio-net", "net1", PCI_SLOT_HP, NULL);
 
-    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        qpci_unplug_acpi_device_test("net1", PCI_SLOT_HP);
-    }
+    qvirtio_unplug_device_test("net1", PCI_SLOT_HP);
 
     test_end();
 }
diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c
index dcecf77..04c4279 100644
--- a/tests/virtio-rng-test.c
+++ b/tests/virtio-rng-test.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 #include "libqos/pci.h"
+#include "libqos/virtio.h"
 
 #define PCI_SLOT_HP             0x06
 
@@ -20,13 +21,9 @@ static void pci_nop(void)
 
 static void hotplug(void)
 {
-    const char *arch = qtest_get_arch();
+    qvirtio_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL);
 
-    qpci_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL);
-
-    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        qpci_unplug_acpi_device_test("rng1", PCI_SLOT_HP);
-    }
+    qvirtio_unplug_device_test("rng1", PCI_SLOT_HP);
 }
 
 int main(int argc, char **argv)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/6] tests: Add qvirtio_(un)plug_device_test wrapper functions

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:12 +0200
Thomas Huth <thuth@redhat.com> wrote:

> To support hot-plugging tests with virtio-ccw later, the current
> tests should become independent from PCI specific functions. Thus
> let's add some proper wrapper function for virtio device hot-plugging
> and -unplugging first.
> It also seems like device unplugging works fine on ppc64 when using
> the generic qtest_hot_unplug_device() function, so hot-unplugging
> is now tested on ppc64, too.

Yeah for more coverage!

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include  |  4 ++--
>  tests/libqos/virtio.c   | 27 +++++++++++++++++++++++++++
>  tests/libqos/virtio.h   |  5 +++++
>  tests/virtio-net-test.c |  8 ++------
>  tests/virtio-rng-test.c |  9 +++------
>  5 files changed, 39 insertions(+), 14 deletions(-)

> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index 635b942..49e733d 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -241,15 +241,11 @@ static void pci_basic(gconstpointer data)
>  
>  static void hotplug(void)
>  {
> -    const char *arch = qtest_get_arch();
> -
>      qtest_start("-device virtio-net-pci");
>  
> -    qpci_plug_device_test("virtio-net-pci", "net1", PCI_SLOT_HP, NULL);
> +    qvirtio_plug_device_test("virtio-net", "net1", PCI_SLOT_HP, NULL);

Same comment as for a previous patch regarding implicit aliasing.

Also, 'PCI_SLOT_HP' sounds a bit pci-y to me?

>  
> -    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -        qpci_unplug_acpi_device_test("net1", PCI_SLOT_HP);
> -    }
> +    qvirtio_unplug_device_test("net1", PCI_SLOT_HP);
>  
>      test_end();
>  }
> diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c
> index dcecf77..04c4279 100644
> --- a/tests/virtio-rng-test.c
> +++ b/tests/virtio-rng-test.c
> @@ -10,6 +10,7 @@
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
>  #include "libqos/pci.h"
> +#include "libqos/virtio.h"
>  
>  #define PCI_SLOT_HP             0x06
>  
> @@ -20,13 +21,9 @@ static void pci_nop(void)
>  
>  static void hotplug(void)
>  {
> -    const char *arch = qtest_get_arch();
> +    qvirtio_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL);

Dito.

>  
> -    qpci_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL);
> -
> -    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -        qpci_unplug_acpi_device_test("rng1", PCI_SLOT_HP);
> -    }
> +    qvirtio_unplug_device_test("rng1", PCI_SLOT_HP);
>  }
>  
>  int main(int argc, char **argv)

[Qemu-devel] [PATCH 6/6] tests: Enable the simple virtio tests on s390x, too

[Thomas Huth]

Most of the simple virtio test can be used on virtio-ccw on
s390x, too, by simply using the bus-independent alias names
of the devices instead of the device names ending in "-pci".
Hot-plugging can also be tested here - we just have to use
the generic hot plug function instead of the PCI hot plug
function in the qvirtio_plug_device_test() function.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include      |  4 ++++
 tests/libqos/virtio.c       |  4 +++-
 tests/virtio-balloon-test.c |  6 +++---
 tests/virtio-console-test.c | 12 ++++++------
 tests/virtio-rng-test.c     | 10 +++++-----
 tests/virtio-serial-test.c  |  8 ++++----
 6 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index e3acdff..ac24aa6 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -364,6 +364,10 @@ check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
 check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
 check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-s390x-y += tests/drive_del-test$(EXESUF)
+check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
+check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
+check-qtest-s390x-y += tests/virtio-rng-test$(EXESUF)
+check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 87d2e65..5ff9ad6 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -349,6 +349,8 @@ void qvirtio_plug_device_test(const char *driver, const char *id,
     if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64") ||
         g_str_equal(arch, "ppc64")) {
         qpci_plug_device_test(driver, id, addr, opts);
+    } else if (g_str_equal(arch, "s390x")) {
+        qtest_hot_plug_device(driver, id, opts ? "%s" : NULL,  opts);
     } else {
         g_assert_not_reached();
     }
@@ -360,7 +362,7 @@ void qvirtio_unplug_device_test(const char *id, uint8_t addr)
 
     if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
         qpci_unplug_acpi_device_test(id, addr);
-    } else if (g_str_equal(arch, "ppc64")) {
+    } else if (g_str_equal(arch, "ppc64") || g_str_equal(arch, "s390x")) {
         qtest_hot_unplug_device(id);
     } else {
         g_assert_not_reached();
diff --git a/tests/virtio-balloon-test.c b/tests/virtio-balloon-test.c
index 0d0046b..45c95dd 100644
--- a/tests/virtio-balloon-test.c
+++ b/tests/virtio-balloon-test.c
@@ -11,7 +11,7 @@
 #include "libqtest.h"
 
 /* Tests only initialization so far. TODO: Replace with functional tests */
-static void pci_nop(void)
+static void balloon_nop(void)
 {
 }
 
@@ -20,9 +20,9 @@ int main(int argc, char **argv)
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/virtio/balloon/pci/nop", pci_nop);
+    qtest_add_func("/virtio/balloon/nop", balloon_nop);
 
-    qtest_start("-device virtio-balloon-pci");
+    qtest_start("-device virtio-balloon");
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/virtio-console-test.c b/tests/virtio-console-test.c
index 1c3de07..d15e182 100644
--- a/tests/virtio-console-test.c
+++ b/tests/virtio-console-test.c
@@ -11,16 +11,16 @@
 #include "libqtest.h"
 
 /* Tests only initialization so far. TODO: Replace with functional tests */
-static void console_pci_nop(void)
+static void console_nop(void)
 {
-    qtest_start("-device virtio-serial-pci,id=vser0 "
+    qtest_start("-device virtio-serial,id=vser0 "
                 "-device virtconsole,bus=vser0.0");
     qtest_end();
 }
 
-static void serialport_pci_nop(void)
+static void serialport_nop(void)
 {
-    qtest_start("-device virtio-serial-pci,id=vser0 "
+    qtest_start("-device virtio-serial,id=vser0 "
                 "-device virtserialport,bus=vser0.0");
     qtest_end();
 }
@@ -28,8 +28,8 @@ static void serialport_pci_nop(void)
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/virtio/console/pci/nop", console_pci_nop);
-    qtest_add_func("/virtio/serialport/pci/nop", serialport_pci_nop);
+    qtest_add_func("/virtio/console/nop", console_nop);
+    qtest_add_func("/virtio/serialport/nop", serialport_nop);
 
     return g_test_run();
 }
diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c
index 04c4279..b58352e 100644
--- a/tests/virtio-rng-test.c
+++ b/tests/virtio-rng-test.c
@@ -15,13 +15,13 @@
 #define PCI_SLOT_HP             0x06
 
 /* Tests only initialization so far. TODO: Replace with functional tests */
-static void pci_nop(void)
+static void rng_nop(void)
 {
 }
 
 static void hotplug(void)
 {
-    qvirtio_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL);
+    qvirtio_plug_device_test("virtio-rng", "rng1", PCI_SLOT_HP, NULL);
 
     qvirtio_unplug_device_test("rng1", PCI_SLOT_HP);
 }
@@ -31,10 +31,10 @@ int main(int argc, char **argv)
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/virtio/rng/pci/nop", pci_nop);
-    qtest_add_func("/virtio/rng/pci/hotplug", hotplug);
+    qtest_add_func("/virtio/rng/nop", rng_nop);
+    qtest_add_func("/virtio/rng/hotplug", hotplug);
 
-    qtest_start("-device virtio-rng-pci");
+    qtest_start("-device virtio-rng");
     ret = g_test_run();
 
     qtest_end();
diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c
index 97f8f52..059f211 100644
--- a/tests/virtio-serial-test.c
+++ b/tests/virtio-serial-test.c
@@ -11,7 +11,7 @@
 #include "libqtest.h"
 
 /* Tests only initialization so far. TODO: Replace with functional tests */
-static void pci_nop(void)
+static void virtioserial_nop(void)
 {
 }
 
@@ -27,10 +27,10 @@ int main(int argc, char **argv)
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/virtio/serial/pci/nop", pci_nop);
-    qtest_add_func("/virtio/serial/pci/hotplug", hotplug);
+    qtest_add_func("/virtio/serial/nop", virtioserial_nop);
+    qtest_add_func("/virtio/serial/hotplug", hotplug);
 
-    qtest_start("-device virtio-serial-pci");
+    qtest_start("-device virtio-serial");
     ret = g_test_run();
 
     qtest_end();
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/6] tests: Enable the simple virtio tests on s390x, too

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:13 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Most of the simple virtio test can be used on virtio-ccw on
> s390x, too, by simply using the bus-independent alias names
> of the devices instead of the device names ending in "-pci".
> Hot-plugging can also be tested here - we just have to use
> the generic hot plug function instead of the PCI hot plug
> function in the qvirtio_plug_device_test() function.

Same comments as before regarding aliasing, and a possible future
desire to test virtio-pci on s390x as well.

But yes, we want the virtio tests on s390x as well.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include      |  4 ++++
>  tests/libqos/virtio.c       |  4 +++-
>  tests/virtio-balloon-test.c |  6 +++---
>  tests/virtio-console-test.c | 12 ++++++------
>  tests/virtio-rng-test.c     | 10 +++++-----
>  tests/virtio-serial-test.c  |  8 ++++----
>  6 files changed, 25 insertions(+), 19 deletions(-)

Re: [Qemu-devel] [PATCH for-2.11 0/6] Enable more qtests for s390x

[Cornelia Huck]

On Thu, 17 Aug 2017 08:25:07 +0200
Thomas Huth <thuth@redhat.com> wrote:

> We currently do not have many tests enabled for QEMU on s390x yet,
> so this series reworks some of the tests to be also usable on s390x
> to get some extended test coverage there.

More coverage on s390x is certainly desirable. I'm wondering how many
other tests are needlessly restricted to certain architectures as
well...

> 
> Along the way, this also cleans up some of the generic test code,
> e.g. by introducing generic functions to do hot-plugging with
> the device_add QMP command.

Cleanup also sounds good :)

> 
> Please review!
> 
>  Thanks,
>   Thomas
> 
> 
> Thomas Huth (6):
>   tests: Run filter-redirector and -mirror test only on POSIX systems
>   tests: Add network filter tests to the check-qtest-s390x list

I think these two are fine as-is.

>   tests: Enable the drive_del test also on s390x
>   tests: Introduce generic device hot-plug/hot-unplug functions
>   tests: Add qvirtio_(un)plug_device_test wrapper functions
>   tests: Enable the simple virtio tests on s390x, too

But these probably need some work (or at least, some thought.)

> 
>  tests/Makefile.include         | 20 +++++++++-----
>  tests/drive_del-test.c         | 13 ++++-----
>  tests/libqos/pci.c             | 19 ++-----------
>  tests/libqos/usb.c             | 30 +++++----------------
>  tests/libqos/virtio.c          | 29 ++++++++++++++++++++
>  tests/libqos/virtio.h          |  5 ++++
>  tests/libqtest.c               | 60 ++++++++++++++++++++++++++++++++++++++++++
>  tests/libqtest.h               | 19 +++++++++++++
>  tests/test-filter-mirror.c     | 14 +++++-----
>  tests/test-filter-redirector.c | 32 +++++++++++-----------
>  tests/test-netfilter.c         | 11 +++++++-
>  tests/usb-hcd-uhci-test.c      | 26 ++----------------
>  tests/usb-hcd-xhci-test.c      | 51 +++--------------------------------
>  tests/virtio-balloon-test.c    |  6 ++---
>  tests/virtio-console-test.c    | 12 ++++-----
>  tests/virtio-net-test.c        |  8 ++----
>  tests/virtio-rng-test.c        | 17 +++++-------
>  tests/virtio-scsi-test.c       | 24 ++---------------
>  tests/virtio-serial-test.c     | 33 +++++------------------
>  19 files changed, 208 insertions(+), 221 deletions(-)
> 

The big question remains again: Who merges this? I can merge this
through the s390x tree if there are no other takers.

Re: [Qemu-devel] [PATCH for-2.11 0/6] Enable more qtests for s390x

[Cornelia Huck]

On Thu, 17 Aug 2017 11:11:16 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 17 Aug 2017 08:25:07 +0200
> Thomas Huth <thuth@redhat.com> wrote:

> > Thomas Huth (6):
> >   tests: Run filter-redirector and -mirror test only on POSIX systems
> >   tests: Add network filter tests to the check-qtest-s390x list  
> 
> I think these two are fine as-is.

(...)

> The big question remains again: Who merges this? I can merge this
> through the s390x tree if there are no other takers.

I just took the liberty to queue these two to s390-next.