diff mbox series

[v3,3/3] selftests: net: Fix udpgro_frglist.sh shellcheck warnings and errors

Message ID 20230127140944.265135-3-andrei.gherzan@canonical.com (mailing list archive)
State New
Headers show
Series [v3,1/3] selftests: net: .gitignore the scratch directory of bpf | expand

Commit Message

Andrei Gherzan Jan. 27, 2023, 2:09 p.m. UTC
This change fixes the following shellcheck warnings and errors:

* SC2155 (warning): Declare and assign separately to avoid masking return
  values.
* SC2124 (warning): Assigning an array to a string! Assign as array, or use
  instead of @ to concatenate.
* SC2034 (warning): ipv4_args appears unused. Verify use (or export if used
  externally).
* SC2242 (error): Can only exit with status 0-255. Other data should be
  written to stdout/stderr.
* SC2068 (error): Double quote array expansions to avoid re-splitting
  elements.

Fixes: edae34a3ed92 ("selftests net: add UDP GRO fraglist + bpf self-tests")
Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
---
 tools/testing/selftests/net/udpgro_frglist.sh | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Maciej ┼╗enczykowski Jan. 27, 2023, 2:32 p.m. UTC | #1
On Fri, Jan 27, 2023 at 6:09 AM Andrei Gherzan
<andrei.gherzan@canonical.com> wrote:
>
> This change fixes the following shellcheck warnings and errors:
>
> * SC2155 (warning): Declare and assign separately to avoid masking return
>   values.
> * SC2124 (warning): Assigning an array to a string! Assign as array, or use
>   instead of @ to concatenate.
> * SC2034 (warning): ipv4_args appears unused. Verify use (or export if used
>   externally).
> * SC2242 (error): Can only exit with status 0-255. Other data should be
>   written to stdout/stderr.
> * SC2068 (error): Double quote array expansions to avoid re-splitting
>   elements.
>
> Fixes: edae34a3ed92 ("selftests net: add UDP GRO fraglist + bpf self-tests")
> Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
> ---
>  tools/testing/selftests/net/udpgro_frglist.sh | 20 +++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/net/udpgro_frglist.sh b/tools/testing/selftests/net/udpgro_frglist.sh
> index e1ca49de2491..97bf20e9afd8 100755
> --- a/tools/testing/selftests/net/udpgro_frglist.sh
> +++ b/tools/testing/selftests/net/udpgro_frglist.sh
> @@ -3,7 +3,8 @@
>  #
>  # Run a series of udpgro benchmarks
>
> -readonly PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
> +PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
> +readonly PEER_NS
>
>  BPF_FILE="../bpf/xdp_dummy.bpf.o"
>  BPF_NAT6TO4_FILE="nat6to4.o"
> @@ -19,7 +20,7 @@ trap cleanup EXIT
>
>  run_one() {
>         # use 'rx' as separator between sender args and receiver args
> -       local -r all="$@"
> +       local -r all="$*"

this should technically use arrays, something like

local -a -r args=("$@")

but perhaps just get rid of args and just use "$@" directly below

>         local -r tx_args=${all%rx*}
>         local rx_args=${all#*rx}
>
> @@ -56,13 +57,13 @@ run_one() {
>  }
>
>  run_in_netns() {
> -       local -r args=$@
> +       local -r args="$*"
>    echo ${args}
>         ./in_netns.sh $0 __subprocess ${args}

ie. here could just use "$@" directly twice instead of defining args.
$0 should be doublequoted - though I guess it'll never be empty, and
is unlikely to include spaces.
>  }
>
>  run_udp() {
> -       local -r args=$@
> +       local -r args="$*"
>
>         echo "udp gso - over veth touching data"
>         run_in_netns ${args} -u -S 0 rx -4 -v
> @@ -72,7 +73,7 @@ run_udp() {
>  }
>
>  run_tcp() {
> -       local -r args=$@
> +       local -r args="$*"
>
>         echo "tcp - over veth touching data"
>         run_in_netns ${args} -t rx -4 -t
> @@ -80,7 +81,6 @@ run_tcp() {
>
>  run_all() {
>         local -r core_args="-l 4"

is this still useful? embed directly in ipv6_args

> -       local -r ipv4_args="${core_args} -4  -D 192.168.1.1"

perhaps this should stay as a comment??

>         local -r ipv6_args="${core_args} -6  -D 2001:db8::1"
>
>         echo "ipv6"
> @@ -90,19 +90,19 @@ run_all() {
>
>  if [ ! -f ${BPF_FILE} ]; then

double quote
"${BPF_FILE}"
in case space in file name

>         echo "Missing ${BPF_FILE}. Build bpf selftest first"
> -       exit -1
> +       exit 1
>  fi
>
>  if [ ! -f "$BPF_NAT6TO4_FILE" ]; then

there seems to be inconsistency around [ vs [[, use [[ if relying on bash anyway

>         echo "Missing nat6to4 helper. Build bpf nat6to4.o selftest first"
> -       exit -1
> +       exit 1
>  fi
>
>  if [[ $# -eq 0 ]]; then
>         run_all
>  elif [[ $1 == "__subprocess" ]]; then

while this does indeed work, imho $1 should be "$1" to be less confusing

>         shift
> -       run_one $@
> +       run_one "$@"
>  else
> -       run_in_netns $@
> +       run_in_netns "$@"
>  fi
> --
> 2.34.1
Andrei Gherzan Jan. 27, 2023, 6:43 p.m. UTC | #2
On 23/01/27 06:32AM, Maciej ┼╗enczykowski wrote:
> On Fri, Jan 27, 2023 at 6:09 AM Andrei Gherzan
> <andrei.gherzan@canonical.com> wrote:
> >
> > This change fixes the following shellcheck warnings and errors:
> >
> > * SC2155 (warning): Declare and assign separately to avoid masking return
> >   values.
> > * SC2124 (warning): Assigning an array to a string! Assign as array, or use
> >   instead of @ to concatenate.
> > * SC2034 (warning): ipv4_args appears unused. Verify use (or export if used
> >   externally).
> > * SC2242 (error): Can only exit with status 0-255. Other data should be
> >   written to stdout/stderr.
> > * SC2068 (error): Double quote array expansions to avoid re-splitting
> >   elements.
> >
> > Fixes: edae34a3ed92 ("selftests net: add UDP GRO fraglist + bpf self-tests")
> > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
> > ---
> >  tools/testing/selftests/net/udpgro_frglist.sh | 20 +++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/udpgro_frglist.sh b/tools/testing/selftests/net/udpgro_frglist.sh
> > index e1ca49de2491..97bf20e9afd8 100755
> > --- a/tools/testing/selftests/net/udpgro_frglist.sh
> > +++ b/tools/testing/selftests/net/udpgro_frglist.sh
> > @@ -3,7 +3,8 @@
> >  #
> >  # Run a series of udpgro benchmarks
> >
> > -readonly PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
> > +PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
> > +readonly PEER_NS
> >
> >  BPF_FILE="../bpf/xdp_dummy.bpf.o"
> >  BPF_NAT6TO4_FILE="nat6to4.o"
> > @@ -19,7 +20,7 @@ trap cleanup EXIT
> >
> >  run_one() {
> >         # use 'rx' as separator between sender args and receiver args
> > -       local -r all="$@"
> > +       local -r all="$*"
> 
> this should technically use arrays, something like
> 
> local -a -r args=("$@")
> 
> but perhaps just get rid of args and just use "$@" directly below

Using it directly would not work with the substitutions later because it
would try to apply it to each positional parameter in turn. Same when
using an intermediate array.

> 
> >         local -r tx_args=${all%rx*}
> >         local rx_args=${all#*rx}
> >
> > @@ -56,13 +57,13 @@ run_one() {
> >  }
> >
> >  run_in_netns() {
> > -       local -r args=$@
> > +       local -r args="$*"
> >    echo ${args}
> >         ./in_netns.sh $0 __subprocess ${args}
> 
> ie. here could just use "$@" directly twice instead of defining args.
> $0 should be doublequoted - though I guess it'll never be empty, and
> is unlikely to include spaces.

That sounds fine to me. I'll include it in v4.

> >  }
> >
> >  run_udp() {
> > -       local -r args=$@
> > +       local -r args="$*"
> >
> >         echo "udp gso - over veth touching data"
> >         run_in_netns ${args} -u -S 0 rx -4 -v
> > @@ -72,7 +73,7 @@ run_udp() {
> >  }
> >
> >  run_tcp() {
> > -       local -r args=$@
> > +       local -r args="$*"
> >
> >         echo "tcp - over veth touching data"
> >         run_in_netns ${args} -t rx -4 -t
> > @@ -80,7 +81,6 @@ run_tcp() {
> >
> >  run_all() {
> >         local -r core_args="-l 4"
> 
> is this still useful? embed directly in ipv6_args
> 
> > -       local -r ipv4_args="${core_args} -4  -D 192.168.1.1"
> 
> perhaps this should stay as a comment??

Well the way I see it is one or the other. We either embed and drop
ipv4_args or keep ipv4_args as a comment but also core_args. I'll do the
former in v4 if no other objections.

> 
> >         local -r ipv6_args="${core_args} -6  -D 2001:db8::1"
> >
> >         echo "ipv6"
> > @@ -90,19 +90,19 @@ run_all() {
> >
> >  if [ ! -f ${BPF_FILE} ]; then
> 
> double quote
> "${BPF_FILE}"
> in case space in file name

This change only targets warning/error issues. There are way more "info"
ones, but I didn't want to splash a big patch for all those.

> 
> >         echo "Missing ${BPF_FILE}. Build bpf selftest first"
> > -       exit -1
> > +       exit 1
> >  fi
> >
> >  if [ ! -f "$BPF_NAT6TO4_FILE" ]; then
> 
> there seems to be inconsistency around [ vs [[, use [[ if relying on bash anyway



> 
> >         echo "Missing nat6to4 helper. Build bpf nat6to4.o selftest first"
> > -       exit -1
> > +       exit 1
> >  fi
> >
> >  if [[ $# -eq 0 ]]; then
> >         run_all
> >  elif [[ $1 == "__subprocess" ]]; then
> 
> while this does indeed work, imho $1 should be "$1" to be less confusing

Agreed and again, there are a good set of other places where this is
needed. Should I just address them all in an extra patch? Again, this
one only scoped warnings/errors to avoid impact.

Thanks for the review.
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/udpgro_frglist.sh b/tools/testing/selftests/net/udpgro_frglist.sh
index e1ca49de2491..97bf20e9afd8 100755
--- a/tools/testing/selftests/net/udpgro_frglist.sh
+++ b/tools/testing/selftests/net/udpgro_frglist.sh
@@ -3,7 +3,8 @@ 
 #
 # Run a series of udpgro benchmarks
 
-readonly PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
+PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
+readonly PEER_NS
 
 BPF_FILE="../bpf/xdp_dummy.bpf.o"
 BPF_NAT6TO4_FILE="nat6to4.o"
@@ -19,7 +20,7 @@  trap cleanup EXIT
 
 run_one() {
 	# use 'rx' as separator between sender args and receiver args
-	local -r all="$@"
+	local -r all="$*"
 	local -r tx_args=${all%rx*}
 	local rx_args=${all#*rx}
 
@@ -56,13 +57,13 @@  run_one() {
 }
 
 run_in_netns() {
-	local -r args=$@
+	local -r args="$*"
   echo ${args}
 	./in_netns.sh $0 __subprocess ${args}
 }
 
 run_udp() {
-	local -r args=$@
+	local -r args="$*"
 
 	echo "udp gso - over veth touching data"
 	run_in_netns ${args} -u -S 0 rx -4 -v
@@ -72,7 +73,7 @@  run_udp() {
 }
 
 run_tcp() {
-	local -r args=$@
+	local -r args="$*"
 
 	echo "tcp - over veth touching data"
 	run_in_netns ${args} -t rx -4 -t
@@ -80,7 +81,6 @@  run_tcp() {
 
 run_all() {
 	local -r core_args="-l 4"
-	local -r ipv4_args="${core_args} -4  -D 192.168.1.1"
 	local -r ipv6_args="${core_args} -6  -D 2001:db8::1"
 
 	echo "ipv6"
@@ -90,19 +90,19 @@  run_all() {
 
 if [ ! -f ${BPF_FILE} ]; then
 	echo "Missing ${BPF_FILE}. Build bpf selftest first"
-	exit -1
+	exit 1
 fi
 
 if [ ! -f "$BPF_NAT6TO4_FILE" ]; then
 	echo "Missing nat6to4 helper. Build bpf nat6to4.o selftest first"
-	exit -1
+	exit 1
 fi
 
 if [[ $# -eq 0 ]]; then
 	run_all
 elif [[ $1 == "__subprocess" ]]; then
 	shift
-	run_one $@
+	run_one "$@"
 else
-	run_in_netns $@
+	run_in_netns "$@"
 fi