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

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

[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")
Cc: petr.vorel@gmail.com

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b7488add..20c760e2 100644
--- a/Makefile
+++ b/Makefile
@@ -119,7 +119,7 @@ clobber:
 
 distclean: clobber
 
-check:
+check: all
 	cd testsuite && $(MAKE) && $(MAKE) alltests
 
 cscope:
-- 
2.19.2

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

[Luca Boccassi]

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: petr.vorel@gmail.com

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 testsuite/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testsuite/Makefile b/testsuite/Makefile
index 46b243b0..9b0f1c15 100644
--- a/testsuite/Makefile
+++ b/testsuite/Makefile
@@ -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
-- 
2.19.2

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

[Luca Boccassi]

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 testsuite/tests/ip/route/add_default_route.t | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testsuite/tests/ip/route/add_default_route.t b/testsuite/tests/ip/route/add_default_route.t
index 569ba1f8..c536e35f 100755
--- a/testsuite/tests/ip/route/add_default_route.t
+++ b/testsuite/tests/ip/route/add_default_route.t
@@ -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
-- 
2.19.2

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

[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 <bluca@debian.org>
---
 testsuite/tests/ip/tunnel/add_tunnel.t | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/testsuite/tests/ip/tunnel/add_tunnel.t b/testsuite/tests/ip/tunnel/add_tunnel.t
index 3f5a9d3c..76f8b011 100755
--- a/testsuite/tests/ip/tunnel/add_tunnel.t
+++ b/testsuite/tests/ip/tunnel/add_tunnel.t
@@ -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
-- 
2.19.2

Re: [PATCH iproute2 1/4] Makefile: have check target depend on all

[Petr Vorel]

Hi Luca,

> 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: petr.vorel@gmail.com

> Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Tested-by: Petr Vorel <petr.vorel@gmail.com>

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


Kind regards,
Petr

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

[Petr Vorel]

Hi Luca,

> 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: petr.vorel@gmail.com

> Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Tested-by: Petr Vorel <petr.vorel@gmail.com>

Again, thanks for fix.


Kind regards,
Petr

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

[Petr Vorel]

Hi Luca,

> Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testsuite/tests/ip/route/add_default_route.t | 2 ++
>  1 file changed, 2 insertions(+)

> diff --git a/testsuite/tests/ip/route/add_default_route.t b/testsuite/tests/ip/route/add_default_route.t
> index 569ba1f8..c536e35f 100755
> --- a/testsuite/tests/ip/route/add_default_route.t
> +++ b/testsuite/tests/ip/route/add_default_route.t
> @@ -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

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

[Petr Vorel]

Hi Luca,

> 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 <bluca@debian.org>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> ---
>  testsuite/tests/ip/tunnel/add_tunnel.t | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

> diff --git a/testsuite/tests/ip/tunnel/add_tunnel.t b/testsuite/tests/ip/tunnel/add_tunnel.t
> index 3f5a9d3c..76f8b011 100755
> --- a/testsuite/tests/ip/tunnel/add_tunnel.t
> +++ b/testsuite/tests/ip/tunnel/add_tunnel.t
> @@ -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.

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

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

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

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

[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 <bluca@debian.org>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Tested-by: Petr Vorel <petr.vorel@gmail.com>
---
v2: added reviewed/tested-by tags, removed Cc

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b7488add..20c760e2 100644
--- a/Makefile
+++ b/Makefile
@@ -119,7 +119,7 @@ clobber:
 
 distclean: clobber
 
-check:
+check: all
 	cd testsuite && $(MAKE) && $(MAKE) alltests
 
 cscope:
-- 
2.19.2

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

[Luca Boccassi]

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 <bluca@debian.org>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Tested-by: Petr Vorel <petr.vorel@gmail.com>
---
v2: added reviewed/tested-by tags, removed Cc

 testsuite/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testsuite/Makefile b/testsuite/Makefile
index 46b243b0..9b0f1c15 100644
--- a/testsuite/Makefile
+++ b/testsuite/Makefile
@@ -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
-- 
2.19.2

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

[Luca Boccassi]

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
v2: fixed copypasta in log message

 testsuite/tests/ip/route/add_default_route.t | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testsuite/tests/ip/route/add_default_route.t b/testsuite/tests/ip/route/add_default_route.t
index 569ba1f8..ded4edc3 100755
--- a/testsuite/tests/ip/route/add_default_route.t
+++ b/testsuite/tests/ip/route/add_default_route.t
@@ -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
-- 
2.19.2

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

[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 <bluca@debian.org>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
v2: applied suggestions from Petr to simplify the modules parsing/unloading

 testsuite/tests/ip/tunnel/add_tunnel.t | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/testsuite/tests/ip/tunnel/add_tunnel.t b/testsuite/tests/ip/tunnel/add_tunnel.t
index 3f5a9d3c..78c1e463 100755
--- a/testsuite/tests/ip/tunnel/add_tunnel.t
+++ b/testsuite/tests/ip/tunnel/add_tunnel.t
@@ -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
-- 
2.19.2

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

[Luca Boccassi]

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

On Sat, 2018-12-15 at 17:32 +0100, Petr Vorel wrote:
> Hi Luca,
> 
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  testsuite/tests/ip/route/add_default_route.t | 2 ++
> >  1 file changed, 2 insertions(+)
> > diff --git a/testsuite/tests/ip/route/add_default_route.t
> > b/testsuite/tests/ip/route/add_default_route.t
> > index 569ba1f8..c536e35f 100755
> > --- a/testsuite/tests/ip/route/add_default_route.t
> > +++ b/testsuite/tests/ip/route/add_default_route.t
> > @@ -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?

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

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Luca Boccassi]

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

On Sat, 2018-12-15 at 18:37 +0100, Petr Vorel wrote:
> Hi Luca,
> 
> > 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 <bluca@debian.org>
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> > ---
> >  testsuite/tests/ip/tunnel/add_tunnel.t | 24
> > ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > diff --git a/testsuite/tests/ip/tunnel/add_tunnel.t
> > b/testsuite/tests/ip/tunnel/add_tunnel.t
> > index 3f5a9d3c..76f8b011 100755
> > --- a/testsuite/tests/ip/tunnel/add_tunnel.t
> > +++ b/testsuite/tests/ip/tunnel/add_tunnel.t
> > @@ -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.

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.

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

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

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Petr Vorel]

Hi Luca,

> Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
> v2: fixed copypasta in log message


Kind regards,
Petr

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

[Petr Vorel]

Hi Luca,

Reviewed-by: Petr Vorel <pvorel@suse.cz>
LGTM, but I'd suggest 2 small changes (see bellow).

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

> +# 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).

> +# 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

Kind regards,
Petr

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

[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 <bluca@debian.org>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Tested-by: Petr Vorel <petr.vorel@gmail.com>
---
v2: added reviewed/tested-by tags, removed Cc

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b7488add..20c760e2 100644
--- a/Makefile
+++ b/Makefile
@@ -119,7 +119,7 @@ clobber:
 
 distclean: clobber
 
-check:
+check: all
 	cd testsuite && $(MAKE) && $(MAKE) alltests
 
 cscope:
-- 
2.19.2

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

[Luca Boccassi]

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 <bluca@debian.org>
Reviewed-by: Petr Vorel <petr.vorel@gmail.com>
Tested-by: Petr Vorel <petr.vorel@gmail.com>
---
v2: added reviewed/tested-by tags, removed Cc

 testsuite/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testsuite/Makefile b/testsuite/Makefile
index 46b243b0..9b0f1c15 100644
--- a/testsuite/Makefile
+++ b/testsuite/Makefile
@@ -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
-- 
2.19.2

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

[Luca Boccassi]

Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
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(+)

diff --git a/testsuite/tests/ip/route/add_default_route.t b/testsuite/tests/ip/route/add_default_route.t
index 569ba1f8..ded4edc3 100755
--- a/testsuite/tests/ip/route/add_default_route.t
+++ b/testsuite/tests/ip/route/add_default_route.t
@@ -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
-- 
2.19.2

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

[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 <bluca@debian.org>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
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(+)

diff --git a/testsuite/tests/ip/tunnel/add_tunnel.t b/testsuite/tests/ip/tunnel/add_tunnel.t
index 3f5a9d3c..65db431c 100755
--- a/testsuite/tests/ip/tunnel/add_tunnel.t
+++ b/testsuite/tests/ip/tunnel/add_tunnel.t
@@ -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
-- 
2.19.2

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

[Luca Boccassi]

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

On Sun, 2018-12-16 at 21:21 +0100, Petr Vorel wrote:
> Hi Luca,
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> LGTM, but I'd suggest 2 small changes (see bellow).
> 
> > +++ 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.
> 
> > +# 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.

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Petr Vorel]

Hi Luca,

> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>
> ---
> v2: applied suggestions from Petr to simplify the modules parsing/unloading
> v3: ditto, added reviewed-by tag


Kind regards,
Petr