Re: [PATCH net-next v5] 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 2 potential
issue(s) to consider.

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

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] In the new log_test_expected()
  (tools/testing/selftests/net/lib.sh), the failure branch calls the…
- [Low] The commit message miscounts one output-format change.

--

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. It converts 31 net selftests so their
local log_test() wrappers call the helper instead of carrying their own
copies. The output format becomes "    TEST: %-60s" in all of them, and
PAUSE, PAUSE_ON_FAIL and VERBOSE are handled the same way in every
converted script.

> In addition to the above, 13 tests that previously used "TEST:" now use
> "    TEST:" (4-space prefix), and srv6 tests plus vrf_strict_mode_test.sh
> that previously used "\n    TEST:" (newline + 4-space prefix) now with no
> leading newline.

[Severity: Low]
Is the count of 13 still right? The patch converts 12 scripts that printed
an unprefixed "TEST:":

  fcnal-test.sh, fdb_flush.sh, fib_nexthop_multiprefix.sh,
  fib_nexthop_nongw.sh, icmp_redirect.sh, l2tp.sh,
  test_bridge_backup_port.sh, test_bridge_neigh_suppress.sh,
  test_vxlan_mdb.sh, test_vxlan_nolocalbypass.sh, vrf-xfrm-tests.sh,
  vrf_route_leaking.sh

The 13th looks like fib_nexthops.sh. It was dropped in v4 and still has its
own log_test() that prints "TEST: %-60s". Could the number be updated?

> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index d46d2cec89e4..b31bd6f6efd8 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -454,6 +454,40 @@ 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"
> +	local a
> +
> +	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
> +	fi

[Severity: Medium]
Could this call end a caller that runs under set -e? The shared
pause_on_fail() in lib.sh ends with an && list:

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

If the user presses Enter, or types anything other than q, the function
returns 1. Here it is called as a plain command, so errexit applies to it.

fib6_gc_test() in fib_tests.sh turns on errexit and then calls log_test:

fib6_gc_test()
{
	...
	set -e
	...
	check_rt_num 0 $($IP -6 route list |grep expires|wc -l)
	log_test $ret 0 "ipv6 route garbage collection"

Take "fib_tests.sh -p -t ipv6_gc" with a wrong route count. Pressing Enter
at the prompt would then exit the whole script with status 1. The remaining
tests and the summary are skipped, and the "return 0" at the end of
log_test_expected() is never reached.

The old log_test() in fib_tests.sh ran the prompt inline in an if body.
There the trailing && list is exempt from errexit, so the script kept
running. The other set -e regions in fib_tests.sh that call log_test
before set +e look affected in the same way.

This also doesn't match the commit message's statement that "The function
always returns 0 to avoid influencing later code". Would something like
"pause_on_fail || true" avoid the early exit?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-self_log_test-v5-1-1f00bd383146%40kylinos.cn