Re: [PATCH net] selftests: fib_tests: use per-test return value in subtests

[Ido Schimmel]

On Thu, Oct 01, 2026 at 04:58:13AM +0000, netdev-bot+sashiko@xxxxxxxxxx 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/178961184152.22033.9267194009793243294@xxxxxxxxxx
> 
> > 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?