Re: [PATCHv2] selftests: Use ktap helpers for runner.sh

[Hangbin Liu <liuhangbin@gmail.com>]

On Tue, Feb 24, 2026 at 11:38:34AM +0000, Brendan Jackman wrote:
> Cool thanks sorry I did not really review the code before but now I'm
> taking a closer look.
> 
> I don't parse KTAP but I did check that this doesn't break my usecase
> [0] so:
> 
> Tested-By: Brendan Jackman <jackmanb@google.com>
> 
> [0]: https://github.com/bjackman/limmat-kernel-nix/blob/master/ktests.nix

Thanks for the testing.

> >  run_in_netns()
> >  {
> > -	local netns=$(mktemp -u ${BASENAME_TEST}-XXXXXX)
> >  	local tmplog="/tmp/$(mktemp -u ${BASENAME_TEST}-XXXXXX)"
> > +	local netns=$(mktemp -u ${BASENAME_TEST}-XXXXXX)
> 
> Nit (no need to respin just for this, but if you're already respinning):
> Diff noise here.

This is to make the variables in reverse Christmas tree based on kernel code
style.

> 
> > +	local rc
> > +
> >  	ip netns add $netns
> >  	if [ $? -ne 0 ]; then
> > -		echo "# Warning: Create namespace failed for $BASENAME_TEST"
> > -		echo "not ok $test_num selftests: $DIR: $BASENAME_TEST # Create NS failed"
> > +		ktap_print_msg "Warning: Create namespace failed for $BASENAME_TEST"
> > +		ktap_test_fail "$test_num selftests: $DIR: $BASENAME_TEST # Create NS failed"
> >  	fi
> >  	ip -n $netns link set lo up
> > +
> >  	in_netns $netns &> $tmplog
> > +	rc=$?
> > +
> >  	ip netns del $netns &> /dev/null
> > +	# Cat the log at once to avoid parallel netns logs.
> >  	cat $tmplog
> >  	rm -f $tmplog
> > +	return $rc
> >  }
> >  
> >  run_many()
> >  {
> > -	echo "TAP version 13"
> >  	DIR="${PWD#${BASE_DIR}/}"
> >  	test_num=0
> > -	total=$(echo "$@" | wc -w)
> > -	echo "1..$total"
> > +	local rc
> > +	pids=()
> > +
> >  	for TEST in "$@"; do
> >  		BASENAME_TEST=$(basename $TEST)
> >  		test_num=$(( test_num + 1 ))
> > @@ -194,10 +200,20 @@ run_many()
> >  		fi
> >  		if [ -n "$RUN_IN_NETNS" ]; then
> >  			run_in_netns &
> > +			pids+=($!)
> >  		else
> >  			run_one "$DIR" "$TEST" "$test_num"
> >  		fi
> >  	done
> >  
> > -	wait
> > +	# These variables are outputs of ktap_helpers.sh but since we've
> > +	# run the test in a subprocess we need to update them manually
> > +	for pid in "${pids[@]}"; do
> > +		wait "$pid"
> > +		rc=$?
> > +		[ "$rc" -eq "$KSFT_PASS" ] && KTAP_CNT_PASS=$((KTAP_CNT_PASS+1))
> > +		[ "$rc" -eq "$KSFT_FAIL" ] && KTAP_CNT_FAIL=$((KTAP_CNT_FAIL+1))
> > +		[ "$rc" -eq "$KSFT_SKIP" ] && KTAP_CNT_SKIP=$((KTAP_CNT_SKIP+1))
> > +		[ "$rc" -eq "$KSFT_XFAIL" ] && KTAP_CNT_XFAIL=$((KTAP_CNT_XFAIL+1))
> > +	done
> 
> Can we please be consistent about how we do these two big conditional
> blocks? Also should they use `case`? Also please add something to not

Ah, yes, we can use case here. Let me update the patch.

> fail silently if $rc has a garbage value (I guess just log a warning).

A log here may break the KTAP output. While run_in_netns() calls run_one()
in the end, which will print the exit value if it's a garbage value. i.e.

    ktap_test_fail "$test_num $TEST_HDR_MSG # exit=$rc"

> 
> FWIW I still think this is very yucky but, it seems switching to the
> helper library is a worthwhile cleanup (plus, this patch drops a level
> of subshell nesting in the tap_timeout pipeline, which seems like a step
> in the right direction!) so I think it would be irrational to block that
> cleanup just coz of this yuckiness.

Thanks
Hangbin