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] In tools/testing/selftests/net/fib_tests.sh, the patch converts check_rt_num() from `ret` to `RET`, but the helper still sets… -- selftests: fib_tests: use per-test return value in subtests This commit changes fib_tests.sh to use RET, as defined in lib.sh, for the per-test return value. The global exit code ret is now only set in log_test(), so a later subtest can no longer clear an earlier failure. Closes: https://lore.kernel.org/all/[email protected] > diff --git a/tools/testing/selftests/net/fib_tests.sh > b/tools/testing/selftests/net/fib_tests.sh > index b338bfb196a27..9ce5623b049c1 100755 > --- a/tools/testing/selftests/net/fib_tests.sh > +++ b/tools/testing/selftests/net/fib_tests.sh [ ... ] > @@ -763,9 +763,9 @@ check_rt_num() > > if [ $num -ne $expected ]; then > echo "FAIL: Expected $expected routes, got $num" > - ret=1 > + RET=1 > else > - ret=0 > + RET=0 > fi > } [Severity: Medium] check_rt_num() still sets RET=0 when a check passes. Can a later passing check_rt_num() in the same subtest hide an earlier failure? lib.sh describes RET as "Per-test return value. Clear at the beginning of each test." Its helpers only ever raise it. For example, ret_set_ksft_status() does: RET=$(ksft_status_merge $RET $ksft_status) Several subtests call check_rt_num() more than once before a single log_test. For example, fib6_ra_to_static() does: check_rt_num 1 $($IP -6 route list |grep expires|wc -l) ... check_rt_num 1 $($IP -6 route list |grep expires|wc -l) ... check_rt_num 2 $($IP -6 route list |grep expires|wc -l) log_test "$RET" 0 "ipv6 promote RA route to static" Say the on-link route still has an expiry after the static address is added. The first check prints "FAIL: Expected 1 routes, got 2". If the last check then passes, it resets RET to 0. log_test() prints [ OK ], nfail is not incremented, the global ret stays 0, and the script exits with 0. The same pattern shows up in two more places: - In fib6_gc_test(), for "promote to permanent routes": three check_rt_num calls run before the final "check_rt_num 0" on route list cache. - In fib6_ra_to_static(), for "RA route with nexthop do not merge into ECMP with static": the "2001:12::/64 via" check is followed by the "nexthop via" check. This looks like the same symptom the commit message describes, only inside a single subtest. Could check_rt_num() set RET=1 only on failure, with RET=0 cleared at the start of each logged subtest? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-self_fib_tests-v1-1-0eab6a3c3a3c%40kylinos.cn
