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

[Petr Vorel <pvorel@suse.cz>]

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