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 ]; thenp=${ns};pj=${j}j=$((j + 1))continuefiveth_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/24ip0="${prefix}${s}${i}${S}1/${prefix_len}"[ "${ns}" = b ] && ip0=""setup_addr_add ${ns} "${ip0}" "${ip1}"# 10.1.C.2/24ip1="${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.