Re: [PATCH net-next v3 2/2] selftests/net: add ICMP unreachable over IPsec tunnel

[Sabrina Dubroca <sd@queasysnail.net>]

Hi Antony,

2024-05-06, 10:05:54 +0200, Antony Antony wrote:
> diff --git a/tools/testing/selftests/net/xfrm_state.sh b/tools/testing/selftests/net/xfrm_state.sh
> new file mode 100755
> index 000000000000..26eac013abcf
> --- /dev/null
> +++ b/tools/testing/selftests/net/xfrm_state.sh
[...]
> +run_test() {
> +	(
> +	tname="$1"
> +	tdesc="$2"
> +
> +
> +	unset IFS
> +
> +	fail="yes"
> +
> +	# Since cleanup() relies on variables modified by this sub shell, it
> +	# has to run in this context.
> +	trap cleanup EXIT
> +
> +	if [ "$VERBOSE" -gt 0 ]; then
> +		printf "\n#####################################################################\n\n"
> +	fi
> +
> +	# if errexit was not set, set it and unset after test eval
> +	errexit=0
> +	if [[ $- =~ "e" ]]; then
> +		errexit=1
> +	else
> +		set -e
> +	fi
> +
> +	eval test_${tname}
> +	ret=$?
> +	fail="no"
> +	[ $errexit -eq 0 ] && set +e # hack until exception is fixed

What needs to be fixed?


> +
> +setup_namespace() {

Is this one actually used? I can't find a reference to "namespace"
(singular) in this script.

> +	setup_ns NS_A
> +	ns_a="ip netns exec ${NS_A}"
> +}
> +


> +veth_add() {
> +	local ns_cmd=$(nscmd $1)
> +	local tn="veth${2}1"
> +	local ln=${3:-eth0}
> +	run_cmd ${ns_cmd} ip link add ${ln} type veth peer name ${tn}

Why not just create the peer directly in the correct namespace and
with the correct name? That would avoid the mess of moving/renaming
with veth_mv, and the really hard to read loop in setup_veths.

> +}

[...]
> +
> +setup_vm_set_v4x() {
> +	ns_set="a r1 s1 r2 s2 b" # Network topology: x
> +	imax=6

It would be more robust to set ns_set, imax, and all other parameters
in every setup, so that the right topology is always used even if the
test order changes. Currently I'm not sure which topology is used in
which test, except the ones that use setup_vm_set_v4x and
setup_vm_set_v6x.

> +	prefix=${prefix4}
> +	s="."
> +	S="."
> +	src="10.1.3.1"
> +	dst="10.1.4.2"
> +	src_net="10.1.1.0/24"
> +	dst_net="10.1.5.0/24"
> +	prefix_len=24
> +
> +	vm_set
> +}

[...]
> +setup_veths() {
> +	i=1
> +	for ns in ${ns_active}; do
> +		[ ${i} = ${imax} ] && continue

IIUC imax should be the last, so s/continue/break/ ?

> +		veth_add ${ns} ${i}
> +		i=$((i + 1))
> +	done
> +
> +	j=1
> +	for ns in ${ns_active}; do
> +		if [ ${j} -eq 1 ]; then
> +			p=${ns};
> +			pj=${j}
> +			j=$((j + 1))
> +			continue
> +		fi
> +		veth_mv ${ns} "${p}" ${pj}
> +		p=${ns}
> +		pj=${j}
> +		j=$((j + 1))
> +	done
> +}
> +
> +setup_routes() {
> +	ip1=""
> +	i=1
> +	for ns in ${ns_active}; do
> +		# 10.1.C.1/24
> +		ip0="${prefix}${s}${i}${S}1/${prefix_len}"
> +		[ "${ns}" = b ] && ip0=""
> +		setup_addr_add ${ns} "${ip0}" "${ip1}"
> +		# 10.1.C.2/24
> +		ip1="${prefix}${s}${i}${S}2/${prefix_len}"
> +		i=$((i + 1))

This loop is really hard to follow :/
It would probably be easier to read if setup_addr_add only installed
exactly one address (instead of conditionally adding maybe 2), and
checking here whether the address needs to be added ("${ns}" != b,
i -ne 1).

> +	done
> +
> +	i=1
> +	nhr=""
> +	for ns in ${ns_active}; do
> +		nhf="${prefix}${s}${i}${S}2"
> +		[ "${ns}" = b ] && nhf=""
> +		route_add ${ns} "${nhf}" "${nhr}" ${i}
> +		nhr="${prefix}${s}${i}${S}1"
> +		i=$((i + 1))

I'd suggest the same here, split route_add into
route_add_{forward,reverse} and only call the right one (or both) for
the current iteration.

> +	done
> +}

[...]
> +setup() {
> +	[ "$(id -u)" -ne 0 ] && echo "  need to run as root" && return $ksft_skip
> +
> +	for arg do
> +		eval setup_${arg} || { echo "  ${arg} not supported"; return 1; }
> +	done
> +}
> +
> +trace() {

Unused?

> +	[ $TRACING -eq 0 ] && return

Then you can also get rid of that variable at the top.


[...]
> +mtu() {
> +	ns_cmd="${1}"
> +	dev="${2}"
> +	mtu="${3}"
> +
> +	${ns_cmd} ip link set dev ${dev} mtu ${mtu}
> +}
> +
> +mtu_parse() {
> +	input="${1}"
> +
> +	next=0
> +	for i in ${input}; do
> +		[ ${next} -eq 1 -a "${i}" = "lock" ] && next=2 && continue
> +		[ ${next} -eq 1 ] && echo "${i}" && return
> +		[ ${next} -eq 2 ] && echo "lock ${i}" && return
> +		[ "${i}" = "mtu" ] && next=1
> +	done
> +}
> +
> +link_get() {
> +	ns_cmd="${1}"
> +	name="${2}"
> +
> +	${ns_cmd} ip link show dev "${name}"
> +}
> +
> +link_get_mtu() {
> +	ns_cmd="${1}"
> +	name="${2}"
> +
> +	mtu_parse "$(link_get "${ns_cmd}" ${name})"
> +}

All those also seem completely unused by this script. Please don't
just c/p from other selftests without checking.

-- 
Sabrina