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

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

Critical: 0 · High: 1 · Medium: 1 (2) · Low: 3

- [High] SKIP-to-FAIL regression in fib_nexthops.sh, based on a factually
  false claim in the commit message.
- [Medium] log_test_expected() in lib.sh unconditionally executes
  `ret=$(ksft_exit_status_merge "$ret" "$ksft_fail")` on the failure…
- [Low] The commit message states "Two callers compared against exit code
  2 are left as-is since code 2 correctly counts as a failure in the…
- [Low] The migration path advertised in the changelog does not work.
- [Low] The changelog justifies a change with a rationale that cannot
  hold: "On failure, ret is set via ksft_exit_status_merge() instead of…

Pre-existing issues:
- [Medium] fdb_flush.sh, fib-onlink-tests.sh and l2tp.sh can print [FAIL]
  subtests while exiting with status 0, because they never consume the…
- [Medium] fib_tests.sh uses the same global `ret` that the shared helper
  treats as cumulative exit status as per-test scratch storage, so a…

--

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 32 net selftests with a thin
wrapper around it. A few scripts also start sourcing lib.sh instead of
defining ksft_skip locally.

Link: https://lore.kernel.org/r/20260831-self_log_test-v2-1-eda2e8490cf9@kylinos.cn (local)
Link: https://lore.kernel.org/r/20260813-self_log_test-v1-1-f88b1107842e@kylinos.cn (local)

A few questions about the conversion below.

[Severity: Medium]
The changelog notes that fdb_flush.sh "used local ret, nsuccess, and nfail,
which are not used outside the function", but this script has no global ret
either.  A grep for '\bret\b' in fdb_flush.sh, fcnal-test.sh,
fib-onlink-tests.sh and l2tp.sh at this commit returns no matches.

The failure path of the new helper reads ret unconditionally:

