Skip to contentSkip to conversation

Makefile: have check target depend on all

PATCH Superseded Luca Boccassi 3 participants 22 messages Dec 15, 2018 – Dec 16, 2018

Subsystems (3)
Networking Network drivers Kbuild
lore
Share & tools
lore

Luca Boccassi

author
#0 lore raw

PATCH 1/4 Makefile: have check target depend on all

Otherwise it will simply fail immediately from a just-cleaned workspace:

$ make check -j1
cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..;
Entering iproute2
make -C tools
Makefile:3: ../../config.mk: No such file or directory
make[2]: *** No rule to make target '../../config.mk'.  Stop.
Fixes 8804a8c0d387 ("Makefile: Add check target")
Cc [email protected]
Signed-off-by Luca Boccassi <[email protected]>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
index b7488add..20c760e2 100644
@@ -119,7 +119,7 @@ clobber:
 
 distclean: clobber
 
-check:
+check: all
 	cd testsuite && $(MAKE) && $(MAKE) alltests
 
 cscope:
Signature · 1 line
2.19.2
author
#1 lore raw

PATCH 2/4 testsuite: declare dependency between $(TESTS) and generate_nlmsg

Parallel make from the top level directory fails since tests are at the same time as generate_nlmsg:

$ make check -j4

...

cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..; Entering iproute2
make -C tools
Removing results dir ...
make[1]: ./tools/generate_nlmsg: Command not found make[1]: ./tools/generate_nlmsg: Command not found Makefile:64: recipe for target 'ip/netns/set_nsid_batch.t' failed make[1]: *** [ip/netns/set_nsid_batch.t] Error 127 make[1]: ./tools/generate_nlmsg: Command not found make[1]: *** Waiting for unfinished jobs....
Makefile:64: recipe for target 'ip/netns/set_nsid.t' failed make[1]: *** [ip/netns/set_nsid.t] Error 127
Makefile:64: recipe for target 'ip/link/show_dev_wo_vf_rate.t' failed make[1]: *** [ip/link/show_dev_wo_vf_rate.t] Error 127
CC generate_nlmsg
Makefile:123: recipe for target 'check' failed
make: *** [check] Error 2

Add an explicit dependency in testuite/Makefile's $(TESTS) rule so that the tool correctly gets compiled before any test runs.

Fixes 3537633dcf44 ("testsuite: Generate generate_nlmsg when needed")
Cc [email protected]
Signed-off-by Luca Boccassi <[email protected]>
---
 testsuite/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testsuite/Makefile +1 -1
index 46b243b0..9b0f1c15 100644
@@ -53,7 +53,7 @@ clean: testclean
 distclean: clean
 	echo "Entering iproute2" && cd iproute2 && $(MAKE) distclean && cd ..;
 
-$(TESTS): testclean
+$(TESTS): generate_nlmsg testclean
 ifeq (,$(IPVERS))
 	$(error Please run make first)
 endif
Signature · 1 line
2.19.2
Reviewed-by Tested-by on v1 2/4
#2 lore raw

Re: [PATCH iproute2 2/4] testsuite: declare dependency between $(TESTS) and generate_nlmsg

Hi Luca,

Quoted 26 lines

Parallel make from the top level directory fails since tests are at the same time as generate_nlmsg:

$ make check -j4

...

cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..; Entering iproute2
make -C tools
Removing results dir ...
make[1]: ./tools/generate_nlmsg: Command not found make[1]: ./tools/generate_nlmsg: Command not found Makefile:64: recipe for target 'ip/netns/set_nsid_batch.t' failed make[1]: *** [ip/netns/set_nsid_batch.t] Error 127 make[1]: ./tools/generate_nlmsg: Command not found make[1]: *** Waiting for unfinished jobs....
Makefile:64: recipe for target 'ip/netns/set_nsid.t' failed make[1]: *** [ip/netns/set_nsid.t] Error 127
Makefile:64: recipe for target 'ip/link/show_dev_wo_vf_rate.t' failed make[1]: *** [ip/link/show_dev_wo_vf_rate.t] Error 127
CC generate_nlmsg
Makefile:123: recipe for target 'check' failed
make: *** [check] Error 2

Quoted 7 lines

Add an explicit dependency in testuite/Makefile's $(TESTS) rule so that the tool correctly gets compiled before any test runs.

Fixes 3537633dcf44 ("testsuite: Generate generate_nlmsg when needed")
Cc [email protected]
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Tested-by Petr Vorel <[email protected]>

