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 <[email protected]>
>
> [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
