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 <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>