Again, thanks for fix.

Kind regards,
Petr

author
#3 lore raw

PATCH 3/4 tests: delete dummy interface after default route test

In reply to Luca Boccassi
Signed-off-by Luca Boccassi <[email protected]>
---
 testsuite/tests/ip/route/add_default_route.t | 2 ++
 1 file changed, 2 insertions(+)
testsuite/tests/ip/route/add_default_route.t +2 -0
index 569ba1f8..c536e35f 100755
@@ -31,3 +31,5 @@ ts_ip "$0" "Add another IPv6 route dst cafe:babe::/64" -6 route add cafe:babe::/
 ts_ip "$0" "Show IPv6 default route" -6 route show default
 test_on "default via dead:beef::2 dev $DEV"
 test_lines_count 1
+
+ts_ip "$0" "Del $NEW_DEV dummy interface"  link del dev $DEV
Signature · 1 line
2.19.2
Reviewed-by on v1 3/4
#4 lore raw

Re: [PATCH iproute2 3/4] tests: delete dummy interface after default route test

Hi Luca,

Quoted 1 line
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Quoted 9 lines
---
 testsuite/tests/ip/route/add_default_route.t | 2 ++
 1 file changed, 2 insertions(+)
testsuite/tests/ip/route/add_default_route.t +0 -0
index 569ba1f8..c536e35f 100755

testsuite/tests/ip/route/add_default_route.t ts_ip "$0" "Add another IPv6 route dst cafe:babe::/64" -6 route add cafe:babe::/ · line 35

Quoted 5 lines
testsuite/tests/ip/route/add_default_route.t +2 -0
@@ -31,3 +31,5 @@ ts_ip "$0" "Add another IPv6 route dst cafe:babe::/64" -6 route add cafe:babe::/
 ts_ip "$0" "Show IPv6 default route" -6 route show default
 test_on "default via dead:beef::2 dev $DEV"
 test_lines_count 1
+
+ts_ip "$0" "Del $NEW_DEV dummy interface"  link del dev $DEV

Shouldn't this be using $DEV instead of $NEW_DEV? ts_ip "$0" "Delete $DEV interface" link del dev $DEV

Copy paste from new_link.t?

Kind regards,
Petr

author
#5 lore raw

Re: [PATCH iproute2 3/4] tests: delete dummy interface after default route test

Quoted 24 lines · Petr Vorel

Hi Luca,

Quoted 1 line
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Quoted 13 lines

---
 testsuite/tests/ip/route/add_default_route.t | 2 ++

 1 file changed, 2 insertions(+)
testsuite/tests/ip/route/add_default_route.t +0 -0
index 569ba1f8..c536e35f 100755
@@ -31,3 +31,5 @@ ts_ip "$0" "Add another IPv6 route dst

cafe:babe::/64" -6 route add cafe:babe::/
 ts_ip "$0" "Show IPv6 default route" -6 route show default test_on "default via dead:beef::2 dev $DEV" test_lines_count 1

testsuite/tests/ip/route/add_default_route.t ts_ip "$0" "Add another IPv6 route dst cafe:babe::/64" -6 route add cafe:babe::/ · line 35

Quoted 2 lines
testsuite/tests/ip/route/add_default_route.t +2 -0
+
+ts_ip "$0" "Del $NEW_DEV dummy interface"  link del dev $DEV

Shouldn't this be using $DEV instead of $NEW_DEV? ts_ip "$0" "Delete $DEV interface" link del dev $DEV

Quoted 1 line

Copy paste from new_link.t?

Ops! Yes indeed, copy-pasta, fixed in v2, thanks.

Signature · 2 lines
Kind regards,
Luca Boccassi
author
#6 lore raw

PATCH 4/4 testsuite: remove gre kmods if the test loads them

In reply to Luca Boccassi

The tunnel test leaves behind link devices created by the GRE kernel modules:

$ ip -br link
...
gre0@NONE    DOWN 0.0.0.0 <NOARP>
gretap0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
erspan0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
ip6tnl0@NONE DOWN :: <NOARP>
ip6gre0@NONE DOWN 00:00:00:00:
$ lsmod | grep gre
ip6_gre      40960  0
ip6_tunnel   40960  1 ip6_gre
ip_gre       32768  0
ip_tunnel    24576  1 ip_gre
gre          16384  2 ip6_gre,ip_gre

