This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at:
https://sashiko.dev/#/patchset/[email protected]?part=6
---
--- Patch [6]: [PATCH net-next 6/6] selftests: net: add kselftest for IEEE
802.1CB FRER tc action ---
commit af0b752d09a70dfd74c1c5880e3ee38044b284c6
Author: Xiaoliang Yang <[email protected]>
selftests: net: add kselftest for IEEE 802.1CB FRER tc action
Add a TAP-format kselftest script covering the FRER tc action. The tests
exercise shared and individual recover modes using bond, point-to-point,
and relay topologies.
> diff --git a/tools/testing/selftests/net/frer_test.sh
> b/tools/testing/selftests/net/frer_test.sh
> new file mode 100755
> index 0000000000000..ecd88952f4958
> --- /dev/null
> +++ b/tools/testing/selftests/net/frer_test.sh
[ ... ]
> +cleanup()
> +{
> + for n in "$NS_TALKER" "$NS_LISTENER" \
> + "$P2P_NS_SRC" "$P2P_NS_DST" \
> + "$R_NS_TALKER" "$R_NS_BRIDGE0" "$R_NS_BRIDGE1"
> "$R_NS_LISTENER"; do
> + ip netns del "$n" 2>/dev/null || true
> + done
> +}
> +trap cleanup EXIT
[Severity: Low]
Does this cleanup handler leak temporary packet capture files?
The test allocates files via mktemp /tmp/frer_... earlier, but this trap
handler only removes network namespaces and does not issue an rm for the
pcap files.
[ ... ]
> +capture_start_on()
> +{
[ ... ]
> + # Wait until tcpdump opens a packet socket (max ~2.5 s).
> + local tries=0
> + while [ $tries -lt 50 ]; do
> + ip netns exec "$ns" grep -q "$iface" /proc/net/packet
> 2>/dev/null && break
Simon says: I believe the common approach for the issue flagged below
is to simply "sleep 1".
Which has the advantage of being simple.
Otherwise the index can be looked up from sysfs:
/sys/class/net/$iface/ifindex
[Severity: Medium]
Will this grep always fail and exhaust the 50-iteration retry limit?
/proc/net/packet exposes the integer ifindex of the interface, not the
string name.
This could cause a 2.5-second block per capture, wasting most of the 4-second
tcpdump timeout window. This leaves only 1.5s for the ping command and could
potentially cause test flakiness.
> + sleep 0.05
> + tries=$((tries + 1))
> + done
> +}
> +
> +capture_stop()
> +{
> + [ -n "$_CAP_PID" ] || return 0
> + wait "$_CAP_PID" 2>/dev/null || true
Simon says: The issue flagged below like a design decision,
I think we can pass on it.
[Severity: Medium]
Could this cause the script to idly block for the remainder of the 4-second
tcpdump timeout?
Because capture_start_on() spawns tcpdump using timeout 4 tcpdump &, running
wait without first sending a termination signal means it waits for the
timeout to expire naturally.
This could add significant delay across the full test suite.
> + _CAP_PID=""
> +}
[ ... ]
> +test_individual_recover_bond()
> +{
[ ... ]
> + # Per-slave capture A: verify veth_a1 passes frames; also use this run
> + # for the overall ping_rc check (do_ping targets bond0->bond1).
> + pcap_a=$(mktemp /tmp/frer_bond_indv_a_XXXXXX.pcap)
> + capture_start_on "$NS_LISTENER" "$VETH_A1" "$pcap_a" "icmp"
Simon says: The issue flagged below seems to be a false-positive.
[Severity: Medium]
Will this BPF filter drop the ingress R-TAG frames?
Since tcpdump opens an AF_PACKET socket, it taps ingress frames in
__netif_receive_skb_core() before sch_handle_ingress() is called.
The ingress frame still contains the FRER R-TAG (EtherType 0xF1C1) and would
bypass the icmp (0x0800) filter, potentially leading to 0 captured ingress
ICMP frames and a deterministic test failure.
> + do_ping || ping_rc=$?
> + capture_stop
[ ... ]
> +main()
> +{
[ ... ]
> + ksft_print_cnts
> +
> + [ "$_ksft_fail" -eq 0 ] && ksft_exit_pass || ksft_exit_fail
[Severity: High]
Can this unconditionally fail the test with a bash syntax error?
When run in the kselftest tree, the script sources kselftest/lib.sh, which
skips the else block that initializes _ksft_fail=0.
Evaluating this with an unset variable triggers an integer expression
expected error and unconditionally takes the ksft_exit_fail path.
> +}
> +
> +main "$@"
