MIME-Version: 1.0
References: <20230127140944.265135-1-andrei.gherzan@canonical.com> <20230127140944.265135-3-andrei.gherzan@canonical.com>
In-Reply-To: <20230127140944.265135-3-andrei.gherzan@canonical.com>
From:   =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.com>
Date:   Fri, 27 Jan 2023 06:32:44 -0800
Message-ID: <CANP3RGchqLRLRAxgWU69DzWfa9R2d0AhgeBdpJhmaE+c-Sszjw@mail.gmail.com>
Subject: Re: [PATCH v3 3/3] selftests: net: Fix udpgro_frglist.sh shellcheck
 warnings and errors
To:     Andrei Gherzan <andrei.gherzan@canonical.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>,
        Lina Wang <lina.wang@mediatek.com>, netdev@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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