Re: [PATCH net-next v4] selftests: net: move log_test to lib file and remove duplicate code

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Loop-variable corruption in fcnal-test.sh via bash dynamic
  scoping: the new shared helper `log_test_expected()` in…

--

selftests: net: move log_test to lib file and remove duplicate code

This adds a shared log_test_expected() helper to
tools/testing/selftests/net/lib.sh and replaces the near-identical
per-script log_test() implementations in 31 net selftests with a call
into it.  The new helper also gains PAUSE_ON_FAIL and PAUSE handling for
scripts that previously had none.

Link: https://lore.kernel.org/r/20260915-self_log_test-v3-1-a17728176334@kylinos.cn
Link: https://lore.kernel.org/r/20260831-self_log_test-v2-1-eda2e8490cf9@kylinos.cn
Link: https://lore.kernel.org/r/20260813-self_log_test-v1-1-f88b1107842e@kylinos.cn

> diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
> index 890c3f8e51bb8..a50609535fedc 100755
> --- a/tools/testing/selftests/net/fcnal-test.sh
> +++ b/tools/testing/selftests/net/fcnal-test.sh
> @@ -97,34 +97,7 @@ fi
>
>  log_test()
>  {
> -	local rc=$1
> -	local expected=$2
> -	local msg="$3"
> -	local ans
> -
> -	[ "${VERBOSE}" = "1" ] && echo
> -
> -	if [ ${rc} -eq ${expected} ]; then
> -		nsuccess=$((nsuccess+1))
> -		printf "TEST: %-70s  [ OK ]\n" "${msg}"
> -	else
> -		nfail=$((nfail+1))
> -		printf "TEST: %-70s  [FAIL]\n" "${msg}"
> -		echo "    expected rc $expected; actual rc $rc"
> -		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
> -			echo
> -			echo "hit enter to continue, 'q' to quit"
> -			read ans
> -			[ "$ans" = "q" ] && exit 1
> -		fi
> -	fi
> -
> -	if [ "${PAUSE}" = "yes" ]; then
> -		echo
> -		echo "hit enter to continue, 'q' to quit"
> -		read ans
> -		[ "$ans" = "q" ] && exit 1
> -	fi
> +	log_test_expected "$1" "$2" "$3"
>
>  	kill_procs
>  }

[Severity: Medium]
The old fcnal-test.sh log_test() declared "local ans" and used "read ans"
in both pause branches.  Was that deliberate?  Many fcnal-test.sh test
functions use "a" as their address loop variable, for example
ipv4_ping_novrf():

	local a
	...
	for a in ${NSB_IP} ${NSB_LO_IP}
	do
		log_start
		run_cmd ping -c1 -w1 ${a}
		log_test_addr ${a} $? 0 "ping out"

		log_start
		run_cmd ping -c1 -w1 -I ${NSA_DEV} ${a}
		log_test_addr ${a} $? 0 "ping out, device bind"

The new shared helper in lib.sh reads into an undeclared "a" instead, and
bash dynamic scoping makes that assignment land in the caller's "local a".
The call chain is:

	ipv4_ping_novrf() -> log_test_addr() -> log_test() ->
		log_test_expected() -> read -r a
	ipv4_ping_novrf() -> log_test_addr() -> log_test() ->
		log_test_expected() -> pause_on_fail() -> read a

Neither log_test_addr(), log_test(), log_test_expected() nor
pause_on_fail() declares "local a".  With "-p" (PAUSE_ON_FAIL=yes) or "-P"
(PAUSE=yes) from the fcnal-test.sh getopts handling, does pressing plain
enter set a="" and leave the remaining subtests in the same loop iteration
pinging no destination and logging an empty address?

> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index d46d2cec89e45..f6f8b100a70c7 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -454,6 +454,39 @@ log_test_xfail()
>  	RET=$ksft_xfail retmsg= log_test "$@"
>  }
>
> +# Log test result with expected return value
> +log_test_expected()
> +{
> +	local rc=$1
> +	local expected=$2
> +	local msg="$3"
> +
> +	if [ "${rc}" -eq "${expected}" ]; then
> +		nsuccess=$((nsuccess+1))
> +		printf "    TEST: %-60s  [ OK ]\n" "${msg}"
> +	else
> +		ret="$ksft_fail"
> +		nfail=$((nfail+1))
> +		printf "    TEST: %-60s  [FAIL]\n" "${msg}"
> +		if [ "$VERBOSE" = "1" ]; then
> +			echo "    rc=$rc, expected $expected"
> +		fi
> +
> +		pause_on_fail

[Severity: Medium]
pause_on_fail() in lib.sh also reads without declaring the variable:

	pause_on_fail()
	{
		if [[ $PAUSE_ON_FAIL == yes ]]; then
			echo "Hit enter to continue, 'q' to quit"
			read a
			[[ $a == q ]] && exit 1
		fi
	}

Would adding "local a" to log_test_expected() (and to pause_on_fail())
keep the caller's "a" intact?

> +	fi
> +
> +	if [ "${PAUSE}" = "yes" ]; then
> +		echo
> +		echo "hit enter to continue, 'q' to quit"
> +		read -r a
                     ^^^^^^^^^

[Severity: Medium]
This is the PAUSE=yes path that fcnal-test.sh reaches with "-P".  Since
the pause fires after every test there, does every fcnal-test.sh function
that declares "local a" lose its loop value here, including
ipv4_ping_vrf(), the ipv4_tcp_* and ipv6_* helpers, and the netfilter_*
helpers?

> +		[ "$a" = "q" ] && exit 1
> +	fi
> +
> +	[ "$VERBOSE" = "1" ] && echo
> +
> +	return 0
> +}
> +
>  log_info()
>  {
>  	local msg=$1

[ ... remaining per-script log_test() conversions snipped ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-self_log_test-v4-1-4c656801f8dc%40kylinos.cn