Re: [PATCH net-next] selftests: net: move log_test to lib file and remove duplicate code

On Thu, Aug 13, 2026 at 04:11:57PM +0800, Hangbin Liu wrote:
> From: Hangbin Liu <[email protected]>
> 
> When reviewing the test code, I saw many tests using the same or similar
> log_test functions. We can move them to lib.sh to save effort. However,
> due to historical reasons, we moved the log_test from the forwarding lib
> first, which has different usage. So, rename the log_test in the net
> folder to log_test_expected. Since renaming all the log_test functions
> in the test cases would change too many lines, I just use a wrapper in
> the old code.
> 
> The fourth argument in icmp_redirect.sh is not needed, as the xfail issue
> has already been fixed and it should always pass. But I still keep the
> xfail logic in the log_test in lib.sh in case other tests need it.
> 
> Signed-off-by: Hangbin Liu <[email protected]>
> ---

[...]

> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> index 3d347126730a..16fe92775ae0 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 b13c89a99ecb..0107af73aef4 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"
>  }

[...]
>  
> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index d46d2cec89e4..e02a6a91ff91 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}"
> +	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
> +}
> +

Reply to sashiko's review.

  """
  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
  	...
  }
  """

Correct, this need to be updated, otherwise the report will be xfail.

  """
  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?
  """

If "expected" itself is ksft_skip/ksft_xfail, and we got the expected number,
then the result should be PASS.


  """
  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.
  """

We can remove the xfail count in icmp_redirect.sh testing


> diff --git a/tools/testing/selftests/net/test_bridge_neigh_suppress.sh b/tools/testing/selftests/net/test_bridge_neigh_suppress.sh
> index e9ed0d750996..9d2dc0faf741 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"
>  }
>  

  """
  Is dropping this "return 0" safe here?
  """

Hmm, yes, we'd better add return 0 in log_test as the read may return 1 if
it's not "q".

I will update the patch.

Thanks
Hangbin