lib.sh:log_test_expected() {
	...
	ret=$(ksft_exit_status_merge "$ret" "$ksft_fail")

With ret unset, the empty first operand ends up as an associative array
subscript in the merge helper:

lib.sh:__ksft_status_merge() {
	...
	if [[ ${weights[$a]} -ge ${weights[$b]} ]]; then

Does bash not print "bad array subscript" here for every failing subtest in
those four scripts?  The merged value still comes out as 1 because the empty
expansion evaluates as 0, so the visible effect is stderr noise from inside
lib.sh, one line per failure (fcnal-test.sh has a lot of subtests).

Would it make sense for the shared helper to default the variable, e.g.
: "${ret:=0}" before the merge, rather than relying on each caller having
defined it?  Also worth noting the copy of this line that previously lived in
test_bridge_neigh_suppress.sh carried a "# shellcheck disable=SC2154" marker
for exactly this referenced-but-not-assigned case, and that marker was
dropped when the code moved into lib.sh.

[Severity: Medium]
This isn't a bug introduced by this patch, but it is worth noting while the
failure bookkeeping is being centralised: fib-onlink-tests.sh, l2tp.sh and
fdb_flush.sh never consume the status the helper records.

fib-onlink-tests.sh ends with:

	if [ "$TESTS" != "none" ]; then
		printf "\nTests passed: %3d\n" ${nsuccess}
		printf "Tests failed: %3d\n"   ${nfail}
	fi

l2tp.sh ends with the same two printf lines, and fdb_flush.sh ends with:

	for t in $TESTS
	do
		setup; $t; cleanup;
	done

None of the three does exit $ret or derives an exit code from nfail, so can
these scripts print [FAIL] subtests and still report success to the kselftest
runner?  Since this patch is what makes ret the shared failure accumulator,
would adding exit $ret to those scripts fit here?

[Severity: High]
The changelog states:

  "this test checked for ksft_skip (exit code 4) inside log_test(),
   but no test in the file produces that code."

Are these two call sites not producing exactly that code?

fib_nexthops.sh:ipv6_grp_fcnal() {
	...
	run_cmd "$IP nexthop add id 103 group 62,1000"
	if [[ $? == 0 ]]; then
		local GRP="id 103 group 62,254/63,255/64,256/65,257/66,65535"
		run_cmd "$IP nexthop replace $GRP"
		check_nexthop "id 103" "$GRP"
		rc=$?
	else
		rc=$ksft_skip
	fi

	$IP nexthop flush >/dev/null 2>&1

	log_test $rc 0 "16-bit weights"
}

ipv6_res_grp_fcnal() has the same shape with "group 62,1000 type resilient
buckets 32".

The else branch is taken whenever iproute2 is too old to accept 16-bit
nexthop group weights.  After this change, log_test_expected() gets rc=4
against expected 0, so both subtests take the failure branch: [FAIL] is
printed, nfail is incremented and ret is merged with $ksft_fail.

Combined with exit $ret at the end of the script, does a run on such a host
change from two [SKIP] lines and exit status 4 to two [FAIL] lines and exit
status 1?

The changelog suggests "callers that need to report a skip should use
log_test_skip() instead", but neither of these two callers was converted.

[Severity: Low]
The changelog says:

  "Two callers compared against exit code 2 are left as-is since code 2
   correctly counts as a failure in the previous log_test()."

These are the two callers in question, in ipv6_fcnal_runtime() and
ipv4_fcnal_runtime(), and they are changed rather than left as-is.  Could the
changelog be updated to describe the substitution?

The change itself looks harmless: run_cmd() assigns rc=$? without declaring
it local, so under bash dynamic scoping it writes into the caller's local rc,
and the else branch is only reached when that value is non-zero, so the
subtest still reports [FAIL] with the same message.

[Severity: High]
With the skip counter line removed and no skip branch left in the shared
helper, is there any remaining way for this script to report a skipped
subtest?  The two 16-bit weight probes above are the only producers of
ksft_skip and they are now counted in nfail.

[Severity: Medium]
This is a pre-existing issue, but it interacts with making ret a shared
cumulative status in lib.sh: fib_tests.sh also uses the same global ret as
per-test scratch storage.

fib_carrier_unicast_test() starts with an unconditional:

	ret=0

and fib6_notify_test()/fib_notify_test() do:

	if [ -z "$err" ];then
		ret=0
	else
		ret=1
	fi

	log_test $ret 0 "ipv6 route add notify"

None of these declare ret local, and the script finishes with exit $ret.  If
an earlier subtest failed and a later function assigns ret=0 and then passes,
does the script exit 0 despite the printed [FAIL] lines?  Should these
functions use a local variable now that the helper owns ret?

[ ... ]

[Severity: Low]
The changelog points callers at log_test_skip() as the replacement for the
removed ksft_skip handling.  Can that helper work in the converted scripts?
It dispatches through log_test:

lib.sh:log_test_skip() {
	RET=$ksft_skip retmsg= log_test "$@"
}

and every script touched here defines its own log_test() after sourcing
lib.sh:

fib_nexthops.sh:log_test() {
	log_test_expected "$1" "$2" "$3"
}

so the local definition shadows lib.sh's log_test().  A call like
log_test_skip "16-bit weights" would then reach log_test_expected with
rc="16-bit weights", expected="" and msg="", i.e.:

	[ "16-bit weights" -eq "" ]

which is an "integer expression expected" error landing in the failure
branch and printing [FAIL] with an empty test name.  There are no callers
today, so nothing breaks right now, but should the wrapper names be
reworked so the suggested migration actually works?

+		ret=$(ksft_exit_status_merge "$ret" "$ksft_fail")

[Severity: Low]
The changelog gives this as the reason for the change:

  "On failure, ret is set via ksft_exit_status_merge() instead of ret=1,
   so we can propagate ksft_xfail status correctly."

Is that achievable here?  ksft_exit_status_merge() orders the operands as:

lib.sh:ksft_exit_status_merge() {
	...
	__ksft_status_merge "$a" "$b" \
		$ksft_xfail $ksft_pass $ksft_skip $ksft_fail
}

so xfail=0, pass=1, skip=2, fail=3, and the operand with the higher weight
wins.  Since $ksft_fail is always passed as the second operand and carries
the maximum weight, doesn't the result come out as $ksft_fail regardless of
the previous value of ret, including $ksft_xfail?  That makes this line
equivalent to the old ret=1.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-self_log_test-v3-1-a17728176334%40kylinos.cn