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 <bl...@debian.org> > > Reviewed-by: Petr Vorel <pvo...@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
signature.asc
Description: This is a digitally signed message part