Check beforehand if the gre kernel module is loaded, and if not unload them all at the end of the test. This should avoid causing problems if a user is already using GRE for other purposes.

Signed-off-by Luca Boccassi <[email protected]>
---
 testsuite/tests/ip/tunnel/add_tunnel.t | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
testsuite/tests/ip/tunnel/add_tunnel.t +24 -0
index 3f5a9d3c..76f8b011 100755
@@ -4,6 +4,15 @@
 
 TUNNEL_NAME="tunnel_test_ip"
 
+# note that checkbashism reports command -v, but dash supports it and it's posix compliant
+if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1
+then
+    KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
+    COUNT_KMODS_BEFORE=$(lsmod | grep -c -e "^ip6_gre" -e "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")
+else
+    KMODS=""
+fi
+
 ts_log "[Testing add/del tunnels]"
 
 ts_ip "$0" "Add GRE tunnel over IPv4" tunnel add name $TUNNEL_NAME mode gre local 1.1.1.1 remote 2.2.2.2

@@ -12,3 +21,18 @@ ts_ip "$0" "Del GRE tunnel over IPv4" tunnel del $TUNNEL_NAME
 ts_ip "$0" "Add GRE tunnel over IPv6" tunnel add name $TUNNEL_NAME mode ip6gre local dead:beef::1 remote dead:beef::2
 ts_ip "$0" "Del GRE tunnel over IPv6" tunnel del $TUNNEL_NAME
 
+
+if [ -n "$KMODS" ]
+then
+    # unload kernel modules to remove dummy interfaces only if they were not in use beforehand
+    COUNT_KMODS_AFTER=$(lsmod | grep -c -e "^ip6_gre" -e "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")
+    if [ "$COUNT_KMODS_BEFORE" -eq 0 ] && [ "$COUNT_KMODS_AFTER" -gt 0 ]
+    then
+        for mod in $KMODS
+        do
+            sudo rmmod "$mod"
+        done
+    else
+        ts_log "[gre kernel module was loaded before test, not removing]"
+    fi
+fi
Signature · 1 line
2.19.2
Reviewed-by on v1 4/4
#7 lore raw

Re: [PATCH iproute2 4/4] testsuite: remove gre kmods if the test loads them

Hi Luca,

Quoted 18 lines

The tunnel test leaves behind link devices created by the GRE kernel modules:

$ ip -br link
...
gre0@NONE    DOWN 0.0.0.0 <NOARP>
gretap0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
erspan0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
ip6tnl0@NONE DOWN :: <NOARP>
ip6gre0@NONE DOWN 00:00:00:00:
$ lsmod | grep gre
ip6_gre      40960  0
ip6_tunnel   40960  1 ip6_gre
ip_gre       32768  0
ip_tunnel    24576  1 ip_gre
gre          16384  2 ip6_gre,ip_gre
Quoted 5 lines

Check beforehand if the gre kernel module is loaded, and if not unload them all at the end of the test. This should avoid causing problems if a user is already using GRE for other purposes.

Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>

testsuite/tests/ip/tunnel/add_tunnel.t line 7

Quoted 13 lines
---
 testsuite/tests/ip/tunnel/add_tunnel.t | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
testsuite/tests/ip/tunnel/add_tunnel.t +1 -0
index 3f5a9d3c..76f8b011 100755
@@ -4,6 +4,15 @@
 
 TUNNEL_NAME="tunnel_test_ip"
 
+# note that checkbashism reports command -v, but dash supports it and it's posix compliant

NOTE: also 'busybox sh' and mksh (used also in older Android) support it.
BTW: yes, it's not optional in POSIX 2008/2013, it's optional in 2004 [1]. But I'd put this info only into commit message, most people probably does not care.

Quoted 7 lines
testsuite/tests/ip/tunnel/add_tunnel.t +7 -0
+if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1
+then
+    KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
+    COUNT_KMODS_BEFORE=$(lsmod | grep -c -e "^ip6_gre" -e "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")
+else
+    KMODS=""
+fi

...

Quoted 4 lines
testsuite/tests/ip/tunnel/add_tunnel.t +4 -0
+if [ -n "$KMODS" ]
+then
+    # unload kernel modules to remove dummy interfaces only if they were not in use beforehand
+    COUNT_KMODS_AFTER=$(lsmod | grep -c -e "^ip6_gre" -e "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")

