| Message ID | 20181215153320.14113-1-bluca@debian.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2,1/4] Makefile: have check target depend on all | expand |
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
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.
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
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(+)