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

[Allison Henderson <allison.henderson@oracle.com>]

On Fri, 2024-06-07 at 21:24 +0100, Simon Horman wrote:
+cc Chuck Lever, Peter Oberparleiter, Vegard Nossum, rds-devel
> 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://urldefense.com/v3/__https://www.shellcheck.net/wiki/SC1128__;!!ACWV5N9M2RV99hQ!OjLySFP6wNEm6wWK90hVJHa9RXAP8310v8V0uKLQ9KvODoyD8hUHX0CRLZpKBl1jQGwfR-vYcBI5w8wRwAI$
>  
> 

Ah, alrighty I will go through and fix all the shell check nits

> > +
> > +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://urldefense.com/v3/__https://www.shellcheck.net/wiki/SC2046__;!!ACWV5N9M2RV99hQ!OjLySFP6wNEm6wWK90hVJHa9RXAP8310v8V0uKLQ9KvODoyD8hUHX0CRLZpKBl1jQGwfR-vYcBI5005wM3k$
>  
> 
> Also, $LOG_DIR is quoted here, but not elsewhere, which seems
> inconsistent.

Noted, yes it should probably be quoted
> 
> > +
> > +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://urldefense.com/v3/__https://www.shellcheck.net/wiki/SC2035__;!!ACWV5N9M2RV99hQ!OjLySFP6wNEm6wWK90hVJHa9RXAP8310v8V0uKLQ9KvODoyD8hUHX0CRLZpKBl1jQGwfR-vYcBI5Es_quwA$
>  
> 
> Although I guess in practice there are no filenames with dashes in
> that directory.
Not at the moment, but its always possible that such a file could be
introduced later.  Will fix :-)

> 
> > +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?
Yes, I think so.  Even if it's not an issue now, it would be nice to
not deal will testcase breakage if file name were to change in the
future.  Will fix this too.

Thanks for the review! :-)
Allison
> 
> > +done)
> > +
> > +dmesg > $LOG_DIR/dmesg.out
> > +
> > +/usr/sbin/poweroff --no-wtmp --force
> 
> ...