Re: [PATCH net-next 5/5] selftests: ovpn: align command flow with TAP

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says: "The second one looks legit, would you prefer to follow
up or respin?"
---
selftests: ovpn: align command flow with TAP

This commit restructures the ovpn selftests to use the TAP infrastructure,
splitting each test into stages with KTAP output, adding command wrappers,
and centralizing behavior control in common.sh.

> diff --git a/tools/testing/selftests/net/ovpn/common.sh b/tools/testing/selftests/net/ovpn/common.sh
> index 2b3f2e5c8cc9d..ec6fea37ceb39 100644
> --- a/tools/testing/selftests/net/ovpn/common.sh
> +++ b/tools/testing/selftests/net/ovpn/common.sh

[ ... ]

> @@ -152,8 +263,8 @@ ovpn_compare_ntfs() {
>  		received="${OVPN_TMP_JSONS[$1]}"
>  		diff_file=$(mktemp)
>  
> -		kill -TERM ${OVPN_LISTENER_PIDS[$1]} || true
> -		wait ${OVPN_LISTENER_PIDS[$1]} || true
> +		kill -TERM ${OVPN_LISTENER_PIDS[$1]} 2>/dev/null || true
> +		wait ${OVPN_LISTENER_PIDS[$1]} 2>/dev/null || true
>  		printf "Checking notifications for peer ${1}... "

Does this leave the reaped PID in OVPN_LISTENER_PIDS?

Later, ovpn_cleanup() calls ovpn_stop_listener() which iterates over the
array and issues another kill -TERM on the same PID. Since the process was
already reaped by the wait here, could the OS recycle the PID, potentially
causing the second kill to terminate an unrelated system process?

[ ... ]

> +ovpn_cleanup_peer_ns() {
> +	local peer="$1"
> +
> +	if ! ip netns list | grep -qx "${peer}"; then
> +		return 0
> +	fi

Can this fail to match and permanently leak network namespaces?

When virtual interfaces are moved into namespaces during setup, ip netns list
often includes an internal ID in its output, such as:
peer0 (id: 0)

Because grep is using the -x flag to enforce an exact whole-line match,
it appears this will fail to match peer0 and return early, skipping the
subsequent namespace deletion.

[ ... ]

> diff --git a/tools/testing/selftests/net/ovpn/test-close-socket.sh b/tools/testing/selftests/net/ovpn/test-close-socket.sh
> index 4f0367c60fdaf..e97affe727383 100755
> --- a/tools/testing/selftests/net/ovpn/test-close-socket.sh
> +++ b/tools/testing/selftests/net/ovpn/test-close-socket.sh
> @@ -9,37 +9,72 @@ set -e
>  
>  source ./common.sh

Is test-close-socket.sh missing the set -eE flag and the ovpn_stage_err
ERR trap?

The other test scripts were updated to use set -eE and register the trap,
which handles emitting the KTAP failure directive. If a command fails here,
will set -e cause the script to exit immediately and bypass the KTAP
failure reporting entirely?