testsuite/tests/ip/tunnel/add_tunnel.t ts_ip "$0" "Del GRE tunnel over IPv4" tunnel del $TUNNEL_NAME · line 38

Quoted 10 lines
testsuite/tests/ip/tunnel/add_tunnel.t +10 -0
+    if [ "$COUNT_KMODS_BEFORE" -eq 0 ] && [ "$COUNT_KMODS_AFTER" -gt 0 ]
+    then
+        for mod in $KMODS
+        do
+            sudo rmmod "$mod"
+        done
+    else
+        ts_log "[gre kernel module was loaded before test, not removing]"
+    fi
+fi

You repeating yourself with module names.
How about something like this:

KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"

# unload kernel modules to remove dummy interfaces only if they were not in use beforehand
KMODS_REMOVE=
if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1; then
	for i in $KMODS; do
		lsmod |grep -q "^$i " || KMODS_REMOVE="$KMODS_REMOVE $i";
	done
fi

...

for mod in $KMODS_REMOVE; do
sudo rmmod "$mod"
done

I.e. keeping modules to remove (you can also loaded modules, if you want to warn about it, but I'd ignore it).

BTW I'd use 'then' and 'do' on the same line (i.e.: if [ ... ]; then)
+ define variable before usage.

[1] https://stackoverflow.com/questions/34572700/is-command-v-option-required-in-a-posix-shell-is-posh-compliant-with-posix

Kind regards,
Petr

author
#8 lore raw

Re: [PATCH iproute2 4/4] testsuite: remove gre kmods if the test loads them

Quoted 41 lines · Petr Vorel

Hi Luca,

Quoted 14 lines

The tunnel test leaves behind link devices created by the GRE kernel
modules:

$ ip -br link
...
gre0@NONE    DOWN 0.0.0.0 <NOARP>
gretap0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
erspan0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
ip6tnl0@NONE DOWN :: <NOARP>
ip6gre0@NONE DOWN 00:00:00:00:
$ lsmod | grep gre
ip6_gre      40960  0
ip6_tunnel   40960  1 ip6_gre
ip_gre       32768  0
Quoted 8 lines
ip_tunnel    24576  1 ip_gre
gre          16384  2 ip6_gre,ip_gre
Check beforehand if the gre kernel module is loaded, and if not
unload
them all at the end of the test. This should avoid causing problems
if
a user is already using GRE for other purposes.
Signed-off-by: Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Quoted 10 lines

---
 testsuite/tests/ip/tunnel/add_tunnel.t | 24
++++++++++++++++++++++++

 1 file changed, 24 insertions(+)
testsuite/tests/ip/tunnel/add_tunnel.t +0 -0
index 3f5a9d3c..76f8b011 100755
@@ -4,6 +4,15 @@
Quoted 3 lines

 TUNNEL_NAME="tunnel_test_ip"
+# note that checkbashism reports command -v, but dash supports it and it's posix compliant

Quoted 7 lines

NOTE: also 'busybox sh' and mksh (used also in older Android) support it.
BTW: yes, it's not optional in POSIX 2008/2013, it's optional in 2004 [1].
But I'd put this info only into commit message, most people probably does not
care.

I left it as a comment as I know some developers are running checkbashism on the scripts in this repo, so it might save them some time in the future.

Quoted 53 lines
Quoted 9 lines

+if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1
+then
+    KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
+    COUNT_KMODS_BEFORE=$(lsmod | grep -c -e "^ip6_gre" -e "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")

testsuite/tests/ip/tunnel/add_tunnel.t +3 -0
+else
+    KMODS=""
+fi

...

Quoted 10 lines
testsuite/tests/ip/tunnel/add_tunnel.t +3 -0
+if [ -n "$KMODS" ]
+then
+    # unload kernel modules to remove dummy interfaces only if

they were not in use beforehand
+    COUNT_KMODS_AFTER=$(lsmod | grep -c -e "^ip6_gre" -e "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre")
+    if [ "$COUNT_KMODS_BEFORE" -eq 0 ] && [ "$COUNT_KMODS_AFTER" -gt 0 ]
+    then
+        for mod in $KMODS

Quoted 8 lines

+        do
+            sudo rmmod "$mod"
+        done
+    else
+        ts_log "[gre kernel module was loaded before test, not removing]"
+    fi
+fi

You repeating yourself with module names.
How about something like this:

KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"

# unload kernel modules to remove dummy interfaces only if they were
not in use beforehand
KMODS_REMOVE=
if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null
2>&1; then
	for i in $KMODS; do
		lsmod |grep -q "^$i " || KMODS_REMOVE="$KMODS_REMOVE
$i";
	done
fi

...

for mod in $KMODS_REMOVE; do
sudo rmmod "$mod"
done

Quoted 6 lines

I.e. keeping modules to remove (you can also loaded modules, if you want to warn
about it, but I'd ignore it).

BTW I'd use 'then' and 'do' on the same line (i.e.: if [ ... ]; then)
+ define variable before usage.

Ok, thanks for the suggestions and the reviews, I've applied them in v2 and tested again, all looks fine.

Signature · 2 lines
Kind regards,
Luca Boccassi
Reviewed-by Tested-by on v1 1/4
#9 lore raw
In reply to Luca Boccassi

Hi Luca,

Quoted 11 lines

Otherwise it will simply fail immediately from a just-cleaned workspace:

$ make check -j1
cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..;
Entering iproute2
make -C tools
Makefile:3: ../../config.mk: No such file or directory
make[2]: *** No rule to make target '../../config.mk'.  Stop.
Quoted 4 lines
Fixes 8804a8c0d387 ("Makefile: Add check target")
Cc [email protected]
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Tested-by Petr Vorel <[email protected]>

Thanks for a fix. I added 92885e19 ("testsuite: Fix make check when need build generate_nlmsg"), but that wasn't enough.

Kind regards,
Petr

author
#10 lore raw

PATCH v2 1/4 Makefile: have check target depend on all

In reply to Luca Boccassi

Otherwise it will simply fail immediately from a just-cleaned workspace:

$ make check -j1
cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..;
Entering iproute2
make -C tools
Makefile:3: ../../config.mk: No such file or directory
make[2]: *** No rule to make target '../../config.mk'.  Stop.
Fixes 8804a8c0d387 ("Makefile: Add check target")
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Tested-by Petr Vorel <[email protected]>

---
v2: added reviewed/tested-by tags, removed Cc

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
index b7488add..20c760e2 100644
@@ -119,7 +119,7 @@ clobber:
 
 distclean: clobber
 
-check:
+check: all
 	cd testsuite && $(MAKE) && $(MAKE) alltests
 
 cscope:
Signature · 1 line
2.19.2
author
#11 lore raw

PATCH v2 2/4 testsuite: declare dependency between $(TESTS) and generate_nlmsg

Parallel make from the top level directory fails since tests are at the same time as generate_nlmsg:

$ make check -j4

...

cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..; Entering iproute2
make -C tools
Removing results dir ...
make[1]: ./tools/generate_nlmsg: Command not found make[1]: ./tools/generate_nlmsg: Command not found Makefile:64: recipe for target 'ip/netns/set_nsid_batch.t' failed make[1]: *** [ip/netns/set_nsid_batch.t] Error 127 make[1]: ./tools/generate_nlmsg: Command not found make[1]: *** Waiting for unfinished jobs....
Makefile:64: recipe for target 'ip/netns/set_nsid.t' failed make[1]: *** [ip/netns/set_nsid.t] Error 127
Makefile:64: recipe for target 'ip/link/show_dev_wo_vf_rate.t' failed make[1]: *** [ip/link/show_dev_wo_vf_rate.t] Error 127
CC generate_nlmsg
Makefile:123: recipe for target 'check' failed
make: *** [check] Error 2

Add an explicit dependency in testuite/Makefile's $(TESTS) rule so that the tool correctly gets compiled before any test runs.

Fixes 3537633dcf44 ("testsuite: Generate generate_nlmsg when needed")
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Tested-by Petr Vorel <[email protected]>

---
v2: added reviewed/tested-by tags, removed Cc

 testsuite/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testsuite/Makefile +1 -1
index 46b243b0..9b0f1c15 100644
@@ -53,7 +53,7 @@ clean: testclean
 distclean: clean
 	echo "Entering iproute2" && cd iproute2 && $(MAKE) distclean && cd ..;
 
-$(TESTS): testclean
+$(TESTS): generate_nlmsg testclean
 ifeq (,$(IPVERS))
 	$(error Please run make first)
 endif
Signature · 1 line
2.19.2
author
#12 lore raw

PATCH v2 3/4 testsuite: delete dummy interface after default route test

In reply to Luca Boccassi
Signed-off-by Luca Boccassi <[email protected]>

---
v2: fixed copypasta in log message

 testsuite/tests/ip/route/add_default_route.t | 2 ++
 1 file changed, 2 insertions(+)
testsuite/tests/ip/route/add_default_route.t +2 -0
index 569ba1f8..ded4edc3 100755
@@ -31,3 +31,5 @@ ts_ip "$0" "Add another IPv6 route dst cafe:babe::/64" -6 route add cafe:babe::/
 ts_ip "$0" "Show IPv6 default route" -6 route show default
 test_on "default via dead:beef::2 dev $DEV"
 test_lines_count 1
+
+ts_ip "$0" "Del $DEV dummy interface"  link del dev $DEV
Signature · 1 line
2.19.2
author
#14 lore raw

PATCH v2 4/4 testsuite: remove gre kmods if the test loads them

In reply to Luca Boccassi

The tunnel test leaves behind link devices created by the GRE kernel modules:

$ ip -br link
...
gre0@NONE    DOWN 0.0.0.0 <NOARP>
gretap0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
erspan0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
ip6tnl0@NONE DOWN :: <NOARP>
ip6gre0@NONE DOWN 00:00:00:00:
$ lsmod | grep gre
ip6_gre      40960  0
ip6_tunnel   40960  1 ip6_gre
ip_gre       32768  0
ip_tunnel    24576  1 ip_gre
gre          16384  2 ip6_gre,ip_gre

Check beforehand if the gre kernel module is loaded, and if not unload them all at the end of the test. This should avoid causing problems if a user is already using GRE for other purposes.

Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>

---
v2: applied suggestions from Petr to simplify the modules parsing/unloading

 testsuite/tests/ip/tunnel/add_tunnel.t | 13 +++++++++++++
 1 file changed, 13 insertions(+)
testsuite/tests/ip/tunnel/add_tunnel.t +13 -0
index 3f5a9d3c..78c1e463 100755
@@ -4,6 +4,16 @@
 
 TUNNEL_NAME="tunnel_test_ip"
 
+# unload kernel modules to remove dummy interfaces only if they were not in use beforehand
+KMODS_REMOVE=
+# note that checkbashism reports command -v, but dash supports it and it's POSIX 2008 compliant
+if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1; then
+    KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
+    for i in $KMODS; do
+        lsmod | grep -q "^$i" || KMODS_REMOVE="$KMODS_REMOVE $i";
+    done
+fi
+
 ts_log "[Testing add/del tunnels]"
 
 ts_ip "$0" "Add GRE tunnel over IPv4" tunnel add name $TUNNEL_NAME mode gre local 1.1.1.1 remote 2.2.2.2

@@ -12,3 +22,6 @@ ts_ip "$0" "Del GRE tunnel over IPv4" tunnel del $TUNNEL_NAME
 ts_ip "$0" "Add GRE tunnel over IPv6" tunnel add name $TUNNEL_NAME mode ip6gre local dead:beef::1 remote dead:beef::2
 ts_ip "$0" "Del GRE tunnel over IPv6" tunnel del $TUNNEL_NAME
 
+for mod in $KMODS_REMOVE; do
+    sudo rmmod "$mod"
+done
Signature · 1 line
2.19.2
Reviewed-by on v2 4/4
#15 lore raw

Re: [PATCH iproute2 v2 4/4] testsuite: remove gre kmods if the test loads them

Hi Luca,

Reviewed-by Petr Vorel <[email protected]>

LGTM, but I'd suggest 2 small changes (see bellow).

Quoted 3 lines

+++ b/testsuite/tests/ip/tunnel/add_tunnel.t

TUNNEL_NAME="tunnel_test_ip"

I'd put KMODS here:
KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"

The reason is maintenance of this script - maybe one day there will be other modules needed to be added, take this list as a configuration (which is usually in shell scripts in the top).
BTW Maintenance was reason why I didn't like duplicity in modules you had in v1.

testsuite/tests/ip/tunnel/add_tunnel.t line 8

Quoted 2 lines
testsuite/tests/ip/tunnel/add_tunnel.t +2 -0
+# unload kernel modules to remove dummy interfaces only if they were not in use beforehand
+KMODS_REMOVE=

As a side effect, this could be lower case (showing it's not a configuration variable, but just normal variable).

Quoted 12 lines
testsuite/tests/ip/tunnel/add_tunnel.t +8 -0
+# note that checkbashism reports command -v, but dash supports it and it's POSIX 2008 compliant
+if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1; then
+    KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
+    for i in $KMODS; do
+        lsmod | grep -q "^$i" || KMODS_REMOVE="$KMODS_REMOVE $i";
+    done
+fi
+
 ts_log "[Testing add/del tunnels]"
 
 ts_ip "$0" "Add GRE tunnel over IPv4" tunnel add name $TUNNEL_NAME mode gre local 1.1.1.1 remote 2.2.2.2

testsuite/tests/ip/tunnel/add_tunnel.t ts_ip "$0" "Del GRE tunnel over IPv4" tunnel del $TUNNEL_NAME · line 27

Quoted 6 lines
testsuite/tests/ip/tunnel/add_tunnel.t +3 -0
@@ -12,3 +22,6 @@ ts_ip "$0" "Del GRE tunnel over IPv4" tunnel del $TUNNEL_NAME
 ts_ip "$0" "Add GRE tunnel over IPv6" tunnel add name $TUNNEL_NAME mode ip6gre local dead:beef::1 remote dead:beef::2
 ts_ip "$0" "Del GRE tunnel over IPv6" tunnel del $TUNNEL_NAME
 
+for mod in $KMODS_REMOVE; do
+    sudo rmmod "$mod"
+done

Kind regards,
Petr

author
#16 lore raw

Re: [PATCH iproute2 v2 4/4] testsuite: remove gre kmods if the test loads them

Quoted 19 lines · Petr Vorel

Hi Luca,

Reviewed-by Petr Vorel <[email protected]>

LGTM, but I'd suggest 2 small changes (see bellow).

Quoted 2 lines

+++ b/testsuite/tests/ip/tunnel/add_tunnel.t
 TUNNEL_NAME="tunnel_test_ip"

I'd put KMODS here:
KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"

The reason is maintenance of this script - maybe one day there will be other
modules needed to be added, take this list as a configuration (which is usually
in shell scripts in the top).
BTW Maintenance was reason why I didn't like duplicity in modules you had in v1.

Quoted 7 lines
Quoted 3 lines

+# unload kernel modules to remove dummy interfaces only if they were not in use beforehand
+KMODS_REMOVE=

As a side effect, this could be lower case (showing it's not a configuration
variable, but just normal variable).

Ok, thanks, done both in v3.

Signature · 2 lines
Kind regards,
Luca Boccassi
author
#17 lore raw

PATCH v3 1/4 Makefile: have check target depend on all

In reply to Luca Boccassi

Otherwise it will simply fail immediately from a just-cleaned workspace:

$ make check -j1
cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..;
Entering iproute2
make -C tools
Makefile:3: ../../config.mk: No such file or directory
make[2]: *** No rule to make target '../../config.mk'.  Stop.
Fixes 8804a8c0d387 ("Makefile: Add check target")
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Tested-by Petr Vorel <[email protected]>

---
v2: added reviewed/tested-by tags, removed Cc

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
index b7488add..20c760e2 100644
@@ -119,7 +119,7 @@ clobber:
 
 distclean: clobber
 
-check:
+check: all
 	cd testsuite && $(MAKE) && $(MAKE) alltests
 
 cscope:
Signature · 1 line
2.19.2
author
#18 lore raw

PATCH v3 2/4 testsuite: declare dependency between $(TESTS) and generate_nlmsg

Parallel make from the top level directory fails since tests are at the same time as generate_nlmsg:

$ make check -j4

...

cd testsuite && make && make alltests
echo "Entering iproute2" && cd iproute2 && make configure && cd ..; Entering iproute2
make -C tools
Removing results dir ...
make[1]: ./tools/generate_nlmsg: Command not found make[1]: ./tools/generate_nlmsg: Command not found Makefile:64: recipe for target 'ip/netns/set_nsid_batch.t' failed make[1]: *** [ip/netns/set_nsid_batch.t] Error 127 make[1]: ./tools/generate_nlmsg: Command not found make[1]: *** Waiting for unfinished jobs....
Makefile:64: recipe for target 'ip/netns/set_nsid.t' failed make[1]: *** [ip/netns/set_nsid.t] Error 127
Makefile:64: recipe for target 'ip/link/show_dev_wo_vf_rate.t' failed make[1]: *** [ip/link/show_dev_wo_vf_rate.t] Error 127
CC generate_nlmsg
Makefile:123: recipe for target 'check' failed
make: *** [check] Error 2

Add an explicit dependency in testuite/Makefile's $(TESTS) rule so that the tool correctly gets compiled before any test runs.

Fixes 3537633dcf44 ("testsuite: Generate generate_nlmsg when needed")
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Tested-by Petr Vorel <[email protected]>

---
v2: added reviewed/tested-by tags, removed Cc

 testsuite/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testsuite/Makefile +1 -1
index 46b243b0..9b0f1c15 100644
@@ -53,7 +53,7 @@ clean: testclean
 distclean: clean
 	echo "Entering iproute2" && cd iproute2 && $(MAKE) distclean && cd ..;
 
-$(TESTS): testclean
+$(TESTS): generate_nlmsg testclean
 ifeq (,$(IPVERS))
 	$(error Please run make first)
 endif
Signature · 1 line
2.19.2
author
#19 lore raw

PATCH v3 3/4 testsuite: delete dummy interface after default route test

In reply to Luca Boccassi
Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>

---
v2: fixed copypasta in log message
v3: added reviewed-by tag

 testsuite/tests/ip/route/add_default_route.t | 2 ++
 1 file changed, 2 insertions(+)
testsuite/tests/ip/route/add_default_route.t +2 -0
index 569ba1f8..ded4edc3 100755
@@ -31,3 +31,5 @@ ts_ip "$0" "Add another IPv6 route dst cafe:babe::/64" -6 route add cafe:babe::/
 ts_ip "$0" "Show IPv6 default route" -6 route show default
 test_on "default via dead:beef::2 dev $DEV"
 test_lines_count 1
+
+ts_ip "$0" "Del $DEV dummy interface"  link del dev $DEV
Signature · 1 line
2.19.2
author
#20 lore raw

PATCH v3 4/4 testsuite: remove gre kmods if the test loads them

In reply to Luca Boccassi

The tunnel test leaves behind link devices created by the GRE kernel modules:

$ ip -br link
...
gre0@NONE    DOWN 0.0.0.0 <NOARP>
gretap0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
erspan0@NONE DOWN 00:00:00:00:00:00 <BROADCAST,MULTICAST>
ip6tnl0@NONE DOWN :: <NOARP>
ip6gre0@NONE DOWN 00:00:00:00:
$ lsmod | grep gre
ip6_gre      40960  0
ip6_tunnel   40960  1 ip6_gre
ip_gre       32768  0
ip_tunnel    24576  1 ip_gre
gre          16384  2 ip6_gre,ip_gre

Check beforehand if the gre kernel module is loaded, and if not unload them all at the end of the test. This should avoid causing problems if a user is already using GRE for other purposes.

Signed-off-by Luca Boccassi <[email protected]>
Reviewed-by Petr Vorel <[email protected]>

---
v2: applied suggestions from Petr to simplify the modules parsing/unloading
v3: ditto, added reviewed-by tag

 testsuite/tests/ip/tunnel/add_tunnel.t | 13 +++++++++++++
 1 file changed, 13 insertions(+)
testsuite/tests/ip/tunnel/add_tunnel.t +13 -0
index 3f5a9d3c..65db431c 100755
@@ -3,6 +3,16 @@
 . lib/generic.sh
 
 TUNNEL_NAME="tunnel_test_ip"
+KMODS="ip6_gre ip6_tunnel ip_gre ip_tunnel gre"
+
+# unload kernel modules to remove dummy interfaces only if they were not in use beforehand
+kmods_remove=
+# note that checkbashism reports command -v, but dash supports it and it's POSIX 2008 compliant
+if command -v lsmod >/dev/null 2>&1 && command -v rmmod >/dev/null 2>&1; then
+    for i in $KMODS; do
+        lsmod | grep -q "^$i" || kmods_remove="$kmods_remove $i";
+    done
+fi
 
 ts_log "[Testing add/del tunnels]"
 

@@ -12,3 +22,6 @@ ts_ip "$0" "Del GRE tunnel over IPv4" tunnel del $TUNNEL_NAME
 ts_ip "$0" "Add GRE tunnel over IPv6" tunnel add name $TUNNEL_NAME mode ip6gre local dead:beef::1 remote dead:beef::2
 ts_ip "$0" "Del GRE tunnel over IPv6" tunnel del $TUNNEL_NAME
 
+for mod in $kmods_remove; do
+    sudo rmmod "$mod"
+done
Signature · 1 line
2.19.2