Re: [RFC net-next 3/3] selftests: rds: add testing infrastructure

[Simon Horman <horms@kernel.org>]

On Thu, Jun 06, 2024 at 05:36:31PM -0700, allison.henderson@oracle.com wrote:
> From: Vegard Nossum <vegard.nossum@oracle.com>
> 
> This adds some basic self-testing infrastructure for RDS.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Hi Allison,

Some minor nits from my side.

> diff --git a/tools/testing/selftests/net/rds/config.sh b/tools/testing/selftests/net/rds/config.sh
> new file mode 100755
> index 000000000000..c2c36756ba1f
> --- /dev/null
> +++ b/tools/testing/selftests/net/rds/config.sh
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#! /bin/bash

#! line needs to be the first line for it to take effect

Flagged by shellcheck.

https://www.shellcheck.net/wiki/SC1128

> +
> +set -e
> +set -u
> +set -x
> +
> +unset KBUILD_OUTPUT
> +
> +# start with a default config
> +make defconfig
> +
> +# no modules
> +scripts/config --disable CONFIG_MODULES
> +
> +# enable RDS
> +scripts/config --enable CONFIG_RDS
> +scripts/config --enable CONFIG_RDS_TCP
> +
> +# instrument RDS and only RDS
> +scripts/config --enable CONFIG_GCOV_KERNEL
> +scripts/config --disable GCOV_PROFILE_ALL
> +scripts/config --enable GCOV_PROFILE_RDS
> +
> +# need network namespaces to run tests with veth network interfaces
> +scripts/config --enable CONFIG_NET_NS
> +scripts/config --enable CONFIG_VETH
> +
> +# simulate packet loss
> +scripts/config --enable CONFIG_NET_SCH_NETEM
> +
> +# generate real .config without asking any questions
> +make olddefconfig
> diff --git a/tools/testing/selftests/net/rds/init.sh b/tools/testing/selftests/net/rds/init.sh
> new file mode 100755
> index 000000000000..a29e3de81ed5
> --- /dev/null
> +++ b/tools/testing/selftests/net/rds/init.sh
> @@ -0,0 +1,49 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +set -u
> +
> +LOG_DIR=/tmp
> +PY_CMD="/usr/bin/python3"
> +while getopts "d:p:" opt; do
> +  case ${opt} in
> +    d)
> +      LOG_DIR=${OPTARG}
> +      ;;
> +    p)
> +      PY_CMD=${OPTARG}
> +      ;;
> +    :)
> +      echo "USAGE: init.sh [-d logdir] [-p python_cmd]"
> +      exit 1
> +      ;;
> +    ?)
> +      echo "Invalid option: -${OPTARG}."
> +      exit 1
> +      ;;
> +  esac
> +done
> +
> +LOG_FILE=$LOG_DIR/rds-strace.txt
> +
> +mount -t proc none /proc
> +mount -t sysfs none /sys
> +mount -t tmpfs none /var/run
> +mount -t debugfs none /sys/kernel/debug
> +
> +echo running RDS tests...
> +echo Traces will be logged to $LOG_FILE
> +rm -f $LOG_FILE
> +strace -T -tt -o "$LOG_FILE" $PY_CMD $(dirname "$0")/test.py -d "$LOG_DIR" || true

Perhaps it can't occur, but I don't think this will behave as
expected if the out put of dirname includes spaces.

Flagged by shellecheck.
https://www.shellcheck.net/wiki/SC2046

Also, $LOG_DIR is quoted here, but not elsewhere, which seems inconsistent.

> +
> +echo saving coverage data...
> +(set +x; cd /sys/kernel/debug/gcov; find * -name '*.gcda' | \

shellcheck warns that:

SC2035 (info): Use ./*glob* or -- *glob* so names with dashes won't become options. 

https://www.shellcheck.net/wiki/SC2035

Although I guess in practice there are no filenames with dashes in
that directory.

> +while read f
> +do
> +	cat < /sys/kernel/debug/gcov/$f > /$f

Again, I guess it doesn't occur in practice.
But should $f be quoted in case it includes whitespace?

> +done)
> +
> +dmesg > $LOG_DIR/dmesg.out
> +
> +/usr/sbin/poweroff --no-wtmp --force

...