On Thu, Oct 01, 2026 at 04:58:13AM +0000, [email protected] wrote: > 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?
This is a pre-existing issue and should be handled by a separate patch. This patch merely renames the variable. Hangbin, can you follow up on this?
