smoke: refactor tests to use bridge topology #352
Conversation
📝 WalkthroughWalkthroughThis pull request introduces new helper functions for test infrastructure initialization (port_add, netns_add, llocal_addr) in smoke/_init.sh and refactors smoke tests to use these helpers instead of manual interface and namespace configuration. The changes consolidate interface creation, VLAN tagging, and namespace management workflows. Additionally, net_tap-specific link event handling is removed from the control plane to use standard port status flow. CLI stdout/stderr are configured for line-buffered I/O, and non-syslog logging output is redirected from stderr to stdout. Multiple test scripts are updated to align with the new initialization patterns and helpers. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smoke/nexthop_ageing_test.sh (1)
11-27: Fix arithmetic expansion to support fractional timeout decrements.Line 23 uses
(( ))arithmetic with decimals, which bash does not support. Usebc,awk, or accumulate loop counts instead.check_nexthop() { local ip="$1" local expect_reacheable="$2" - local timeout=5 - while [ "$timeout" -gt 0 ]; do + local timeout=50 + local count=0 + while [ $count -lt $timeout ]; do grcli nexthop show internal | grep -qE "$ip.+reachable" local result=$? if [ "$expect_reacheable" = "true" ] && [ "$result" -eq 0 ]; then return 0 fi if [ "$expect_reacheable" = "false" ] && [ "$result" -ne 0 ]; then return 0 fi sleep 0.2 - timeout=$((timeout - 0.2)) + count=$((count + 1)) donesmoke/_init.sh (1)
216-227: Fix arithmetic expansion to support fractional timeout increments.Line 226 uses
(( ))arithmetic with decimals, which bash does not support. Convert to integer-based loop counting.frrinit.sh start - timeout=5 elapsed=0 + max_loops=25 + loop_count=0 # wait that zebra_dplane_grout get iface event from grout while ! grep -q "GROUT:.*iface/ip events" "$zlog" 2>/dev/null; do - if [ "$elapsed" -ge "$timeout" ]; then - echo "Zebra is not listening grout event after ${timeout} seconds." + if [ "$loop_count" -ge "$max_loops" ]; then + echo "Zebra is not listening grout event after 5 seconds." exit 1 fi sleep 0.2 - elapsed=$((elapsed + 0.2)) + loop_count=$((loop_count + 1)) done
🧹 Nitpick comments (1)
cli/main.c (1)
149-156: Consider returning directly instead of goto end.At this early point in
main(), no resources have been allocated yet (clientandcmdlistare still NULL,optsis uninitialized). Jumping to the cleanup label works if the cleanup functions handle NULL gracefully, but returningEXIT_FAILUREdirectly would be clearer.Apply this diff if you prefer the more direct approach:
ret = EXIT_FAILURE; if (setvbuf(stdout, NULL, _IOLBF, 0) < 0) { perror("setvbuf(stdout)"); - goto end; + return EXIT_FAILURE; } if (setvbuf(stderr, NULL, _IOLBF, 0) < 0) { perror("setvbuf(stderr)"); - goto end; + return EXIT_FAILURE; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cli/main.c(1 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(5 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip6_rs_ra_test.shsmoke/snat44_test.shsmoke/ip_forward_test.shsmoke/ip6_add_del_test.shsmoke/ipip_encap_test.shsmoke/vlan_forward_test.shsmoke/ip_builtin_icmp_test.shsmoke/srv6_test.shsmoke/nexthop_ageing_test.shsmoke/_init_frr.shsmoke/ip_add_del_test.shsmoke/srv6_frr_test.shsmoke/ip6_same_peer_test.shsmoke/ip6_forward_frr_test.shsmoke/ip_forward_frr_test.shsmoke/cross_vrf_forward_frr_test.shsmoke/dnat44_test.shsmoke/vrf_forward_test.shsmoke/_init.shsmoke/ip6_forward_test.shsmoke/vrf_forward_frr_test.shsmoke/cross_vrf_forward_test.shsmoke/ip6_builtin_icmp_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/main.c
🧬 Code graph analysis (20)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/snat44_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/srv6_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(122-132)fail(87-90)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-119)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(122-132)netns_add(98-119)llocal_addr(134-136)fail(87-90)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-119)llocal_addr(134-136)
smoke/ip_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-119)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-119)
smoke/dnat44_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(122-132)netns_add(98-119)llocal_addr(134-136)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-119)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
smoke/ip6_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(122-132)netns_add(98-119)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 23-23: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_same_peer_test.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 20-20: Quote this to prevent word splitting.
(SC2046)
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_frr_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 226-226: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_forward_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
🔇 Additional comments (38)
smoke/nexthop_ageing_test.sh (1)
36-38: LGTM: Helper function usage is correct.Proper invocation of
port_addandnetns_addwith VLAN IDs in the established pattern.smoke/_init.sh (3)
134-136: LGTM: Helper function is well-designed.The
llocal_addr()function correctly extracts link-local IPv6 addresses fromgrcli address showoutput.
98-119: LGTM: Network namespace setup is comprehensive.The
netns_add()function properly configures IPv6 sysctl options, creates veth pairs, sets up VLAN tagging, and registers cleanup commands. Good defensive setup.
163-168: LGTM: TTY-aware colored output improves debugging.Conditional colored xtrace output when stdout is a TTY enhances readability during test execution without affecting CI pipelines.
smoke/ip_add_del_test.sh (1)
9-9: LGTM: MAC parameter removal aligns with port abstraction.Removal of the MAC parameter is consistent with the shift to
port_add()helper, which now derives MAC addresses based on interface name. Address lifecycle tests remain unaffected.smoke/ipip_encap_test.sh (2)
11-12: LGTM: Port creation with VLAN isolation is correct.
port_addinvocations with distinct VLAN IDs (400 vs 401) ensure proper bridge VLAN filtering for tunnel endpoints.
18-27: LGTM: Namespace and tunnel setup follows expected pattern.Network namespace creation with matching VLAN IDs followed by IPIP tunnel configuration within each namespace is logically sound.
smoke/ip_builtin_icmp_test.sh (2)
10-17: LGTM: Loop-based port and namespace provisioning is clean.The loop correctly chains
port_add→grcli address add→netns_addwith corresponding VLAN IDs, followed by in-namespace addressing and routing.
18-18: Clarify intent of extra address assignment.Line 18 adds a second address to
$p0after the loop. Confirm this is intentional test coverage rather than an oversight (e.g., testing multi-address behavior).smoke/vlan_forward_test.sh (2)
26-27: LGTM: Ping tests validate cross-VLAN connectivity.The two pings confirm bidirectional connectivity across VLAN-isolated namespaces.
14-24: Clarify bridge VLAN operations sequence.Lines 17–18 delete and immediately re-add the same VLAN on the port. Verify this is intentional (e.g., resetting VLAN state) versus redundant—typically, a single
bridge vlan addshould suffice afterport_addhas configured the default VLAN.smoke/ip6_add_del_test.sh (1)
9-11: LGTM: IPv6 interface creation aligns with refactored pattern.Removal of MAC parameters from both interface additions is consistent with the port abstraction layer now handling MAC derivation and assignment.
smoke/ip_forward_test.sh (2)
10-22: LGTM: Loop-based provisioning with port and namespace helpers is clean.The loop correctly chains port creation, address assignment, and namespace provisioning with matching VLAN IDs, followed by in-namespace configuration.
24-26: LGTM: Centralized routing via grcli route/nexthop improves test clarity.Moving route and nexthop definitions outside the setup loop makes the routing topology explicit and easier to verify.
smoke/srv6_test.sh (1)
10-19: LGTM!The migration to helper functions is clean. Loop correctly creates ports and namespaces with proper VLAN assignments, and address configuration is appropriately deferred.
smoke/dnat44_test.sh (1)
10-26: LGTM!Loop properly sets up ports and namespaces. Address configuration and routes are correct for the DNAT test. The added ping check validates reachability through the configured DNAT rules.
smoke/snat44_test.sh (1)
10-26: LGTM!Port and namespace setup is correct. While split across sequential and loop invocations (intentional for this test), VLAN assignments remain consistent. Namespace and per-namespace address configurations are proper.
smoke/cross_vrf_forward_test.sh (1)
10-20: LGTM!VRF parameter is correctly passed to
port_add. Dynamic VLAN and network calculations are sound. Per-namespace setup is complete and properly scoped within the loop.smoke/ip6_rs_ra_test.sh (1)
11-16: LGTM!Straightforward refactoring to helper functions. Single port and namespace setup with consistent VLAN values. Address configuration across both sides is correct.
smoke/vrf_forward_test.sh (1)
12-36: LGTM!Dual-loop VRF setup is correctly structured. Variable
xproperly indexes network subnets per node, and VRF isolation ensures address reuse is safe. Consistent pattern between both VRF groups.smoke/cross_vrf_forward_frr_test.sh (2)
10-21: LGTM!FRR helpers (
create_interface,set_ip_address,set_vrf_iface) are properly invoked with correct argument passing. VRF calculation and namespace setup follow the established patterns.
31-32: LGTM!Updated ping tests correctly reference the redefined namespace names (
$p0and$p1). Namespace variable scope is maintained.smoke/ip6_builtin_icmp_test.sh (2)
10-17: LGTM!Loop correctly instantiates ports and namespaces with dynamic IPv6 addressing. VLAN values match between helpers. Route coverage via
/62properly spans both.0and.1subnets.
19-24: LGTM!Ping tests are properly structured. Failure assertions correctly use
&& failto validate that unreachable hosts are unreachable.smoke/ip_forward_frr_test.sh (2)
7-22: Refactoring to new helper functions looks correct.Setup flow is clean:
create_interface→set_ip_address→netns_addfollowed by namespace-scoped address/route config. Variable naming ($p0/$p1) is consistent with loop-based setup.
27-36: Namespace references updated correctly.All test invocations use
ip netns exec $p0/ip netns exec $p1matching the new naming scheme, with millisecond intervals (-i0.01) preserved from original.smoke/_init_frr.sh (2)
8-23: Correct delegation pattern forcreate_interface.Function now shifts the interface name from positional args and forwards remaining arguments to
port_add. The error handling change from direct exit tofail()call is appropriate. Retry timing reduced from 1.0s to 0.2s for performance.
25-56: Consistent timing updates across all polling loops.All retry intervals reduced from 1.0s to 0.2s (lines 53, 116, 170, 233). Uniform change improves test suite execution without affecting retry count (max_tries=5 unchanged).
Also applies to: 79-119, 121-173, 175-235
smoke/ip6_forward_frr_test.sh (2)
10-22: IPv6 setup follows the refactored pattern correctly.Sequential helper invocation (create_interface → netns_add → set_ip_address) matches the refactoring design. IPv6 addressing (fd00:ba4:$n::/64 for bridges, fd00:f00:$n::/64 for loopback) is appropriate.
27-28: Dynamic link-local address resolution viallocal_addrimproves maintainability.Using
$(llocal_addr $p1)/$(llocal_addr $p2)eliminates hardcoded fe80:: addresses; helper extracts actual link-local from FRR/grcli. Shellcheck SC2046 warnings on word splitting are benign here—command substitution must remain unquoted for proper field splitting.Also applies to: 35-38
smoke/ip6_forward_test.sh (2)
12-25: Correct use ofport_addandgrclihelpers for non-FRR test.Differs appropriately from FRR variant: uses
port_adddirectly and manages routes viagrclirather thanset_ip_route. Loop-based setup reduces code duplication.
27-28: Dynamic link-local address resolution consistent with other IPv6 tests.Shellcheck SC2046 on word splitting is expected and correct here.
smoke/ip6_same_peer_test.sh (2)
10-17: Setup simplified compared to earlier versions; uses consistent helper pattern.Loop-based port and namespace creation via
port_addandnetns_addis clean. Removed redundant per-node manual configuration.
19-21: Guard clause on line 21 is intentional but could be clearer.The pattern
ping6 ... && fail "..."succeeds only if ping6 unexpectedly succeeds to the foreign link-local address. If ping6 fails as expected, the fail() is skipped. This is correct logic but could benefit from a clarifying comment in the script about expected behavior.Confirm that the
fail()function properly marks the test as failed in your test harness (or if you'd like, I can verify by checking the _init.sh implementation).smoke/srv6_frr_test.sh (2)
10-19: SRv6 interface and namespace setup follows refactored pattern.Loop-based setup with
create_interface→netns_addis clean. Namespace-scoped addresses via-n $pXappropriately target the veth peers.
36-54: SRv6-specific configuration correctly targeted to namespaces.Sysctls (seg6_enabled, forwarding) and encap/decap routes all use
ip netns exec $pXorip -n $pXas appropriate. Mix of set_* helpers (set_srv6_route, set_srv6_localsid) and direct ip commands is correct (FRR helpers handle global routing; ip commands handle namespace-local SRv6 policy).smoke/vrf_forward_frr_test.sh (2)
12-34: VRF-based refactoring correctly uses parameterized helpers and modulo addressing.Two loops implement two isolated VRFs: ports 0/1 in VRF 1, ports 2/3 in VRF 2. Modulo arithmetic (lines 14, 26) correctly maps 0/2 and 1/3 to the same address ranges (0, 1) without conflict due to VRF isolation.
create_interfaceandset_ip_routecorrectly receive vrf parameters (lines 15, 27, 20, 32). Loop-based approach eliminates redundant per-node configuration.
36-39: Test invocations correctly target cross-VRF connectivity.Ping tests from p0 (VRF 1) reach p1 (same VRF) and p2 (VRF 2) via routing. Pattern demonstrates proper VRF isolation and inter-VRF routing, validating the refactored setup.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smoke/nexthop_ageing_test.sh (1)
11-23: Fix decimal arithmetic in bash—usebcor track timeout differently.Bash's
(( ))arithmetic doesn't support decimals, so line 23 (timeout=$((timeout - 0.2))) will fail or produce unexpected behavior. The timeout variable won't decrement properly on each 0.2-second sleep.Consider using
bcfor decimal arithmetic or rewrite with integer milliseconds:-check_nexthop() { - local ip="$1" - local expect_reacheable="$2" - local timeout=5 - while [ "$timeout" -gt 0 ]; do +check_nexthop() { + local ip="$1" + local expect_reacheable="$2" + local timeout=50 # 50 × 0.1s = 5s + while [ "$timeout" -gt 0 ]; do grcli nexthop show internal | grep -qE "$ip.+reachable" local result=$? if [ "$expect_reacheable" = "true" ] && [ "$result" -eq 0 ]; then return 0 fi if [ "$expect_reacheable" = "false" ] && [ "$result" -ne 0 ]; then return 0 fi - sleep 0.2 - timeout=$((timeout - 0.2)) + sleep 0.1 + timeout=$((timeout - 1)) done return 1 }Alternatively, use
bc:- timeout=$((timeout - 0.2)) + timeout=$(bc <<< "$timeout - 0.2")smoke/_init.sh (1)
214-224: Fix decimal arithmetic in timeout calculation—bash(( ))doesn't support decimals.Line 224 (
elapsed=$((elapsed + 0.2))) will fail; bash arithmetic only handles integers. Theelapsedcounter will not increment properly, breaking the zebra readiness timeout.Use integer arithmetic instead:
- timeout=5 + timeout=50 # 50 × 0.1s = 5s elapsed=0 # wait that zebra_dplane_grout get iface event from grout while ! grep -q "GROUT:.*iface/ip events" "$zlog" 2>/dev/null; do if [ "$elapsed" -ge "$timeout" ]; then echo "Zebra is not listening grout event after ${timeout} seconds." exit 1 fi - sleep 0.2 - elapsed=$((elapsed + 0.2)) + sleep 0.1 + elapsed=$((elapsed + 1)) done
♻️ Duplicate comments (1)
smoke/_init.sh (1)
120-130: Separate variable declaration and command substitution to capture return value.Line 124 combines
localwith command substitution, masking the return value ofmd5sum. Per prior review feedback, split the declaration and assignment.tap_counter=0 port_add() { local name="$1" local vlan="$2" shift 2 - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac="$mac_output" grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" ip link set "$name" master "$bridge" address "$mac"
🧹 Nitpick comments (2)
smoke/ip_add_del_test.sh (1)
9-9: Consider using port_add helper for consistency.Unlike other smoke tests in this PR, this file still uses raw
grcli interface addinstead of theport_addhelper. If this test specifically validates the low-level interface command, this is fine. Otherwise, consider aligning with the pattern used in other tests.smoke/vlan_forward_test.sh (1)
9-10: Remove dead code: pre-computed VLAN names.Lines 9–10 define
v0andv1, but these are recomputed and shadowed by lines 14–15 in the loop. Remove the unused pre-definitions.-v0=$p0.42 -v1=$p1.43 - for n in 0 1; do
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cli/main.c(1 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(5 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (9)
- smoke/srv6_test.sh
- smoke/vrf_forward_frr_test.sh
- smoke/srv6_frr_test.sh
- smoke/cross_vrf_forward_frr_test.sh
- smoke/ip6_builtin_icmp_test.sh
- smoke/ip6_add_del_test.sh
- smoke/_init_frr.sh
- smoke/dnat44_test.sh
- smoke/snat44_test.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip_builtin_icmp_test.shsmoke/ip6_rs_ra_test.shsmoke/vrf_forward_test.shsmoke/nexthop_ageing_test.shsmoke/ip_forward_test.shsmoke/ip6_forward_frr_test.shsmoke/ip_forward_frr_test.shsmoke/vlan_forward_test.shsmoke/ipip_encap_test.shsmoke/ip_add_del_test.shsmoke/_init.shsmoke/cross_vrf_forward_test.shsmoke/ip6_forward_test.shsmoke/ip6_same_peer_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/main.c
🧬 Code graph analysis (12)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(132-134)
smoke/ip_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/vlan_forward_test.sh (2)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)modules/infra/cli/port.c (1)
port_add(124-151)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-130)netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-130)netns_add(98-117)llocal_addr(132-134)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-130)netns_add(98-117)llocal_addr(132-134)fail(87-90)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 23-23: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_forward_frr_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 124-124: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 224-224: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_forward_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_same_peer_test.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 20-20: Quote this to prevent word splitting.
(SC2046)
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (15)
smoke/ip_forward_test.sh (1)
10-27: LGTM! Clean refactoring to helper-based setup.The refactoring successfully adopts
port_addandnetns_addhelpers, separating infrastructure provisioning from routing configuration. VLAN IDs and addressing are consistent.smoke/ipip_encap_test.sh (1)
11-23: LGTM! Consistent conversion to helper functions.The switch from raw
grcli interface addtoport_addand the addition ofnetns_addcalls align with the PR's refactoring goals. IPIP tunnel configuration remains intact.smoke/ip_builtin_icmp_test.sh (1)
10-18: LGTM! Proper use of new helpers.The loop-based setup with
port_addandnetns_addis clean and consistent. The additional address assignment top0after the loop is appropriate for the test's requirements.smoke/ip6_rs_ra_test.sh (1)
11-16: LGTM! Simplified setup for single-namespace test.The conversion to
port_addandnetns_addis appropriate. The simplified linear flow is clearer than the previous per-iteration approach.smoke/cross_vrf_forward_test.sh (1)
10-20: LGTM! Clean VRF-aware setup.The loop-based provisioning with VRF assignment via
port_addis well-structured. Thevrf $((n + 1))computation correctly assigns VRFs 1 and 2.smoke/vrf_forward_test.sh (1)
12-36: LGTM! Well-structured dual-VRF setup.The two loops cleanly separate VRF 1 and VRF 2 provisioning. The
x = n % 2computation correctly alternates addressing between 172.16.0.* and 172.16.1.* networks within each VRF.smoke/nexthop_ageing_test.sh (1)
36-41: Consistent use of new provisioning helpers.Port setup and namespace configuration correctly use the new
port_add,grcli address add, andnetns_addhelpers for streamlined provisioning.smoke/ip6_same_peer_test.sh (1)
10-27: Correct adoption of new provisioning helpers.Port and namespace setup correctly uses
port_add,grcli address add, andnetns_addhelpers. Thellocal_addrdynamic address lookup is appropriate for link-local reachability tests, and the fail condition on line 21 (rejecting foreign link-local responses) is a good validation step.smoke/ip_forward_frr_test.sh (1)
12-36: Correct adoption of FRR-aware helpers.
create_interfaceproperly wraps port creation with FRR interface readiness polling. Conditional address assignment (lines 16–20) is appropriate. All namespace operations correctly reference$p0and$p1variables, andset_ip_routeensures routes are propagated correctly via FRR.smoke/ip6_forward_frr_test.sh (1)
10-38: Correct FRR and IPv6 helper adoption.
create_interfaceandnetns_addare properly applied. Address ordering is sensible: in-namespace addresses first (lines 14–20), then interface configuration viaset_ip_address(line 21). Route setup viaset_ip_routeis correct. Link-local address retrieval viallocal_addris appropriate for connectivity tests.smoke/ip6_forward_test.sh (1)
10-38: Correct helper adoption and grcli route management.Port and namespace setup correctly uses new helpers. The shift from namespace-local
ip route(which would configure Linux kernel routing) togrcli route add/grcli nexthop add(lines 23–25) is appropriate for testing grout's internal routing. Link-local address retrieval viallocal_addris correct.smoke/vlan_forward_test.sh (1)
12-24: Correct VLAN provisioning with new helpers.Dynamic VLAN naming (
vlan=$((n + 42))) ensures unique VLAN IDs. Bridge VLAN configuration is correct.grcli interface add vlanproperly creates tagged interfaces. Address setup—with the VLAN interface getting a modulo-2 address and the namespace interface getting a unique per-namespace address—supports cross-VLAN testing.netns_addcorrectly receives VLAN context.smoke/_init.sh (3)
98-134: Well-structured provisioning helpers.The new
netns_add(),port_add(), andllocal_addr()functions provide consistent abstractions for namespace/port provisioning. VLAN tagging, IPv6 settings, and cleanup registration are correct. (Address the SC2155 issue noted above in the preceding comment.)
161-212: Excellent TTY-aware colored output.Conditional TTY detection and colorized output (xtrace in cyan, grout logs in blue, events in yellow, zebra logs in magenta) significantly improves log readability during test execution. Fallback to plain output when not a TTY is appropriate.
228-235: Bridge setup with VLAN filtering.Bridge creation with
vlan_filtering 1, VLAN 1 self-bridge disable, and isolated mode is correct for segmenting test traffic. Cleanup registration ensures bridge and FDB state are removed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smoke/nexthop_ageing_test.sh (1)
11-23: Bash arithmetic doesn't support decimals—loop will fail.Lines 23 use
$((timeout - 0.2))but bash arithmetic(( ))only handles integers. The timeout never decrements, breaking the loop logic.Replace with bc or awk:
- while [ "$timeout" -gt 0 ]; do + while (( $(echo "$timeout > 0" | bc) )); do grcli nexthop show internal | grep -qE "$ip.+reachable" local result=$? if [ "$expect_reacheable" = "true" ] && [ "$result" -eq 0 ]; then return 0 fi if [ "$expect_reacheable" = "false" ] && [ "$result" -ne 0 ]; then return 0 fi sleep 0.2 - timeout=$((timeout - 0.2)) + timeout=$(echo "$timeout - 0.2" | bc) donesmoke/_init.sh (1)
217-227: Bash arithmetic doesn't support decimals—timeout loop is broken.Line 227 uses
$((elapsed + 0.2))but bash arithmetic only handles integers. The elapsed time never advances correctly.Replace with bc or awk:
- timeout=5 - elapsed=0 + timeout=5 + elapsed=0 # wait that zebra_dplane_grout get iface event from grout while ! grep -q "GROUT:.*iface/ip events" "$zlog" 2>/dev/null; do - if [ "$elapsed" -ge "$timeout" ]; then + if (( $(echo "$elapsed >= $timeout" | bc) )); then echo "Zebra is not listening grout event after ${timeout} seconds." exit 1 fi sleep 0.2 - elapsed=$((elapsed + 0.2)) + elapsed=$(echo "$elapsed + 0.2" | bc) done
♻️ Duplicate comments (1)
smoke/_init.sh (1)
128-128: Separate variable declaration and assignment to capture errors.Combining
localwith command substitution masks the return value of the md5sum pipeline. Split the declaration from assignment to detect failures.port_add() { local name="$1" local vlan="$2" shift 2 grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac="$mac_output" ip link set "$name" master "$bridge" address "$mac"
🧹 Nitpick comments (3)
smoke/ip6_same_peer_test.sh (1)
19-21: Quote command substitutions to prevent word splitting.The shellcheck warnings flag unquoted
$(llocal_addr ...)expansions. Whilellocal_addroutput is typically a single IPv6 address without spaces, quoting command substitutions is defensive and aligns with shell best practices.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"smoke/ip6_forward_frr_test.sh (1)
27-28: Quote command substitutions for defensive shell programming.Lines 27–28 use unquoted
$(llocal_addr ...)in command arguments, triggering SC2046 warnings. While the output is typically a single IPv6 address, quoting is safer and more idiomatic.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/ip6_forward_test.sh (1)
27-28: Quote command substitutions to align with best practices.Similar to other test files, lines 27–28 should quote the
$(llocal_addr ...)command substitutions to prevent potential word-splitting and align with shell best practices.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
modules/infra/control/port.c(0 hunks)smoke/_init.sh(5 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (7)
- smoke/vlan_forward_test.sh
- smoke/ip6_builtin_icmp_test.sh
- smoke/ip_forward_frr_test.sh
- smoke/ip_builtin_icmp_test.sh
- smoke/dnat44_test.sh
- smoke/vrf_forward_test.sh
- smoke/ip6_add_del_test.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/snat44_test.shsmoke/nexthop_ageing_test.shsmoke/srv6_frr_test.shsmoke/ip6_forward_frr_test.shsmoke/ip_forward_test.shsmoke/cross_vrf_forward_test.shsmoke/vrf_forward_frr_test.shsmoke/ip_add_del_test.shsmoke/_init_frr.shsmoke/ip6_forward_test.shsmoke/cross_vrf_forward_frr_test.shsmoke/srv6_test.shsmoke/ip6_rs_ra_test.shsmoke/ipip_encap_test.shsmoke/_init.shsmoke/ip6_same_peer_test.sh
🧬 Code graph analysis (14)
smoke/snat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-117)
smoke/srv6_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 23-23: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_forward_frr_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 227-227: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_same_peer_test.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 20-20: Quote this to prevent word splitting.
(SC2046)
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (15)
smoke/ip_add_del_test.sh (1)
9-9: Consistent with new port_add abstraction.Removing the MAC argument aligns with the port_add helper, which now handles deterministic MAC generation internally.
smoke/cross_vrf_forward_test.sh (1)
10-20: Well-structured loop-based setup.Port/namespace provisioning using helpers is clean. VRF-aware configuration (vrf parameter to port_add) and consistent VLAN IDs (40$n) follow the pattern.
smoke/nexthop_ageing_test.sh (1)
36-38: Helper functions applied correctly.Port/namespace setup using the new helpers is consistent and correct.
smoke/_init.sh (2)
98-137: Well-designed helper abstractions.The
netns_addandport_addfunctions effectively encapsulate namespace/port provisioning with VLAN setup and cleanup handling. Thellocal_addrhelper is a clean addition for extracting link-local addresses.
164-237: TTY-aware colorization and bridge topology setup look solid.Xtrace, grout, events, and zebra log coloring are conditional on TTY presence. Bridge VLAN filtering, cleanup handling, and reduced polling intervals (0.2s) are sensible optimizations.
smoke/ipip_encap_test.sh (1)
11-12: Consistent helper usage.Port and namespace setup using the new abstractions is correct. VLAN alignment and per-namespace tunnel configuration follow the pattern.
Also applies to: 18-18, 22-22
smoke/ip_forward_test.sh (1)
10-22: Clean loop-based port/namespace provisioning.Helper usage is consistent; grcli-based addressing and routing replace manual commands, improving readability.
smoke/srv6_test.sh (1)
10-19: Helpers applied correctly to SRv6 test setup.Port/namespace initialization uses the new abstractions; dual-sided addressing (grcli and Linux) is properly maintained.
smoke/snat44_test.sh (1)
10-11: Consistent VLAN and helper usage.Port/namespace setup with matching VLAN IDs (400/401 and 40$n) is correct; pattern aligns with other tests.
Also applies to: 19-22
smoke/srv6_frr_test.sh (1)
10-20: Good integration of helpers with proper parameter flow.The refactoring correctly adopts
create_interfaceandnetns_addhelpers, maintaining the VLAN numbering (40$n) and address configuration patterns consistently. Namespace naming transitions from n-$p to $p cleanly.smoke/vrf_forward_frr_test.sh (1)
12-33: Well-structured loop-driven VRF setup.The dual-loop pattern with x computation and VRF parameterization cleanly separates the two VRF contexts. Helper delegation to
create_interfaceandset_ip_routeis correct, with proper handling of VRF IDs (1 and 2).smoke/cross_vrf_forward_frr_test.sh (2)
17-17: Verify necessity of explicit link-up in namespace.Line 17 sets the namespace interface up explicitly. Confirm whether
netns_add(which callsip -n "$ns" link set "$ns" up) makes this redundant, or if this is needed due to ordering withset_vrf_ifaceor other VRF configuration steps.
24-29: Cross-VRF route setup is correct.The use of
set_ip_routewith explicit nexthop VRF IDs andvrf_name_from_idfor VRF name resolution is appropriate for the cross-VRF forwarding pattern.smoke/_init_frr.sh (2)
8-23: Clean refactoring ofcreate_interfacedelegation pattern.The shift to delegating argument passing to
port_addvia shift and "$@" is correct and flexible. Error handling improvement (usingfailinstead ofexit 1) improves consistency with the codebase. Sleep reduction from 1s to 0.2s supports the PR goal of faster test execution.
53-53: Consistent polling interval reduction improves test performance.All wait loops now use 0.2s sleep intervals instead of 1s, reducing test latency proportionally while maintaining functional correctness.
Also applies to: 116-116, 170-170, 233-233
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smoke/_init.sh (1)
217-228: Decimal arithmetic in$(())` expansion will fail.Line 227 attempts
timeout=$((timeout - 0.2)), but POSIX arithmetic expansions don't support decimals. This will cause a syntax error. Useawkorbcinstead, or track in integer milliseconds and adjust the comparison logic.- timeout=5 - elapsed=0 + timeout=50 # 5 seconds in 0.1-second units + elapsed=0 # wait that zebra_dplane_grout get iface event from grout while ! grep -q "GROUT:.*iface/ip events" "$zlog" 2>/dev/null; do - if [ "$elapsed" -ge "$timeout" ]; then + if [ "$elapsed" -ge 5000 ]; then # 5 seconds in milliseconds echo "Zebra is not listening grout event after ${timeout} seconds." exit 1 fi sleep 0.2 - elapsed=$((elapsed + 0.2)) + elapsed=$((elapsed + 1)) donesmoke/nexthop_ageing_test.sh (1)
10-24: Decimal arithmetic will fail in line 23.The expression
timeout=$((timeout - 0.2))uses a decimal in POSIX arithmetic, which is not supported. Use integer-based timing (e.g., milliseconds or tenths of seconds) and adjust the loop condition accordingly.check_nexthop() { local ip="$1" local expect_reacheable="$2" - local timeout=5 + local timeout=50 # 5 seconds in 0.1-second units while [ "$timeout" -gt 0 ]; do grcli nexthop show internal | grep -qE "$ip.+reachable" local result=$? @@ -18,7 +18,7 @@ check_nexthop() { fi sleep 0.2 - timeout=$((timeout - 0.2)) + timeout=$((timeout - 1)) done return 1
♻️ Duplicate comments (1)
smoke/_init.sh (1)
128-128: Separate variable declaration and command substitution to capture return value.Line 128 combines
localdeclaration with command substitution, masking the return value of the pipeline. Ifmd5sumorsedfails, the error goes undetected. Split the declaration from the assignment and validate the result before use.port_add() { local name="$1" local vlan="$2" shift 2 grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac + mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + if [ -z "$mac" ]; then + fail "Failed to derive MAC address for $name" + return 1 + fi ip link set "$name" master "$bridge" address "$mac"
🧹 Nitpick comments (3)
smoke/ip6_same_peer_test.sh (1)
19-21: Quote thellocal_addrcommand substitution to prevent word splitting.Lines 19–21 use unquoted
$(llocal_addr $pN), which could cause unexpected word splitting if the address parsing produces multi-word output. Add quotes for safety.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"smoke/ip6_forward_test.sh (1)
27-28: Quote thellocal_addrcommand substitution.Lines 27–28 use unquoted
$(llocal_addr $pN). Add quotes for consistency with shell best practices.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/ip6_forward_frr_test.sh (1)
27-28: Quote thellocal_addrcommand substitution.Lines 27–28 use unquoted
$(llocal_addr $pN). Add quotes for consistency.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
smoke/_init.sh(5 hunks)smoke/_init_frr.sh(5 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- smoke/ip6_builtin_icmp_test.sh
- smoke/srv6_test.sh
- smoke/ip6_rs_ra_test.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/nexthop_ageing_test.shsmoke/srv6_frr_test.shsmoke/_init.shsmoke/ip6_forward_test.shsmoke/_init_frr.shsmoke/ip6_same_peer_test.shsmoke/ip6_forward_frr_test.sh
🧬 Code graph analysis (6)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (1)
create_interface(8-23)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
smoke/_init_frr.sh (2)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)modules/infra/cli/port.c (1)
port_add(124-151)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 23-23: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 227-227: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_forward_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_same_peer_test.sh
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
[warning] 20-20: Quote this to prevent word splitting.
(SC2046)
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_frr_test.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: deb
- GitHub Check: rpm
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
🔇 Additional comments (9)
smoke/_init_frr.sh (2)
8-23: Clean refactor delegating interface creation toport_add.The shift to using
failinstead ofexitfor error handling is appropriate and consistent with thefailhelper defined in _init.sh. Sleep reduction from 1s to 0.2s aligns with the PR objective to reduce test execution time.
53-53: Timing reduction across polling loops is consistent with PR objectives.The 1s → 0.2s sleep reduction throughout synchronization loops is applied uniformly and supports faster test execution without altering retry logic.
Also applies to: 116-116, 170-170, 226-226, 233-233
smoke/_init.sh (2)
164-169: Clean TTY-aware colored logging implementation.The conditional color formatting for xtrace, grout logs, events, and Zebra logs is properly gated on TTY detection and won't interfere with non-interactive test runs. Using BASH_XTRACEFD for xtrace coloring is idiomatic.
Also applies to: 180-186, 199-204, 210-215
231-238: Bridge setup with VLAN filtering is well-structured.The bridge topology initialization, VLAN filtering enablement, and cleanup are properly ordered. The isolation and VLAN 1 self-filtering align with the bridge topology refactor objective.
smoke/srv6_frr_test.sh (1)
10-60: Consistent and correct use of new helpers.The test properly uses
create_interfaceandnetns_addhelpers with the new namespace naming scheme, and all namespace-scoped operations are correctly prefixed withip -n. The refactor successfully migrates from low-level netns wiring to the helper-based model.smoke/ip6_same_peer_test.sh (1)
10-17: Correct helper-based setup with proper address layering.The separation of Grout-side addresses (via
grcli address add) and namespace-side addresses (viaip) is properly structured and aligns with the new test infrastructure.smoke/nexthop_ageing_test.sh (1)
34-41: Consistent helper-based setup for nexthop testing.The per-node initialization uses
port_add,grcli address add, andnetns_addcorrectly, establishing both Grout-side and namespace-side addressing for nexthop verification.smoke/ip6_forward_test.sh (1)
10-25: Clean per-node setup with proper routing hierarchy.The loop correctly establishes Grout addresses via
grcli, namespace addresses viaip, and properly defers grcli-level route and nexthop configuration to avoid per-iteration redundancy. The per-node loopback branching logic for fd00:f00 is clear.smoke/ip6_forward_frr_test.sh (1)
10-22: Proper use of FRR-aware helpers with namespace scoping.The test correctly uses
create_interface(which delegates toport_addand waits for FRR registration) alongsidenetns_add, followed by FRR address configuration viaset_ip_address. The sequencing ensures FRR visibility before routing setup.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smoke/nexthop_ageing_test.sh (1)
10-27: Usebcorawkfor decimal arithmetic; bash(( ))only supports integers.Line 23 attempts to decrement by 0.2, but bash arithmetic doesn't support decimals.
check_nexthop() { local ip="$1" local expect_reacheable="$2" local timeout=5 - while [ "$timeout" -gt 0 ]; do + while (( $(echo "$timeout > 0" | bc) )); do grcli nexthop show internal | grep -qE "$ip.+reachable" local result=$? if [ "$expect_reacheable" = "true" ] && [ "$result" -eq 0 ]; then return 0 fi if [ "$expect_reacheable" = "false" ] && [ "$result" -ne 0 ]; then return 0 fi sleep 0.2 - timeout=$((timeout - 0.2)) + timeout=$(echo "$timeout - 0.2" | bc) done return 1 }smoke/_init.sh (1)
217-228: Usebcorawkfor decimal arithmetic; bash(( ))only supports integers.Line 227 attempts to add 0.2 using bash arithmetic, which truncates decimals. The elapsed timeout will not decrement correctly.
frrinit.sh start timeout=5 elapsed=0 # wait that zebra_dplane_grout get iface event from grout while ! grep -q "GROUT:.*iface/ip events" "$zlog" 2>/dev/null; do if [ "$elapsed" -ge "$timeout" ]; then echo "Zebra is not listening grout event after ${timeout} seconds." exit 1 fi sleep 0.2 - elapsed=$((elapsed + 0.2)) + elapsed=$(echo "$elapsed + 0.2" | bc) done fi
♻️ Duplicate comments (1)
smoke/_init.sh (1)
119-133: Separate variable declaration and command substitution to capture return value.Line 128 combines
localdeclaration with command substitution, masking the return value of the pipeline. If themd5sumorsedinvocation fails,$maccould silently be empty, leading to incorrect MAC address assignment.tap_counter=0 port_add() { local name="$1" local vlan="$2" shift 2 grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" - # Ensure the Linux net device has a different mac address from grout's. - # This is required to avoid Linux from wrongfully assuming the packets - # sent by grout originated locally. - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + # Ensure the Linux net device has a different mac address from grout's. + # This is required to avoid Linux from wrongfully assuming the packets + # sent by grout originated locally. + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + if [ -z "$mac_output" ]; then + fail "Failed to generate MAC address from $name" + return 1 + fi + local mac="$mac_output" ip link set "$name" master "$bridge" address "$mac" bridge vlan del dev "$name" vid 1 bridge vlan add dev "$name" vid "$vlan" pvid untagged tap_counter=$((tap_counter + 1)) }
🧹 Nitpick comments (1)
smoke/snat44_test.sh (1)
10-11: Pattern inconsistency with other tests.port_addcalls happen outside the loop (lines 10–11), whilenetns_addhappens inside the loop (lines 19–22). Other tests (e.g.,dnat44_test.sh) unify both in a single loop. Consider refactoring for consistency:-port_add $p0 400 -port_add $p1 401 grcli address add 172.16.0.1/24 iface $p0 grcli address add 10.99.0.1/24 iface $p1 grcli snat44 add interface $p0 subnet 10.99.0.0/24 replace 172.16.0.1 grcli snat44 show grcli conntrack show grcli conntrack config set max 1024 closed-timeout 2 for n in 0 1; do p=$run_id$n + port_add $p 40$n netns_add $p 40$n doneAlso applies to: 19-22
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
modules/infra/control/port.c(0 hunks)smoke/_init.sh(5 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (7)
- smoke/cross_vrf_forward_frr_test.sh
- smoke/ip6_rs_ra_test.sh
- smoke/ip_forward_frr_test.sh
- smoke/ipip_encap_test.sh
- smoke/ip6_add_del_test.sh
- smoke/srv6_test.sh
- smoke/ip_add_del_test.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/nexthop_ageing_test.shsmoke/dnat44_test.shsmoke/ip_builtin_icmp_test.shsmoke/ip_forward_test.shsmoke/srv6_frr_test.shsmoke/vrf_forward_frr_test.shsmoke/ip6_same_peer_test.shsmoke/ip6_builtin_icmp_test.shsmoke/vrf_forward_test.shsmoke/_init_frr.shsmoke/cross_vrf_forward_test.shsmoke/vlan_forward_test.shsmoke/_init.shsmoke/snat44_test.shsmoke/ip6_forward_frr_test.shsmoke/ip6_forward_test.sh
🧬 Code graph analysis (15)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/dnat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (1)
create_interface(8-23)smoke/_init.sh (1)
netns_add(98-117)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
smoke/ip6_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/snat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 23-23: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 227-227: (( )) doesn't support decimals. Use bc or awk.
(SC2079)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
🔇 Additional comments (13)
smoke/ip_builtin_icmp_test.sh (1)
12-18: Looks good. Pattern is clean:port_add→grcli address→netns_add→ in-namespace setup. VLAN IDs and address assignments follow the intended structure.smoke/ip_forward_test.sh (1)
12-26: LGTM. Standard port/namespace setup followed by route and nexthop configuration. Sequencing correct.smoke/dnat44_test.sh (1)
10-26: LGTM. Port and namespace created in loop, DNAT config applied, in-namespace setup correct.smoke/vrf_forward_test.sh (1)
12-36: LGTM. VRF-driven setup with parameterized loops. Address mapping viax=$((n % 2))correctly reuses addressing for VRF separation. Routes via peer namespace addresses are intentional for the test.smoke/srv6_frr_test.sh (1)
12-19: LGTM. Consistent use ofcreate_interface+netns_addwith proper FRR polling. Namespace context correctly unified to$pX(notn-$pX). SRv6 routes and policies use correct namespaces.Also applies to: 38-64
smoke/vrf_forward_frr_test.sh (1)
12-39: LGTM. Dual-loop VRF setup withcreate_interface+netns_add. Address mapping viax=$((n % 2))correct. FRR helpers called with appropriate VRF ID. Test namespace context correctly unified to$p0/$p2.smoke/cross_vrf_forward_test.sh (1)
10-20: Well-structured refactor to port/netns topology.The loop correctly uses the new helpers with appropriate VRF and VLAN assignments. The VRF context is properly propagated to
port_add.smoke/ip6_same_peer_test.sh (1)
21-23: Quote command substitutions to prevent word splitting.While link-local IPv6 addresses are unlikely to split, quoting is defensive and aligns with best practices.
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"smoke/ip6_forward_frr_test.sh (1)
29-40: Quote command substitutions to prevent word splitting.Lines 29-30 contain unquoted
llocal_addrcommand substitutions.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/_init_frr.sh (1)
8-23: Clean delegation toport_addhelper.The refactored
create_interfacecorrectly delegates port setup and simplifies error handling withfail. Timing consistency (0.2s) across all wait loops is good.smoke/ip6_forward_test.sh (1)
29-30: Quote command substitutions to prevent word splitting.Lines 29-30 contain unquoted
llocal_addrcommand substitutions.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/ip6_builtin_icmp_test.sh (1)
10-19: Clean refactor to port/netns topology.The per-port provisioning is well-structured and aligns with the updated test scaffolding.
smoke/_init.sh (1)
164-238: Excellent foundational improvements for the test suite.The new helpers (
netns_add,port_add,llocal_addr) provide a clean abstraction for port/namespace provisioning. TTY-aware colorization improves observability, and the bridge setup with VLAN filtering enables proper network isolation. The 0.2s polling reduces test execution time significantly.Aside from the critical issues flagged above, verify that the bridge VLAN filtering (lines 231-238) correctly isolates traffic and that the per-VLAN port assignments in helpers maintain proper segmentation.
aharivel
left a comment
aharivel
left a comment
There was a problem hiding this comment.
Mostly refactoring code here and cleanup - just a little questions added
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
smoke/vlan_forward_test.sh (1)
17-18: Remove redundant VLAN operations that overrideport_addconfiguration.As flagged previously,
port_addalready configures the VLAN withpvid untaggedflags (smoke/_init.sh line 131). These lines delete and re-add without those flags, breaking the setup.port_add $p $vlan - bridge vlan del dev $p vid $vlan - bridge vlan add dev $p vid $vlan grcli interface add vlan $v parent $p vlan_id $vlansmoke/_init.sh (1)
128-128: Consider splitting local declaration and command substitution.Line 128 combines
localdeclaration with command substitution, which masks the return value of the md5sum pipeline. While low-risk here, this is a code smell worth addressing in a future cleanup.
🧹 Nitpick comments (1)
smoke/cross_vrf_forward_test.sh (1)
19-19: Remove redundantip -n $p addr show.This informational command has no side effects and aligns with the PR objective to remove redundant operations.
ip -n $p route add default via 172.16.$n.1 - ip -n $p addr show done
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
modules/infra/control/port.c(0 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (3)
- smoke/ip6_builtin_icmp_test.sh
- smoke/ip6_rs_ra_test.sh
- smoke/srv6_frr_test.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip_forward_frr_test.shsmoke/cross_vrf_forward_frr_test.shsmoke/vrf_forward_test.shsmoke/nexthop_ageing_test.shsmoke/ip_forward_test.shsmoke/_init_frr.shsmoke/dnat44_test.shsmoke/_init.shsmoke/ip_builtin_icmp_test.shsmoke/vrf_forward_frr_test.shsmoke/srv6_test.shsmoke/ipip_encap_test.shsmoke/ip6_same_peer_test.shsmoke/vlan_forward_test.shsmoke/snat44_test.shsmoke/cross_vrf_forward_test.shsmoke/ip6_forward_test.shsmoke/ip6_forward_frr_test.sh
🧬 Code graph analysis (17)
smoke/ip_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-117)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/dnat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/srv6_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/snat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
🪛 Shellcheck (0.11.0)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 219-219: timeout is referenced but not assigned.
(SC2154)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (17)
smoke/srv6_test.sh (1)
10-19: Correct refactoring to use port and namespace helpers.Port and namespace creation properly consolidated via
port_addandnetns_add. Address configuration sequencing aligns with the refactored pattern.smoke/ip_forward_test.sh (1)
10-26: Well-structured refactoring with clear separation of concerns.Port/namespace setup properly decoupled from routing configuration. grcli route/nexthop additions are correctly positioned after topology is established.
smoke/ipip_encap_test.sh (1)
11-12: Appropriate use of helpers with correct VLAN ID assignments.Port and namespace creation follows established patterns. Per-namespace tunnel configuration remains properly isolated.
Also applies to: 18-18, 22-22
smoke/ip_builtin_icmp_test.sh (1)
12-18: Proper application of refactoring helpers.Loop-based port/namespace setup with additional address assignment for multi-address testing is appropriate.
smoke/dnat44_test.sh (1)
10-14: Streamlined port and namespace setup.Loop-based creation using helpers eliminates boilerplate while maintaining clarity. Integration with DNAT configuration is clean.
smoke/snat44_test.sh (1)
10-11: Correct helper usage with appropriate VLAN parameterization.Port creation and namespace addition follow established pattern. Visibility checks (grcli show) are well-positioned.
Also applies to: 21-21
smoke/ip6_same_peer_test.sh (2)
10-16: Port and namespace creation follows refactoring pattern correctly.Helper functions are properly applied with consistent VLAN parameterization.
21-23: Quote command substitutions to prevent word splitting.The unquoted
$(llocal_addr ...)calls will fail if the helper returns unexpected output or contains whitespace. Wrap with quotes to ensure robust parsing.Apply this diff to fix the word-splitting vulnerability:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"Likely an incorrect or invalid review comment.
smoke/vrf_forward_test.sh (1)
12-21: Proper VRF-aware refactoring with correct parameterization.Modulo arithmetic correctly distributes addresses across VRFs. VRF parameters are consistently applied to both
port_addand grcli routing commands. Two-loop structure appropriately isolates VRF 1 and VRF 2 configurations.Also applies to: 26-36
smoke/ip6_forward_test.sh (1)
29-30: Quote command substitutions to prevent word splitting.While
llocal_addrreturns an IPv6 address without spaces, shellcheck recommends quoting for defensive programming.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/nexthop_ageing_test.sh (1)
10-24: Poll refactor improves both correctness and performance.The switch from timeout-based polling with decimal arithmetic to an attempts counter is cleaner and avoids the prior arithmetic error. The 5x faster polling (0.2s vs 1s) accelerates test execution while maintaining adequate coverage.
smoke/ip_forward_frr_test.sh (1)
12-22: Helper-based setup is clean and consistent.The refactor to
create_interface,set_ip_address, andset_ip_routehelpers abstracts away complexity while maintaining clarity. Namespace naming with40$nis consistent across the test.smoke/vrf_forward_frr_test.sh (1)
12-34: Parameterized loop structure cleanly separates VRF contexts.The two loops with parameterization via
x = (n % 2)andvrfelegantly handle two separate VRF domains while reusing the same setup pattern. Consistent use of helpers and naming conventions.smoke/ip6_forward_frr_test.sh (1)
29-30: Quote command substitutions to prevent word splitting.Same issue as
ip6_forward_test.sh:llocal_addrreturns an IPv6 address without spaces, but shellcheck recommends quoting for consistency and defensive programming.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/_init_frr.sh (2)
8-23: Helper consolidation improves maintainability.Delegating to
port_addcentralizes interface setup logic. Usingfailinstead ofexitprovides consistent error handling across the test suite. Clean and maintainable.
20-20: Polling accelerated across all wait loops.Sleep intervals reduced from 1s to 0.2s consistently throughout, speeding up test execution 5x while maintaining adequate probe windows. Aligns with PR goal to reduce execution time.
Also applies to: 53-53, 116-116, 170-170, 233-233
smoke/cross_vrf_forward_frr_test.sh (1)
7-32: LGTM!The refactoring to use the new helper functions (create_interface, set_ip_address, set_vrf_iface, netns_add) is clean and the variable scoping is correct. The namespace names created during the loop ($p when n=0 and n=1) properly align with p0 and p1 used in the ping verification commands.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
cli/main.c (1)
149-156: Portability: setvbuf return check flagged in prior review.The existing review comment correctly notes that
setvbufreturns 0 on success and non-zero on failure per POSIX, so the checks should use!= 0rather than< 0to catch all failure cases.smoke/ip6_rs_ra_test.sh (1)
16-16: Redundant command remains despite past review.The
ip -n $p1 addr showcommand at line 16 contradicts the PR objective to remove redundant address display commands. A prior review flagged this and marked it as addressed, but the command is still present.Apply this diff:
-ip -n $p1 addr showsmoke/vlan_forward_test.sh (1)
16-18: Remove redundant bridge VLAN operations after port_add. Lines 17–18 delete and re-add the same VLAN, losing thepvid untaggedflags already set byport_addon line 16.Apply this diff:
port_add $p $vlan - bridge vlan del dev $p vid $vlan - bridge vlan add dev $p vid $vlan grcli interface add vlan $v parent $p vlan_id $vlansmoke/_init.sh (2)
103-106: Fix invalid kill syntax in cleanup script.The
xargs -r kill --timeout 500 KILLcommand uses invalid syntax;killdoesn't accept--timeout. This will cause the cleanup to fail, leaving netns pids running.Apply this diff:
cat >> $tmp/cleanup <<EOF -ip netns pids "$ns" | xargs -r kill --timeout 500 KILL +ip netns pids "$ns" | xargs -r -- kill -KILL ip netns del "$ns"
128-128: Separate variable declaration and command substitution to capture errors.Line 128 combines
localdeclaration with command substitution, which masks the return value of the pipeline. Ifmd5sumorsedfails, the error is hidden.Apply this diff:
- local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac="$mac_output"
🧹 Nitpick comments (3)
smoke/ip6_forward_test.sh (1)
29-30: Quote command substitutions for word-splitting safety. While IPv6 addresses don't contain whitespace, quoting is a defensive coding practice.Apply this diff:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/ip6_forward_frr_test.sh (1)
29-30: Quote command substitutions for consistency and robustness. Same defensive coding practice as elsewhere in the suite.Apply this diff:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/ip6_same_peer_test.sh (1)
21-23: Quote command substitutions for robustness. SC2046 recommends quoting to prevent word splitting.Apply this diff:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cli/main.c(1 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (4)
- smoke/ipip_encap_test.sh
- smoke/dnat44_test.sh
- smoke/ip6_add_del_test.sh
- smoke/ip6_builtin_icmp_test.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip_builtin_icmp_test.shsmoke/srv6_frr_test.shsmoke/ip_forward_frr_test.shsmoke/vrf_forward_frr_test.shsmoke/ip6_forward_test.shsmoke/srv6_test.shsmoke/ip6_same_peer_test.shsmoke/cross_vrf_forward_test.shsmoke/_init.shsmoke/ip6_rs_ra_test.shsmoke/_init_frr.shsmoke/vlan_forward_test.shsmoke/vrf_forward_test.shsmoke/nexthop_ageing_test.shsmoke/ip_forward_test.shsmoke/cross_vrf_forward_frr_test.shsmoke/snat44_test.shsmoke/ip_add_del_test.shsmoke/ip6_forward_frr_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/main.c
🧬 Code graph analysis (17)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
smoke/srv6_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-117)
smoke/snat44_test.sh (2)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)modules/infra/cli/port.c (1)
port_add(123-150)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
🪛 Shellcheck (0.11.0)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (18)
smoke/ip_add_del_test.sh (1)
9-9: LGTM: MAC parameter removed.Correctly removes the MAC address parameter, aligning with the PR objective to eliminate stable MAC requirements.
smoke/srv6_test.sh (1)
10-19: LGTM: port_add/netns_add adoption.The refactoring correctly uses the new helpers with VLAN IDs (40$n) and moves address configuration outside the loop for cleaner per-run provisioning.
smoke/ip_forward_test.sh (1)
10-26: LGTM: clean helper adoption.The port-based provisioning with
port_add/netns_addand deferred route configuration is correct and aligns with the bridge topology refactor.smoke/snat44_test.sh (1)
10-21: LGTM: helper adoption and visibility improvements.Correctly uses
port_addwith explicit VLAN IDs and simplifies namespace setup. The addedsnat44 showandconntrack showcommands improve test observability.smoke/ip_builtin_icmp_test.sh (1)
10-18: LGTM: port-based provisioning.The helper-based setup with
port_add/netns_addis correct. The additional address on p0 (line 18) supports the expanded connectivity tests.smoke/vrf_forward_frr_test.sh (1)
12-39: LGTM: VRF-aware loop-based provisioning.The refactoring correctly uses
x = n % 2for per-pair addressing, delegates tocreate_interface/set_ip_address/set_ip_routehelpers with VRF context, and updates ping tests to execute in the appropriate namespaces.smoke/vrf_forward_test.sh (1)
12-22: Solid refactoring to loop-driven VRF setup. The dual-VRF layout is cleanly parameterized with port_add/netns_add helpers and grcli-based routing. Logic is correct.Also applies to: 26-36
smoke/cross_vrf_forward_test.sh (1)
10-20: Clean loop-driven setup for cross-VRF forwarding. The dynamic VRF assignment and routing configuration align well with the refactoring objectives.smoke/nexthop_ageing_test.sh (2)
10-23: Good fix for polling logic. Switching from floating-point arithmetic timeout to an attempts counter with 0.2s sleep intervals resolves the previous shell arithmetic error and is clearer.
34-40: Solid migration to port_add and netns_add helpers. Setup is consistent with the refactoring pattern.smoke/ip_forward_frr_test.sh (1)
10-22: Correct FRR-integrated refactoring. The create_interface and set_ip_address helpers properly encapsulate FRR configuration with validation polling. Setup is sound.smoke/ip6_forward_test.sh (1)
10-22: Good use of helpers for IPv6 setup. The port_add/netns_add refactoring and grcli-based provisioning are solid. Routing and loopback address conditionals are correct.smoke/ip6_forward_frr_test.sh (1)
10-22: Sound FRR-integrated IPv6 setup. Deferred set_ip_address ensures FRR interface readiness. Helpers are applied correctly.smoke/ip6_same_peer_test.sh (1)
10-17: Good test refactoring with proper failure assertion. The dynamic local address retrieval via llocal_addr and the expected-failure check on line 23 improve test robustness. Setup is clean.smoke/srv6_frr_test.sh (1)
12-19: ✓ Good refactoring to use new helpers.Port and namespace setup correctly uses
create_interfaceandnetns_addfor cleaner initialization. Address configuration properly targets the created interfaces.smoke/_init_frr.sh (1)
8-23: Verify error handling increate_interface.Line 18 uses
fail()which returns 1 but doesn't cause the script to exit. Sinceset -eis enabled in _init.sh, the return status propagates, but confirm this is the intended behavior for test teardown. Ifcreate_interfaceis called in a subshell or the return is not checked, the error could be silently ignored.smoke/cross_vrf_forward_frr_test.sh (1)
10-21: ✓ Clean VRF-aware helper usage.Loop correctly computes VRF IDs and passes them to
create_interface. Address and VRF configuration properly sequenced.smoke/_init.sh (1)
226-233: Verify bridge VLAN and isolation configuration intent.The bridge setup removes default VLAN 1, enables VLAN filtering, and isolates the bridge. Ensure this configuration aligns with the expected forwarding behavior for veth-attached ports and doesn't inadvertently block legitimate traffic (e.g., ARP, multicast). Verify that ports attached via
netns_addandport_addcan still communicate across the bridge.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
smoke/vlan_forward_test.sh (1)
17-18: Remove redundant bridge VLAN operations (duplicate of past review).Lines 17–18 delete and re-add the VLAN immediately after
port_add, which already configured it withpvid untagged. This sequence loses those flags.Apply this diff:
port_add $p $vlan - bridge vlan del dev $p vid $vlan - bridge vlan add dev $p vid $vlan grcli interface add vlan $v parent $p vlan_id $vlansmoke/_init.sh (2)
103-106: The kill command syntax in the cleanup script is still invalid—this was flagged before.
xargs -r kill --timeout 500 KILLwill fail;killdoesn't support--timeout. This prevents PIDs in the namespace from being properly terminated before deletion.Apply this fix:
-ip netns pids "$ns" | xargs -r kill --timeout 500 KILL +ip netns pids "$ns" | xargs -r kill -KILL
119-133: Thelocal mac=$(...)assignment at line 128 masks the return value of the command substitution—this was flagged before.This violates SC2155 and prevents proper error detection if
md5sumor the pipeline fails. The issue remains unfixed from prior review.Separate declaration and assignment:
port_add() { local name="$1" local vlan="$2" shift 2 grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac="$mac_output" ip link set "$name" master "$bridge" address "$mac"
🧹 Nitpick comments (5)
docs/grout.8.md (1)
14-30: Minor: Inconsistent option ordering between SYNOPSIS and OPTIONS.The SYNOPSIS lists options as uppercase-first (B D L M S T V h m o p...) while the OPTIONS section orders them case-insensitive alphabetically (B D h L M m o...). For consistency and maintainability, align both sections to the same ordering convention.
The new CLI options (-S, -T, -V, -h) are well-documented with clear descriptions and examples. The changes align correctly with the CLI enhancements described in the PR.
Also applies to: 32-146
smoke/ip6_same_peer_test.sh (1)
21-23: Quote command substitutions to prevent word splitting.Shellcheck warns (SC2046) that these command substitutions should be quoted. While IPv6 addresses don't contain spaces in practice, quoting is good practice and improves shellcheck compliance.
Apply this diff:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"smoke/ip6_forward_frr_test.sh (1)
29-30: Quote command substitutions for robustness.Lines 29–30 and 38–40 should quote the
$(llocal_addr ...)substitutions to guard against word splitting and globbing.Apply this diff:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"And similarly for lines 38–40:
-ip netns exec $p1 traceroute -N1 -n fd00:ba4:2::2 +ip netns exec $p1 traceroute -N1 -n fd00:ba4:2::2(The traceroute lines don't use llocal_addr, so no change needed there.)
Also applies to: 38-40
smoke/ip6_forward_test.sh (1)
29-30: Quote command substitution for consistency.Same as smoke/ip6_forward_frr_test.sh: lines 29–30 should quote
$(llocal_addr ...)to align with shell best practices.Apply this diff:
-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"main/main.c (1)
34-76: LGTM: usage() refactored correctly.The signature change from
usage(const char *prog)tousage(void)is appropriate for this application. The multiple printf calls could be consolidated for minor efficiency gains, but this is not a critical path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
docs/grout.8.md(1 hunks)main/dpdk.c(1 hunks)main/main.c(6 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (6)
- smoke/vrf_forward_frr_test.sh
- smoke/vrf_forward_test.sh
- smoke/snat44_test.sh
- smoke/ipip_encap_test.sh
- smoke/ip6_rs_ra_test.sh
- smoke/ip_add_del_test.sh
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/dpdk.cmain/main.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/ip6_add_del_test.shsmoke/ip6_same_peer_test.shsmoke/srv6_test.shsmoke/dnat44_test.shsmoke/ip_forward_frr_test.shsmoke/ip_forward_test.shsmoke/_init_frr.shsmoke/vlan_forward_test.shsmoke/cross_vrf_forward_test.shsmoke/ip6_builtin_icmp_test.shsmoke/ip_builtin_icmp_test.shsmoke/nexthop_ageing_test.shsmoke/ip6_forward_test.shsmoke/srv6_frr_test.shsmoke/ip6_forward_frr_test.shsmoke/cross_vrf_forward_frr_test.sh
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: - Never comment about setext vs ATX style headings.
Files:
docs/grout.8.md
🧬 Code graph analysis (16)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
smoke/srv6_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/dnat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
main/main.c (1)
api/gr_errno.h (1)
errno_set(9-12)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-117)
🪛 Shellcheck (0.11.0)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
🔇 Additional comments (23)
smoke/ip_forward_frr_test.sh (1)
12-22: Good refactor using the new helpers.The transition to port_add, netns_add, and grcli-based address provisioning is clean. The added connectivity tests (lines 27–36) properly verify the routing setup end-to-end.
smoke/_init_frr.sh (1)
20-20: Good: Reduced polling intervals from 1s to 0.2s.These sleep reductions align with the PR goal to reduce test execution time. Consistent application across all wait loops in the file.
Also applies to: 53-53, 116-116, 170-170, 233-233
smoke/ip6_add_del_test.sh (1)
9-11: Removal of MAC parameter is consistent with PR goals.These lines remove explicit MAC specification from the
grcli interface addcalls, aligning with the PR's shift to helper-based MAC generation. Unlike other tests, this file directly usesgrcli interface add(not theport_addhelper), which appears intentional for testing interface add/del operations.smoke/srv6_test.sh (1)
10-19: Clean refactor using loop-based port/namespace setup.Moving port_add and netns_add into the loop, then address configuration outside, reduces redundancy and keeps setup isolated. Aligns well with the PR's use of helpers and loops.
smoke/ip_builtin_icmp_test.sh (1)
10-18: Consistent helper-based setup with multi-address provisioning.The loop uses port_add and netns_add consistently, then adds an extra address for p0 outside the loop. This pattern aligns with other refactored tests in the PR.
smoke/dnat44_test.sh (1)
10-26: Good consolidation of port/namespace setup with improved test coverage.Replacing explicit interface creation with the loop-based port_add/netns_add pattern reduces boilerplate. The added connectivity test (line 26) verifies the DNAT rule works end-to-end. Refactoring preserves all DNAT configuration while improving clarity.
smoke/ip6_builtin_icmp_test.sh (1)
10-17: Consistent IPv6 test refactor using helpers.The loop-based port_add/netns_add pattern with grcli address provisioning follows the same structure as other IPv6 tests in this PR. Setup is clear and maintainable.
smoke/nexthop_ageing_test.sh (2)
10-27: Polling refactor looks good.The switch from timeout-based to attempts-based polling (5s equivalent via 25 × 0.2s) is cleaner and more testable.
34-41: Port/netns/address setup is correct.Follows the new helper pattern (port_add → grcli address add → netns_add) consistently.
smoke/cross_vrf_forward_test.sh (1)
10-20: VRF-based setup is correct.Port creation with VRF parameter, address provisioning via grcli, and namespace wiring all follow the expected pattern. Namespace IP assignment and default routing are properly configured.
smoke/ip6_forward_frr_test.sh (1)
10-22: Setup flow is correct.
create_interface→netns_add→ address/routing configuration follows the new pattern cleanly.smoke/srv6_frr_test.sh (1)
10-14: Setup pattern is consistent.Interface creation via
create_interfacefollowed bynetns_addis applied correctly; global namespace identifiers used throughout.smoke/ip_forward_test.sh (1)
10-26: Port-first setup and routing are correct.Clean progression from port creation through address provisioning to namespace wiring, followed by grcli-based routing. The connectivity test suite at the end exercises multiple paths.
smoke/ip6_forward_test.sh (1)
10-25: IPv6 setup and routing flow is solid.Port creation → grcli address → netns_add → local routing via grcli follows the established pattern correctly.
smoke/cross_vrf_forward_frr_test.sh (1)
10-20: VRF-integrated FRR setup is correct.Loop properly computes VRF ID, passes it to
create_interface, configures FRR VRF context viaset_vrf_iface, and wires the namespace. Addresses and routes are set correctly within each VRF context.smoke/_init.sh (5)
161-166: TTY-aware bash xtrace colorization is well-implemented.The conditional xtrace output (cyan, line 164) and PS4 setup provide good visual clarity for interactive test runs while remaining silent in CI/CD pipelines.
177-184: TTY-aware grout output colorization is well-implemented.Colored stdout (blue) and stderr (bold red) make logs easier to follow interactively. The fallback for non-TTY ensures CI compatibility.
197-213: TTY-aware event and zebra log colorization improves readability.Yellow for events (line 199) and magenta for zebra logs (line 210) provide good visual separation. Fallback branches handle non-TTY environments correctly.
215-224: Zebra readiness polling now uses a concrete timeout message.The hardcoded "Zebra is not listening grout events." at line 220 correctly replaces the previously undefined
${timeout}variable reference. The fixed 25 attempts × 0.2s = 5 seconds is reasonable.
227-234: Bridge setup with VLAN filtering is sound.The bridge creation (line 227), FDB tracking in cleanup (lines 228-231), VLAN 1 removal (line 232), isolation (line 233), and bringing up the bridge (line 234) are all correct for the bridge topology refactor.
main/main.c (3)
25-25: LGTM: Required for varargs in perr().The stdarg.h include is necessary for the new perr() function.
78-89: LGTM: Solid error reporting utility.The perr() function provides standardized error output with safe formatting and consistent return values.
144-249: LGTM: Consistent error handling refactor.The parse_args() updates consistently use perr() for all error cases, improving code uniformity. The change to exit with EXIT_SUCCESS when -h is specified is correct behavior for help output.
Set stdout and stderr to line buffered mode to ensure output is flushed after each newline. This is needed when output is piped through awk for colorization, as block buffering would delay output until the buffer fills, making the smoke tests appear to hang. Signed-off-by: Robin Jarry <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
main/dpdk.c (1)
65-65: Error and warning logs should go to stderr.The past review comment on this line remains unaddressed. All log levels currently write to stdout, violating Unix conventions where errors (EMERG through WARNING) belong on stderr.
If this change is intentional for smoke test requirements, verify that production error visibility is not impacted. Otherwise, apply the suggested fix from the previous review to route error levels to stderr.
main/main.c (2)
91-108: Type mismatch flagged in past review.The existing review comments already note that parse_uint() accepts
unsigned int *vbut is called with incompatible pointer types (uid_t *,gid_t *,mode_t *) at lines 127, 135, and 191.
135-135: Type cast error flagged in past review.Existing review comments already note that line 135 should use
(gid_t)-1instead of(uid_t)-1.smoke/vlan_forward_test.sh (1)
16-18: Remove redundant VLAN operations.
port_addalready configures the VLAN withbridge vlan add dev "$name" vid "$vlan" pvid untagged. Lines 17–18 redundantly delete and re-add the same VLAN, losing thepvid untaggedflags that enable proper VLAN processing.Remove these lines:
port_add $p $vlan - bridge vlan del dev $p vid $vlan - bridge vlan add dev $p vid $vlan grcli interface add vlan $v parent $p vlan_id $vlansmoke/_init_frr.sh (1)
8-23: Critical: Infinite loop when max tries exceeded.The
failfunction only returns but doesn't exit. After line 18 executes, the while loop condition (line 16) is re-evaluated. If vtysh still returns "Can't find interface", the loop continues indefinitely instead of failing the test.Add
return 1after thefailcall to exit the function:while vtysh -c "show interface $p" 2>&1 | grep -q "% Can't find interface"; do if [ "$count" -ge "$max_tries" ]; then - fail "Interface $p not found after $max_tries attempts." + fail "Interface $p not found after $max_tries attempts." + return 1 fi sleep 0.2 count=$((count + 1)) donesmoke/_init.sh (2)
98-117: netns_add function: Fix kill command syntax (duplicate issue).Line 104 uses invalid kill syntax:
kill --timeout 500 KILL. The kill command doesn't support--timeout; this will fail. This was flagged in a previous review.Apply this diff to fix the syntax:
ip netns add "$ns" cat >> $tmp/cleanup <<EOF -ip netns pids "$ns" | xargs -r kill --timeout 500 KILL +ip netns pids "$ns" | xargs -r kill -KILL ip netns del "$ns"
120-133: port_add function: Separate declaration from command substitution (duplicate issue).Line 128 combines
localdeclaration with command substitution, which masks the md5sum pipeline's return value (SC2155). This was flagged in a previous review.Apply this diff to properly capture and detect pipeline failures:
local name="$1" local vlan="$2" shift 2 grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac="$mac_output" ip link set "$name" master "$bridge" address "$mac"
🧹 Nitpick comments (2)
smoke/ip6_same_peer_test.sh (1)
21-23: Quote command substitutions to prevent word splitting.Lines 21–23 should quote the
$(llocal_addr ...)output per shellcheck (SC2046), even though practical impact is minimal. While these return single IPv6 addresses, consistent quoting improves robustness.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"smoke/cross_vrf_forward_frr_test.sh (1)
10-21: VRF setup correctly uses new create_interface signature.The vrf parameter integration and address/route configuration look correct. However, line 17 (
ip -n $p link set $p up) is redundant—netns_add already brings the namespace interface up (see _init_frr.sh line 112).Apply this diff to remove the redundant line:
set_ip_address $p 172.16.$n.1/24 set_vrf_iface $vrf netns_add $p 40$n - ip -n $p link set $p up ip -n $p addr add 172.16.$n.2/24 dev $p
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cli/main.c(1 hunks)docs/grout.8.md(1 hunks)main/dpdk.c(1 hunks)main/main.c(6 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (4)
- smoke/srv6_test.sh
- smoke/ip6_rs_ra_test.sh
- smoke/ip_add_del_test.sh
- cli/main.c
🧰 Additional context used
📓 Path-based instructions (3)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip6_add_del_test.shsmoke/srv6_frr_test.shsmoke/_init_frr.shsmoke/ip_forward_frr_test.shsmoke/ip6_builtin_icmp_test.shsmoke/dnat44_test.shsmoke/ip_forward_test.shsmoke/ip_builtin_icmp_test.shsmoke/cross_vrf_forward_test.shsmoke/_init.shsmoke/ip6_same_peer_test.shsmoke/ip6_forward_frr_test.shsmoke/vrf_forward_frr_test.shsmoke/vlan_forward_test.shsmoke/vrf_forward_test.shsmoke/ip6_forward_test.shsmoke/ipip_encap_test.shsmoke/snat44_test.shsmoke/nexthop_ageing_test.shsmoke/cross_vrf_forward_frr_test.sh
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: - Never comment about setext vs ATX style headings.
Files:
docs/grout.8.md
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/dpdk.cmain/main.c
🧬 Code graph analysis (19)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/ip_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/ip6_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/dnat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/snat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
main/main.c (1)
api/gr_errno.h (1)
errno_set(9-12)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-117)
🪛 Shellcheck (0.11.0)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: rpm
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
🔇 Additional comments (26)
docs/grout.8.md (1)
19-22: Documentation properly reflects new CLI options.The new
-S/--syslog,-T/--trace, and-V/--versionoptions are clearly documented in both SYNOPSIS and OPTIONS sections with appropriate descriptions and examples (e.g., trace regex patterns, multiplicity limits). Formatting is consistent with existing option documentation.Also applies to: 101-103, 115-132, 144-146
main/main.c (2)
34-76: LGTM: usage() refactored.Hardcoding the program name simplifies the code.
78-89: LGTM: perr() helper.Clean error reporting utility.
smoke/ip6_add_del_test.sh (1)
9-11: LGTM: MAC address arguments removed.Aligns with PR objective to eliminate stable MAC addresses.
smoke/snat44_test.sh (1)
10-22: LGTM: migrated to port_add/netns_add.VLAN IDs are consistent (400/401), and the added show commands improve observability.
smoke/ipip_encap_test.sh (1)
11-22: LGTM: refactored to use helpers.Consistent VLAN IDs and clean migration to port_add/netns_add.
smoke/ip_builtin_icmp_test.sh (1)
10-18: LGTM: loop-based port/netns setup.Clean refactor with consistent VLAN IDs (400/401).
smoke/vrf_forward_frr_test.sh (2)
12-34: LGTM: VRF-aware loop-based setup.Modulo arithmetic reuses address space across VRFs, which is intentional. VLAN IDs are unique (400-403).
36-39: LGTM: VRF connectivity tests.Tests verify inter-VRF isolation and routing.
smoke/dnat44_test.sh (1)
10-29: LGTM: loop-based setup with DNAT.Clean refactor with consistent VLAN IDs (400/401). The
type dnatparameter on line 29 filters nexthop output appropriately.smoke/ip6_builtin_icmp_test.sh (1)
10-19: LGTM: IPv6 setup with DAD wait.Clean loop-based refactor with appropriate DAD delay.
smoke/vrf_forward_test.sh (1)
14-22: LGTM. Proper loop-based VRF setup with helpers; addressing and routing are consistent.smoke/nexthop_ageing_test.sh (2)
10-27: LGTM. Polling refactor is solid: attempts-based loop is cleaner and more predictable than seq-based timeout.
34-44: LGTM. Port/namespace setup and connectivity checks are consistent with refactoring objectives.smoke/srv6_frr_test.sh (1)
10-20: LGTM. Loop-based interface/namespace setup with consistent naming; SRv6 configuration follows expected patterns.smoke/ip_forward_frr_test.sh (1)
10-22: LGTM. FRR-based setup with proper conditional lo-address assignment; address/route provisioning is correct.smoke/_init_frr.sh (1)
20-20: LGTM. Sleep times reduced to 0.2s throughout, aligning with test execution time reduction objective.Also applies to: 53-53, 116-116, 170-170, 233-233
smoke/cross_vrf_forward_test.sh (1)
10-20: LGTM. Loop-based VRF setup with helpers; port_add properly delegates VRF argument; addressing and routing are consistent.smoke/ip6_forward_frr_test.sh (1)
10-22: Setup and helper usage look correct.The loop properly uses the new helpers (create_interface, netns_add, set_ip_address) with correct signatures and arguments.
smoke/ip_forward_test.sh (1)
10-26: Setup flow is clean and consistent.Correct use of port_add, grcli address/route configuration, and netns_add. The per-port and per-namespace setup is well-structured.
smoke/ip6_forward_test.sh (1)
10-25: IPv6 setup properly mirrors IPv4 patterns.Port and namespace configuration follows the same structure as ip_forward_test.sh with appropriate IPv6 addresses and grcli-based provisioning.
smoke/_init.sh (5)
161-166: TTY-based xtrace coloring is well-implemented.The cyan coloring for trace output when stdout is a TTY improves visibility. Redirection to FD 9 and proper PS4 format are correct.
177-184: Colorized grout output improves observability.Separate coloring of stdout (blue) and stderr (bold red) when TTY is present provides clear visual separation. Process substitution with awk is the right approach here.
197-202: Colorized events output is well-structured.Conditional coloring (yellow when TTY, plain otherwise) for events stream keeps test output readable without compromising log capture in non-interactive environments.
208-224: Improved Zebra readiness check with finite polling.Replaces previous approach with a finite 25-attempt loop (5 seconds total) and fixed error message. More reliable than open-ended waiting.
227-234: Bridge setup with VLAN filtering is comprehensive.Creates bridge with filtering enabled, isolates it, removes default VLAN 1 self-bridging, and updates cleanup appropriately. Proper setup for the new test topology.
Keep stderr for actual errors. Signed-off-by: Robin Jarry <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main/main.c (1)
195-196: Errno overwrite may lose specific error information.If
parse_uint()insideparse_sock_owner()sets a specific errno (e.g.,ERANGE), callingerrno_set(EINVAL)here discards it. While the user sees the correct error viaperr(), any downstream errno check sees onlyEINVAL.Consider propagating the original errno:
case 'o': if (parse_sock_owner(optarg) < 0) - return errno_set(EINVAL); + return -1; break;
♻️ Duplicate comments (4)
smoke/ip6_rs_ra_test.sh (1)
16-16: Remove redundantip addr showoutput.Line 16 contradicts the PR objective to remove redundant address show commands.
-ip -n $p1 addr showsmoke/vlan_forward_test.sh (1)
17-18: ** Remove redundant VLAN operations.**Lines 17–18 delete and re-add the VLAN that
port_addalready configured withpvid untaggedflags (see smoke/_init.sh lines 130–131). This redundancy loses those flags.smoke/_init.sh (1)
103-106: Critical: Fix invalid kill command syntax in cleanup.Line 104 uses
kill --timeout 500 KILL, but thekillcommand doesn't support--timeout. This will cause cleanup to fail with "unknown signal" error, leaving PIDs unkilled.cat >> $tmp/cleanup <<EOF - ip netns pids "$ns" | xargs -r kill --timeout 500 KILL + ip netns pids "$ns" | xargs -r kill -KILL ip netns del "$ns" EOFmain/main.c (1)
91-108: Type mismatch still present in parse_uint() signature.The function declares
unsigned int *vbut is called withuid_t *(line 127),gid_t *(line 135), andmode_t *(line 191). While these types often aliasunsigned int, POSIX doesn't guarantee it.
🧹 Nitpick comments (4)
smoke/ip6_same_peer_test.sh (1)
21-23: Quote command substitutions to follow shell best practices.While
llocal_addrreturns a single address without spaces, shellcheck recommends quoting command substitutions to avoid potential word-splitting issues.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"smoke/_init.sh (1)
128-128: Best practice: Split declaration and assignment to avoid masking return values.Line 128 combines
localdeclaration with command substitution. Withset -e, if the pipeline fails, the exit status is masked by thelocaldeclaration. Whilemd5sumandsedare unlikely to fail, splitting ensures the script terminates cleanly on pipeline failures.- local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac="$mac_output"main/main.c (2)
91-108: Consider rejecting empty strings explicitly.If
strtoulreceives an empty string,endptr == sand*endptr == '\0'both hold, so an empty argument might be accepted as 0. Adding an explicit check improves clarity:errno = 0; val = strtoul(s, &endptr, base); + if (endptr == s) + return errno_set(EINVAL); if (errno != 0) return errno_set(errno);
182-185: Dead code after exit().The
breakat line 185 is unreachable afterexit(EXIT_SUCCESS). While harmless, it can be removed.case 'h': usage(); exit(EXIT_SUCCESS); - break;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
docs/grout.8.md(1 hunks)main/dpdk.c(1 hunks)main/main.c(6 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (10)
- smoke/dnat44_test.sh
- smoke/vrf_forward_frr_test.sh
- smoke/ip6_add_del_test.sh
- smoke/cross_vrf_forward_frr_test.sh
- smoke/ip_forward_frr_test.sh
- docs/grout.8.md
- smoke/ip_add_del_test.sh
- main/dpdk.c
- smoke/cross_vrf_forward_test.sh
- smoke/nexthop_ageing_test.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip6_rs_ra_test.shsmoke/ip_forward_test.shsmoke/srv6_frr_test.shsmoke/vrf_forward_test.shsmoke/ip6_builtin_icmp_test.shsmoke/ip6_forward_frr_test.shsmoke/vlan_forward_test.shsmoke/_init.shsmoke/ipip_encap_test.shsmoke/snat44_test.shsmoke/ip6_forward_test.shsmoke/ip_builtin_icmp_test.shsmoke/srv6_test.shsmoke/_init_frr.shsmoke/ip6_same_peer_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/main.c
🧬 Code graph analysis (15)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-117)
smoke/vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ipip_encap_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/snat44_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
main/main.c (2)
cli/main.c (1)
usage(27-30)api/gr_errno.h (1)
errno_set(9-12)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
🪛 Shellcheck (0.11.0)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (22)
smoke/ipip_encap_test.sh (1)
11-12: Refactoring to new helpers is clean and consistent.The test properly migrates to
port_addandnetns_addhelpers with matching VLAN IDs (400 for p0, 401 for p1) and consistent variable usage throughout.Also applies to: 18-18, 22-22
smoke/snat44_test.sh (1)
10-22: LGTM!Setup correctly uses
port_addandnetns_addhelpers with appropriate VLAN assignments. The namespaces created in the loop properly reference the ports.smoke/ip_builtin_icmp_test.sh (1)
10-18: LGTM!Loop correctly sets up ports and namespaces with matching VLANs. The extra address on p0 (line 18) properly extends the setup for multi-homed connectivity testing.
smoke/ip6_builtin_icmp_test.sh (1)
10-17: LGTM!Loop correctly sets up IPv6 addresses with proper variable substitution. Port/namespace VLAN alignment is correct.
smoke/srv6_frr_test.sh (1)
12-13: LGTM!FRR-specific setup correctly creates interface and namespace with matching VLAN (40$n).
smoke/vrf_forward_test.sh (1)
12-36: LGTM!VRF-based setup correctly uses modulo arithmetic to cycle address ranges. Port VRF assignments (1 and 2) properly separate the two test domains.
smoke/srv6_test.sh (1)
10-19: LGTM!Loop cleanly separates port/namespace creation from address configuration. Post-loop address setup correctly references p0 and p1.
smoke/ip6_forward_frr_test.sh (2)
12-22: Clean refactor with helpers.The loop correctly uses
create_interfaceandnetns_addwith numeric VLANs. Address setup and routing are properly configured.
29-40: Clean use of dynamic link-local addresses.The
$(llocal_addr $p)substitutions correctly extract link-local addresses at runtime. Ping/traceroute targets are properly set up.smoke/ip_forward_test.sh (1)
12-26: Clean helper-based setup.Port creation, address provisioning, and routing are cleanly done via helpers and grcli. Flow is well structured.
smoke/ip6_forward_test.sh (2)
12-25: Clean IPv6 setup with helpers.Port creation, IPv6 addresses, and routing are properly configured via helpers and grcli.
29-30: Dynamic link-local address extraction is correct.Using
$(llocal_addr $p)to get link-local addresses at runtime is the right approach.smoke/_init_frr.sh (2)
8-23: Simplified interface creation with delegation.The refactored
create_interfacecorrectly delegates toport_addfor setup and maintains retry logic for FRR interface discovery. Withset -e, thefailcall (line 18) properly terminates on max retries.
20-20: Consistent sleep optimization.All wait loops reduced to 0.2s sleep intervals. This tightens polling while maintaining readability—good for test execution time reduction per PR objective.
Also applies to: 53-53, 116-116, 170-170, 233-233
smoke/_init.sh (4)
92-96: Clean test initialization.Temporary workspace, trap handler, and unique run_id setup are well structured for test isolation.
135-137: Clean link-local address extraction helper.The
llocal_addrfunction correctly extracts IPv6 fe80 link-local addresses from grcli output.
161-224: Good: TTY-aware colored output and optimized readiness checks.Conditional coloring via TTY detection is clean. Zebra readiness check uses finite polling with 0.2s intervals (optimized from previous version). Error message is concrete and no longer references undefined variables. ✓
227-234: Correct bridge topology setup.Bridge created with VLAN filtering, default VLAN 1 removed, and isolated mode enabled. Cleanup logs FDB state before deletion. Order and configuration are correct for the new test topology.
main/main.c (4)
78-89: LGTM: Error reporting helper is well-implemented.The
perr()function correctly usesvsnprintfwith bounds checking and provides consistent error formatting.
112-142: Error handling improved with perr().The function now provides clear, user-friendly error messages and correctly uses
parse_uint()for numeric fallback when name lookup fails.
34-76: Usage text is clear and comprehensive.The refactored
usage()function provides well-organized help text covering all options. Hardcoding "grout" is acceptable for a fixed binary name.
191-192: Argument validation and error reporting improved.The unified error handling via
perr()provides clear, consistent messages. Range checks for socket mode (octal 0-07777) and MTU (512-16384) are appropriate.Also applies to: 226-228, 237-237, 239-239, 245-246
When stdout is a TTY, colorize different output streams to improve readability during test execution: - Bash xtrace (set -x) in cyan - grout daemon logs in blue - grout daemon errors in bold red - grcli events in yellow - FRR zebra logs in magenta Each stream is piped through awk to apply ANSI color codes. Signed-off-by: Robin Jarry <[email protected]>
Replace the bash-specific SRANDOM variable with /dev/urandom for generating random test run identifiers. Reading from /dev/urandom provides better randomness than SRANDOM and shortens the line. Add || : to prevent pipeline failures from terminating the test script when head closes the pipe early after reading one line. Signed-off-by: Robin Jarry <[email protected]>
Introduce the port_add() helper function to centralize port creation logic. The helper generates a deterministic MAC address from the port name using md5sum and assigns it to the Linux tap interface. By default, net_tap ports have the same MAC address on both the grout side and the Linux side. This causes Linux to wrongfully assume packets sent by grout originated locally, which breaks bridging and MAC learning. Assign a different locally administered MAC address to the Linux side to avoid this issue. Replace all inline port creation calls across smoke tests with calls to this helper. Signed-off-by: Robin Jarry <[email protected]>
Move the `ip -n $ns link set lo up` command into the netns_add() helper. Every network namespace needs the loopback interface up, so it's redundant to call this in each individual test. Remove the explicit lo up commands from all smoke tests since they are now handled by netns_add(). Signed-off-by: Robin Jarry <[email protected]>
Remove the "n-" prefix from network namespace names in FRR smoke tests. Use the port name directly as the network namespace name, consistent with non-FRR tests. This simplifies the test code by eliminating the need to track separate port and namespace names. Signed-off-by: Robin Jarry <[email protected]>
Remove `ip addr show` commands from smoke tests. These commands were used for debugging during test development but add no value during normal test execution and only clutter the output. Signed-off-by: Robin Jarry <[email protected]>
Stop passing explicit MAC addresses to port_add(). The MAC addresses were only needed to derive predictable IPv6 link-local addresses for ping tests. Instead, introduce the llocal_addr() helper function to dynamically query the link-local address of an interface from grcli. This removes the need to track and coordinate MAC addresses across test files. Signed-off-by: Robin Jarry <[email protected]>
Replace repetitive port and namespace creation code with for loops that iterate over port numbers. This reduces code duplication and makes the test files more maintainable. Most tests follow a pattern of creating two or more similar ports and namespaces with incrementing numbers. Use a loop variable to construct names and addresses dynamically. Signed-off-by: Robin Jarry <[email protected]>
Create a Linux bridge with VLAN filtering and connect all ports and network namespaces to it using VLAN assignments for isolation. Previously, tap interfaces were moved directly into network namespaces. The net_tap DPDK driver relies on interface names for ioctl calls in the namespace where the tap was created. Moving the interface to another namespace causes these calls to fail. Keep tap interfaces in the default namespace and use veth pairs to connect network namespaces to a bridge. This topology also enables testing of bonding scenarios where multiple ports share the same L2 segment. Modify netns_add() to accept a VLAN ID and create the veth pair. Modify port_add() to accept a VLAN ID and attach the port to the bridge. Signed-off-by: Robin Jarry <[email protected]>
Remove the workaround that assumed tap interfaces are always running. This was needed when tap interfaces were moved to network namespaces, which caused the kernel to signal them as down even though they could still transmit and receive packets. Since tap interfaces now stay in the default namespace, link status detection works correctly and the workaround is no longer needed. Signed-off-by: Robin Jarry <[email protected]>
Reduce sleep intervals in retry loops from 1 second to 200ms across all smoke test infrastructure. This includes wait loops in create_interface, set_ip_address, set_ip_route, set_srv6_localsid, and set_srv6_route helpers. Reduce FRR startup timeout from 15 seconds to 5 seconds. Remove the sleep after frrinit.sh stop in cleanup as it is not necessary. Refactor the nexthop_ageing_test.sh check loop to use a while loop with decrementing timeout instead of a seq-based for loop to support sub-second sleep intervals. Signed-off-by: Robin Jarry <[email protected]>
The interface, nexthop and route show commands provide information that is not essential for debugging smoke test failures. Keep only software stats and trace output which are more useful for troubleshooting. Signed-off-by: Robin Jarry <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
smoke/ip6_rs_ra_test.sh (1)
11-16: Remove redundantip addr showcommand.Line 16 contradicts the PR objective to eliminate redundant address-show commands. This command produces no functional output and should be removed.
port_add $p1 400 grcli address add fd00:ba4:1::1/64 iface $p1 netns_add $p1 400 ip -n $p1 addr add fd00:ba4:1::2/64 dev $p1 -ip -n $p1 addr showsmoke/_init.sh (2)
103-106: Fix invalidkillcommand syntax in cleanup.The xargs/kill combination uses unsupported syntax:
kill --timeout 500 KILL. Thekillcommand doesn't have a--timeoutflag; this will fail with "unknown signal" error and leave pids unkilled.Apply this diff:
cat >> $tmp/cleanup <<EOF -ip netns pids "$ns" | xargs -r kill --timeout 500 KILL +ip netns pids "$ns" | xargs -r kill -KILL cat >> $tmp/cleanup <<EOF
120-133: Splitlocaldeclaration and command substitution to capture errors.Line 128 combines
localwith command substitution, masking the return value of the md5sum pipeline. Ifmd5sumorsedfails, the error is hidden and$macbecomes empty, leading to downstream failures.Apply this diff:
port_add() { local name="$1" local vlan="$2" shift 2 grcli interface add port "$name" devargs "net_tap$tap_counter,iface=$name" "$@" # Ensure the Linux net device has a different mac address from grout's. # This is required to avoid Linux from wrongfully assuming the packets # sent by grout originated locally. - local mac=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + local mac_output + mac_output=$(echo "$name" | md5sum | sed -E 's/(..)(..)(..)(..)(..).*/02:\1:\2:\3:\4:\5/') + if [ -z "$mac_output" ]; then + fail "Failed to generate MAC for port $name" + fi + local mac="$mac_output" ip link set "$name" master "$bridge" address "$mac"smoke/vlan_forward_test.sh (1)
17-18: Remove redundant bridge VLAN operations.These lines delete and re-add the VLAN that
port_addalready configured withpvid untaggedflags, losing those critical flags in the process.port_add $p $vlan - bridge vlan del dev $p vid $vlan - bridge vlan add dev $p vid $vlan grcli interface add vlan $v parent $p vlan_id $vlan
🧹 Nitpick comments (1)
smoke/ip6_same_peer_test.sh (1)
21-23: Quote command substitutions to prevent word splitting.Lines 21–23 use unquoted command substitutions. While
llocal_addroutput is unlikely to contain spaces, quote the substitution for defensive shell scripting and consistency:-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) -ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p2) && fail "Unexpected answer from foreign link local address" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)" && fail "Unexpected answer from foreign link local address"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
main/dpdk.c(1 hunks)modules/infra/control/port.c(0 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(2 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/dnat44_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_frr_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/ip_add_del_test.sh(1 hunks)smoke/ip_builtin_icmp_test.sh(1 hunks)smoke/ip_forward_frr_test.sh(1 hunks)smoke/ip_forward_test.sh(1 hunks)smoke/ipip_encap_test.sh(1 hunks)smoke/nexthop_ageing_test.sh(3 hunks)smoke/snat44_test.sh(2 hunks)smoke/srv6_frr_test.sh(2 hunks)smoke/srv6_test.sh(1 hunks)smoke/vlan_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/port.c
🚧 Files skipped from review as they are similar to previous changes (10)
- smoke/ip_forward_frr_test.sh
- smoke/srv6_test.sh
- smoke/vrf_forward_test.sh
- smoke/ip_add_del_test.sh
- smoke/nexthop_ageing_test.sh
- smoke/snat44_test.sh
- smoke/ip6_add_del_test.sh
- smoke/ipip_encap_test.sh
- main/dpdk.c
- smoke/ip6_builtin_icmp_test.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/ip_forward_test.shsmoke/ip6_forward_frr_test.shsmoke/dnat44_test.shsmoke/ip6_rs_ra_test.shsmoke/_init.shsmoke/ip_builtin_icmp_test.shsmoke/srv6_frr_test.shsmoke/_init_frr.shsmoke/vlan_forward_test.shsmoke/ip6_forward_test.shsmoke/vrf_forward_frr_test.shsmoke/cross_vrf_forward_frr_test.shsmoke/cross_vrf_forward_test.shsmoke/ip6_same_peer_test.sh
🧬 Code graph analysis (13)
smoke/ip_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (2)
netns_add(98-117)llocal_addr(135-137)
smoke/dnat44_test.sh (2)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)modules/infra/cli/port.c (1)
port_add(123-150)
smoke/ip6_rs_ra_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip_builtin_icmp_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/srv6_frr_test.sh (2)
smoke/_init_frr.sh (4)
create_interface(8-23)set_srv6_route(182-235)set_ip_route(79-119)set_srv6_localsid(126-173)smoke/_init.sh (1)
netns_add(98-117)
smoke/_init_frr.sh (1)
smoke/_init.sh (2)
port_add(120-133)fail(87-90)
smoke/vlan_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_forward_test.sh (1)
smoke/_init.sh (3)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_ip_route(79-119)smoke/_init.sh (1)
netns_add(98-117)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(8-23)set_ip_address(25-56)set_vrf_iface(68-77)smoke/_init.sh (1)
netns_add(98-117)
smoke/cross_vrf_forward_test.sh (1)
smoke/_init.sh (2)
port_add(120-133)netns_add(98-117)
smoke/ip6_same_peer_test.sh (1)
smoke/_init.sh (4)
port_add(120-133)netns_add(98-117)llocal_addr(135-137)fail(87-90)
🪛 Shellcheck (0.11.0)
smoke/ip6_forward_frr_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/_init.sh
[warning] 128-128: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/ip6_forward_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
smoke/ip6_same_peer_test.sh
[warning] 21-21: Quote this to prevent word splitting.
(SC2046)
[warning] 22-22: Quote this to prevent word splitting.
(SC2046)
[warning] 23-23: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (12)
smoke/ip_builtin_icmp_test.sh (1)
10-19: Verify post-loop address assignment scope.Line 18 references
$p0after the loop, but$pin the loop is set to$run_id$n. After the loop completes withn=1,$pequals$run_id1, not$run_id0. Confirm this is intentional or use the explicit port variable.smoke/cross_vrf_forward_test.sh (1)
10-20: Loop-based port/netns setup is well-structured.The refactoring consolidates port creation, addressing, and namespace setup into a single loop. VRF context integration is clean, and namespace references are consistent. LGTM.
smoke/ip_forward_test.sh (1)
10-22: Loop structure and conditional address assignment are correct.The per-node setup cleanly consolidates port creation, namespace initialization, and conditional address assignment (lo vs port-based depending on node). Sequencing is proper.
smoke/_init.sh (1)
215-224: Zebra readiness check error message is clear.The timeout-based polling loop (25 attempts × 0.2s = 5 seconds) is well-structured, and the error message now correctly reports "Zebra is not listening grout events" without referencing an undefined variable. Good fix.
smoke/srv6_frr_test.sh (1)
10-14: Namespace refactoring is consistent and complete.Transition from per-namespace names (n-$pX) to global references ($pX) is uniformly applied throughout the file. Helper functions are correctly integrated, and all test commands properly reference the updated namespace variables.
smoke/dnat44_test.sh (1)
10-29: Port/netns consolidation and DNAT configuration are sound.Loop-based port and namespace creation follows the established pattern. DNAT destination update to
172.16.0.99aligns with the port interface IP172.16.0.1/24. Nexthop filtering totype dnatprovides clearer output.smoke/vrf_forward_frr_test.sh (1)
12-22: Loop-based refactoring with VRF-aware helpers looks solid.The use of x=$((n % 2)) correctly manages the IP addressing across two loops with different VRF contexts. Helpers are properly delegated.
smoke/ip6_forward_frr_test.sh (1)
29-30: Quote command substitutions to prevent word splitting.If
llocal_addrever returns multi-word output, unquoted expansion could split the argument.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/cross_vrf_forward_frr_test.sh (1)
10-21: VRF-aware loop structure with cross-VRF routing is well-integrated.Proper use of
create_interfacewith VRF parameters,set_vrf_iface, and cross-VRF nexthop specifications.smoke/ip6_forward_test.sh (1)
29-30: Quote command substitutions to prevent word splitting.Same as smoke/ip6_forward_frr_test.sh — unquoted
$(llocal_addr ...)should be quoted.-ip netns exec $p1 ping6 -i0.01 -c3 -n $(llocal_addr $p1) -ip netns exec $p2 ping6 -i0.01 -c3 -n $(llocal_addr $p2) +ip netns exec $p1 ping6 -i0.01 -c3 -n "$(llocal_addr $p1)" +ip netns exec $p2 ping6 -i0.01 -c3 -n "$(llocal_addr $p2)"smoke/_init_frr.sh (2)
8-23: Streamlinedcreate_interfacedelegation toport_addis clean.Reduces code duplication and aligns with the new helper-driven initialization pattern. Error handling via
failwithset -eworks correctly.
20-20: Standardized sleep intervals improve readability.Consistent
0.2ssleeps across all wait loops (previously mixed durations) make retry logic predictable.Also applies to: 53-53, 116-116, 170-170, 233-233
2 participants
Summary by CodeRabbit
Infrastructure
Test Infrastructure