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

[netdev-bot+sashiko@kernel.org]

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@kernel.org

> 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