From: Josef Bacik <[email protected]>
This is to test for a regression introduced by
b9470c27607b ("inet: kill smallest_size and smallest_port")
which introduced a problem with reuseaddr and bind conflicts.
Signed-off-by: Josef Bacik <[email protected]>
---
tools/testing/selftests/net/.gitignore | 1 +
tools/testing/selftests/net/Makefile | 2 +-
tools/testing/selftests/net/reuseaddr_conflict.c | 114 +++++++++++++++++++++++
3 files changed, 116 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/net/reuseaddr_conflict.c
diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 9801253e4802..c612d6e38c62 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -6,3 +6,4 @@ reuseport_bpf
reuseport_bpf_cpu
reuseport_bpf_numa
reuseport_dualstack
+reuseaddr_conflict
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index de1f5772b878..3df542c84610 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -7,7 +7,7 @@ TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetl
TEST_GEN_FILES = socket
TEST_GEN_FILES += psock_fanout psock_tpacket
TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
-TEST_GEN_FILES += reuseport_dualstack msg_zerocopy
+TEST_GEN_FILES += reuseport_dualstack msg_zerocopy reuseaddr_conflict
include ../lib.mk
diff --git a/tools/testing/selftests/net/reuseaddr_conflict.c b/tools/testing/selftests/net/reuseaddr_conflict.c
new file mode 100644
index 000000000000..7c5b12664b03
--- /dev/null
+++ b/tools/testing/selftests/net/reuseaddr_conflict.c
@@ -0,0 +1,114 @@
+/*
+ * Test for the regression introduced by
+ *
+ * b9470c27607b ("inet: kill smallest_size and smallest_port")
+ *
+ * If we open an ipv4 socket on a port with reuseaddr we shouldn't reset the tb
+ * when we open the ipv6 conterpart, which is what was happening previously.
+ */
+#include <errno.h>
+#include <error.h>
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <sys/socket.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#define PORT 9999
+
+int open_port(int ipv6, int any)
+{
+ int fd = -1;
+ int reuseaddr = 1;
+ int v6only = 1;
+ int addrlen;
+ int ret = -1;
+ struct sockaddr *addr;
+ int family = ipv6 ? AF_INET6 : AF_INET;
+
+ struct sockaddr_in6 addr6 = {
+ .sin6_family = AF_INET6,
+ .sin6_port = htons(PORT),
+ .sin6_addr = in6addr_any
+ };
+ struct sockaddr_in addr4 = {
+ .sin_family = AF_INET,
+ .sin_port = htons(PORT),
+ .sin_addr.s_addr = any ? htonl(INADDR_ANY) : inet_addr("127.0.0.1"),
+ };
+
+
+ if (ipv6) {
+ addr = (struct sockaddr*)&addr6;
+ addrlen = sizeof(addr6);
+ } else {
+ addr = (struct sockaddr*)&addr4;
+ addrlen = sizeof(addr4);
+ }
+
+ if ((fd = socket(family, SOCK_STREAM, IPPROTO_TCP)) < 0) {
+ perror("socket");
+ goto out;
+ }
+
+ if (ipv6 && setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (void*)&v6only,
+ sizeof(v6only)) < 0) {
+ perror("setsockopt IPV6_V6ONLY");
+ goto out;
+ }
+
+ if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &reuseaddr,
+ sizeof(reuseaddr)) < 0) {
+ perror("setsockopt SO_REUSEADDR");
+ goto out;
+ }
+
+ if (bind(fd, addr, addrlen) < 0) {
+ perror("bind");
+ goto out;
+ }
+
+ if (any)
+ return fd;
+
+ if (listen(fd, 1) < 0) {
+ perror("listen");
+ goto out;
+ }
+ return fd;
+out:
+ close(fd);
+ return ret;
+}
+
+int main(void)
+{
+ int listenfd;
+ int fd1, fd2;
+
+ fprintf(stderr, "Opening 127.0.0.1:%d\n", PORT);
+ listenfd = open_port(0, 0);
+ if (listenfd < 0)
+ error(1, errno, "Couldn't open listen socket");
+ fprintf(stderr, "Opening INADDR_ANY:%d\n", PORT);
+ fd1 = open_port(0, 1);
+ if (fd1 >= 0)
+ error(1, 0, "Was allowed to create an ipv4 reuseport on a already bound non-reuseport socket");
+ fprintf(stderr, "Opening in6addr_any:%d\n", PORT);
+ fd1 = open_port(1, 1);
+ if (fd1 < 0)
+ error(1, errno, "Couldn't open ipv6 reuseport");
+ fprintf(stderr, "Opening INADDR_ANY:%d\n", PORT);
+ fd2 = open_port(0, 1);
+ if (fd2 >= 0)
+ error(1, 0, "Was allowed to create an ipv4 reuseport on a already bound non-reuseport socket");
+ close(fd1);
+ fprintf(stderr, "Opening INADDR_ANY:%d after closing ipv6 socket\n", PORT);
+ fd1 = open_port(0, 1);
+ if (fd1 >= 0)
+ error(1, 0, "Was allowed to create an ipv4 reuseport on an already bound non-reuseport socket with no ipv6");
+ fprintf(stderr, "Success");
+ return 0;
+}
--
2.7.4
From: Josef Bacik <[email protected]>
These self tests are just self contained binaries, they are not run by
any of the scripts in the directory. This means they need to be marked
with TEST_GEN_PROGS to actually be run, not TEST_GEN_FILES.
Signed-off-by: Josef Bacik <[email protected]>
---
tools/testing/selftests/net/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 3df542c84610..45a4e77a47c4 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -6,8 +6,8 @@ CFLAGS += -I../../../../usr/include/
TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
TEST_GEN_FILES = socket
TEST_GEN_FILES += psock_fanout psock_tpacket
-TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
-TEST_GEN_FILES += reuseport_dualstack msg_zerocopy reuseaddr_conflict
+TEST_GEN_PROGS += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
+TEST_GEN_PROGS += reuseport_dualstack msg_zerocopy reuseaddr_conflict
include ../lib.mk
--
2.7.4
From: Josef Bacik <[email protected]>
Some of the networking tests are very noisy and make it impossible to
see if we actually passed the tests as they run. Default to suppressing
the output from any tests run in order to make it easier to track what
failed.
Signed-off-by: Josef Bacik <[email protected]>
---
tools/testing/selftests/lib.mk | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 693616651da5..223234cd98e9 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -24,7 +24,7 @@ define RUN_TESTS
echo "selftests: Warning: file $$BASENAME_TEST is not executable, correct this.";\
echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; \
else \
- cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
+ cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
fi; \
done;
endef
@@ -55,7 +55,7 @@ endif
define EMIT_TESTS
@for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \
BASENAME_TEST=`basename $$TEST`; \
- echo "(./$$BASENAME_TEST && echo \"selftests: $$BASENAME_TEST [PASS]\") || echo \"selftests: $$BASENAME_TEST [FAIL]\""; \
+ echo "(./$$BASENAME_TEST > /dev/null 2>&1 && echo \"selftests: $$BASENAME_TEST [PASS]\") || echo \"selftests: $$BASENAME_TEST [FAIL]\""; \
done;
endef
--
2.7.4
On 09/18/2017 11:37 AM, [email protected] wrote:
> From: Josef Bacik <[email protected]>
>
> Some of the networking tests are very noisy and make it impossible to
> see if we actually passed the tests as they run. Default to suppressing
> the output from any tests run in order to make it easier to track what
> failed.
>
> Signed-off-by: Josef Bacik <[email protected]>
> --
This change suppresses pass/fail wrapper output for all tests, not just the
networking tests.
Could you please send me before and after results for what you are trying
to fix.
> tools/testing/selftests/lib.mk | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index 693616651da5..223234cd98e9 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -24,7 +24,7 @@ define RUN_TESTS
> echo "selftests: Warning: file $$BASENAME_TEST is not executable, correct this.";\
> echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; \
> else \
> - cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
> + cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
> fi; \
> done;
> endef
> @@ -55,7 +55,7 @@ endif
> define EMIT_TESTS
> @for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \
> BASENAME_TEST=`basename $$TEST`; \
> - echo "(./$$BASENAME_TEST && echo \"selftests: $$BASENAME_TEST [PASS]\") || echo \"selftests: $$BASENAME_TEST [FAIL]\""; \
> + echo "(./$$BASENAME_TEST > /dev/null 2>&1 && echo \"selftests: $$BASENAME_TEST [PASS]\") || echo \"selftests: $$BASENAME_TEST [FAIL]\""; \
> done;
> endef
>
>
thanks,
-- Shuah
On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
> On 09/18/2017 11:37 AM, [email protected] wrote:
> > From: Josef Bacik <[email protected]>
> >
> > Some of the networking tests are very noisy and make it impossible to
> > see if we actually passed the tests as they run. Default to suppressing
> > the output from any tests run in order to make it easier to track what
> > failed.
> >
> > Signed-off-by: Josef Bacik <[email protected]>
> > --
>
> This change suppresses pass/fail wrapper output for all tests, not just the
> networking tests.
>
> Could you please send me before and after results for what you are trying
> to fix.
>
Yeah I wanted to suppress extraneous output from everybody, I just happened to
notice it because I was testing net. The default thing already spits out what
it's running and pass/fail, there's no need to include all of the random output
unless the user wants to go and run the test manually. As it is now it's
_impossible_ to tell what ran and what passed/failed because of all the random
output.
Ideally kselftests would work like xfstests does and simply capture the output
to a log so you could go check afterwards, but that's a lot more work. Making
it easier to tell which tests passed/failed is a good enough first step.
Thanks,
Josef
On 09/18/2017 11:52 AM, Josef Bacik wrote:
> On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
>> On 09/18/2017 11:37 AM, [email protected] wrote:
>>> From: Josef Bacik <[email protected]>
>>>
>>> Some of the networking tests are very noisy and make it impossible to
>>> see if we actually passed the tests as they run. Default to suppressing
>>> the output from any tests run in order to make it easier to track what
>>> failed.
>>>
>>> Signed-off-by: Josef Bacik <[email protected]>
>>> --
>>
>> This change suppresses pass/fail wrapper output for all tests, not just the
>> networking tests.
>>
>> Could you please send me before and after results for what you are trying
>> to fix.
>>
>
> Yeah I wanted to suppress extraneous output from everybody, I just happened to
> notice it because I was testing net. The default thing already spits out what
> it's running and pass/fail, there's no need to include all of the random output
> unless the user wants to go and run the test manually. As it is now it's
> _impossible_ to tell what ran and what passed/failed because of all the random
> output.
Unfortunately kselftests have lots of users that want different things. A recent
request is to use TAP13 format for output for external parsers to be able to parse.
That is what this change to add TAP13 header does.
The output you are seeing is the TAP 13 format to indicate the test has passed.
The right fix would be to suppress the Pass/Fail from the individual shell script
and have the shell script exit with error code. kselftest lib.mk will handle the
error code and print out pass/fail like it is doing now.
Using the common logic will help avoid duplicate code in tests/test scripts and
also makes the pass/fail messages consistent.
In the following output the individual test output can be eliminated since lib.mk
run_tests does that for you. In addition, you will also get a count of tests at
the end of the run of all tests in a test directory.
TAP version 13
selftests: run_netsocktests
========================================
--------------------
running socket test
--------------------
[PASS]
ok 1..1 selftests: run_netsocktests [PASS]
selftests: run_afpackettests
========================================
must be run as root
ok 1..2 selftests: run_afpackettests [PASS]
selftests: test_bpf.sh
========================================
test_bpf: [FAIL]
not ok 1..3 selftests: test_bpf.sh [FAIL]
selftests: netdevice.sh
========================================
SKIP: Need root privileges
ok 1..4 selftests: netdevice.sh [PASS]
If you eliminate that you will just see the common lib.mk results.
TAP version 13
selftests: run_netsocktests
========================================
ok 1..1 selftests: run_netsocktests [PASS]
selftests: run_afpackettests
========================================
must be run as root
ok 1..2 selftests: run_afpackettests [PASS]
========================================
selftests: test_bpf.sh
========================================
not ok 1..3 selftests: test_bpf.sh [FAIL]
selftests: netdevice.sh
========================================
SKIP: Need root privileges
ok 1..4 selftests: netdevice.sh [PASS]
If you would like to fix the duplicate output, please send me patches
to remove pass/fail output strings from tests instead. It is on my
todo to do that this release.
thanks,
-- Shuah
On Mon, Sep 18, 2017 at 12:13:40PM -0600, Shuah Khan wrote:
> On 09/18/2017 11:52 AM, Josef Bacik wrote:
> > On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
> >> On 09/18/2017 11:37 AM, [email protected] wrote:
> >>> From: Josef Bacik <[email protected]>
> >>>
> >>> Some of the networking tests are very noisy and make it impossible to
> >>> see if we actually passed the tests as they run. Default to suppressing
> >>> the output from any tests run in order to make it easier to track what
> >>> failed.
> >>>
> >>> Signed-off-by: Josef Bacik <[email protected]>
> >>> --
> >>
> >> This change suppresses pass/fail wrapper output for all tests, not just the
> >> networking tests.
> >>
> >> Could you please send me before and after results for what you are trying
> >> to fix.
> >>
> >
> > Yeah I wanted to suppress extraneous output from everybody, I just happened to
> > notice it because I was testing net. The default thing already spits out what
> > it's running and pass/fail, there's no need to include all of the random output
> > unless the user wants to go and run the test manually. As it is now it's
> > _impossible_ to tell what ran and what passed/failed because of all the random
> > output.
>
> Unfortunately kselftests have lots of users that want different things. A recent
> request is to use TAP13 format for output for external parsers to be able to parse.
> That is what this change to add TAP13 header does.
>
> The output you are seeing is the TAP 13 format to indicate the test has passed.
>
> The right fix would be to suppress the Pass/Fail from the individual shell script
> and have the shell script exit with error code. kselftest lib.mk will handle the
> error code and print out pass/fail like it is doing now.
>
> Using the common logic will help avoid duplicate code in tests/test scripts and
> also makes the pass/fail messages consistent.
>
> In the following output the individual test output can be eliminated since lib.mk
> run_tests does that for you. In addition, you will also get a count of tests at
> the end of the run of all tests in a test directory.
>
> TAP version 13
> selftests: run_netsocktests
> ========================================
> --------------------
> running socket test
> --------------------
> [PASS]
> ok 1..1 selftests: run_netsocktests [PASS]
> selftests: run_afpackettests
> ========================================
> must be run as root
> ok 1..2 selftests: run_afpackettests [PASS]
> selftests: test_bpf.sh
> ========================================
> test_bpf: [FAIL]
> not ok 1..3 selftests: test_bpf.sh [FAIL]
> selftests: netdevice.sh
> ========================================
> SKIP: Need root privileges
> ok 1..4 selftests: netdevice.sh [PASS]
>
> If you eliminate that you will just see the common lib.mk results.
>
> TAP version 13
> selftests: run_netsocktests
> ========================================
> ok 1..1 selftests: run_netsocktests [PASS]
> selftests: run_afpackettests
> ========================================
> must be run as root
> ok 1..2 selftests: run_afpackettests [PASS]
> ========================================
> selftests: test_bpf.sh
> ========================================
> not ok 1..3 selftests: test_bpf.sh [FAIL]
> selftests: netdevice.sh
> ========================================
> SKIP: Need root privileges
> ok 1..4 selftests: netdevice.sh [PASS]
>
>
> If you would like to fix the duplicate output, please send me patches
> to remove pass/fail output strings from tests instead. It is on my
> todo to do that this release.
>
I'm confused, this is exactly what my patch does, it strips all of the
extraneous output and leaves only the TAP13 output. Here is the output without
my suppression patch
https://da.gd/pup0
and here is the output with my suppression patch
https://da.gd/3olKj
Unless I'm missing something subtle it appears to be exactly the output you
want, without the random crap from the other tests. The only thing I'm
redirecting is the output of the _test_ itself, $$BASENAME_TEST from what I can
tell is the actual test we're running, not the wrapper, so everything is as it
should be. Thanks,
Josef
On 09/18/2017 12:24 PM, Josef Bacik wrote:
> On Mon, Sep 18, 2017 at 12:13:40PM -0600, Shuah Khan wrote:
>> On 09/18/2017 11:52 AM, Josef Bacik wrote:
>>> On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
>>>> On 09/18/2017 11:37 AM, [email protected] wrote:
>>>>> From: Josef Bacik <[email protected]>
>>>>>
>>>>> Some of the networking tests are very noisy and make it impossible to
>>>>> see if we actually passed the tests as they run. Default to suppressing
>>>>> the output from any tests run in order to make it easier to track what
>>>>> failed.
>>>>>
>>>>> Signed-off-by: Josef Bacik <[email protected]>
>>>>> --
>>>>
>>>> This change suppresses pass/fail wrapper output for all tests, not just the
>>>> networking tests.
>>>>
>>>> Could you please send me before and after results for what you are trying
>>>> to fix.
>>>>
>>>
>>> Yeah I wanted to suppress extraneous output from everybody, I just happened to
>>> notice it because I was testing net. The default thing already spits out what
>>> it's running and pass/fail, there's no need to include all of the random output
>>> unless the user wants to go and run the test manually. As it is now it's
>>> _impossible_ to tell what ran and what passed/failed because of all the random
>>> output.
>>
>> Unfortunately kselftests have lots of users that want different things. A recent
>> request is to use TAP13 format for output for external parsers to be able to parse.
>> That is what this change to add TAP13 header does.
>>
>> The output you are seeing is the TAP 13 format to indicate the test has passed.
>>
>> The right fix would be to suppress the Pass/Fail from the individual shell script
>> and have the shell script exit with error code. kselftest lib.mk will handle the
>> error code and print out pass/fail like it is doing now.
>>
>> Using the common logic will help avoid duplicate code in tests/test scripts and
>> also makes the pass/fail messages consistent.
>>
>> In the following output the individual test output can be eliminated since lib.mk
>> run_tests does that for you. In addition, you will also get a count of tests at
>> the end of the run of all tests in a test directory.
>>
>> TAP version 13
>> selftests: run_netsocktests
>> ========================================
>> --------------------
>> running socket test
>> --------------------
>> [PASS]
>> ok 1..1 selftests: run_netsocktests [PASS]
>> selftests: run_afpackettests
>> ========================================
>> must be run as root
>> ok 1..2 selftests: run_afpackettests [PASS]
>> selftests: test_bpf.sh
>> ========================================
>> test_bpf: [FAIL]
>> not ok 1..3 selftests: test_bpf.sh [FAIL]
>> selftests: netdevice.sh
>> ========================================
>> SKIP: Need root privileges
>> ok 1..4 selftests: netdevice.sh [PASS]
>>
>> If you eliminate that you will just see the common lib.mk results.
>>
>> TAP version 13
>> selftests: run_netsocktests
>> ========================================
>> ok 1..1 selftests: run_netsocktests [PASS]
>> selftests: run_afpackettests
>> ========================================
>> must be run as root
>> ok 1..2 selftests: run_afpackettests [PASS]
>> ========================================
>> selftests: test_bpf.sh
>> ========================================
>> not ok 1..3 selftests: test_bpf.sh [FAIL]
>> selftests: netdevice.sh
>> ========================================
>> SKIP: Need root privileges
>> ok 1..4 selftests: netdevice.sh [PASS]
>>
>>
>> If you would like to fix the duplicate output, please send me patches
>> to remove pass/fail output strings from tests instead. It is on my
>> todo to do that this release.
>>
>
> I'm confused, this is exactly what my patch does, it strips all of the
> extraneous output and leaves only the TAP13 output. Here is the output without
> my suppression patch
>
> https://da.gd/pup0
>
> and here is the output with my suppression patch
>
> https://da.gd/3olKj
>
> Unless I'm missing something subtle it appears to be exactly the output you
> want, without the random crap from the other tests. The only thing I'm
> redirecting is the output of the _test_ itself, $$BASENAME_TEST from what I can
> tell is the actual test we're running, not the wrapper, so everything is as it
> should be. Thanks,
>
> Josef
>
Yes you are right. Yes I see that you are redirecting all
output from the test with (./$$BASENAME_TEST > /dev/null 2>&1)
My bad.
cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
However, it also suppresses important error messages from individual tests
like in the case of rtnetlink.sh from your before and after output.
User knows the test failed, but doesn't know why. One way to solve
the problem is creating a temp file with the output for reference.
Something like:
(./$$BASENAME_TEST > /tmp/kselftest.out 2>&1
Without the change:
ok 1..10 selftests: netdevice.sh [PASS]
selftests: rtnetlink.sh
========================================
RTNETLINK answers: Operation not supported
Cannot find device "test-dummy0"
FAIL: cannot add dummy interface
not ok 1..11 selftests: rtnetlink.sh [FAIL]
selftests: rtnetlink.sh
========================================
not ok 1..11 selftests: rtnetlink.sh [FAIL]
thanks,
-- Shuah
On Mon, Sep 18, 2017 at 01:48:31PM -0600, Shuah Khan wrote:
> On 09/18/2017 12:24 PM, Josef Bacik wrote:
> > On Mon, Sep 18, 2017 at 12:13:40PM -0600, Shuah Khan wrote:
> >> On 09/18/2017 11:52 AM, Josef Bacik wrote:
> >>> On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
> >>>> On 09/18/2017 11:37 AM, [email protected] wrote:
> >>>>> From: Josef Bacik <[email protected]>
> >>>>>
> >>>>> Some of the networking tests are very noisy and make it impossible to
> >>>>> see if we actually passed the tests as they run. Default to suppressing
> >>>>> the output from any tests run in order to make it easier to track what
> >>>>> failed.
> >>>>>
> >>>>> Signed-off-by: Josef Bacik <[email protected]>
> >>>>> --
> >>>>
> >>>> This change suppresses pass/fail wrapper output for all tests, not just the
> >>>> networking tests.
> >>>>
> >>>> Could you please send me before and after results for what you are trying
> >>>> to fix.
> >>>>
> >>>
> >>> Yeah I wanted to suppress extraneous output from everybody, I just happened to
> >>> notice it because I was testing net. The default thing already spits out what
> >>> it's running and pass/fail, there's no need to include all of the random output
> >>> unless the user wants to go and run the test manually. As it is now it's
> >>> _impossible_ to tell what ran and what passed/failed because of all the random
> >>> output.
> >>
> >> Unfortunately kselftests have lots of users that want different things. A recent
> >> request is to use TAP13 format for output for external parsers to be able to parse.
> >> That is what this change to add TAP13 header does.
> >>
> >> The output you are seeing is the TAP 13 format to indicate the test has passed.
> >>
> >> The right fix would be to suppress the Pass/Fail from the individual shell script
> >> and have the shell script exit with error code. kselftest lib.mk will handle the
> >> error code and print out pass/fail like it is doing now.
> >>
> >> Using the common logic will help avoid duplicate code in tests/test scripts and
> >> also makes the pass/fail messages consistent.
> >>
> >> In the following output the individual test output can be eliminated since lib.mk
> >> run_tests does that for you. In addition, you will also get a count of tests at
> >> the end of the run of all tests in a test directory.
> >>
> >> TAP version 13
> >> selftests: run_netsocktests
> >> ========================================
> >> --------------------
> >> running socket test
> >> --------------------
> >> [PASS]
> >> ok 1..1 selftests: run_netsocktests [PASS]
> >> selftests: run_afpackettests
> >> ========================================
> >> must be run as root
> >> ok 1..2 selftests: run_afpackettests [PASS]
> >> selftests: test_bpf.sh
> >> ========================================
> >> test_bpf: [FAIL]
> >> not ok 1..3 selftests: test_bpf.sh [FAIL]
> >> selftests: netdevice.sh
> >> ========================================
> >> SKIP: Need root privileges
> >> ok 1..4 selftests: netdevice.sh [PASS]
> >>
> >> If you eliminate that you will just see the common lib.mk results.
> >>
> >> TAP version 13
> >> selftests: run_netsocktests
> >> ========================================
> >> ok 1..1 selftests: run_netsocktests [PASS]
> >> selftests: run_afpackettests
> >> ========================================
> >> must be run as root
> >> ok 1..2 selftests: run_afpackettests [PASS]
> >> ========================================
> >> selftests: test_bpf.sh
> >> ========================================
> >> not ok 1..3 selftests: test_bpf.sh [FAIL]
> >> selftests: netdevice.sh
> >> ========================================
> >> SKIP: Need root privileges
> >> ok 1..4 selftests: netdevice.sh [PASS]
> >>
> >>
> >> If you would like to fix the duplicate output, please send me patches
> >> to remove pass/fail output strings from tests instead. It is on my
> >> todo to do that this release.
> >>
> >
> > I'm confused, this is exactly what my patch does, it strips all of the
> > extraneous output and leaves only the TAP13 output. Here is the output without
> > my suppression patch
> >
> > https://da.gd/pup0
> >
> > and here is the output with my suppression patch
> >
> > https://da.gd/3olKj
> >
> > Unless I'm missing something subtle it appears to be exactly the output you
> > want, without the random crap from the other tests. The only thing I'm
> > redirecting is the output of the _test_ itself, $$BASENAME_TEST from what I can
> > tell is the actual test we're running, not the wrapper, so everything is as it
> > should be. Thanks,
> >
> > Josef
> >
>
> Yes you are right. Yes I see that you are redirecting all
> output from the test with (./$$BASENAME_TEST > /dev/null 2>&1)
> My bad.
>
> cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
>
> However, it also suppresses important error messages from individual tests
> like in the case of rtnetlink.sh from your before and after output.
>
> User knows the test failed, but doesn't know why. One way to solve
> the problem is creating a temp file with the output for reference.
> Something like:
>
> (./$$BASENAME_TEST > /tmp/kselftest.out 2>&1
>
>
So I didn't do a global file because we'd have to append to it to make sure we
didn't lose any history. If this doesn't bother you I can do that instead, and
then just remove the file whenever we start running things. Let me know if
that's what you would prefer. Thanks,
Josef
On 09/18/2017 02:19 PM, Josef Bacik wrote:
> On Mon, Sep 18, 2017 at 01:48:31PM -0600, Shuah Khan wrote:
>> On 09/18/2017 12:24 PM, Josef Bacik wrote:
>>> On Mon, Sep 18, 2017 at 12:13:40PM -0600, Shuah Khan wrote:
>>>> On 09/18/2017 11:52 AM, Josef Bacik wrote:
>>>>> On Mon, Sep 18, 2017 at 11:46:18AM -0600, Shuah Khan wrote:
>>>>>> On 09/18/2017 11:37 AM, [email protected] wrote:
>>>>>>> From: Josef Bacik <[email protected]>
>>>>>>>
>>>>>>> Some of the networking tests are very noisy and make it impossible to
>>>>>>> see if we actually passed the tests as they run. Default to suppressing
>>>>>>> the output from any tests run in order to make it easier to track what
>>>>>>> failed.
>>>>>>>
>>>>>>> Signed-off-by: Josef Bacik <[email protected]>
>>>>>>> --
>>>>>>
>>>>>> This change suppresses pass/fail wrapper output for all tests, not just the
>>>>>> networking tests.
>>>>>>
>>>>>> Could you please send me before and after results for what you are trying
>>>>>> to fix.
>>>>>>
>>>>>
>>>>> Yeah I wanted to suppress extraneous output from everybody, I just happened to
>>>>> notice it because I was testing net. The default thing already spits out what
>>>>> it's running and pass/fail, there's no need to include all of the random output
>>>>> unless the user wants to go and run the test manually. As it is now it's
>>>>> _impossible_ to tell what ran and what passed/failed because of all the random
>>>>> output.
>>>>
>>>> Unfortunately kselftests have lots of users that want different things. A recent
>>>> request is to use TAP13 format for output for external parsers to be able to parse.
>>>> That is what this change to add TAP13 header does.
>>>>
>>>> The output you are seeing is the TAP 13 format to indicate the test has passed.
>>>>
>>>> The right fix would be to suppress the Pass/Fail from the individual shell script
>>>> and have the shell script exit with error code. kselftest lib.mk will handle the
>>>> error code and print out pass/fail like it is doing now.
>>>>
>>>> Using the common logic will help avoid duplicate code in tests/test scripts and
>>>> also makes the pass/fail messages consistent.
>>>>
>>>> In the following output the individual test output can be eliminated since lib.mk
>>>> run_tests does that for you. In addition, you will also get a count of tests at
>>>> the end of the run of all tests in a test directory.
>>>>
>>>> TAP version 13
>>>> selftests: run_netsocktests
>>>> ========================================
>>>> --------------------
>>>> running socket test
>>>> --------------------
>>>> [PASS]
>>>> ok 1..1 selftests: run_netsocktests [PASS]
>>>> selftests: run_afpackettests
>>>> ========================================
>>>> must be run as root
>>>> ok 1..2 selftests: run_afpackettests [PASS]
>>>> selftests: test_bpf.sh
>>>> ========================================
>>>> test_bpf: [FAIL]
>>>> not ok 1..3 selftests: test_bpf.sh [FAIL]
>>>> selftests: netdevice.sh
>>>> ========================================
>>>> SKIP: Need root privileges
>>>> ok 1..4 selftests: netdevice.sh [PASS]
>>>>
>>>> If you eliminate that you will just see the common lib.mk results.
>>>>
>>>> TAP version 13
>>>> selftests: run_netsocktests
>>>> ========================================
>>>> ok 1..1 selftests: run_netsocktests [PASS]
>>>> selftests: run_afpackettests
>>>> ========================================
>>>> must be run as root
>>>> ok 1..2 selftests: run_afpackettests [PASS]
>>>> ========================================
>>>> selftests: test_bpf.sh
>>>> ========================================
>>>> not ok 1..3 selftests: test_bpf.sh [FAIL]
>>>> selftests: netdevice.sh
>>>> ========================================
>>>> SKIP: Need root privileges
>>>> ok 1..4 selftests: netdevice.sh [PASS]
>>>>
>>>>
>>>> If you would like to fix the duplicate output, please send me patches
>>>> to remove pass/fail output strings from tests instead. It is on my
>>>> todo to do that this release.
>>>>
>>>
>>> I'm confused, this is exactly what my patch does, it strips all of the
>>> extraneous output and leaves only the TAP13 output. Here is the output without
>>> my suppression patch
>>>
>>> https://da.gd/pup0
>>>
>>> and here is the output with my suppression patch
>>>
>>> https://da.gd/3olKj
>>>
>>> Unless I'm missing something subtle it appears to be exactly the output you
>>> want, without the random crap from the other tests. The only thing I'm
>>> redirecting is the output of the _test_ itself, $$BASENAME_TEST from what I can
>>> tell is the actual test we're running, not the wrapper, so everything is as it
>>> should be. Thanks,
>>>
>>> Josef
>>>
>>
>> Yes you are right. Yes I see that you are redirecting all
>> output from the test with (./$$BASENAME_TEST > /dev/null 2>&1)
>> My bad.
>>
>> cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
>>
>> However, it also suppresses important error messages from individual tests
>> like in the case of rtnetlink.sh from your before and after output.
>>
>> User knows the test failed, but doesn't know why. One way to solve
>> the problem is creating a temp file with the output for reference.
>> Something like:
>>
>> (./$$BASENAME_TEST > /tmp/kselftest.out 2>&1
>>
>>
>
> So I didn't do a global file because we'd have to append to it to make sure we
> didn't lose any history. If this doesn't bother you I can do that instead, and
> then just remove the file whenever we start running things. Let me know if
> that's what you would prefer. Thanks,
>
> Josef
>
Right. Let's not worry about global file. Let's just use /tmp/$$BASENAME_TEST
for now. Something like the below diff:
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 223234cd98e9..317393e430c9 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -24,7 +24,8 @@ define RUN_TESTS
echo "selftests: Warning: file $$BASENAME_TEST is not executable, correct this.";\
echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; \
else \
- cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /dev/null 2>&1 && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
+ echo "Please check /tmp/$$BASENAME_TEST for full test output"; \
+ cd `dirname $$TEST` > /dev/null; (./$$BASENAME_TEST > /tmp/$$BASENAME_TEST && echo "ok 1..$$test_num selftests: $$BASENAME_TEST [PASS]") || echo "not ok 1..$$test_num selftests: $$BASENAME_TEST [FAIL]"; cd - > /dev/null;\
fi; \
done;
endef
thanks,
-- Shuah
On 09/18/2017 11:32 AM, [email protected] wrote:
> From: Josef Bacik <[email protected]>
>
> These self tests are just self contained binaries, they are not run by
> any of the scripts in the directory. This means they need to be marked
> with TEST_GEN_PROGS to actually be run, not TEST_GEN_FILES.
>
> Signed-off-by: Josef Bacik <[email protected]>
> ---
> tools/testing/selftests/net/Makefile | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 3df542c84610..45a4e77a47c4 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -6,8 +6,8 @@ CFLAGS += -I../../../../usr/include/
> TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
> TEST_GEN_FILES = socket
> TEST_GEN_FILES += psock_fanout psock_tpacket
> -TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
> -TEST_GEN_FILES += reuseport_dualstack msg_zerocopy reuseaddr_conflict
> +TEST_GEN_PROGS += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
> +TEST_GEN_PROGS += reuseport_dualstack msg_zerocopy reuseaddr_conflict
Hmm. I see msg_zerocopy.sh for running msg_zerocopy. msg_zerocopy should
still stay in TEST_GEN_FILES and msg_zerocopy.sh needs to be added to
TEST_PROGS so it runs.
thanks,
-- Shuah
On Mon, Sep 18, 2017 at 04:14:41PM -0600, Shuah Khan wrote:
> On 09/18/2017 11:32 AM, [email protected] wrote:
> > From: Josef Bacik <[email protected]>
> >
> > These self tests are just self contained binaries, they are not run by
> > any of the scripts in the directory. This means they need to be marked
> > with TEST_GEN_PROGS to actually be run, not TEST_GEN_FILES.
> >
> > Signed-off-by: Josef Bacik <[email protected]>
> > ---
> > tools/testing/selftests/net/Makefile | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> > index 3df542c84610..45a4e77a47c4 100644
> > --- a/tools/testing/selftests/net/Makefile
> > +++ b/tools/testing/selftests/net/Makefile
> > @@ -6,8 +6,8 @@ CFLAGS += -I../../../../usr/include/
> > TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
> > TEST_GEN_FILES = socket
> > TEST_GEN_FILES += psock_fanout psock_tpacket
> > -TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
> > -TEST_GEN_FILES += reuseport_dualstack msg_zerocopy reuseaddr_conflict
> > +TEST_GEN_PROGS += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
> > +TEST_GEN_PROGS += reuseport_dualstack msg_zerocopy reuseaddr_conflict
>
> Hmm. I see msg_zerocopy.sh for running msg_zerocopy. msg_zerocopy should
> still stay in TEST_GEN_FILES and msg_zerocopy.sh needs to be added to
> TEST_PROGS so it runs.
>
Actually the shell script requires arguments, it doesn't just run the test.
I'll fix this to just omit the test for now as it's not setup to run properly.
Willem, could you follow up with a patch so that the zero copy test is run
properly the way you envision it running? You need to make sure that
make -C tools/testing/selftests TARGETS=net run_tests
actually runs your zero copy test the way you expect it to, otherwise it's just
sitting there collecting dust. Thanks,
Josef
On Tue, Sep 19, 2017 at 9:34 AM, Josef Bacik <[email protected]> wrote:
> On Mon, Sep 18, 2017 at 04:14:41PM -0600, Shuah Khan wrote:
>> On 09/18/2017 11:32 AM, [email protected] wrote:
>> > From: Josef Bacik <[email protected]>
>> >
>> > These self tests are just self contained binaries, they are not run by
>> > any of the scripts in the directory. This means they need to be marked
>> > with TEST_GEN_PROGS to actually be run, not TEST_GEN_FILES.
>> >
>> > Signed-off-by: Josef Bacik <[email protected]>
>> > ---
>> > tools/testing/selftests/net/Makefile | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
>> > index 3df542c84610..45a4e77a47c4 100644
>> > --- a/tools/testing/selftests/net/Makefile
>> > +++ b/tools/testing/selftests/net/Makefile
>> > @@ -6,8 +6,8 @@ CFLAGS += -I../../../../usr/include/
>> > TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
>> > TEST_GEN_FILES = socket
>> > TEST_GEN_FILES += psock_fanout psock_tpacket
>> > -TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
>> > -TEST_GEN_FILES += reuseport_dualstack msg_zerocopy reuseaddr_conflict
>> > +TEST_GEN_PROGS += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
>> > +TEST_GEN_PROGS += reuseport_dualstack msg_zerocopy reuseaddr_conflict
>>
>> Hmm. I see msg_zerocopy.sh for running msg_zerocopy. msg_zerocopy should
>> still stay in TEST_GEN_FILES and msg_zerocopy.sh needs to be added to
>> TEST_PROGS so it runs.
>>
>
> Actually the shell script requires arguments, it doesn't just run the test.
> I'll fix this to just omit the test for now as it's not setup to run properly.
>
> Willem, could you follow up with a patch so that the zero copy test is run
> properly the way you envision it running? You need to make sure that
>
> make -C tools/testing/selftests TARGETS=net run_tests
>
> actually runs your zero copy test the way you expect it to, otherwise it's just
> sitting there collecting dust. Thanks,
Will do.
In its current state, this test is really only meant to be run manually.
It demonstrates the API and outputs some information on stderr.
Zerocopy itself requires a two-host test. The feature is expressly
disabled over loopback.
But I can make this a pass/fail tests that exercises the interface
and notification channel and verifies that data was copied. It will
be a bit more work than just changing the default invocation of
msg_zerocopy.sh
On 09/19/2017 12:14 PM, Willem de Bruijn wrote:
> On Tue, Sep 19, 2017 at 9:34 AM, Josef Bacik <[email protected]> wrote:
>> On Mon, Sep 18, 2017 at 04:14:41PM -0600, Shuah Khan wrote:
>>> On 09/18/2017 11:32 AM, [email protected] wrote:
>>>> From: Josef Bacik <[email protected]>
>>>>
>>>> These self tests are just self contained binaries, they are not run by
>>>> any of the scripts in the directory. This means they need to be marked
>>>> with TEST_GEN_PROGS to actually be run, not TEST_GEN_FILES.
>>>>
>>>> Signed-off-by: Josef Bacik <[email protected]>
>>>> ---
>>>> tools/testing/selftests/net/Makefile | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
>>>> index 3df542c84610..45a4e77a47c4 100644
>>>> --- a/tools/testing/selftests/net/Makefile
>>>> +++ b/tools/testing/selftests/net/Makefile
>>>> @@ -6,8 +6,8 @@ CFLAGS += -I../../../../usr/include/
>>>> TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
>>>> TEST_GEN_FILES = socket
>>>> TEST_GEN_FILES += psock_fanout psock_tpacket
>>>> -TEST_GEN_FILES += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
>>>> -TEST_GEN_FILES += reuseport_dualstack msg_zerocopy reuseaddr_conflict
>>>> +TEST_GEN_PROGS += reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
>>>> +TEST_GEN_PROGS += reuseport_dualstack msg_zerocopy reuseaddr_conflict
>>>
>>> Hmm. I see msg_zerocopy.sh for running msg_zerocopy. msg_zerocopy should
>>> still stay in TEST_GEN_FILES and msg_zerocopy.sh needs to be added to
>>> TEST_PROGS so it runs.
>>>
>>
>> Actually the shell script requires arguments, it doesn't just run the test.
>> I'll fix this to just omit the test for now as it's not setup to run properly.
>>
>> Willem, could you follow up with a patch so that the zero copy test is run
>> properly the way you envision it running? You need to make sure that
>>
>> make -C tools/testing/selftests TARGETS=net run_tests
>>
>> actually runs your zero copy test the way you expect it to, otherwise it's just
>> sitting there collecting dust. Thanks,
>
> Will do.
>
> In its current state, this test is really only meant to be run manually.
> It demonstrates the API and outputs some information on stderr.
>
> Zerocopy itself requires a two-host test. The feature is expressly
> disabled over loopback.
Running this manually is [perfectly fine. TEST_GEN_FILES is the right
place for it as TEST_GEN_FILES will get built and installed, but won't
be run bt lib.mk. No changes needed.
>
> But I can make this a pass/fail tests that exercises the interface
> and notification channel and verifies that data was copied. It will
> be a bit more work than just changing the default invocation of
> msg_zerocopy.sh
>
No need to make changes as this test is intended to be run manually.
Josef's v2 doesn't change the location of msg_zerocopy. We are all set.
Thanks for clearing this up.
-- Shuah