| Message ID | 20210209074518.849999-1-bjorn.topel@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf,v3] selftests/bpf: convert test_xdp_redirect.sh to bash | expand |
On Mon, Feb 8, 2021 at 11:45 PM Björn Töpel <bjorn.topel@gmail.com> wrote: > > From: Björn Töpel <bjorn.topel@intel.com> > > The test_xdp_redirect.sh script uses a bash feature, '&>'. On systems, > e.g. Debian, where '/bin/sh' is dash, this will not work as > expected. Use bash in the shebang to get the expected behavior. > > Further, using 'set -e' means that the error of a command cannot be > captured without the command being executed with '&&' or '||'. Let us > use '||' to capture the return value of a failed command. > > v3: Reintroduced /bin/bash, and kept 'set -e'. (Andrii) > v2: Kept /bin/sh and removed bashisms. (Randy) > > Acked-by: William Tu <u9012063@gmail.com> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > --- Not exactly what I had in mind (see below), but it's ok like this as well, so: Acked-by: Andrii Nakryiko <andrii@kernel.org> Does it have to go through the bpf tree or bpf-next is fine? And what about Fixes: tag? > tools/testing/selftests/bpf/test_xdp_redirect.sh | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh > index dd80f0c84afb..3f85a82f1c89 100755 > --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh > +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # Create 2 namespaces with two veth peers, and > # forward packets in-between using generic XDP > # > @@ -43,6 +43,8 @@ cleanup() > test_xdp_redirect() > { > local xdpmode=$1 > + local ret1=0 > + local ret2=0 > > setup > > @@ -57,10 +59,8 @@ test_xdp_redirect() > ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 &> /dev/null > ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 &> /dev/null > > - ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null > - local ret1=$? > - ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null > - local ret2=$? > + ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null || ret1=$? > + ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null || ret2=$? You didn't like if ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null && ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null; then echo "selftests: test_xdp_redirect $xdpmode [PASS]" ... ? > > if [ $ret1 -eq 0 -a $ret2 -eq 0 ]; then > echo "selftests: test_xdp_redirect $xdpmode [PASS]"; > > base-commit: 6183f4d3a0a2ad230511987c6c362ca43ec0055f > -- > 2.27.0 >
On 2021-02-10 21:13, Andrii Nakryiko wrote: > On Mon, Feb 8, 2021 at 11:45 PM Björn Töpel <bjorn.topel@gmail.com> wrote: >> >> From: Björn Töpel <bjorn.topel@intel.com> >> >> The test_xdp_redirect.sh script uses a bash feature, '&>'. On systems, >> e.g. Debian, where '/bin/sh' is dash, this will not work as >> expected. Use bash in the shebang to get the expected behavior. >> >> Further, using 'set -e' means that the error of a command cannot be >> captured without the command being executed with '&&' or '||'. Let us >> use '||' to capture the return value of a failed command. >> >> v3: Reintroduced /bin/bash, and kept 'set -e'. (Andrii) >> v2: Kept /bin/sh and removed bashisms. (Randy) >> >> Acked-by: William Tu <u9012063@gmail.com> >> Signed-off-by: Björn Töpel <bjorn.topel@intel.com> >> --- > > Not exactly what I had in mind (see below), but it's ok like this as well, so: > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Does it have to go through the bpf tree or bpf-next is fine? And what > about Fixes: tag? > > >> tools/testing/selftests/bpf/test_xdp_redirect.sh | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh >> index dd80f0c84afb..3f85a82f1c89 100755 >> --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh >> +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh >> @@ -1,4 +1,4 @@ >> -#!/bin/sh >> +#!/bin/bash >> # Create 2 namespaces with two veth peers, and >> # forward packets in-between using generic XDP >> # >> @@ -43,6 +43,8 @@ cleanup() >> test_xdp_redirect() >> { >> local xdpmode=$1 >> + local ret1=0 >> + local ret2=0 >> >> setup >> >> @@ -57,10 +59,8 @@ test_xdp_redirect() >> ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 &> /dev/null >> ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 &> /dev/null >> >> - ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null >> - local ret1=$? >> - ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null >> - local ret2=$? >> + ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null || ret1=$? >> + ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null || ret2=$? > > You didn't like > > if ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null && > ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null; then > echo "selftests: test_xdp_redirect $xdpmode [PASS]" > ... > > ? Ah, yeah that's better. I'll spin a v4 with this and proper fixes-tag. Thanks, Andrii! Björn
diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh index dd80f0c84afb..3f85a82f1c89 100755 --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # Create 2 namespaces with two veth peers, and # forward packets in-between using generic XDP # @@ -43,6 +43,8 @@ cleanup() test_xdp_redirect() { local xdpmode=$1 + local ret1=0 + local ret2=0 setup @@ -57,10 +59,8 @@ test_xdp_redirect() ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 &> /dev/null ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 &> /dev/null - ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null - local ret1=$? - ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null - local ret2=$? + ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null || ret1=$? + ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null || ret2=$? if [ $ret1 -eq 0 -a $ret2 -eq 0 ]; then echo "selftests: test_xdp_redirect $xdpmode [PASS]";