From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Boccassi Subject: Re: [PATCH iproute2 4/4] testsuite: remove gre kmods if the test loads them Date: Sun, 16 Dec 2018 13:52:29 +0000 Message-ID: <1544968349.4766.4.camel@debian.org> References: <20181215153051.13166-1-bluca@debian.org> <20181215153320.14113-1-bluca@debian.org> <20181215173707.GB29950@x230> Mime-Version: 1.0 Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-Zh1vYDpLnnpzGcNovK/3" Cc: netdev@vger.kernel.org, stephen@networkplumber.org, Petr Vorel To: Petr Vorel Return-path: Received: from mail-wm1-f66.google.com ([209.85.128.66]:40933 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730382AbeLPNwd (ORCPT ); Sun, 16 Dec 2018 08:52:33 -0500 Received: by mail-wm1-f66.google.com with SMTP id q26so9986881wmf.5 for ; Sun, 16 Dec 2018 05:52:32 -0800 (PST) In-Reply-To: <20181215173707.GB29950@x230> Sender: netdev-owner@vger.kernel.org List-ID: --=-Zh1vYDpLnnpzGcNovK/3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, 2018-12-15 at 18:37 +0100, Petr Vorel wrote: > Hi Luca, >=20 > > The tunnel test leaves behind link devices created by the GRE > > kernel > > modules: > > $ ip -br link > > ... > > gre0@NONE=C2=A0=C2=A0=C2=A0=C2=A0DOWN 0.0.0.0 > > gretap0@NONE DOWN 00:00:00:00:00:00 > > erspan0@NONE DOWN 00:00:00:00:00:00 > > ip6tnl0@NONE DOWN :: > > ip6gre0@NONE DOWN 00:00:00:00: > > $ lsmod | grep gre > > ip6_gre=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A040960=C2=A0=C2=A00 > > ip6_tunnel=C2=A0=C2=A0=C2=A040960=C2=A0=C2=A01 ip6_gre > > ip_gre=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A032768=C2=A0=C2=A00 > > ip_tunnel=C2=A0=C2=A0=C2=A0=C2=A024576=C2=A0=C2=A01 ip_gre > > gre=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A016384=C2= =A0=C2=A02 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 >=20 > Reviewed-by: Petr Vorel >=20 > > --- > > =C2=A0testsuite/tests/ip/tunnel/add_tunnel.t | 24 > > ++++++++++++++++++++++++ > > =C2=A01 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 @@ > > =C2=A0TUNNEL_NAME=3D"tunnel_test_ip" > > +# note that checkbashism reports command -v, but dash supports it > > and it's posix compliant >=20 > 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 > > +=C2=A0=C2=A0=C2=A0=C2=A0KMODS=3D"ip6_gre ip6_tunnel ip_gre ip_tunnel g= re" > > +=C2=A0=C2=A0=C2=A0=C2=A0COUNT_KMODS_BEFORE=3D$(lsmod | grep -c -e "^ip= 6_gre" -e > > "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre") > > +else > > +=C2=A0=C2=A0=C2=A0=C2=A0KMODS=3D"" > > +fi >=20 > ... > > +if [ -n "$KMODS" ] > > +then > > +=C2=A0=C2=A0=C2=A0=C2=A0# unload kernel modules to remove dummy interf= aces only if > > they were not in use beforehand > > +=C2=A0=C2=A0=C2=A0=C2=A0COUNT_KMODS_AFTER=3D$(lsmod | grep -c -e "^ip6= _gre" -e > > "^ip6_tunnel" -e "^ip_gre" -e "^ip_tunnel" -e "^gre") > > +=C2=A0=C2=A0=C2=A0=C2=A0if [ "$COUNT_KMODS_BEFORE" -eq 0 ] && [ "$COUN= T_KMODS_AFTER" > > -gt 0 ] > > +=C2=A0=C2=A0=C2=A0=C2=A0then > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for mod in $KMODS > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0sudo rmmod "$mod" > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0done > > +=C2=A0=C2=A0=C2=A0=C2=A0else > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ts_log "[gre kernel mo= dule was loaded before test, not > > removing]" > > +=C2=A0=C2=A0=C2=A0=C2=A0fi > > +fi >=20 > You repeating yourself with module names. > How about something like this: >=20 > KMODS=3D"ip6_gre ip6_tunnel ip_gre ip_tunnel gre" >=20 > # unload kernel modules to remove dummy interfaces only if they were > not in use beforehand > KMODS_REMOVE=3D > 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=3D"$KMODS_REMOVE > $i"; > done > fi >=20 > ... >=20 > for mod in $KMODS_REMOVE; do > sudo rmmod "$mod" > done >=20 >=20 > I.e. keeping modules to remove (you can also loaded modules, if you > want to warn > about it, but I'd ignore it). >=20 > 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. --=20 Kind regards, Luca Boccassi --=-Zh1vYDpLnnpzGcNovK/3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEE6g0RLAGYhL9yp9G8SylmgFB4UWIFAlwWWJ0ACgkQSylmgFB4 UWIBwAf/YVeYfBViAevNJ7gWwPZ1b21119DYpfMC8KRIjK2wsOVxBlxkiJMJzLq1 h2NSble1eRqVqF64UluEFLzV0NPjn5/jTqXl0rdhF3lj4199HlhytxhK7ZFZJwKD SaCIM7WHBAyC8hxXV0WwnKY/YJm5UZSirPwi8SmaXDMvwhxWXjnTzN9y08GyGuJJ H3Q2EGG//pxQUY+g+96zOpaIZk7r7R/tZ8FHNl5gBuWjBvCC/NhBRWJ+MXbmDkYs BpyIeowdlnxmzYAopqKRKDTOCIPcZ+b8H1/T+KdgmUA8Im4m4Hek7C7+j6spf1h3 86eq9ruufA5W9ZC4FbU270SaVWFNbQ== =7Jrn -----END PGP SIGNATURE----- --=-Zh1vYDpLnnpzGcNovK/3--