This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: net: move log_test to lib file and remove duplicate code
This moves the various open coded log_test() helpers from 33 net selftest
scripts into a single log_test_expected() in tools/testing/selftests/net/lib.sh,
leaving a thin log_test() wrapper in each script. The fourth (xfail)
argument of icmp_redirect.sh's log_test() is dropped, while the xfail
handling is kept in the new helper.
A couple of questions about the new shared helper below.
> diff --git a/tools/testing/selftests/net/drop_monitor_tests.sh
> b/tools/testing/selftests/net/drop_monitor_tests.sh
[ ... ]
> diff --git a/tools/testing/selftests/net/fib_nexthops.sh
> b/tools/testing/selftests/net/fib_nexthops.sh
> index 3d347126730a9..16fe92775ae0a 100755
> --- a/tools/testing/selftests/net/fib_nexthops.sh
> +++ b/tools/testing/selftests/net/fib_nexthops.sh
> @@ -70,44 +70,7 @@ nsid=100
>
> log_test()
> {
> - local rc=$1
> - local expected=$2
> - local msg="$3"
> -
> - if [ ${rc} -eq ${expected} ]; then
> - printf "TEST: %-60s [ OK ]\n" "${msg}"
> - nsuccess=$((nsuccess+1))
> - else
> - if [[ $rc -eq $ksft_skip ]]; then
> - [[ $ret -eq 0 ]] && ret=$ksft_skip
> - nskip=$((nskip+1))
> - printf "TEST: %-60s [SKIP]\n" "${msg}"
> - else
> - ret=1
> - nfail=$((nfail+1))
> - printf "TEST: %-60s [FAIL]\n" "${msg}"
> - fi
> -
> - if [ "$VERBOSE" = "1" ]; then
> - echo " rc=$rc, expected $expected"
> - fi
> -
> - if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
> - echo
> - echo "hit enter to continue, 'q' to quit"
> - read a
> - [ "$a" = "q" ] && exit 1
> - fi
> - fi
> -
> - if [ "${PAUSE}" = "yes" ]; then
> - echo
> - echo "hit enter to continue, 'q' to quit"
> - read a
> - [ "$a" = "q" ] && exit 1
> - fi
> -
> - [ "$VERBOSE" = "1" ] && echo
> + log_test_expected "$1" "$2" "$3"
> }
>
> run_cmd()
[ ... ]
> diff --git a/tools/testing/selftests/net/icmp_redirect.sh
> b/tools/testing/selftests/net/icmp_redirect.sh
> index b13c89a99ecba..0107af73aef4d 100755
> --- a/tools/testing/selftests/net/icmp_redirect.sh
> +++ b/tools/testing/selftests/net/icmp_redirect.sh
> @@ -61,28 +61,7 @@ log_section()
>
> log_test()
> {
> - local rc=$1
> - local expected=$2
> - local msg="$3"
> - local xfail=$4
> -
> - if [ ${rc} -eq ${expected} ]; then
> - printf "TEST: %-60s [ OK ]\n" "${msg}"
> - nsuccess=$((nsuccess+1))
> - elif [ ${rc} -eq ${xfail} ]; then
> - printf "TEST: %-60s [XFAIL]\n" "${msg}"
> - nxfail=$((nxfail+1))
> - else
> - ret=1
> - nfail=$((nfail+1))
> - printf "TEST: %-60s [FAIL]\n" "${msg}"
> - if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
> - echo
> - echo "hit enter to continue, 'q' to quit"
> - read a
> - [ "$a" = "q" ] && exit 1
> - fi
> - fi
> + log_test_expected "$1" "$2" "$3"
> }
>
> log_debug()
[ ... ]
> diff --git a/tools/testing/selftests/net/lib.sh
> b/tools/testing/selftests/net/lib.sh
> index d46d2cec89e45..e02a6a91ff913 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -454,6 +454,44 @@ 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"
> +
> + if [ "${rc}" -eq "${expected}" ]; then
> + nsuccess=$((nsuccess+1))
> + printf "TEST: %-60s [ OK ]\n" "${msg}"
> + elif [ "${rc}" -eq "${ksft_skip}" ]; then
> + [[ "$ret" -eq 0 ]] && ret="$ksft_skip"
> + nskip=$((nskip+1))
> + printf "TEST: %-60s [SKIP]\n" "${msg}"
> + elif [ "${rc}" -eq "${ksft_xfail}" ]; then
> + nxfail=$((nxfail+1))
> + printf "TEST: %-60s [XFAIL]\n" "${msg}"
[Severity: High]
Can these two arms turn real failures into [XFAIL] or [SKIP]?
The callers pass a raw exit status from ip, bridge, ping or grep, and the
new arms only look at that raw value, never at "expected". With
ksft_xfail=2 and ksft_skip=4, any command that exits 2 or 4 is reclassified
even when 0 was expected.
fib_nexthops.sh has two unconditional hard-failure calls:
tools/testing/selftests/net/fib_nexthops.sh:ipv6_fcnal_runtime() {
...
else
log_test 2 0 "Ping - multipath failed"
fi
...
}
and the same in ipv4_fcnal_runtime(). Before this patch these reached the
FAIL arm (nfail++ and ret=1); now they hit the XFAIL arm, which touches
neither nfail nor ret. The summary in that script prints only
nsuccess/nfail/nskip and it ends with "exit $ret", so does this failure now
disappear from both the output and the exit status?
The same concern applies to ordinary positive checks written as
"log_test $? 0 ...". iproute2 returns 2 for kernel-reported errors and ping
returns 2 for local errors such as "Network is unreachable", for example in
fcnal-test.sh:
run_cmd ping -c1 -w1 -I br0 ${NSB_IP}
log_test $? 0 "Bridge into VRF - IPv4 ping out"
and fcnal-test.sh decides its verdict from nfail alone:
if [ $nfail -ne 0 ]; then
exit 1 # KSFT_FAIL
Would it be safer to only take the SKIP/XFAIL arms when "expected" itself is
ksft_skip/ksft_xfail?
Two related observations: the SKIP arm also rewrites ret from 0 to
ksft_skip, and the srv6 scripts then normalize that away in
print_log_test_results():
if [ "${ret}" -ne 1 ]; then
ret=0
fi
In the other direction, icmp_redirect.sh's checks are grep pipelines that
only produce 0 or 1, so after dropping the fourth argument its nxfail
counter and the "Tests xfailed" line can no longer be reached.
> + else
> + ret=$(ksft_exit_status_merge "$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
> +
> + if [ "${PAUSE}" = "yes" ]; then
> + echo
> + echo "hit enter to continue, 'q' to quit"
> + read -r a
> + [ "$a" = "q" ] && exit 1
> + fi
> +
> + [ "$VERBOSE" = "1" ] && echo
> +}
> +
> log_info()
> {
> local msg=$1
[ ... ]
> diff --git a/tools/testing/selftests/net/test_bridge_neigh_suppress.sh
> b/tools/testing/selftests/net/test_bridge_neigh_suppress.sh
> index e9ed0d7509962..9d2dc0faf741d 100755
> --- a/tools/testing/selftests/net/test_bridge_neigh_suppress.sh
> +++ b/tools/testing/selftests/net/test_bridge_neigh_suppress.sh
> @@ -72,39 +72,7 @@ PAUSE=no
>
> log_test()
> {
[ ... ]
> - [ "$VERBOSE" = "1" ] && echo
> - return 0
> + log_test_expected "$1" "$2" "$3"
> }
[Severity: Medium]
Is dropping this "return 0" safe here?
The last command of log_test_expected() in lib.sh is:
[ "$VERBOSE" = "1" ] && echo
so with the default VERBOSE=0 the helper, and therefore this wrapper,
returns 1. In this script the test functions end with a
"log_test $? 0 ..." call, and the driver loop merges their status into ret:
setup
$t
ret=$(ksft_exit_status_merge "$ret" $?)
cleanup
with "exit $ret" at the end. Since ksft_exit_status_merge 0 1 yields 1,
does the script now exit 1 on every run while still printing
"Tests failed: 0"? The removed "return 0" looks like it existed for
exactly this reason.
[ ... ]
--
pw-bot: cr
