Re: [PATCH net-next] selftests: Add debugging options to pmtu.sh

[Stefano Brivio <sbrivio@redhat.com>]

Hi David,

On Wed,  3 Apr 2019 18:18:24 -0700
David Ahern <dsahern@kernel.org> wrote:

> From: David Ahern <dsahern@gmail.com>
> 
> pmtu.sh script runs a number of tests and dumps a summary of pass/fail.
> If a test fails, it is near impossible to debug why. For example:
> 
>     TEST: ipv6: PMTU exceptions                       [FAIL]
> 
> There are a lot of commands run behind the scenes for this test. Which
> one is failing?
> 
> Add a VERBOSE option to show commands that are run and any output from
> those commands. Add a PAUSE_ON_FAIL option to halt the script if a test
> fails allowing users to poke around with the setup in the failed state.

Thanks for doing this, I have to admit I occasionally had to sprinkle
this script with sleep/read/exit in the past. A few comments:

> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  tools/testing/selftests/net/pmtu.sh | 215 +++++++++++++++++++++---------------
>  1 file changed, 127 insertions(+), 88 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
> index 912b2dc50be3..28e8c97b5c9e 100755
> --- a/tools/testing/selftests/net/pmtu.sh
> +++ b/tools/testing/selftests/net/pmtu.sh
> @@ -116,6 +116,9 @@
>  # Kselftest framework requirement - SKIP code is 4.
>  ksft_skip=4
>  
> +PAUSE_ON_FAIL=no
> +VERBOSE=0

For consistency, I'd also rename 'tracing' below to TRACING and assign
it here.

>  # Some systems don't have a ping6 binary anymore
>  which ping6 > /dev/null 2>&1 && ping6=$(which ping6) || ping6=$(which ping)
>  
> @@ -222,6 +225,26 @@ err_flush() {
>  	err_buf=
>  }
>  
> +run_cmd() {
> +	local cmd="$*"
> +	local out
> +	local stderr="2>/dev/null"

'local' is not POSIX, and I think it actually breaks (at least) on
ksh93 (maybe not a big deal, but I kept everything else POSIX, so I
wouldn't break it just for this).

Besides, for 'ping' commands, it's stdout that needs to be suppressed
(we can just suppress both stdout and stderr if not in verbose mode).

> +	if [ "$VERBOSE" = "1" ]; then
> +		printf "    COMMAND: $cmd\n"
> +		stderr=
> +	fi
> +
> +	out=$(eval $cmd $stderr)

I think this needs quoting. Is eval really needed, by the way?

> +	rc=$?
> +	if [ "$VERBOSE" = "1" -a -n "$out" ]; then
> +		echo "    $out"
> +		echo
> +	fi
> +
> +	return $rc
> +}
>
> [...]
>
> +while getopts :ptv o

Oh, and this is now POSIX and well defined, I didn't know. Thanks for
making this readable :)

> +do
> +	case $o in
> +	p) PAUSE_ON_FAIL=yes;;
> +	v) VERBOSE=1;;
> +	t) if which tcpdump > /dev/null 2>&1; then
> +		tracing=1
> +	   else
> +		echo "=== tcpdump not available, tracing disabled"
> +	   fi
> +	   ;;
> +	*) usage;;
> +	esac
> +done
> +shift $(($OPTIND-1))
> +
>  IFS="	
>  "
>  
> -tracing=0
>  for arg do
> -	if [ "${arg}" != "${arg#--*}" ]; then
> -		opt="${arg#--}"
> -		if [ "${opt}" = "trace" ]; then
> -			if which tcpdump > /dev/null 2>&1; then
> -				tracing=1
> -			else
> -				echo "=== tcpdump not available, tracing disabled"
> -			fi
> -		else
> -			usage
> -		fi
> -	else
> -		# Check first that all requested tests are available before
> -		# running any
> -		command -v > /dev/null "test_${arg}" || { echo "=== Test ${arg} not found"; usage; }
> -	fi
> +	# Check first that all requested tests are available before
> +	# running any

This fits on a single line now.

> +	command -v > /dev/null "test_${arg}" || { echo "=== Test ${arg} not found"; usage; }
>  done
>  
>  trap cleanup EXIT
> @@ -1124,6 +1153,11 @@ for t in ${tests}; do
>  
>  	(
>  		unset IFS
> +
> +		if [ "$VERBOSE" = "1" ]; then
> +			printf "\n##########################################################################\n\n"
> +		fi
> +
>  		eval test_${name}
>  		ret=$?
>  		cleanup
> @@ -1132,6 +1166,11 @@ for t in ${tests}; do
>  			printf "TEST: %-60s  [ OK ]\n" "${t}"
>  		elif [ $ret -eq 1 ]; then
>  			printf "TEST: %-60s  [FAIL]\n" "${t}"
> +			if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
> +				echo
> +				echo "Pausing. Hit enter to continue"
> +				read a
> +			fi
>  			err_flush
>  			exit 1
>  		elif [ $ret -eq 2 ]; then

-- 
Stefano