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/[email protected] Link: https://lore.kernel.org/r/[email protected] A few questions about the conversion below. > diff --git a/tools/testing/selftests/net/fdb_flush.sh > b/tools/testing/selftests/net/fdb_flush.sh > index 9931a1e36e3db..4965e52d5ef54 100755 > --- a/tools/testing/selftests/net/fdb_flush.sh > +++ b/tools/testing/selftests/net/fdb_flush.sh > @@ -67,40 +67,7 @@ run_cmd() > > log_test() > { > - local rc=$1 > - local expected=$2 > - local msg="$3" > - local nsuccess > - local nfail > - local ret > - > - if [ ${rc} -eq ${expected} ]; then > - printf "TEST: %-60s [ OK ]\n" "${msg}" > - nsuccess=$((nsuccess+1)) > - else > - ret=1 > - nfail=$((nfail+1)) > - printf "TEST: %-60s [FAIL]\n" "${msg}" > - if [ "$VERBOSE" = "1" ]; then > - echo " rc=$rc, expected $expected" > - fi > - > - if [ "${PAUSE_ON_FAIL}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > - fi > - > - if [ "${PAUSE}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > - > - [ "$VERBOSE" = "1" ] && echo > + log_test_expected "$1" "$2" "$3" > } [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. > diff --git a/tools/testing/selftests/net/fib-onlink-tests.sh > b/tools/testing/selftests/net/fib-onlink-tests.sh > index e0d45292a2983..a26075abf7d27 100755 > --- a/tools/testing/selftests/net/fib-onlink-tests.sh > +++ b/tools/testing/selftests/net/fib-onlink-tests.sh > @@ -85,23 +85,7 @@ PBR_TABLE=101 > > log_test() > { > - local rc=$1 > - local expected=$2 > - local msg="$3" > - > - if [ ${rc} -eq ${expected} ]; then > - nsuccess=$((nsuccess+1)) > - printf " TEST: %-50s [ OK ]\n" "${msg}" > - else > - nfail=$((nfail+1)) > - printf " TEST: %-50s [FAIL]\n" "${msg}" > - if [ "${PAUSE_ON_FAIL}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > - fi > + log_test_expected "$1" "$2" "$3" > } [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? > diff --git a/tools/testing/selftests/net/fib_nexthops.sh > b/tools/testing/selftests/net/fib_nexthops.sh > index 431d7bed76224..d0d979e8e438b 100755 > --- a/tools/testing/selftests/net/fib_nexthops.sh > +++ b/tools/testing/selftests/net/fib_nexthops.sh > @@ -70,44 +70,7 @@ nsid=100 > > log_test() > { > - local rc=$1 > - local expected=$2 > - local msg="$3" > - > - if [ ${rc} -eq ${expected} ]; then > - printf "TEST: %-60s [ OK ]\n" "${msg}" > - nsuccess=$((nsuccess+1)) > - else > - if [[ $rc -eq $ksft_skip ]]; then > - [[ $ret -eq 0 ]] && ret=$ksft_skip > - nskip=$((nskip+1)) > - printf "TEST: %-60s [SKIP]\n" "${msg}" > - else > - ret=1 > - nfail=$((nfail+1)) > - printf "TEST: %-60s [FAIL]\n" "${msg}" > - fi [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. > - > - if [ "$VERBOSE" = "1" ]; then > - echo " rc=$rc, expected $expected" > - fi > - > - if [ "${PAUSE_ON_FAIL}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > - fi > - > - if [ "${PAUSE}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > - > - [ "$VERBOSE" = "1" ] && echo > + log_test_expected "$1" "$2" "$3" > } > > run_cmd() > @@ -1258,7 +1221,7 @@ ipv6_fcnal_runtime() > run_cmd "ip netns exec $me ping -c1 -w$PING_TIMEOUT > 2001:db8:101::1" > log_test $? 0 "Ping - group blackhole replaced with gateways" > else > - log_test 2 0 "Ping - multipath failed" > + log_test $rc 0 "Ping - multipath failed" > fi [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. > > # > @@ -1915,7 +1878,7 @@ ipv4_fcnal_runtime() > run_cmd "ip netns exec $me ping -c1 -w$PING_TIMEOUT > 172.16.101.1" > log_test $? 0 "Ping - group blackhole replaced with gateways" > else > - log_test 2 0 "Ping - multipath failed" > + log_test $rc 0 "Ping - multipath failed" > fi > > # > @@ -2729,7 +2692,6 @@ done > if [ "$TESTS" != "none" ]; then > printf "\nTests passed: %3d\n" ${nsuccess} > printf "Tests failed: %3d\n" ${nfail} > - printf "Tests skipped: %2d\n" ${nskip} > fi > > exit $ret [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. > diff --git a/tools/testing/selftests/net/fib_tests.sh > b/tools/testing/selftests/net/fib_tests.sh > index b338bfb196a27..7df967a2d6697 100755 > --- a/tools/testing/selftests/net/fib_tests.sh > +++ b/tools/testing/selftests/net/fib_tests.sh > @@ -24,31 +24,7 @@ which ping6 > /dev/null 2>&1 && ping6=$(which ping6) || > ping6=$(which ping) > > log_test() > { > - local rc=$1 > - local expected=$2 > - local msg="$3" > - > - if [ ${rc} -eq ${expected} ]; then > - printf " TEST: %-60s [ OK ]\n" "${msg}" > - nsuccess=$((nsuccess+1)) > - else > - ret=1 > - nfail=$((nfail+1)) > - printf " TEST: %-60s [FAIL]\n" "${msg}" [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? > - if [ "${PAUSE_ON_FAIL}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > - fi > - > - if [ "${PAUSE}" = "yes" ]; then > - echo > - echo "hit enter to continue, 'q' to quit" > - read a > - [ "$a" = "q" ] && exit 1 > - fi > + log_test_expected "$1" "$2" "$3" > } > > setup() [ ... ] > diff --git a/tools/testing/selftests/net/lib.sh > b/tools/testing/selftests/net/lib.sh > index d46d2cec89e45..f698fa3d2bd5f 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 [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. > + nfail=$((nfail+1)) > + printf " TEST: %-60s [FAIL]\n" "${msg}" > + if [ "$VERBOSE" = "1" ]; then > + echo " rc=$rc, expected $expected" > + fi > + > + pause_on_fail > + fi > + > + if [ "${PAUSE}" = "yes" ]; then > + echo > + echo "hit enter to continue, 'q' to quit" > + read -r a > + [ "$a" = "q" ] && exit 1 > + fi > + > + [ "$VERBOSE" = "1" ] && echo > + > + return 0 > +} > + > log_info() > { > local msg=$1 [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-self_log_test-v3-1-a17728176334%40kylinos.cn
