On Mon, Sep 28, 2026 at 07:50:26AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> 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 the new log_test_expected()
> (tools/testing/selftests/net/lib.sh), the failure branch calls the…
> - [Low] The commit message miscounts one output-format change.
>
> --
>
> 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. It converts 31 net selftests so their
> local log_test() wrappers call the helper instead of carrying their own
> copies. The output format becomes " TEST: %-60s" in all of them, and
> PAUSE, PAUSE_ON_FAIL and VERBOSE are handled the same way in every
> converted script.
>
> > In addition to the above, 13 tests that previously used "TEST:" now use
> > " TEST:" (4-space prefix), and srv6 tests plus vrf_strict_mode_test.sh
> > that previously used "\n TEST:" (newline + 4-space prefix) now with no
> > leading newline.
>
> [Severity: Low]
> Is the count of 13 still right? The patch converts 12 scripts that printed
> an unprefixed "TEST:":
OK...
>
> fcnal-test.sh, fdb_flush.sh, fib_nexthop_multiprefix.sh,
> fib_nexthop_nongw.sh, icmp_redirect.sh, l2tp.sh,
> test_bridge_backup_port.sh, test_bridge_neigh_suppress.sh,
> test_vxlan_mdb.sh, test_vxlan_nolocalbypass.sh, vrf-xfrm-tests.sh,
> vrf_route_leaking.sh
>
> The 13th looks like fib_nexthops.sh. It was dropped in v4 and still has its
> own log_test() that prints "TEST: %-60s". Could the number be updated?
>
> > diff --git a/tools/testing/selftests/net/lib.sh
> > b/tools/testing/selftests/net/lib.sh
> > index d46d2cec89e4..b31bd6f6efd8 100644
> > --- a/tools/testing/selftests/net/lib.sh
> > +++ b/tools/testing/selftests/net/lib.sh
> > @@ -454,6 +454,40 @@ 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"
> > + local a
> > +
> > + if [ "${rc}" -eq "${expected}" ]; then
> > + nsuccess=$((nsuccess+1))
> > + printf " TEST: %-60s [ OK ]\n" "${msg}"
> > + else
> > + ret="$ksft_fail"
> > + nfail=$((nfail+1))
> > + printf " TEST: %-60s [FAIL]\n" "${msg}"
> > + if [ "$VERBOSE" = "1" ]; then
> > + echo " rc=$rc, expected $expected"
> > + fi
> > +
> > + pause_on_fail
> > + fi
>
> [Severity: Medium]
> Could this call end a caller that runs under set -e? The shared
> pause_on_fail() in lib.sh ends with an && list:
>
> pause_on_fail()
> {
> if [[ $PAUSE_ON_FAIL == yes ]]; then
> echo "Hit enter to continue, 'q' to quit"
> read a
> [[ $a == q ]] && exit 1
> fi
> }
>
> If the user presses Enter, or types anything other than q, the function
> returns 1. Here it is called as a plain command, so errexit applies to it.
>
> fib6_gc_test() in fib_tests.sh turns on errexit and then calls log_test:
>
> fib6_gc_test()
> {
> ...
> set -e
> ...
> check_rt_num 0 $($IP -6 route list |grep expires|wc -l)
> log_test $ret 0 "ipv6 route garbage collection"
>
> Take "fib_tests.sh -p -t ipv6_gc" with a wrong route count. Pressing Enter
> at the prompt would then exit the whole script with status 1. The remaining
> tests and the summary are skipped, and the "return 0" at the end of
> log_test_expected() is never reached.
>
> The old log_test() in fib_tests.sh ran the prompt inline in an if body.
> There the trailing && list is exempt from errexit, so the script kept
> running. The other set -e regions in fib_tests.sh that call log_test
> before set +e look affected in the same way.
Does anyone really want to keep running the test when set *PAUSE_ON_FAIL*
and "set -e", and the test failed somehow?
>
> This also doesn't match the commit message's statement that "The function
> always returns 0 to avoid influencing later code". Would something like
> "pause_on_fail || true" avoid the early exit?
If someone asked, I can do an update for this.
Thanks
Hangbin
