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

[Luca Boccassi <bluca@debian.org>]

[-- 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 --]