| Message ID | 20210206092654.155239-1-bjorn.topel@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v2] selftests/bpf: remove bash feature in test_xdp_redirect.sh | expand |
On 2/6/21 1:26 AM, Björn Töpel wrote: > From: Björn Töpel <bjorn.topel@intel.com> > > The test_xdp_redirect.sh script uses a bash redirect feature, > '&>/dev/null'. Use '>/dev/null 2>&1' instead. > > Also remove the 'set -e' since the script actually relies on that the > return value can be used to determine pass/fail of the test. > > Acked-by: William Tu <u9012063@gmail.com> > Fixes: 996139e801fd ("selftests: bpf: add a test for XDP redirect") > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> Acked-by: Randy Dunlap <rdunlap@infradead.org> Thanks. > --- > William, I kept your Acked-by. > > v2: Kept /bin/sh and removed bashisms. (Randy) > --- > tools/testing/selftests/bpf/test_xdp_redirect.sh | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh > index dd80f0c84afb..4d4887da175c 100755 > --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh > +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh > @@ -46,20 +46,20 @@ test_xdp_redirect() > > setup > > - ip link set dev veth1 $xdpmode off &> /dev/null > + ip link set dev veth1 $xdpmode off >/dev/null 2>&1 > if [ $? -ne 0 ];then > echo "selftests: test_xdp_redirect $xdpmode [SKIP]" > return 0 > fi > > - ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null > - ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null > - 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 -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy >/dev/null 2>&1 > + ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy >/dev/null 2>&1 > + ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 >/dev/null 2>&1 > + ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 >/dev/null 2>&1 > > - ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null > + ip netns exec ns1 ping -c 1 10.1.1.22 >/dev/null 2>&1 > local ret1=$? > - ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null > + ip netns exec ns2 ping -c 1 10.1.1.11 >/dev/null 2>&1 > local ret2=$? > > if [ $ret1 -eq 0 -a $ret2 -eq 0 ]; then > @@ -72,7 +72,6 @@ test_xdp_redirect() > cleanup > } > > -set -e > trap cleanup 2 3 6 9 > > test_xdp_redirect xdpgeneric > > base-commit: 6183f4d3a0a2ad230511987c6c362ca43ec0055f >
On Sat, Feb 6, 2021 at 1:29 AM 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 redirect feature, > '&>/dev/null'. Use '>/dev/null 2>&1' instead. We have plenty of explicit bash uses in selftest scripts, I'm not sure it's a good idea to make scripts more verbose. > > Also remove the 'set -e' since the script actually relies on that the > return value can be used to determine pass/fail of the test. This sounds like a dubious decision. The script checks return results only of last two commands, for which it's better to write and if [<first command>] && [<second command>] check and leave set -e intact. > > Acked-by: William Tu <u9012063@gmail.com> > Fixes: 996139e801fd ("selftests: bpf: add a test for XDP redirect") > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > --- > William, I kept your Acked-by. > > v2: Kept /bin/sh and removed bashisms. (Randy) > --- > tools/testing/selftests/bpf/test_xdp_redirect.sh | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh > index dd80f0c84afb..4d4887da175c 100755 > --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh > +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh > @@ -46,20 +46,20 @@ test_xdp_redirect() > > setup > > - ip link set dev veth1 $xdpmode off &> /dev/null > + ip link set dev veth1 $xdpmode off >/dev/null 2>&1 > if [ $? -ne 0 ];then > echo "selftests: test_xdp_redirect $xdpmode [SKIP]" > return 0 > fi > > - ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null > - ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null > - 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 -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy >/dev/null 2>&1 > + ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy >/dev/null 2>&1 > + ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 >/dev/null 2>&1 > + ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 >/dev/null 2>&1 > > - ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null > + ip netns exec ns1 ping -c 1 10.1.1.22 >/dev/null 2>&1 > local ret1=$? > - ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null > + ip netns exec ns2 ping -c 1 10.1.1.11 >/dev/null 2>&1 > local ret2=$? > > if [ $ret1 -eq 0 -a $ret2 -eq 0 ]; then > @@ -72,7 +72,6 @@ test_xdp_redirect() > cleanup > } > > -set -e > trap cleanup 2 3 6 9 > > test_xdp_redirect xdpgeneric > > base-commit: 6183f4d3a0a2ad230511987c6c362ca43ec0055f > -- > 2.27.0 >
On 2021-02-09 06:52, Andrii Nakryiko wrote: > On Sat, Feb 6, 2021 at 1:29 AM 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 redirect feature, >> '&>/dev/null'. Use '>/dev/null 2>&1' instead. > > We have plenty of explicit bash uses in selftest scripts, I'm not sure > it's a good idea to make scripts more verbose. > $ cd tools/testing/selftests $ git grep '\#!/bin/bash'|wc -l 282 $ git grep '\#!/bin/sh'|wc -l 164 Andrii/Randy, I'm fine with whatever. I just want to be able to run the test on Debian-derived systems. ;-) >> >> Also remove the 'set -e' since the script actually relies on that the >> return value can be used to determine pass/fail of the test. > > This sounds like a dubious decision. The script checks return results > only of last two commands, for which it's better to write and if > [<first command>] && [<second command>] check and leave set -e intact. > Ok! Please decide on the shell flavor, and I'll respin a v3. Björn
On 2/8/21 10:41 PM, Björn Töpel wrote: > On 2021-02-09 06:52, Andrii Nakryiko wrote: >> On Sat, Feb 6, 2021 at 1:29 AM 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 redirect feature, >>> '&>/dev/null'. Use '>/dev/null 2>&1' instead. >> >> We have plenty of explicit bash uses in selftest scripts, I'm not sure >> it's a good idea to make scripts more verbose. >> > > $ cd tools/testing/selftests > $ git grep '\#!/bin/bash'|wc -l > 282 > $ git grep '\#!/bin/sh'|wc -l > 164 > > Andrii/Randy, I'm fine with whatever. I just want to be able to run the > test on Debian-derived systems. ;-) > > >>> >>> Also remove the 'set -e' since the script actually relies on that the >>> return value can be used to determine pass/fail of the test. >> >> This sounds like a dubious decision. The script checks return results >> only of last two commands, for which it's better to write and if >> [<first command>] && [<second command>] check and leave set -e intact. >> > > Ok! > > Please decide on the shell flavor, and I'll respin a v3. In general shell scripts in the kernel try not to use bash (we have taken several patches to convert from /bin/bash to /bin/sh scripts). OTOH, perf and bpf seem to be large exceptions to this trend, so it is apparently OK to use bash. :) Sorry to sidetrack you.
diff --git a/tools/testing/selftests/bpf/test_xdp_redirect.sh b/tools/testing/selftests/bpf/test_xdp_redirect.sh index dd80f0c84afb..4d4887da175c 100755 --- a/tools/testing/selftests/bpf/test_xdp_redirect.sh +++ b/tools/testing/selftests/bpf/test_xdp_redirect.sh @@ -46,20 +46,20 @@ test_xdp_redirect() setup - ip link set dev veth1 $xdpmode off &> /dev/null + ip link set dev veth1 $xdpmode off >/dev/null 2>&1 if [ $? -ne 0 ];then echo "selftests: test_xdp_redirect $xdpmode [SKIP]" return 0 fi - ip -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null - ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy &> /dev/null - 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 -n ns1 link set veth11 $xdpmode obj xdp_dummy.o sec xdp_dummy >/dev/null 2>&1 + ip -n ns2 link set veth22 $xdpmode obj xdp_dummy.o sec xdp_dummy >/dev/null 2>&1 + ip link set dev veth1 $xdpmode obj test_xdp_redirect.o sec redirect_to_222 >/dev/null 2>&1 + ip link set dev veth2 $xdpmode obj test_xdp_redirect.o sec redirect_to_111 >/dev/null 2>&1 - ip netns exec ns1 ping -c 1 10.1.1.22 &> /dev/null + ip netns exec ns1 ping -c 1 10.1.1.22 >/dev/null 2>&1 local ret1=$? - ip netns exec ns2 ping -c 1 10.1.1.11 &> /dev/null + ip netns exec ns2 ping -c 1 10.1.1.11 >/dev/null 2>&1 local ret2=$? if [ $ret1 -eq 0 -a $ret2 -eq 0 ]; then @@ -72,7 +72,6 @@ test_xdp_redirect() cleanup } -set -e trap cleanup 2 3 6 9 test_xdp_redirect xdpgeneric