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

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>

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

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

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...

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

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  ]

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>