Srv6 l2 dx2 #466
Srv6 l2 dx2 #466
Conversation
icmp and icmp6 have the same static symbols for init/fini. While this isn't an issue, it is better to have unique function names to have an non ambiguous callstack or in a debug session. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
Similar to "control_output: fix use-after-free on shutdown",
packets are still in the icmp(6) queue when grout quits, which ends up
into a 'use-after-free':
==51938==ERROR: AddressSanitizer: heap-use-after-free on address 0x7f6e3084f080 at pc 0x560293c3dc87 bp 0x7ffe3e9298b0 sp 0x7ffe3e9298a0
READ of size 8 at 0x7f6e3084f080 thread T0
#0 0x560293c3dc86 in rte_pktmbuf_free ../subprojects/dpdk/lib/mbuf/rte_mbuf.h:1522
DPDK#1 0x560293c42a8a in icmp6_queue_pop ../modules/ip6/control/icmp6.c:30
DPDK#2 0x560293c49cb8 in icmp_fini ../modules/ip6/control/icmp6.c:177
DPDK#3 0x560293ac1068 in modules_fini ../main/module.c:123
DPDK#4 0x560293abfb52 in main ../main/main.c:326
DPDK#5 0x7f6e5ca2a1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
DPDK#6 0x7f6e5ca2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
DPDK#7 0x560293ab5974 in _start (/home/runner/work/grout/grout/build/grout+0x312974) (BuildId: 9f73d7b419f769b2197f675b4a3d768d8259e548)
Similar to other modules, don't attempt to free individual elements.
Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
In the control folder, decoupling allows more flexibility when a new interface mode is added, with a new callback when the interface mode is updated. For the CLI, add iface mode callbacks similar to iface type callbacks. This allows additional flexibility in interface mode definition. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
Implement "end.dx2" behavior, which removes the IPv6 header and forward the ethernet frame to a specified interface. Even if FRR doesn't support (yet) the behavior end dx.2, take it into account to build properly the plugin. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
Add L2 encapsulation from an interface: all packets received on that interface will be encapsulated and sent to the srv6_output node. As it is hard to get l2encap mode with linux, use scapy to encap the packets in the smoke test. Update README and github workflow dependencies. Signed-off-by: Christophe Fontaine <cfontain@redhat.com>
0736fa8 to
5fb7619
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds SRv6 L2 encapsulation end-to-end: new SRv6 behavior SR_BEHAVIOR_END_DX2, datapath nodes for L2 encap and DX2 handling, and Ethernet output/decap paths. Introduces a plugin-like interface mode API and multiple mode implementations (L3, L1 XC, SRv6 L2 encap) with CLI integration, mode-specific init/reconfig/show/list hooks, and iface mode_data storage. Adds nexthop-to-interface mapping and nexthop deletion cleanup for SRv6 XC. Includes CLI, control, and datapath source additions, build file updates, smoke test scripts, and adds scapy to CI/test dependencies. Removes certain ICMP queue draining on shutdown. 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/cli/iface.c (1)
295-330: NULL pointer dereference:mode_from_idcan return NULL.If an interface has an unregistered mode,
mode_from_idreturns NULL and setserrno = ENODEV. Line 313 then dereferencesmode->str, causing a crash. Unliketypewhich has anassert(type != NULL)at line 322,modehas no such check.const struct cli_iface_type *type = type_from_id(iface->type); const struct cli_iface_mode *mode = mode_from_id(iface->mode); + if (mode == NULL) { + ret = -1; + continue; + } struct libscols_line *line = scols_table_new_line(table, NULL);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
.github/workflows/check.yml(1 hunks)README.md(1 hunks)frr/rt_grout.c(1 hunks)modules/infra/api/gr_infra.h(3 hunks)modules/infra/cli/gr_cli_iface.h(4 hunks)modules/infra/cli/iface.c(12 hunks)modules/infra/cli/port.c(3 hunks)modules/infra/control/gr_iface.h(1 hunks)modules/infra/control/iface.c(6 hunks)modules/infra/control/xconnect.c(1 hunks)modules/srv6/api/gr_srv6.h(3 hunks)modules/srv6/cli/iface_nh.c(1 hunks)modules/srv6/cli/localsid.c(5 hunks)modules/srv6/cli/meson.build(1 hunks)modules/srv6/control/iface_nh.c(1 hunks)modules/srv6/control/meson.build(1 hunks)modules/srv6/datapath/l2_encap.c(1 hunks)modules/srv6/datapath/meson.build(1 hunks)modules/srv6/datapath/srv6.h(1 hunks)modules/srv6/datapath/srv6_local.c(6 hunks)modules/srv6/datapath/srv6_output.c(2 hunks)smoke/srv6_l2_encap_scapy.py(1 hunks)smoke/srv6_l2_encap_test.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/srv6/datapath/srv6.hmodules/infra/api/gr_infra.hmodules/infra/control/gr_iface.hmodules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cfrr/rt_grout.cmodules/srv6/api/gr_srv6.hmodules/infra/cli/iface.cmodules/infra/control/iface.cmodules/infra/cli/gr_cli_iface.hmodules/infra/cli/port.cmodules/srv6/cli/localsid.cmodules/infra/control/xconnect.cmodules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_local.c
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: - Never comment about setext vs ATX style headings.
Files:
README.md
🧠 Learnings (16)
📓 Common learnings
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/srv6/control/meson.buildmodules/srv6/cli/meson.build
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
smoke/srv6_l2_encap_test.shmodules/infra/api/gr_infra.hmodules/srv6/datapath/l2_encap.cmodules/infra/control/iface.cmodules/infra/cli/gr_cli_iface.hmodules/srv6/cli/localsid.cmodules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
modules/infra/api/gr_infra.h
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/api/gr_infra.hmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-09T07:36:34.893Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T07:36:34.893Z
Learning: In the SRv6 routing header structure, the flag and tag fields are used for per-flow metadata and Group-Based Policy use cases. Only 5 bytes are needed to access the basic SRv6 fields: next_hdr, hdr_len, type, segments_left, and last_entry.
Applied to files:
modules/infra/api/gr_infra.hmodules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cmodules/srv6/api/gr_srv6.h
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/l2_encap.cfrr/rt_grout.cmodules/srv6/cli/localsid.cmodules/srv6/control/iface_nh.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
Applied to files:
modules/srv6/datapath/srv6_output.cmodules/srv6/api/gr_srv6.hmodules/srv6/control/iface_nh.c
📚 Learning: 2025-09-09T07:08:40.398Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:106-111
Timestamp: 2025-09-09T07:08:40.398Z
Learning: For SRv6 header validation, the correct check for last_entry bounds is `srh->last_entry > (srh->hdrlen / 2) - 1` where hdr_len represents the length in 8-byte units excluding the first 8 bytes, and each segment is 16 bytes, so hdr_len/2 gives the maximum number of segments.
Applied to files:
modules/srv6/datapath/srv6_output.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/srv6/datapath/srv6_output.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/srv6/cli/meson.buildmodules/srv6/cli/localsid.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
modules/srv6/cli/localsid.c
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/srv6/cli/localsid.cmodules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/srv6/control/iface_nh.cmodules/srv6/cli/iface_nh.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/srv6/control/iface_nh.c
🧬 Code graph analysis (5)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
modules/infra/cli/gr_cli_iface.h (1)
modules/infra/cli/iface.c (6)
register_iface_type(23-25)register_iface_mode(27-29)type_from_name(55-69)type_from_id(90-99)mode_from_id(101-110)with_iface_set_callback(31-40)
modules/infra/cli/port.c (1)
modules/infra/cli/iface.c (5)
iface_from_name(134-147)complete_iface_names(112-132)iface_from_id(149-157)constructor(665-669)register_iface_mode(27-29)
modules/infra/control/xconnect.c (1)
modules/infra/control/iface.c (1)
iface_mode_register(58-64)
modules/srv6/control/iface_nh.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-25)modules/infra/control/nexthop.c (1)
nexthop_lookup_by_id(491-498)modules/infra/control/iface.c (1)
iface_mode_register(58-64)main/event.c (1)
gr_event_subscribe(16-18)
🪛 Cppcheck (2.18.0)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.8)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
17-17: conf may be undefined, or defined from star imports
(F405)
34-34: sendp may be undefined, or defined from star imports
(F405)
34-34: Ether may be undefined, or defined from star imports
(F405)
39-39: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (8)
.github/workflows/check.yml (1)
76-76: LGTM!Adding
scapyto CI dependencies aligns with the new SRv6 L2 encapsulation test requirements.modules/srv6/control/meson.build (1)
4-8: LGTM!Build file correctly adds the new
iface_nh.csource to the SRv6 control module.modules/infra/control/gr_iface.h (1)
60-75: LGTM!The new
iface_modeAPI follows the established pattern ofiface_typeand provides a clean extension point for mode-specific init/reconfig hooks.smoke/srv6_l2_encap_scapy.py (1)
17-17: Unclear purpose of route lookup.
conf.route6.route()returns routing information but doesn't modify anything. If this is meant to verify the route exists, consider adding an assertion or removing it if unnecessary.modules/srv6/datapath/srv6.h (1)
1-9: LGTM!The private mbuf data type definition is clean and properly structured for carrying nexthop state through the SRv6 DX2 datapath.
modules/infra/control/xconnect.c (1)
38-44: LGTM!The L1 XConnect mode registration follows the established pattern.
modules/srv6/datapath/srv6_output.c (1)
90-93: LGTM!The Ethernet L2 encapsulation path correctly retrieves the nexthop from mbuf private data and uses appropriate payload length and protocol values.
modules/srv6/control/iface_nh.c (1)
1-83: LGTM!The SRv6 interface-nexthop lifecycle management is well-structured. The cleanup handler properly clears stale mode_data references when nexthops are deleted, and the init/reconfig hooks correctly resolve nexthop IDs to pointers.
| case SR_BEHAVIOR_END_DX2: | ||
| action = ZEBRA_SEG6_LOCAL_ACTION_END_DX2; | ||
| break; |
There was a problem hiding this comment.
Missing reverse mapping for END_DX2 in grout_add_nexthop.
The forward mapping (grout → FRR) is added here, but the reverse mapping in grout_add_nexthop (around lines 703-738) lacks a case for ZEBRA_SEG6_LOCAL_ACTION_END_DX2. This means FRR-originated END_DX2 nexthops will hit the default error path and fail to sync to grout.
Add the missing case:
case ZEBRA_SEG6_LOCAL_ACTION_END_DT46:
sr6_local->behavior = SR_BEHAVIOR_END_DT46;
sr6_local->out_vrf_id = ifindex_frr_to_grout(
nh->nh_srv6->seg6local_ctx.table
);
break;
+ case ZEBRA_SEG6_LOCAL_ACTION_END_DX2:
+ sr6_local->behavior = SR_BEHAVIOR_END_DX2;
+ sr6_local->out_vrf_id = GR_VRF_ID_ALL;
+ break;
default:Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frr/rt_grout.c around lines 703 to 738, grout_add_nexthop is missing a
reverse mapping for ZEBRA_SEG6_LOCAL_ACTION_END_DX2 which causes FRR-originated
END_DX2 nexthops to fall through to the default error path; add a case for
ZEBRA_SEG6_LOCAL_ACTION_END_DX2 that sets the corresponding grout behavior to
SR_BEHAVIOR_END_DX2 (and populate any behavior-specific fields/parameters the
same way other END_* cases do), then ensure the nexthop is built and returned as
the other mapped behaviors are handled.
837e8f5 to
f5c98d0
Compare
f5c98d0 to
f15039d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
.github/workflows/check.yml(1 hunks)README.md(1 hunks)frr/rt_grout.c(1 hunks)modules/infra/api/gr_infra.h(3 hunks)modules/infra/cli/gr_cli_iface.h(4 hunks)modules/infra/cli/iface.c(12 hunks)modules/infra/cli/port.c(3 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/iface.c(6 hunks)modules/infra/control/xconnect.c(1 hunks)modules/ip/control/icmp.c(0 hunks)modules/ip6/control/icmp6.c(2 hunks)modules/srv6/api/gr_srv6.h(3 hunks)modules/srv6/cli/iface_nh.c(1 hunks)modules/srv6/cli/localsid.c(5 hunks)modules/srv6/cli/meson.build(1 hunks)modules/srv6/control/iface_nh.c(1 hunks)modules/srv6/control/meson.build(1 hunks)modules/srv6/datapath/l2_encap.c(1 hunks)modules/srv6/datapath/meson.build(1 hunks)modules/srv6/datapath/srv6.h(1 hunks)modules/srv6/datapath/srv6_local.c(6 hunks)modules/srv6/datapath/srv6_output.c(2 hunks)smoke/srv6_l2_encap_scapy.py(1 hunks)smoke/srv6_l2_encap_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/ip/control/icmp.c
🚧 Files skipped from review as they are similar to previous changes (11)
- modules/infra/api/gr_infra.h
- modules/srv6/control/meson.build
- .github/workflows/check.yml
- modules/srv6/control/iface_nh.c
- README.md
- modules/srv6/datapath/srv6_output.c
- modules/srv6/datapath/srv6.h
- modules/srv6/datapath/meson.build
- frr/rt_grout.c
- modules/infra/cli/gr_cli_iface.h
- modules/srv6/cli/localsid.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/iface.cmodules/srv6/datapath/srv6_local.cmodules/ip6/control/icmp6.cmodules/infra/cli/iface.cmodules/srv6/api/gr_srv6.hmodules/infra/cli/port.cmodules/infra/control/xconnect.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/srv6_l2_encap_test.sh
🧠 Learnings (14)
📚 Learning: 2025-12-17T17:32:11.230Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:11.230Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/iface.cmodules/srv6/datapath/srv6_local.cmodules/infra/cli/iface.cmodules/srv6/api/gr_srv6.hmodules/infra/control/xconnect.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/iface.csmoke/srv6_l2_encap_test.shmodules/srv6/datapath/srv6_local.cmodules/infra/control/gr_iface.hmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/srv6/cli/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/srv6/cli/meson.build
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/srv6/datapath/srv6_local.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/srv6/datapath/srv6_local.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/srv6/datapath/srv6_local.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-09T07:36:34.893Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T07:36:34.893Z
Learning: In the SRv6 routing header structure, the flag and tag fields are used for per-flow metadata and Group-Based Policy use cases. Only 5 bytes are needed to access the basic SRv6 fields: next_hdr, hdr_len, type, segments_left, and last_entry.
Applied to files:
modules/srv6/api/gr_srv6.hmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/srv6/cli/iface_nh.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/srv6/cli/iface_nh.cmodules/srv6/datapath/l2_encap.c
📚 Learning: 2025-09-09T07:08:40.398Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:106-111
Timestamp: 2025-09-09T07:08:40.398Z
Learning: For SRv6 header validation, the correct check for last_entry bounds is `srh->last_entry > (srh->hdrlen / 2) - 1` where hdr_len represents the length in 8-byte units excluding the first 8 bytes, and each segment is 16 bytes, so hdr_len/2 gives the maximum number of segments.
Applied to files:
modules/srv6/datapath/l2_encap.c
🧬 Code graph analysis (6)
modules/infra/control/iface.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-26)
smoke/srv6_l2_encap_test.sh (1)
smoke/_init.sh (2)
netns_add(107-115)move_to_netns(117-131)
modules/srv6/datapath/srv6_local.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/control/iface.c (1)
iface_from_id(284-291)
modules/infra/control/xconnect.c (1)
modules/infra/control/iface.c (1)
iface_mode_register(58-64)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (4)
iface_type_register(39-45)iface_mode_register(58-64)iface_type_get(30-37)iface_mode_get(49-56)
modules/srv6/datapath/l2_encap.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-26)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(592-619)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(23-26)modules/infra/datapath/port_rx.c (1)
register_interface_mode(33-37)
🪛 Cppcheck (2.18.0)
modules/srv6/cli/iface_nh.c
[warning] 23-23: Uninitialized variable
(uninitvar)
modules/srv6/datapath/l2_encap.c
[error] 9-9: There is an unknown macro here somewhere. Configuration is required. If GR_MBUF_PRIV_DATA_TYPE is a macro then please configure it.
(unknownMacro)
🪛 Ruff (0.14.8)
smoke/srv6_l2_encap_scapy.py
5-5: from scapy.all import * used; unable to detect undefined names
(F403)
17-17: conf may be undefined, or defined from star imports
(F405)
34-34: sendp may be undefined, or defined from star imports
(F405)
34-34: Ether may be undefined, or defined from star imports
(F405)
39-39: sniff may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
smoke/srv6_l2_encap_test.sh
[warning] 5-5: 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). (6)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, 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-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: build-cross-aarch64
🔇 Additional comments (7)
modules/srv6/cli/meson.build (1)
4-8: LGTM!Build file correctly adds the new SRv6 CLI source file following the project's modular build pattern.
modules/srv6/datapath/l2_encap.c (1)
16-44: LGTM!The L2 encapsulation datapath node correctly retrieves the nexthop from
iface->mode_info, handles tracing, and routes mbufs appropriately based on nexthop availability.modules/infra/cli/port.c (1)
192-268: LGTM!The L1 XC mode implementation properly manages resources and follows the project's interface mode registration pattern.
modules/srv6/cli/iface_nh.c (1)
17-27: LGTM!The
nh_from_idhelper correctly streams through nexthops and safely returns NULL if no matching nexthop is found, avoiding the out-of-bounds access issue from the prior approach.modules/srv6/api/gr_srv6.h (1)
100-114: LGTM!The new interface nexthop mapping API follows the existing request type pattern and provides a clean interface for SRv6 L2 encapsulation configuration.
modules/infra/control/iface.c (2)
49-64: LGTM!The
iface_mode_getandiface_mode_registerfunctions properly mirror the existingiface_type_*pattern with appropriate validation and error handling.
96-104: LGTM!The NULL check for mode has been properly added, addressing the previously flagged issue about potential NULL pointer dereference.
| scols_line_sprintf(line, 3, "%u", iface->mode); | ||
| break; | ||
| } | ||
| scols_line_sprintf(line, 3, "%s", mode->str); |
There was a problem hiding this comment.
Potential NULL pointer dereference if mode is unregistered.
If mode_from_id(iface->mode) returns NULL (e.g., an interface has a mode ID that wasn't registered), mode->str will crash. Unlike iface_show which uses assert(mode), this path has no guard.
🔎 Suggested fix:
+ const char *mode_str = mode ? mode->str : "?";
// mode
- scols_line_sprintf(line, 3, "%s", mode->str);
+ scols_line_sprintf(line, 3, "%s", mode_str);🤖 Prompt for AI Agents
In modules/infra/cli/iface.c around line 313, the call scols_line_sprintf(line,
3, "%s", mode->str) can dereference a NULL when mode_from_id(iface->mode)
returned NULL; add a NULL check before using mode->str and handle the case by
either asserting (assert(mode)) to maintain current behavior or by passing a
safe placeholder string (e.g. "<unknown mode>") to scols_line_sprintf and
optionally logging a warning; ensure the function does not dereference mode when
it is NULL.
| if (mode->list_info) { | ||
| buf[0] = 0; | ||
| mode->list_info(c, iface, buf, sizeof(buf)); | ||
| scols_line_set_data(line, 7, buf); | ||
| } |
There was a problem hiding this comment.
Same NULL dereference risk for mode->list_info.
If mode is NULL, the check mode->list_info will crash before even evaluating the condition.
🔎 Suggested fix:
- if (mode->list_info) {
+ if (mode && mode->list_info) {
buf[0] = 0;
mode->list_info(c, iface, buf, sizeof(buf));
scols_line_set_data(line, 7, buf);
}🤖 Prompt for AI Agents
In modules/infra/cli/iface.c around lines 326 to 330, the condition currently
dereferences mode (mode->list_info) and can NULL-deref if mode is NULL; update
the guard to first check mode is non-NULL before accessing its member (e.g., if
(mode && mode->list_info) { ... }), then proceed to call mode->list_info and set
the column data as before.
| static struct iface_mode iface_mode_xconnect = { | ||
| .id = GR_IFACE_MODE_L1_XCONNECT, | ||
| }; | ||
|
|
||
| RTE_INIT(l2_constructor) { | ||
| iface_mode_register(&iface_mode_xconnect); | ||
| gr_register_api_handler(&l2_mode_set_handler); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for both mode constants in the codebase
rg -n "GR_IFACE_MODE_L1_X" --type=c --type-add 'c:*.h' -A 2 -B 2Repository: DPDK/grout
Length of output: 1756
🏁 Script executed:
#!/bin/bash
# Search for the XCONNECT variant specifically
rg "GR_IFACE_MODE_L1_XCONNECT" --type=c --type-add 'c:*.h'Repository: DPDK/grout
Length of output: 120
Fix undefined constant in mode registration.
Line 39 uses GR_IFACE_MODE_L1_XCONNECT, which does not exist in the enum. The correct constant is GR_IFACE_MODE_L1_XC as defined in modules/infra/api/gr_infra.h and used throughout the codebase.
🤖 Prompt for AI Agents
In modules/infra/control/xconnect.c around lines 38 to 44 the iface_mode struct
is initialized with an undefined enum constant GR_IFACE_MODE_L1_XCONNECT;
replace that constant with the correct enum value GR_IFACE_MODE_L1_XC so the
registration uses the defined mode; update the .id initializer to
GR_IFACE_MODE_L1_XC and keep the surrounding registration calls unchanged.
| SAFE_BUF(snprintf, len, "SRV6 L2 Encap: "); | ||
| cli_nexthop_format(&buf[n], len + n, c, nh, false); |
There was a problem hiding this comment.
Buffer size calculation bug: potential overflow.
After SAFE_BUF writes n bytes, the remaining buffer space at &buf[n] is len - n, not len + n. Passing len + n tells cli_nexthop_format it has more space than the original buffer, which could cause a buffer overflow.
🔎 Apply this diff to fix the buffer size:
SAFE_BUF(snprintf, len, "SRV6 L2 Encap: ");
- cli_nexthop_format(&buf[n], len + n, c, nh, false);
+ cli_nexthop_format(&buf[n], len - n, c, nh, false);🤖 Prompt for AI Agents
In modules/srv6/cli/iface_nh.c around lines 52 to 53, the call to
cli_nexthop_format passes an incorrect remaining buffer size (len + n) which can
overflow; change the size argument to the actual remaining space (len - n) when
calling cli_nexthop_format(&buf[n], len - n, c, nh, false), ensure n is the
number of bytes written by SAFE_BUF and add a guard to handle n >= len (e.g.,
avoid the call or truncate) so you never pass a negative/overflowing length.
There was a problem hiding this comment.
You can probably change to:
SAFE_BUF(cli_nexthop_format, len, c, nh, false);There was a problem hiding this comment.
@rjarry That's a much cleaner solution! Using SAFE_BUF consistently for both operations eliminates the manual pointer arithmetic and buffer size calculations entirely, making the code more maintainable and less error-prone.
The revised code would be:
SAFE_BUF(snprintf, len, "SRV6 L2 Encap: ");
SAFE_BUF(cli_nexthop_format, len, c, nh, false);This handles all the buffer tracking automatically through the macro. Thank you for the suggestion!
🧠 Learnings used
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:11.230Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
| ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py & | ||
| sleep 3 |
There was a problem hiding this comment.
Verify the background Python script started successfully.
Starting the Python script in the background without checking if it launched successfully can lead to confusing test failures. If the script fails to start (e.g., missing scapy dependency, wrong path, permission issues), the ping test will fail with an error message that doesn't indicate the root cause.
🔎 Add verification after starting the background process:
ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py &
+SCAPY_PID=$!
sleep 3
+
+# Verify the scapy script is still running
+if ! kill -0 $SCAPY_PID 2>/dev/null; then
+ echo "ERROR: SRv6 encapsulation script failed to start"
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py & | |
| sleep 3 | |
| ip netns exec n1 python3 smoke/srv6_l2_encap_scapy.py & | |
| SCAPY_PID=$! | |
| sleep 3 | |
| # Verify the scapy script is still running | |
| if ! kill -0 $SCAPY_PID 2>/dev/null; then | |
| echo "ERROR: SRv6 encapsulation script failed to start" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In smoke/srv6_l2_encap_test.sh around lines 54-55, after launching the Python
script in the background you must verify it actually started: start the process
with its stdout/stderr redirected to a logfile, capture its PID ($!)
immediately, wait a short period, then check the PID is still alive (e.g., kill
-0 $PID or ps -p $PID) and if not, print the logfile and exit the test with a
non-zero status so failures surface immediately.
| struct icmp_queue_item *i, *tmp; | ||
| STAILQ_FOREACH_SAFE (i, &icmp_queue, next, tmp) | ||
| icmp_queue_pop(i, true); |
There was a problem hiding this comment.
Could you note in the commit message that it happens because:
- Since icmp{,6}_init do not depend on any other init callbacks, they are executed first. And their _fini counterparts are executed last.
- Other _fini routines are executed before them (such as iface_fini or port_fini). These callbacks actually free the mbuf memory pools used by datapath.
- Packets present in the icmp{,6} queues contain references to the mbufs which were freed by iface_fini. When icmp{,6}_fini try to free them again, it causes a double free (reported as use-after-free).
| ec_node_uint("VRF", 0, UINT16_MAX - 1, 10) \ | ||
| ) | ||
| ), \ | ||
| with_help("interface mode", EC_NODE_OR("IFACE_MODE", ec_node_str("", "l3"))) |
There was a problem hiding this comment.
Nit picking:
with_help( \
"Interface mode.", \
EC_NODE_OR("IFACE_MODE", with_help("L3 routing.", ec_node_str("", "l3"))) \
)| while ((iface_mode = ec_node_find_next(root, iface_mode, "IFACE_MODE"))) { | ||
| STAILQ_FOREACH (mode, &modes, next) { | ||
| if ((ret = mode->init(iface_mode)) < 0) | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Can you add a comment explaining that there are as many IFACE_MODE nodes as number of interfaces and that we need to call mode->init() on all of them?
| with_help( | ||
| "l1 cross connect", | ||
| EC_NODE_SEQ( | ||
| "", | ||
| ec_node_str("", "xconnect"), | ||
| with_iface_set_callback( | ||
| l1_xc_set_peer, | ||
| with_help( | ||
| "Peer interface for xconnect", | ||
| ec_node_dyn( | ||
| "PEER", | ||
| complete_iface_names, | ||
| INT2PTR(GR_IFACE_TYPE_PORT) |
There was a problem hiding this comment.
This will insert the l1 mode in all interface types (vlan, bond, ipip). It would be interesting to only have this mode available on ports.
| return ec_node_or_add( | ||
| mode, | ||
| with_help( | ||
| "l1 cross connect", |
There was a problem hiding this comment.
Nit pick:
"L1 cross-connect.",| m->hash.usr = rte_be_to_cpu_32(ip6_info->ip6_hdr->vtc_flow) & RTE_IPV6_HDR_FL_MASK; | ||
| rte_pktmbuf_adj(m, ip6_info->ext_offset); | ||
| return edge; | ||
| break; |
| gr_api_client_stream_foreach (nh, ret, c, GR_NH_LIST, sizeof(req), &req) { | ||
| if (nh->nh_id == nh_id) | ||
| return nh; | ||
| } |
There was a problem hiding this comment.
Returning before the end of the loop causes a memory leak. The allocated nh will not be freed. Also, all messages in the stream response must be consumed.
| // Set interface to nexthop mapping for SRv6 L2 encapsulation. | ||
| #define GR_SRV6_IFACE_NH_SET REQUEST_TYPE(GR_SRV6_MODULE, 0x0008) | ||
| struct gr_srv6_iface_nh_set_req { | ||
| uint16_t iface_id; | ||
| uint32_t nh_id; | ||
| }; | ||
|
|
||
| // Delete interface to nexthop mapping. | ||
| #define GR_SRV6_IFACE_NH_DEL REQUEST_TYPE(GR_SRV6_MODULE, 0x0009) | ||
| struct gr_srv6_iface_nh_del_req { | ||
| uint16_t iface_id; | ||
| }; |
There was a problem hiding this comment.
This would benefit from some explanations in the commit message. What is the purpose of these new API endpoints? They are not used anywhere.
| "srv6 l2 encap", | ||
| EC_NODE_SEQ( | ||
| "", | ||
| ec_node_str("", "srv6_l2_encap"), |
There was a problem hiding this comment.
For consistency with gr_infra.h (and use kebab-case):
ec_node_str("", "srv6-evpn"),| uint16_t vrf_id; // L3 addressing and routing domain | ||
| uint16_t domain_id; // L2 xconnect peer interface id | ||
| }; | ||
| uint32_t mode_data; // Abstract mode data |
There was a problem hiding this comment.
This feels a bit hacky. Could you explain the reasoning behind this opaque parameter?
| m->hash.usr = rte_be_to_cpu_32(ip6_info->ip6_hdr->vtc_flow) & RTE_IPV6_HDR_FL_MASK; | ||
| rte_pktmbuf_adj(m, ip6_info->ext_offset); | ||
| return edge; | ||
| break; |
There was a problem hiding this comment.
I don't really like how it's coded here.
I would prefer have 2 functions:
process_behav_ip_decap and process_behave_decap_eth
Else you need to factorize more the code in process_behav_decap to manage both case eth & ip.
No description provided.