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

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