iface rework #340
iface rework #340
Conversation
📝 WalkthroughWalkthroughIntroduces GR_IFACE_INFO and per-type accessors to replace direct iface->info casts across infra and ipip modules. Expands struct iface_type with explicit per-attribute setter callbacks and makes iface_type_get const-correct. Updates control modules (iface, port, vlan, loopback, ipip, mempool) to use accessors and new setters; adjusts subinterface mutability and event handling. Datapath nodes (eth_input/eth_output, ipip in/out, port_output, l1_xconnect) add an IFACE_DOWN edge and route admin-down interfaces to iface_input_admin_down. Miscellaneous CLI and test updates reflect these API/implementation changes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/infra/datapath/l1_xconnect.c (1)
27-46: Null-deref risk: peer may be NULL from iface_from_id().peer->type is dereferenced without a NULL check. Guard to avoid crashes under reconfig/races.
Apply:
- peer = iface_from_id(iface->domain_id); + peer = iface_from_id(iface->domain_id); ... - if (peer->type == GR_IFACE_TYPE_PORT) { + if (peer != NULL && peer->type == GR_IFACE_TYPE_PORT) { port = iface_info_port(peer); mbuf->port = port->port_id; edge = OUTPUT; struct iface_stats *tx_stats = iface_get_stats(rte_lcore_id(), peer->id); tx_stats->tx_packets++; tx_stats->tx_bytes += rte_pktmbuf_pkt_len(mbuf); } else { edge = NO_PORT; }modules/infra/control/gr_port.h (1)
30-40: Add<stdbool.h>include
boolis now used here but the header never pulls in<stdbool.h>, so any TU including this file alone will fail to compile. Add the missing include near the top.#include <gr_iface.h> +#include <stdbool.h>modules/infra/control/vlan.c (1)
60-89: Parent switch does not migrate MAC filter or refresh inherited MACWhen changing parent without GR_VLAN_SET_MAC, previous MAC filter is not removed/added and VLAN MAC may remain tied to the old parent. This leaves stale filters and a wrong MAC on the VLAN.
Consider migrating explicit MACs and refreshing inherited MACs on parent change:
@@ - if (reconfig) { + if (reconfig) { // reconfig, *not initial config* struct vlan_key cur_key = {cur->parent_id, cur->vlan_id}; rte_hash_del_key(vlan_hash, &cur_key); iface_del_subinterface(cur_parent, iface); // remove previous vlan filter (ignore errors) iface_del_vlan(cur->parent_id, cur->vlan_id); } - ret = iface_add_vlan(next->parent_id, next->vlan_id); + ret = iface_add_vlan(next->parent_id, next->vlan_id); if (ret < 0) return ret; - cur->parent_id = next->parent_id; + uint16_t old_parent_id = cur->parent_id; + cur->parent_id = next->parent_id; cur->vlan_id = next->vlan_id; iface_add_subinterface(next_parent, iface); iface->state = next_parent->state; iface->mtu = next_parent->mtu; + + // If parent changed but MAC not explicitly set, migrate/refresh MAC. + if (reconfig && !(set_attrs & GR_VLAN_SET_MAC)) { + struct rte_ether_addr old_mac; + // If VLAN had an explicit MAC (different from old parent MAC), move the filter. + if (iface_get_eth_addr(old_parent_id, &old_mac) == 0 && + !rte_is_zero_ether_addr(&cur->mac)) { + if (!rte_is_same_ether_addr(&cur->mac, &old_mac)) { + // explicit VLAN MAC: move filter + iface_del_eth_addr(old_parent_id, &cur->mac); // ignore errors + ret = iface_add_eth_addr(next->parent_id, &cur->mac); + if (ret < 0) + return ret; + } else { + // inherited MAC: refresh from new parent + ret = iface_get_eth_addr(next->parent_id, &cur->mac); + if (ret < 0) + return ret; + } + } else { + // No prior MAC: inherit from new parent + ret = iface_get_eth_addr(next->parent_id, &cur->mac); + if (ret < 0) + return ret; + } + } @@ if (set_attrs & GR_VLAN_SET_MAC) { if (rte_is_zero_ether_addr(&next->mac)) { if ((ret = iface_get_eth_addr(next->parent_id, &cur->mac)) < 0) return ret; } else { if ((ret = iface_add_eth_addr(next->parent_id, &next->mac)) < 0) return ret; cur->mac = next->mac; } }Also applies to: 92-101
🧹 Nitpick comments (7)
main/vec_test.c (2)
89-97: LGTM; add a tiny postcondition check (optional).After gr_strvec_free(vec), asserting vec is NULL would lock in the contract.
Apply:
static void dyn_str_vec(void **) { gr_vec char **vec = NULL; gr_vec_add(vec, strdup("foo")); gr_vec_add(vec, strdup("bar")); gr_vec_add(vec, strdup("baz")); gr_strvec_free(vec); + assert_null(vec); }Based on learnings
99-127: LGTM; consider adding extend edge-case coverage (optional).
- Extend into NULL destination should copy all items.
- Extend with empty source should no-op.
Example snippet to add:
static void ext_vec(void **) { gr_vec const char **vec1 = NULL; gr_vec const char **vec2 = NULL; + gr_vec const char **empty = NULL; gr_vec_add(vec1, "foo1"); gr_vec_add(vec1, "bar1"); gr_vec_add(vec1, "baz1"); gr_vec_add(vec2, "foo2"); gr_vec_add(vec2, "bar2"); gr_vec_add(vec2, "baz2"); assert_int_equal(gr_vec_len(vec1), 3); assert_int_equal(gr_vec_len(vec2), 3); gr_vec_extend(vec1, vec2); assert_int_equal(gr_vec_len(vec1), 6); assert_string_equal(vec1[0], "foo1"); assert_string_equal(vec1[1], "bar1"); assert_string_equal(vec1[2], "baz1"); assert_string_equal(vec1[3], "foo2"); assert_string_equal(vec1[4], "bar2"); assert_string_equal(vec1[5], "baz2"); + + // extend into NULL dest copies items + gr_vec const char **copy = NULL; + gr_vec_extend(copy, vec2); + assert_int_equal(gr_vec_len(copy), gr_vec_len(vec2)); + assert_string_equal(copy[0], "foo2"); + gr_vec_free(copy); + + // extend with empty src is a no-op + uint32_t before = gr_vec_len(vec1); + gr_vec_extend(vec1, empty); + assert_int_equal(gr_vec_len(vec1), before); gr_vec_free(vec1); gr_vec_free(vec2); }main/gr_vec.h (1)
142-155: Guard against self-extend to avoid undefined behavior.gr_vec_extend(v, v) would memcpy overlapping memory. Add a defensive check.
Apply:
static inline void *__gr_vec_extend(void *vec, const void *v, size_t item_size) { uint32_t ext_len = gr_vec_len(v); if (ext_len == 0) return vec; + // Prevent overlap/UB if someone extends a vector with itself + if (v == vec) { +#ifndef NDEBUG + __gr_vec_abort("gr_vec_extend: source and destination are identical"); +#endif + return vec; + } vec = __gr_vec_grow(vec, item_size, ext_len, 0); memcpy((void *)((uintptr_t)vec + (item_size * gr_vec_len(vec))), v, ext_len * item_size); __gr_vec_hdr(vec)->len += ext_len; return vec; }modules/infra/datapath/main_loop.c (1)
86-111: Solid reload/visibility/cleanup refactor; preserve error cause (optional).Great improvements. One nit: err: always returns -ENOMEM, masking other causes (e.g., EINVAL from topo_sort or rte_* APIs). Consider propagating the most relevant errno.
Example:
-err: +err: { + int err = rte_errno ? rte_errno : (errno ? errno : ENOMEM); gr_vec_free(nodes); if (ctx->stats != NULL) { rte_graph_cluster_stats_destroy(ctx->stats); ctx->stats = NULL; } rte_free(ctx->w_stats); ctx->w_stats = NULL; rte_free(ctx->node_to_index); ctx->node_to_index = NULL; - return -ENOMEM; + return -err; +}Minor: qsort(nodes, count, sizeof(*nodes), node_name_cmp) reads a bit clearer than sizeof(void *).
Also applies to: 112-131, 133-145, 146-155, 157-168, 176-183, 227-230, 292-293
modules/infra/datapath/gr_rxtx.h (1)
34-36: Public API name too generic; prefer namespacingregister_interface_mode risks collisions. Consider a prefixed name (e.g., port_rx_register_interface_mode).
modules/infra/control/worker.c (1)
278-282: Use iface_info_port() accessor instead of raw castDirectly casting iface->info bypasses the accessor abstraction and can break if layout changes.
Apply:
- while ((iface = iface_next(GR_IFACE_TYPE_PORT, iface)) != NULL) - gr_vec_add(ports, (struct iface_info_port *)iface->info); + while ((iface = iface_next(GR_IFACE_TYPE_PORT, iface)) != NULL) + gr_vec_add(ports, iface_info_port(iface));modules/infra/control/gr_iface.h (1)
24-29: Const-correctness hazard in GR_IFACE_INFO accessorReturning non-const info* from a const iface* enables accidental mutation through const handles. Add a const-safe accessor alongside the mutable one.
#define GR_IFACE_INFO(type_id, type_name, fields) \ struct type_name fields __attribute__((__may_alias__, aligned(alignof(void *)))); \ - static inline struct type_name *type_name(const struct iface *iface) { \ + /* Mutable accessor */ \ + static inline struct type_name *type_name(struct iface *iface) { \ assert(iface->type == type_id); \ return (struct type_name *)iface->info; \ } \ + /* Const accessor */ \ + static inline const struct type_name *type_name##_const(const struct iface *iface) { \ + assert(iface->type == type_id); \ + return (const struct type_name *)iface->info; \ + }Adopting this avoids breaking existing callers while enabling const-safe reads.
📜 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 (41)
main/gr_vec.h(2 hunks)main/vec_test.c(1 hunks)meson.build(1 hunks)modules/infra/api/affinity.c(1 hunks)modules/infra/api/gr_infra.h(2 hunks)modules/infra/api/stats.c(6 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_graph.h(0 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/graph.c(11 hunks)modules/infra/control/graph_priv.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/mempool.c(1 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(6 hunks)modules/infra/control/worker.c(7 hunks)modules/infra/control/worker_priv.h(1 hunks)modules/infra/control/worker_test.c(4 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(4 hunks)modules/infra/datapath/gr_eth.h(1 hunks)modules/infra/datapath/gr_icmp6.h(5 hunks)modules/infra/datapath/gr_rxtx.h(1 hunks)modules/infra/datapath/l1_xconnect.c(4 hunks)modules/infra/datapath/main_loop.c(5 hunks)modules/infra/datapath/meson.build(1 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/infra/datapath/port_rx.c(1 hunks)modules/infra/datapath/port_tx.c(1 hunks)modules/infra/datapath/rx.c(0 hunks)modules/infra/datapath/tx.c(0 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)smoke/_init.sh(1 hunks)smoke/affinity_test.sh(1 hunks)subprojects/dpdk.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/infra/control/gr_graph.h
- modules/infra/datapath/tx.c
- modules/infra/datapath/rx.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
PR: DPDK/grout#326
File: modules/infra/control/graph.c:360-364
Timestamp: 2025-10-02T07:42:42.135Z
Learning: In the grout codebase, `gr_vec_free()` is a macro defined in `main/gr_vec.h` that automatically sets the vector pointer to NULL after freeing the memory, preventing double-free issues. The macro definition is: `#define gr_vec_free(v) ((v) ? free(__gr_vec_hdr(v)) : (void)0, (v) = NULL)`.
Applied to files:
main/gr_vec.h
🧬 Code graph analysis (22)
modules/infra/control/worker_priv.h (1)
modules/infra/control/worker.c (2)
port_unplug(145-182)port_plug(184-223)
modules/infra/datapath/gr_eth.h (1)
modules/infra/datapath/eth_output.c (1)
eth_output_register_interface_type(27-34)
modules/ipip/control.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/graph.c (3)
api/string.c (1)
astrcat(15-50)main/gr_vec.h (1)
gr_vec_len(47-51)api/gr_errno.h (1)
errno_set(9-12)
modules/infra/api/stats.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/api/gr_infra.h (2)
gr_iface_type_name(322-338)gr_iface_mode_name(341-351)
modules/infra/datapath/eth_output.c (4)
modules/infra/api/gr_infra.h (1)
gr_iface_type_name(322-338)modules/infra/control/graph.c (1)
gr_node_attach_parent(36-64)modules/infra/control/iface.c (2)
iface_from_id(254-261)iface_get_eth_addr(263-276)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)
modules/infra/cli/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
main/vec_test.c (1)
main/gr_vec.h (1)
gr_vec_len(47-51)
modules/infra/datapath/main_loop.c (1)
main/sort.c (1)
topo_sort(53-97)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/graph_priv.h (1)
modules/infra/control/graph.c (2)
worker_graph_reload(207-230)worker_graph_reload_all(304-322)
modules/infra/control/iface.c (6)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)modules/infra/control/worker_test.c (2)
gr_event_push(36-36)iface_from_id(40-42)modules/ip6/control/router_advert.c (1)
iface_event_handler(223-239)main/event.c (1)
gr_event_subscribe(14-16)
modules/infra/control/worker_test.c (2)
modules/infra/control/graph.c (2)
worker_graph_reload(207-230)worker_graph_reload_all(304-322)modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (3)
port_unplug(145-182)port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
modules/infra/control/worker.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (1)
iface_next(237-252)modules/infra/control/graph.c (2)
worker_graph_reload_all(304-322)worker_graph_reload(207-230)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/port_tx.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/datapath/trace.c (3)
trace_log_packet(354-504)gr_mbuf_trace_add(520-547)gr_mbuf_trace_finish(549-565)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/port_rx.c (1)
rxtx_trace_format(25-28)
modules/infra/control/vlan.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_type_get(28-35)
modules/infra/datapath/port_output.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(520-547)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)modules/infra/datapath/eth_output.c (1)
eth_output_register_interface_type(27-34)
modules/infra/datapath/port_rx.c (2)
modules/infra/control/graph.c (1)
gr_node_attach_parent(36-64)modules/infra/datapath/trace.c (2)
gr_mbuf_trace_add(520-547)trace_log_packet(354-504)
modules/infra/control/gr_iface.h (2)
modules/infra/control/worker_test.c (1)
iface_type_register(35-35)modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
⏰ 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: rpm
- GitHub Check: deb
- 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-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (30)
modules/infra/datapath/gr_icmp6.h (1)
38-38: LGTM! Standardization improves portability.Replacing
__rte_packedwith explicit__attribute__((__packed__))across all ICMPv6 structures is a good practice. The change is syntactically equivalent and makes the packing intent more explicit. Alignment attribute correctly preserved on line 131.Also applies to: 43-43, 48-48, 53-53, 58-58, 64-64, 70-70, 75-75, 88-88, 94-94, 106-106, 122-122, 131-131
modules/infra/control/mempool.c (2)
113-113: Assert guards against underflow.The assertion correctly prevents unsigned integer underflow when
count > mt->reserved. This aligns with the PR's goal to guard against programmer errors.However, if this is a public API, verify whether assert-only validation is sufficient, as asserts may be compiled out in release builds. If invalid counts from external callers are a concern, consider adding a runtime check that returns an error or logs a warning.
121-121: Clearer condition for unsigned type.Changing
mt->reserved <= 0tomt->reserved == 0is clearer and more precise. Sincemt->reservedis unsigned (uint32_t), the old condition was functionally equivalent but less explicit about intent.modules/infra/datapath/meson.build (1)
15-17: LGTM: Datapath refactoring aligns with PR objectives.The split of rx.c and tx.c into port_output.c, port_rx.c, and port_tx.c aligns with the PR's datapath rework and port-centric approach.
meson.build (1)
70-70: Confirm Intel driver paths
Intel PMDs i40e, ice, iavf, ixgbe all reside under drivers/net/intel in DPDK v25.07, sonet/intel/<driver>is correct.modules/infra/datapath/l1_xconnect.c (1)
15-18: LGTM: OUTPUT edge and node/drop renaming are consistent.Edge enum, next_nodes to "port_output", and GR_DROP_REGISTER(xconnect_no_port) align with the new OUTPUT semantics.
Also applies to: 57-65, 77-77
main/gr_vec.h (1)
156-163: LGTM: string-vector free and extend macros are sane.
- __gr_strvec_free correctly frees items and header, safe on NULL.
- gr_strvec_free and gr_vec_extend macros wire up as expected.
Also applies to: 171-173, 186-188
modules/infra/cli/port.c (1)
48-54: SAFE_BUF usage is correct
Macro subtracts the running offset (n) from the supplied buffer size, so passing the total size (len) is appropriate.modules/infra/api/gr_infra.h (1)
322-322: LGTM! Consistent naming convention.The rename from
iface_type_to_str/iface_mode_to_strtogr_iface_type_name/gr_iface_mode_nameimproves API consistency with the module prefix pattern.Also applies to: 341-341
modules/ipip/control.c (2)
54-54: LGTM! Accessor pattern improves type safety.Replacing direct casts with
iface_info_ipip(iface)accessor is cleaner and aligns with the accessor pattern introduced across the codebase.Also applies to: 86-86, 113-113
95-102: LGTM! Clear default MTU initialization.The explicit MTU default of 1480 for IPIP tunnels is appropriate and makes the initialization logic clearer.
modules/infra/api/stats.c (1)
60-61: LGTM! Consistent accessor pattern and naming.The migration to
iface_info_port(iface)accessor and updated function names (gr_iface_type_name,gr_iface_mode_name) aligns with the broader refactoring patterns in this PR.Also applies to: 131-131, 243-243, 281-281, 359-359, 376-376
modules/infra/datapath/eth_input.c (1)
19-19: LGTM! Proper admin-down interface handling.The new
IFACE_DOWNedge correctly routes packets when interfaces lack theGR_IFACE_F_UPflag, preventing unnecessary processing and clearly separating operational and administrative states.Also applies to: 61-64, 84-87, 160-160, 180-180
modules/infra/datapath/port_output.c (1)
18-38: LGTM! Well-structured port output routing.The implementation correctly routes packets to appropriate port-specific edges. The assert on line 33 is appropriate for catching programming errors during development if the edges array is improperly sized.
modules/infra/control/loopback.c (1)
34-38: LGTM! Consistent accessor pattern migration.The migration to
GR_IFACE_INFOmacro andiface_info_loopback(iface)accessor provides type-safe access and aligns with the accessor pattern used across other interface types.Also applies to: 47-47, 59-59, 118-118, 192-192, 273-273
modules/infra/control/worker_test.c (1)
55-56: LGTM! Test updates match API changes.The mock signature updates and accessor pattern migration (
iface_info_port) correctly align with the new worker graph reload API and broader refactoring.Also applies to: 159-159, 365-365, 403-403
modules/infra/datapath/port_tx.c (1)
24-53: LGTM! Solid TX implementation.The TX processing logic is sound, with proper error handling via the
TX_ERRORedge. The FIXME comment on line 43 appropriately documents the known race condition with tracing mbufs after they're passed to the driver.modules/infra/control/port.c (1)
336-356: Prefer accessor in loop for clarity/consistencyMinor: consistently use iface_info_port(i) (already done here) across the file. Good change.
Double-check all other modules switched to iface_info_port() to avoid mixed styles.
modules/infra/datapath/eth_output.c (1)
119-121: Node registration verifiedThe node
"iface_input_admin_down"is registered viaGR_DROP_REGISTER, so the mapping is valid.modules/infra/control/iface.c (2)
536-539: Good: propagate RUNNING state and events to subinterfacesState sync and event fan-out on STATUS_UP/DOWN is correct and minimal.
Also applies to: 543-546
28-35: Const-correct iface_type_getReturning const struct iface_type* is a nice tightening of API surface.
modules/infra/api/affinity.c (1)
89-89: LGTM: Accessor pattern improves type safety.The switch from direct casting to
iface_info_port(iface)aligns with the broader refactoring toward centralized accessor functions.modules/ipip/datapath_in.c (3)
24-24: LGTM: New edge for admin-down state.Adding
IFACE_DOWNedge enables proper routing of packets for interfaces that are administratively down.
65-68: LGTM: Admin-down check prevents processing.The guard correctly routes packets to
IFACE_DOWNwhen the interface lacks theGR_IFACE_F_UPflag, preventing further processing on administratively disabled interfaces.
103-103: LGTM: Edge wiring complete.The
IFACE_DOWNedge correctly maps toiface_input_admin_down, completing the admin-down routing path.modules/infra/datapath/gr_eth.h (1)
39-39: LGTM: New API for type-driven routing.The function declaration expands the public API to support registering interface-type-specific next-nodes, aligning with the broader datapath refactoring.
modules/ipip/datapath_out.c (4)
26-26: LGTM: Consistent edge addition.The
IFACE_DOWNedge mirrors the addition indatapath_in.c, ensuring symmetric admin-down handling on both input and output paths.
55-58: LGTM: Admin-down guard prevents encapsulation.The check prevents encapsulation when the IPIP interface is administratively down, correctly routing to the admin-down handler.
60-60: LGTM: Accessor pattern for IPIP info.The switch from direct casting to
iface_info_ipip(iface)aligns with the refactoring toward centralized accessor functions, matching the pattern used inaffinity.c.
106-106: LGTM: Edge wiring matches input path.The
IFACE_DOWNmapping toiface_input_admin_downis consistent with the input-side routing indatapath_in.c.
| ctx->burst_size = RTE_GRAPH_BURST_SIZE / gr_vec_len(worker->rxqs); | ||
| } |
There was a problem hiding this comment.
Prevent zero burst size on large rx maps.
When gr_vec_len(worker->rxqs) exceeds RTE_GRAPH_BURST_SIZE, the integer division yields 0, so every port_rx clone ends up with burst_size == 0, effectively stalling dequeue. Use the count of enabled RX queues and clamp to at least 1.
🤖 Prompt for AI Agents
In modules/infra/control/graph.c around lines 151-152, the current computation
ctx->burst_size = RTE_GRAPH_BURST_SIZE / gr_vec_len(worker->rxqs); can yield
zero when gr_vec_len(worker->rxqs) > RTE_GRAPH_BURST_SIZE, stalling dequeues;
change it to compute the number of enabled RX queues (iterate worker->rxqs and
count non-NULL/active entries), use that count as the divisor, then clamp the
result to a minimum of 1 (e.g., if computed burst_size == 0 set it to 1) so
every port_rx clone gets at least a burst of 1.
| if (type->init(iface, api_info) < 0) | ||
| goto fail; | ||
|
|
||
| if (type->set_mtu != NULL && type->set_mtu(iface, iface->mtu) < 0) | ||
| goto fail; | ||
| if (type->set_promisc != NULL | ||
| && type->set_promisc(iface, iface->flags & GR_IFACE_F_PROMISC) < 0) | ||
| goto fail; | ||
| if (type->set_allmulti != NULL | ||
| && type->set_allmulti(iface, iface->flags & GR_IFACE_F_ALLMULTI) < 0) | ||
| goto fail; | ||
| if (type->set_up_down != NULL && type->set_up_down(iface, iface->flags & GR_IFACE_F_UP) < 0) | ||
| goto fail; | ||
|
|
There was a problem hiding this comment.
Call type->fini on creation failure after init to avoid leaks/state residue
If any set_* fails after init, iface is freed without calling fini. This can leak resources or leave hardware configured.
@@
- if (type->init(iface, api_info) < 0)
- goto fail;
+ bool inited = false;
+ if (type->init(iface, api_info) < 0)
+ goto fail;
+ inited = true;
@@
fail:
if (iface != NULL) {
+ if (inited && type != NULL)
+ type->fini(iface);
if (vrf_ref)
vrf_decref(iface->vrf_id);
free(iface->name);
}Also applies to: 137-146
🤖 Prompt for AI Agents
In modules/infra/control/iface.c around lines 116 to 129 (and similarly 137 to
146), if type->init succeeds but a subsequent type->set_* call fails the code
jumps to fail without calling type->fini, leaving resources or hardware state
configured; update the failure path so that before freeing iface you detect that
init completed and call type->fini(iface) (guarded by type->fini != NULL) to
clean up state, then proceed to free and return the error; apply the same change
to the failure handling at lines 137-146.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/api/stats.c (1)
403-404: Use iface_info_port() accessor for consistency.Lines 403-404 use a direct cast to
iface_info_port *instead of theiface_info_port(iface)accessor used elsewhere in this file (lines 131, 243, 281).Apply this diff to use the consistent accessor:
- struct iface_info_port *port = (struct - iface_info_port *)iface->info; + struct iface_info_port *port = iface_info_port(iface);
♻️ Duplicate comments (4)
modules/infra/control/vlan.c (2)
106-138: Use public API instead of direct vtable call.Line 125 directly invokes
parent_type->del_eth_addr, which can segfault if NULL. Useiface_del_eth_addrinstead, which performs NULL/support checks.Based on past review comments.
Apply this diff:
- ret = parent_type->del_eth_addr(parent, &vlan->mac); + ret = iface_del_eth_addr(vlan->parent_id, &vlan->mac);
80-81: Propagate the actual error from iface_add_vlan.Returning
-errnocan report the wrong error.iface_add_vlanreturns a negative errno value, so propagate it directly.Based on past review comments.
Apply this diff:
- if (iface_add_vlan(next->parent_id, next->vlan_id) < 0) - return -errno; + ret = iface_add_vlan(next->parent_id, next->vlan_id); + if (ret < 0) + return ret;modules/infra/control/port.c (1)
302-307: Name the unused parameter.C requires named parameters in function definitions. The third parameter is unnamed, which is a compile error.
Based on past review comments.
Apply this diff:
static int iface_port_reconfig( struct iface *iface, uint64_t set_attrs, - const struct gr_iface *, + const struct gr_iface *conf, const void *api_info ) { + (void)conf;modules/infra/datapath/eth_output.c (1)
56-72: Still present: NULL dereference and redundant assignment.The issues flagged in the previous review remain unresolved:
- Line 58 assigns
priv->ifacefromiface_from_idwithout NULL check- Line 59 dereferences
priv->iface->flags(potential NULL pointer dereference)- Line 71 redundantly reassigns
priv->iface(already done at line 58)Apply the previously suggested fix: check for NULL after line 58 before dereferencing, and remove the redundant assignment at line 71.
🧹 Nitpick comments (3)
modules/infra/control/worker_test.c (2)
344-346: Use accessor consistently.Line 346 directly casts
iface->info, while line 366 uses theiface_info_portaccessor. For consistency and type safety, use the accessor here as well.Apply this diff:
gr_vec struct iface_info_port **ports = NULL; for (unsigned i = 0; i < ARRAY_DIM(ifaces); i++) - gr_vec_add(ports, (struct iface_info_port *)ifaces[i]->info); + gr_vec_add(ports, iface_info_port(ifaces[i]));
381-383: Use accessor consistently.Same issue as in
queue_distribute_reduce: line 383 casts directly while line 404 uses the accessor.Apply this diff:
gr_vec struct iface_info_port **ports = NULL; for (unsigned i = 0; i < ARRAY_DIM(ifaces); i++) - gr_vec_add(ports, (struct iface_info_port *)ifaces[i]->info); + gr_vec_add(ports, iface_info_port(ifaces[i]));modules/infra/control/worker.c (1)
278-281: Use accessor for consistency.Line 281 directly casts
iface->info. Use theiface_info_portaccessor instead.Apply this diff:
gr_vec struct iface_info_port **ports = NULL; struct iface *iface = NULL; while ((iface = iface_next(GR_IFACE_TYPE_PORT, iface)) != NULL) - gr_vec_add(ports, (struct iface_info_port *)iface->info); + gr_vec_add(ports, iface_info_port(iface));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
modules/infra/api/affinity.c(1 hunks)modules/infra/api/stats.c(6 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(6 hunks)modules/infra/control/worker.c(7 hunks)modules/infra/control/worker_test.c(4 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(4 hunks)modules/infra/datapath/l1_xconnect.c(4 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/ipip/datapath_out.c
- modules/infra/datapath/l1_xconnect.c
- modules/ipip/control.c
- modules/ipip/datapath_in.c
🧰 Additional context used
🧬 Code graph analysis (10)
modules/infra/control/worker.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (1)
iface_next(237-252)modules/infra/control/graph.c (2)
worker_graph_reload_all(304-322)worker_graph_reload(207-230)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/vlan.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_type_get(28-35)
modules/infra/api/stats.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/api/gr_infra.h (2)
gr_iface_type_name(322-338)gr_iface_mode_name(341-351)
modules/infra/datapath/eth_output.c (6)
modules/infra/api/gr_infra.h (1)
gr_iface_type_name(322-338)modules/infra/control/graph.c (1)
gr_node_attach_parent(36-64)modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker_test.c (1)
iface_from_id(40-42)modules/infra/control/iface.c (2)
iface_from_id(254-261)iface_get_eth_addr(263-276)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)
modules/infra/datapath/port_output.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(520-547)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)modules/infra/datapath/eth_output.c (1)
eth_output_register_interface_type(27-34)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/worker_test.c (2)
modules/infra/control/graph.c (2)
worker_graph_reload(207-230)worker_graph_reload_all(304-322)modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (3)
port_unplug(145-182)port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
⏰ 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-cross-aarch64
- 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 (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: deb
- GitHub Check: rpm
🔇 Additional comments (28)
modules/infra/control/gr_port.h (1)
30-40: LGTM! Macro-based type definition is clean.The GR_IFACE_INFO macro provides consistent type aliasing and accessor generation across interface types. The new fields (started, pool, devargs) are well-defined and properly aligned.
modules/infra/datapath/port_output.c (1)
40-61: Node registration and cleanup look correct.The fini function properly frees allocated context, and the registration with eth_output follows the established pattern for interface-type-based routing.
modules/infra/api/stats.c (1)
60-61: Port name matching updated correctly.The prefix check now recognizes
port_rx-prefixed nodes, aligning with the renamed rx/tx source files.modules/infra/control/gr_iface.h (3)
24-29: Well-designed type alias macro.The GR_IFACE_INFO macro cleanly generates both the type-specific struct definition and a type-safe accessor function with compile-time type checking via assert.
37-55: Function pointer refactor expands interface type capabilities.Replacing typedef-based callbacks with inline function pointers in struct iface_type adds granular control for MAC, MTU, promisc, allmulti, and VLAN operations per interface type.
61-82: Public API expansion is well-structured.The const return for
iface_type_getimproves const-correctness. New setter functions (set_eth_addr, set_mtu, set_up_down, set_promisc, set_allmulti, add/del_vlan) provide consistent interface configuration APIs.modules/ipip/ipip_priv.h (1)
16-16: LGTM! Consistent macro adoption.Using GR_IFACE_INFO for IPIP aligns with the PORT and VLAN interface type definitions.
modules/infra/api/affinity.c (1)
89-89: Accessor usage is correct.Using
iface_info_port(iface)aligns with the type-safe accessor pattern introduced across the codebase.modules/infra/control/gr_vlan.h (1)
14-14: LGTM! Macro usage is consistent.The GR_IFACE_INFO macro provides uniform type aliasing for VLAN interface info.
modules/infra/datapath/eth_input.c (2)
19-19: Admin-down handling is well-implemented.Adding IFACE_DOWN routing for interfaces that are not UP prevents processing on inactive interfaces, which is good defensive programming and aligns with the broader interface status handling pattern.
Also applies to: 61-64, 84-87
160-160: Drop registration is correct.Mapping IFACE_DOWN to iface_input_admin_down with corresponding drop registration properly handles packets on inactive interfaces.
Also applies to: 180-180
modules/infra/control/worker_test.c (2)
55-56: LGTM!Mock signatures correctly reflect the updated API.
154-166: LGTM!Test setup correctly uses the
iface_info_portaccessor and initializes port objects properly.modules/infra/control/loopback.c (2)
34-38: LGTM!The
GR_IFACE_INFOmacro correctly defines the loopback info structure and accessor.
47-47: LGTM!Consistent use of the
iface_info_loopbackaccessor throughout the file.Also applies to: 59-59, 118-118, 192-192, 273-273
modules/infra/control/worker.c (3)
145-182: LGTM!The updated
port_unplugcorrectly uses the port object, builds the ports vector, and reloads the graph.
184-223: LGTM!The updated
port_plugcorrectly collects ports and reloads the graph.
302-424: LGTM!The
worker_queue_distributefunction correctly passes the ports vector to graph reload functions.modules/infra/control/port.c (5)
149-300: LGTM!The new port control functions properly use the accessor, handle errors, and update interface flags/state correctly.
308-369: LGTM!The reconfig logic correctly unplugs, reconfigures, and plugs the port using the new helper functions.
373-418: LGTM!The fini function correctly uses accessors and includes proper NULL checks for driver_name.
420-471: LGTM!The init function correctly uses accessors and calls reconfig with appropriate parameters.
775-794: LGTM!The iface_type_port structure correctly registers all new callback functions.
modules/infra/control/vlan.c (2)
42-104: LGTM!The reconfig logic correctly uses accessors and higher-level helpers for VLAN and MAC management.
140-182: LGTM!The remaining functions correctly use the
iface_info_vlanaccessor throughout.modules/infra/datapath/eth_output.c (3)
27-34: LGTM: Registration function is well-designed.The validation logic correctly rejects invalid types and prevents duplicate registrations. Using ABORT for programming errors during initialization is appropriate.
73-81: LGTM: Edge routing logic is sound.The non-VLAN MAC lookup and interface-type-based edge routing correctly handles error cases (NO_MAC for missing MAC, INVAL for unregistered types).
82-108: LGTM: Ethernet header and stats handling is correct.Stats are only updated on successful packet processing. Error paths (no headroom, no MAC, interface down, invalid type) correctly bypass stats and route to appropriate drop/error nodes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/infra/control/port.c (1)
99-103: Guard driver_name before strcmp to avoid NULL derefdriver_name can be NULL. Add a check.
- if (strcmp(info.driver_name, "net_tap") == 0) { + if (info.driver_name != NULL && strcmp(info.driver_name, "net_tap") == 0) {modules/infra/control/vlan.c (1)
60-90: Reconfig with unchanged parent/VLAN wrongly errors (EADDRINUSE)You check for EADDRINUSE before removing the current key, so no-op reconfigs fail. Move the lookup after deleting the current key or skip when unchanged.
- if (rte_hash_lookup(vlan_hash, &next_key) >= 0) - return errno_set(EADDRINUSE); - - if (reconfig) { + if (reconfig) { // reconfig, *not initial config* struct vlan_key cur_key = {cur->parent_id, cur->vlan_id}; rte_hash_del_key(vlan_hash, &cur_key); iface_del_subinterface(cur_parent, iface); // remove previous vlan filter (ignore errors) iface_del_vlan(cur->parent_id, cur->vlan_id); - } + } + if (!reconfig || next->parent_id != cur->parent_id || next->vlan_id != cur->vlan_id) { + if (rte_hash_lookup(vlan_hash, &next_key) >= 0) + return errno_set(EADDRINUSE); + }
♻️ Duplicate comments (8)
modules/infra/datapath/port_output.c (1)
24-38: Add bounds check before array access.Line 32 indexes
ctx->edges[port->port_id]without validatingport->port_id. The assertion on line 33 may be compiled out in production builds, leading to undefined behavior.Add a bounds check:
for (unsigned i = 0; i < nb_objs; i++) { struct rte_mbuf *mbuf = objs[i]; iface = mbuf_data(mbuf)->iface; port = iface_info_port(iface); + if (unlikely(port->port_id >= ctx->n_edges)) { + rte_pktmbuf_free(mbuf); + continue; + } + if (gr_mbuf_is_traced(mbuf)) gr_mbuf_trace_add(mbuf, node, 0); edge = ctx->edges[port->port_id]; assert(edge != RTE_EDGE_ID_INVALID); rte_node_enqueue_x1(graph, node, edge, mbuf); }Note: Adjust
n_edgesto match the actual field name inport_output_edges.modules/infra/datapath/eth_output.c (1)
57-64: Fix possible NULL deref on VLAN parent and remove redundant assignmentiface_from_id(sub->parent_id) can return NULL. You dereference priv->iface without checking and assign priv->iface twice. Guard for NULL and only assign after validations; use the parent's flags via a temp pointer.
- const struct iface_info_vlan *sub = iface_info_vlan(priv->iface); - priv->iface = iface_from_id(sub->parent_id); - if (!(priv->iface->flags & GR_IFACE_F_UP)) { - edge = IFACE_DOWN; - goto next; - } + const struct iface_info_vlan *sub = iface_info_vlan(priv->iface); + const struct iface *parent = iface_from_id(sub->parent_id); + if (parent == NULL) { + edge = IFACE_DOWN; + goto next; + } + if (!(parent->flags & GR_IFACE_F_UP)) { + edge = IFACE_DOWN; + goto next; + } vlan = (struct rte_vlan_hdr *)rte_pktmbuf_prepend(mbuf, sizeof(*vlan)); if (unlikely(vlan == NULL)) { edge = NO_HEADROOM; goto next; } vlan->vlan_tci = rte_cpu_to_be_16(sub->vlan_id); vlan->eth_proto = priv->ether_type; priv->ether_type = RTE_BE16(RTE_ETHER_TYPE_VLAN); - priv->iface = iface_from_id(sub->parent_id); + priv->iface = parent; src_mac = sub->mac;Also applies to: 71-75
modules/infra/control/port.c (1)
302-309: Compile error: unnamed parameter in function definitionName the third parameter and mark unused to satisfy C and avoid warnings.
static int iface_port_reconfig( struct iface *iface, uint64_t set_attrs, - const struct gr_iface *, + const struct gr_iface *cfg /*unused*/, const void *api_info ) { + (void)cfg;modules/infra/control/vlan.c (3)
36-43: Compile error: unnamed parameter in function definitionName the third parameter and mark unused.
static int iface_vlan_reconfig( struct iface *iface, uint64_t set_attrs, - const struct gr_iface *, + const struct gr_iface *conf /*unused*/, const void *api_info ) { + (void)conf;
80-83: Return ret from iface_add_vlan, not -errnoPropagate the actual error code from the callee.
- if (iface_add_vlan(next->parent_id, next->vlan_id) < 0) - return -errno; + ret = iface_add_vlan(next->parent_id, next->vlan_id); + if (ret < 0) + return ret;
125-133: Null-call risk: use public API instead of raw vtable del_eth_addrCalling parent_type->del_eth_addr may be NULL and segfault. Use iface_del_eth_addr().
- ret = parent_type->del_eth_addr(parent, &vlan->mac); + ret = iface_del_eth_addr(vlan->parent_id, &vlan->mac);modules/infra/control/iface.c (2)
116-129: Call type->fini on failure after successful init to avoid leaks/state residueIf any set_* fails after init, you jump to fail without cleaning up.
- if (type->init(iface, api_info) < 0) - goto fail; + bool inited = false; + if (type->init(iface, api_info) < 0) + goto fail; + inited = true; @@ fail: if (iface != NULL) { + if (inited && type != NULL && type->fini != NULL) + type->fini(iface); if (vrf_ref) vrf_decref(iface->vrf_id); free(iface->name); }Also applies to: 137-146
404-412: Bug: NULL-check inverted in iface_set_allmulti (will call NULL fp)Use != NULL; otherwise you dereference a NULL function pointer.
- if (type->set_allmulti == NULL) - return type->set_allmulti(iface, enabled); + if (type->set_allmulti != NULL) + return type->set_allmulti(iface, enabled);
🧹 Nitpick comments (3)
modules/infra/control/worker_test.c (1)
366-368: Use accessor when building the ports vector tooYou correctly use iface_info_port(...) when reading p. For consistency, also replace raw casts when building the ports vector earlier in these tests (e.g., Lines 346 and 383) to avoid coupling to struct layout.
Example:
- gr_vec_add(ports, (struct iface_info_port *)ifaces[i]->info); + gr_vec_add(ports, iface_info_port(ifaces[i]));Also applies to: 404-406
modules/infra/api/stats.c (1)
359-361: Telemetry: prefer iface_info_port over raw castsType/mode name helpers swap looks good. For consistency and safety, also replace the direct cast in the telemetry HW stats block with the accessor:
- struct iface_info_port *port = (struct iface_info_port *)iface->info; + struct iface_info_port *port = iface_info_port(iface);Also applies to: 375-377
modules/infra/control/worker.c (1)
278-282: Use iface_info_port accessor when building portsAvoid raw casts to iface->info. Improves robustness and consistency.
- while ((iface = iface_next(GR_IFACE_TYPE_PORT, iface)) != NULL) - gr_vec_add(ports, (struct iface_info_port *)iface->info); + while ((iface = iface_next(GR_IFACE_TYPE_PORT, iface)) != NULL) + gr_vec_add(ports, iface_info_port(iface));Also applies to: 285-286, 295-295, 298-299
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
modules/infra/api/affinity.c(1 hunks)modules/infra/api/stats.c(6 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(6 hunks)modules/infra/control/worker.c(7 hunks)modules/infra/control/worker_test.c(4 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(4 hunks)modules/infra/datapath/l1_xconnect.c(4 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/ipip/control.c
- modules/ipip/datapath_in.c
- modules/infra/datapath/l1_xconnect.c
🧰 Additional context used
🧬 Code graph analysis (13)
modules/infra/api/stats.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/api/gr_infra.h (2)
gr_iface_type_name(322-338)gr_iface_mode_name(341-351)
modules/infra/control/vlan.c (1)
modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_type_get(28-35)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/eth_output.c (4)
modules/infra/api/gr_infra.h (1)
gr_iface_type_name(322-338)modules/infra/control/graph.c (1)
gr_node_attach_parent(36-64)modules/infra/control/iface.c (2)
iface_from_id(254-261)iface_get_eth_addr(263-276)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/cli/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/worker_test.c (2)
modules/infra/control/graph.c (2)
worker_graph_reload(207-230)worker_graph_reload_all(304-322)modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/worker.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (1)
iface_next(237-252)modules/infra/control/graph.c (2)
worker_graph_reload_all(304-322)worker_graph_reload(207-230)
modules/infra/control/iface.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)modules/infra/control/worker_test.c (1)
gr_event_push(36-36)main/event.c (1)
gr_event_subscribe(14-16)
modules/infra/control/gr_iface.h (2)
modules/infra/control/worker_test.c (1)
iface_type_register(35-35)modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (3)
port_unplug(145-182)port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
modules/infra/datapath/port_output.c (4)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(520-547)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)modules/infra/datapath/eth_output.c (1)
eth_output_register_interface_type(27-34)
⏰ 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 (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 (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-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (22)
modules/infra/datapath/eth_input.c (3)
15-21: LGTM: IFACE_DOWN edge addition.The new edge is properly declared in the enum and will be wired up correctly in the node registration.
61-64: LGTM: Admin-down interface handling.Both checks correctly route packets to IFACE_DOWN when the interface (base or VLAN) lacks the UP flag. This provides consistent behavior for downed interfaces across the datapath.
Also applies to: 84-87
160-160: LGTM: Edge registration.The IFACE_DOWN edge and corresponding drop registration are properly wired up.
Also applies to: 180-180
modules/infra/datapath/port_output.c (1)
40-61: LGTM: Node registration and cleanup.The fini function properly frees allocated context, and the registration integrates port_output with the eth_output routing.
modules/infra/control/gr_iface.h (3)
24-29: LGTM: Well-designed GR_IFACE_INFO macro.The macro properly handles type aliasing and alignment while providing type-safe access to per-interface-type data. The assertion ensures runtime type checking.
33-58: LGTM: Function pointer expansion.The explicit function pointers replace typedefs and add new interface operations (MTU, promisc, allmulti, VLAN). This provides a cleaner, more extensible interface type API.
61-61: LGTM: Const-correctness improvement.Returning
const struct iface_type *prevents unintended modifications to registered types.modules/infra/control/gr_port.h (1)
30-40: LGTM: Macro adoption and field additions.The use of GR_IFACE_INFO aligns with the new pattern, and the added fields (started, pool, devargs) support proper port lifecycle management.
modules/infra/api/affinity.c (1)
81-94: LGTM: Accessor usage.The change from direct cast to
iface_info_port(iface)aligns with the type-safe accessor pattern introduced by the GR_IFACE_INFO macro.modules/ipip/datapath_out.c (2)
22-28: LGTM: IFACE_DOWN integration.The interface down check and edge routing are consistent with the pattern established in eth_input.c, ensuring uniform handling of admin-down interfaces across datapath modules.
Also applies to: 55-58, 106-106
60-60: LGTM: Accessor usage.Using
iface_info_ipip(iface)instead of direct cast aligns with the type-safe accessor pattern.modules/ipip/ipip_priv.h (1)
6-6: LGTM: Macro adoption.The change to use GR_IFACE_INFO is consistent with the broader refactor, and the gr_iface.h include is now required for the macro.
Also applies to: 16-16
modules/infra/control/gr_vlan.h (1)
14-14: LGTM: Macro adoption.Consistent use of GR_IFACE_INFO for type-safe VLAN interface info access.
modules/infra/control/worker_test.c (2)
55-56: Mocks updated to new API — looks goodSignatures match the new worker_graph_reload_all contract.
160-162: Proper init and accessor usageSetting iface->type and using iface_info_port(iface) is correct and const‑safe.
modules/infra/api/stats.c (2)
60-61: Correct prefix match for RX nodesMatching "port_rx-" is in line with new node naming.
131-131: Consistent use of iface_info_portGood migration to accessor in HW stats fetch and resets.
Also applies to: 243-247, 281-284
modules/infra/control/loopback.c (1)
34-39: Nice consolidation to GR_IFACE_INFO + accessorsReplacing raw info casts with iface_info_loopback(...) across init, poll, tx, and fini improves type safety and readability.
Also applies to: 47-49, 59-60, 118-119, 192-193, 273-275
modules/infra/control/worker.c (2)
145-182: Port plug/unplug flow is coherentToggling qmap enabled and reloading graphs with the right ports set is sound.
Also applies to: 184-223
420-421: Confirm reload_all handles NULL portsworker_queue_distribute can be called with ports == NULL (e.g., initial start). Ensure worker_graph_reload_all and worker_graph_nodes_add_missing gracefully handle NULL vectors.
modules/infra/datapath/eth_output.c (1)
25-34: Edge registration API looks goodPer-type next-node registration with guards is clean.
modules/infra/control/port.c (1)
127-129: Drop NULL check and pointer deref on dev_flags
dev_flagsis a value bitmask, not a pointer. Update to:- if (info.dev_flags != NULL && *info.dev_flags & RTE_ETH_DEV_INTR_LSC) { + if (info.dev_flags & RTE_ETH_DEV_INTR_LSC) { conf.intr_conf.lsc = 1; }Confirm against DPDK v25.07 headers.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/control/iface.c (1)
278-293: Simplified subinterface iteration pattern.Switching from
gr_vec_foreach_reftogr_vec_foreachis appropriate here since direct pointer access suffices. The non-const parameters align with the actual usage patterns where subinterface state may be modified.
♻️ Duplicate comments (9)
smoke/_init.sh (1)
55-56: Handle SIGTERM exit code before failing.
waitreturns 143 after our SIGTERM, so|| failstill flags every graceful shutdown as a crash. Store$?right afterwaitand only callfailfor unexpected codes (anything other than 0/143, etc.).modules/infra/cli/port.c (1)
51-53: Buffer overflow: use remaining buffer size.Lines 51 and 53 pass
lento SAFE_BUF afternbytes were already written at line 48. This tells snprintf the full buffer is available when onlylen - nbytes remain, allowing writes past the buffer end.Apply this diff:
if (iface->mode == GR_IFACE_MODE_L1_XC) { if ((peer = iface_from_id(c, iface->domain_id)) != NULL) - SAFE_BUF(snprintf, len, " xc_peer=%s", peer->name); + SAFE_BUF(snprintf, len - n, " xc_peer=%s", peer->name); else - SAFE_BUF(snprintf, len, " xc_peer=%u", iface->domain_id); + SAFE_BUF(snprintf, len - n, " xc_peer=%u", iface->domain_id); }modules/infra/datapath/port_output.c (1)
32-33: Add bounds check before array access.Line 32 indexes
ctx->edges[port->port_id]without validating thatport->port_idis within bounds. The assertion on line 33 may be compiled out in production builds.Add a bounds check:
+ if (unlikely(port->port_id >= RTE_MAX_ETHPORTS)) { + rte_pktmbuf_free(mbuf); + continue; + } edge = ctx->edges[port->port_id]; assert(edge != RTE_EDGE_ID_INVALID);modules/infra/datapath/gr_rxtx.h (1)
9-9: Missing include for RTE_MAX_ETHPORTS.Line 31 uses
RTE_MAX_ETHPORTSbut<rte_ethdev.h>is not included, which can cause compilation failures depending on include order.Apply this diff:
#include <rte_build_config.h> +#include <rte_ethdev.h> #include <rte_graph.h>Also applies to: 30-32
modules/infra/control/port.c (3)
205-213: Propagate promisc failures.We still mask rte_eth_promiscuous_{en,dis}able errors and report success, so callers cannot react.
switch (ret) { case 0: case -ENOSYS: case -EOPNOTSUPP: break; default: - errno_log(-ret, "rte_eth_promiscuous_{en,dis}able"); + return errno_log(-ret, "rte_eth_promiscuous_{en,dis}able"); }
231-238: Propagate allmulti failures.Same issue: we swallow rte_eth_allmulticast_* errors instead of bubbling them up.
switch (ret) { case 0: case -ENOSYS: case -EOPNOTSUPP: break; default: - errno_log(-ret, "rte_eth_allmulticast_{en,dis}able"); + return errno_log(-ret, "rte_eth_allmulticast_{en,dis}able"); }
302-310: Name the gr_iface parameter.The third parameter is still unnamed, so this file will not compile. Give it an identifier and silence the unused warning if you don’t need it.
static int iface_port_reconfig( struct iface *iface, uint64_t set_attrs, - const struct gr_iface *, + const struct gr_iface *cfg /* unused */, const void *api_info ) { + (void)cfg;modules/infra/datapath/port_rx.c (1)
32-36: Validate mode before indexing.Without a bounds check, a bad mode can walk past edges[] and scribble memory.
void register_interface_mode(gr_iface_mode_t mode, const char *next_node) { + if (mode == IFACE_MODE_UNKNOWN || mode >= GR_IFACE_MODE_COUNT) + ABORT("invalid interface mode %u", mode); if (edges[mode] != IFACE_MODE_UNKNOWN) ABORT("next node already registered for interface mode %u", mode); edges[mode] = gr_node_attach_parent(RX_NODE_BASE, next_node);modules/infra/control/graph.c (1)
136-152: Clamp RX burst size.Using gr_vec_len(worker->rxqs) lets burst_size drop to 0 once the worker tracks more than RTE_GRAPH_BURST_SIZE queues, stalling that RX path.
- gr_vec_foreach_ref (qmap, worker->rxqs) { + uint16_t burst = RTE_GRAPH_BURST_SIZE / n_rxqs; + if (burst == 0) + burst = 1; + + gr_vec_foreach_ref (qmap, worker->rxqs) { if (!qmap->enabled) continue; snprintf(node_name, sizeof(node_name), RX_NODE_FMT, qmap->port_id, qmap->queue_id); @@ ctx->iface = RTE_PTR_SUB(p, offsetof(struct iface, info)); break; } assert(ctx->iface != NULL); ctx->rxq.port_id = qmap->port_id; ctx->rxq.queue_id = qmap->queue_id; - ctx->burst_size = RTE_GRAPH_BURST_SIZE / gr_vec_len(worker->rxqs); + ctx->burst_size = burst;
🧹 Nitpick comments (1)
main/vec_test.c (1)
89-97: Test coverage: No assertions indyn_str_vec.The test allocates strings but doesn't verify they were created correctly before freeing. Consider adding assertions to check the vector length and string values.
static void dyn_str_vec(void **) { gr_vec char **vec = NULL; gr_vec_add(vec, strdup("foo")); gr_vec_add(vec, strdup("bar")); gr_vec_add(vec, strdup("baz")); + + assert_int_equal(gr_vec_len(vec), 3); + assert_string_equal(vec[0], "foo"); + assert_string_equal(vec[1], "bar"); + assert_string_equal(vec[2], "baz"); gr_strvec_free(vec); }
📜 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 (39)
main/gr_vec.h(2 hunks)main/vec_test.c(1 hunks)meson.build(2 hunks)modules/infra/api/affinity.c(1 hunks)modules/infra/api/gr_infra.h(2 hunks)modules/infra/api/stats.c(6 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_graph.h(0 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/graph.c(11 hunks)modules/infra/control/graph_priv.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/mempool.c(1 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(5 hunks)modules/infra/control/worker.c(7 hunks)modules/infra/control/worker_priv.h(1 hunks)modules/infra/control/worker_test.c(4 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(4 hunks)modules/infra/datapath/gr_eth.h(1 hunks)modules/infra/datapath/gr_rxtx.h(1 hunks)modules/infra/datapath/l1_xconnect.c(4 hunks)modules/infra/datapath/main_loop.c(5 hunks)modules/infra/datapath/meson.build(1 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/infra/datapath/port_rx.c(1 hunks)modules/infra/datapath/port_tx.c(1 hunks)modules/infra/datapath/rx.c(0 hunks)modules/infra/datapath/tx.c(0 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)smoke/_init.sh(1 hunks)smoke/affinity_test.sh(1 hunks)
💤 Files with no reviewable changes (3)
- modules/infra/control/gr_graph.h
- modules/infra/datapath/tx.c
- modules/infra/datapath/rx.c
🚧 Files skipped from review as they are similar to previous changes (9)
- modules/infra/control/gr_vlan.h
- modules/infra/datapath/eth_output.c
- modules/infra/datapath/eth_input.c
- modules/infra/api/stats.c
- modules/infra/control/mempool.c
- modules/infra/datapath/l1_xconnect.c
- smoke/affinity_test.sh
- modules/infra/control/vlan.c
- modules/infra/datapath/port_tx.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
PR: DPDK/grout#326
File: modules/infra/control/graph.c:360-364
Timestamp: 2025-10-02T07:42:42.135Z
Learning: In the grout codebase, `gr_vec_free()` is a macro defined in `main/gr_vec.h` that automatically sets the vector pointer to NULL after freeing the memory, preventing double-free issues. The macro definition is: `#define gr_vec_free(v) ((v) ? free(__gr_vec_hdr(v)) : (void)0, (v) = NULL)`.
Applied to files:
main/gr_vec.h
🧬 Code graph analysis (18)
main/vec_test.c (1)
main/gr_vec.h (1)
gr_vec_len(47-51)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/cli/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/worker_test.c (2)
modules/infra/control/graph.c (2)
worker_graph_reload(207-230)worker_graph_reload_all(304-322)modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/gr_eth.h (1)
modules/infra/datapath/eth_output.c (1)
eth_output_register_interface_type(27-34)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/worker.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (1)
iface_next(237-252)modules/infra/control/graph.c (2)
worker_graph_reload_all(304-322)worker_graph_reload(207-230)
modules/infra/datapath/main_loop.c (1)
main/sort.c (1)
topo_sort(53-97)
modules/ipip/control.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/port_output.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_is_traced(52-54)modules/infra/datapath/trace.c (1)
gr_mbuf_trace_add(520-547)modules/infra/control/gr_graph.h (1)
rte_node_enqueue_x1(22-25)modules/infra/datapath/eth_output.c (1)
eth_output_register_interface_type(27-34)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (3)
port_unplug(145-182)port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
modules/infra/control/graph_priv.h (1)
modules/infra/control/graph.c (2)
worker_graph_reload(207-230)worker_graph_reload_all(304-322)
modules/infra/control/iface.c (4)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)main/event.c (1)
gr_event_subscribe(14-16)
modules/infra/datapath/port_rx.c (2)
modules/infra/control/graph.c (1)
gr_node_attach_parent(36-64)modules/infra/datapath/trace.c (2)
gr_mbuf_trace_add(520-547)trace_log_packet(354-504)
modules/infra/control/worker_priv.h (1)
modules/infra/control/worker.c (2)
port_unplug(145-182)port_plug(184-223)
modules/infra/control/graph.c (3)
api/string.c (1)
astrcat(15-50)main/gr_vec.h (1)
gr_vec_len(47-51)api/gr_errno.h (1)
errno_set(9-12)
modules/infra/control/gr_iface.h (2)
modules/infra/control/worker_test.c (1)
iface_type_register(35-35)modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
⏰ 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-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: 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: rpm
- GitHub Check: deb
🔇 Additional comments (31)
main/vec_test.c (1)
99-126: LGTM!The test thoroughly validates vector concatenation with proper assertions for length and content.
main/gr_vec.h (2)
142-154: LGTM!The function correctly handles empty vectors, grows the destination as needed, and updates the length after copying.
156-162: LGTM!The cleanup function properly frees each string before freeing the header. The NULL return value works well with the macro to set the pointer to NULL.
modules/infra/datapath/main_loop.c (2)
86-168: Good: Centralized error handling with proper cleanup.The refactoring consolidates error paths and ensures resources (nodes vector, stats, allocations) are properly cleaned up. The goto-based error handling is appropriate for this scenario.
177-182: LGTM!Good defensive programming:
- Explicit initialization of stats_context fields
- Clearing stats before reconfig (prevents stale data visibility)
- Added cleanup of node_to_index in shutdown path
Also applies to: 227-229, 292-292
modules/infra/datapath/gr_eth.h (1)
38-39: LGTM!Clean API addition for interface-type-specific routing registration.
modules/ipip/datapath_out.c (1)
55-60: LGTM!Correctly routes down interfaces to admin-down node and uses the new type-safe accessor pattern.
modules/ipip/ipip_priv.h (2)
6-6: LGTM!Required include for the GR_IFACE_INFO macro.
16-16: LGTM!Adopts the standard GR_IFACE_INFO pattern for type safety and uniform accessor generation.
modules/infra/api/gr_infra.h (2)
322-322: LGTM!Consistent naming convention with
gr_prefix.
341-341: LGTM!Consistent naming convention with
gr_prefix.modules/infra/control/gr_iface.h (4)
19-19: LGTM!Removing const allows modification of subinterfaces as needed. Aligns with add/del operations.
24-29: LGTM!Well-designed macro providing type-safe accessors with proper alignment and aliasing attributes.
37-55: LGTM!Comprehensive interface-type callback system. Much better than scattered typedefs.
61-61: LGTM!Good const-correctness - registered types shouldn't be modified after registration.
modules/infra/datapath/meson.build (1)
15-17: LGTM!Build config correctly reflects the datapath file reorganization.
meson.build (1)
61-61: DPDK 25.07 exists; no ABI break vs 24.11. Release notes remove rte_function_versioning.h and drop ZUC-256 support—ensure no code references these removed headers/APIs.modules/ipip/datapath_in.c (1)
24-24: LGTM!The IFACE_DOWN edge handling correctly routes packets for admin-down interfaces to the drop node.
Also applies to: 65-68, 103-103
modules/infra/control/graph_priv.h (1)
6-7: LGTM!Function signatures correctly updated to accept port vectors for graph reload operations. Includes are appropriately added.
Also applies to: 10-11
modules/infra/control/gr_port.h (1)
30-40: LGTM!The GR_IFACE_INFO macro usage is consistent with the broader refactor, and the new fields (started, pool, devargs) appropriately support extended port state management.
modules/ipip/control.c (2)
54-54: LGTM!Consistent use of
iface_info_ipip()accessor replaces direct casts, aligning with the broader accessor pattern refactor.Also applies to: 86-86, 113-113
98-99: LGTM!Default MTU of 1480 is appropriate for IPIP tunnels (accounts for 20-byte IP header overhead).
modules/infra/control/worker_priv.h (1)
9-10: LGTM!API change from
port_idto fulliface_info_portpointer is consistent with the port-aware refactor.modules/infra/control/loopback.c (2)
34-38: LGTM!GR_IFACE_INFO macro usage is consistent with other interface types in this refactor.
47-47: LGTM!Consistent use of
iface_info_loopback()accessor throughout the file.Also applies to: 59-59, 118-118, 192-192, 273-273
modules/infra/control/iface.c (6)
119-128: Attribute application during interface creation is well-structured.The code properly calls type-specific configuration callbacks (MTU, promisc, allmulti, up/down) after initialization, with appropriate NULL checks and error handling. This allows hardware configuration to be applied consistently during both creation and reconfiguration.
184-185: Good addition of VRF overflow validation.The explicit check prevents silent failure since
vrf_increfonly returns void. ReturningEOVERFLOWgives proper error indication to the caller.
191-220: Reconfig logic properly handles transaction semantics.The refactored flow correctly treats
type->reconfigas the main configuration transaction. VRF changes are reverted only if reconfig fails. Once reconfig succeeds, the VRF change is committed, and subsequent attribute updates (MTU, flags) are applied independently. This allows partial success to be reported properly—if an attribute update fails, the main configuration remains valid.
340-357: MTU validation covers maximum but consider minimum bounds.The function correctly validates against
gr_config.max_mtu. Verify that minimum MTU constraints (e.g., 68 for IPv4, 1280 for IPv6) are enforced either in type-specific callbacks or at the API boundary to prevent underflow issues.
379-413: Promisc and allmulti fallback logic is correct.Returning
EOPNOTSUPPonly when attempting to enable without callback support is the right approach. Disabling becomes a no-op (returns success) when the feature isn't supported, which aligns with expected behavior.
536-546: Event propagation to subinterfaces is implemented correctly.The state transitions properly cascade to all subinterfaces when the parent interface goes up or down. Using
gr_vec_foreachto iterate and directly modifying theRUNNINGstate before pushing events ensures consistent state across the interface hierarchy.
rjarry
force-pushed
the
iface-rework
branch
3 times, most recently
from
8bae4be to
08bdeb2
Compare
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 (4)
modules/infra/control/gr_iface.h (1)
4-15: Missing include for assert used in GR_IFACE_INFO.This header defines inline functions that call assert(), but <assert.h> isn’t included here. Add it to avoid compile errors in TUs that don’t include assert first.
#pragma once +#include <assert.h> + #include <gr_bitops.h> #include <gr_infra.h> #include <gr_vec.h>modules/infra/datapath/l1_xconnect.c (1)
30-38: Guard against NULL peer from iface_from_id().
peercan be NULL; dereferencingpeer->typewill crash. Check for NULL before use.- if (peer->type == GR_IFACE_TYPE_PORT) { + if (peer != NULL && peer->type == GR_IFACE_TYPE_PORT) { port = iface_info_port(peer); mbuf->port = port->port_id; edge = OUTPUT; } else { edge = NO_PORT; }modules/infra/control/worker_test.c (2)
346-346: Use theiface_info_portaccessor consistently.Line 346 casts
ifaces[i]->infodirectly instead of using theiface_info_port(ifaces[i])accessor, which is inconsistent with the accessor pattern used elsewhere in this file (lines 161, 366, 404).Apply this diff:
- gr_vec_add(ports, (struct iface_info_port *)ifaces[i]->info); + gr_vec_add(ports, iface_info_port(ifaces[i]));
383-383: Use theiface_info_portaccessor consistently.Line 383 casts
ifaces[i]->infodirectly instead of using theiface_info_port(ifaces[i])accessor, which is inconsistent with the accessor pattern used elsewhere in this file (lines 161, 366, 404).Apply this diff:
- gr_vec_add(ports, (struct iface_info_port *)ifaces[i]->info); + gr_vec_add(ports, iface_info_port(ifaces[i]));
♻️ Duplicate comments (9)
modules/infra/datapath/port_output.c (1)
24-35: Add bounds check before indexing ctx->edges by port_id.
ctx->edges[port->port_id]is accessed without validatingport_idis within bounds. The assert may be compiled out.iface = mbuf_data(mbuf)->iface; port = iface_info_port(iface); + if (unlikely(port->port_id >= ctx->n_edges)) { + rte_pktmbuf_free(mbuf); + continue; + } + if (gr_mbuf_is_traced(mbuf)) gr_mbuf_trace_add(mbuf, node, 0); edge = ctx->edges[port->port_id];modules/infra/cli/port.c (1)
50-53: Buffer overflow: use remaining size (len - n) for subsequent SAFE_BUF writes.Second writes still pass
leninstead of remaining space. Uselen - nto avoid writing past the buffer end. Compute remaining as non-negative.- if ((peer = iface_from_id(c, iface->domain_id)) != NULL) - SAFE_BUF(snprintf, len, " xc_peer=%s", peer->name); + if ((peer = iface_from_id(c, iface->domain_id)) != NULL) + SAFE_BUF(snprintf, len - n, " xc_peer=%s", peer->name); else - SAFE_BUF(snprintf, len, " xc_peer=%u", iface->domain_id); + SAFE_BUF(snprintf, len - n, " xc_peer=%u", iface->domain_id);modules/infra/control/vlan.c (2)
39-39: Name the unused parameter.C requires named parameters in function definitions.
Based on past review comment.
80-82: Propagate the return value directly instead of using-errno.Lines 80-81 call
iface_add_vlanand return-errnoon failure, which can report the wrong error. Capture the return value and propagate it directly.Apply this diff:
- if (iface_add_vlan(next->parent_id, next->vlan_id) < 0) - return -errno; + ret = iface_add_vlan(next->parent_id, next->vlan_id); + if (ret < 0) + return ret;Based on past review comment.
modules/infra/control/port.c (4)
305-305: Name the unused parameter.C requires named parameters in function definitions.
Based on past review comment.
205-212: Propagate promiscuous mode errors to callers.The default case logs the error but returns success (falls through), so callers cannot detect the failure.
Apply this diff:
default: - errno_log(-ret, "rte_eth_promiscuous_{en,dis}able"); + return errno_log(-ret, "rte_eth_promiscuous_{en,dis}able");Based on past review comment.
231-238: Propagate allmulticast errors to callers.The default case logs the error but returns success (falls through), so callers cannot detect the failure.
Apply this diff:
default: - errno_log(-ret, "rte_eth_allmulticast_{en,dis}able"); + return errno_log(-ret, "rte_eth_allmulticast_{en,dis}able");Based on past review comment.
252-262: Don't setiface->mtuwhen the hardware rejects the MTU change.Lines 252-262 overwrite
iface->mtuwith the requested value even whenrte_eth_dev_set_mtureturns-ENOSYSor-EOPNOTSUPP, leaving the control plane state inconsistent with the hardware.Apply this diff:
if (mtu != 0) { ret = rte_eth_dev_set_mtu(p->port_id, mtu); switch (ret) { case 0: + iface->mtu = mtu; + break; case -ENOSYS: case -EOPNOTSUPP: - break; + return errno_log(-ret, "rte_eth_dev_set_mtu"); default: return errno_log(-ret, "rte_eth_dev_set_mtu"); } - iface->mtu = mtu;Based on past review comment.
modules/infra/control/iface.c (1)
116-129: Still missing type->fini on post-init failureIf
type->initsucceeds but any subsequenttype->set_*callback fails (lines 119-128), the failure path doesn't calltype->fini, risking resource leaks or stale hardware state.Apply the previously suggested fix:
+ bool inited = false; if (type->init(iface, api_info) < 0) goto fail; + inited = true; if (type->set_mtu != NULL && type->set_mtu(iface, iface->mtu) < 0) @@ fail: if (iface != NULL) { + if (inited && type != NULL && type->fini != NULL) + type->fini(iface); if (vrf_ref) vrf_decref(iface->vrf_id);
🧹 Nitpick comments (2)
modules/infra/control/gr_iface.h (1)
24-29: Improve const-correctness of iface_info accessors.Accessor returns non-const struct pointer from a const iface*. Prefer returning const in the common accessor and add a separate mutating variant for non-const iface.
Example macro shape (illustrative):
#define GR_IFACE_INFO(type_id, type_name, fields) \ struct type_name fields __attribute__((__may_alias__, aligned(alignof(void *)))); \ static inline const struct type_name *type_name(const struct iface *iface) { \ assert(iface->type == type_id); \ return (const struct type_name *)iface->info; \ } \ static inline struct type_name *type_name##_mut(struct iface *iface) { \ assert(iface->type == type_id); \ return (struct type_name *)iface->info; \ }modules/infra/datapath/l1_xconnect.c (1)
14-18: Consider handling admin-down explicitly (consistency with other nodes).Other datapath nodes added an IFACE_DOWN edge. Here, xconnect routes directly to port_output even if the peer is admin-down. Consider adding IFACE_DOWN and routing accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
modules/infra/api/affinity.c(1 hunks)modules/infra/api/stats.c(3 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(5 hunks)modules/infra/control/worker.c(2 hunks)modules/infra/control/worker_test.c(3 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(3 hunks)modules/infra/datapath/l1_xconnect.c(1 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/infra/control/worker.c
- modules/infra/api/stats.c
- modules/ipip/datapath_in.c
- modules/infra/api/affinity.c
- modules/infra/control/gr_port.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
modules/infra/datapath/port_output.cmodules/ipip/control.cmodules/infra/control/gr_vlan.hmodules/ipip/datapath_out.cmodules/infra/control/vlan.cmodules/infra/control/worker_test.cmodules/infra/control/loopback.cmodules/infra/cli/port.cmodules/infra/control/iface.cmodules/infra/control/port.cmodules/infra/control/gr_iface.hmodules/infra/datapath/l1_xconnect.cmodules/infra/datapath/eth_input.cmodules/infra/datapath/eth_output.cmodules/ipip/ipip_priv.h
🧬 Code graph analysis (10)
modules/infra/datapath/port_output.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/ipip/control.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/vlan.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_del_subinterface(286-293)
modules/infra/control/worker_test.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/iface.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)modules/infra/control/worker_test.c (2)
gr_event_push(36-36)iface_from_id(40-42)main/event.c (1)
gr_event_subscribe(14-16)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (2)
port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
modules/infra/datapath/eth_output.c (2)
modules/infra/control/worker_test.c (1)
iface_from_id(40-42)modules/infra/control/iface.c (1)
iface_from_id(254-261)
⏰ 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-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: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (12)
modules/infra/control/gr_vlan.h (1)
14-16: Macro-based iface_info looks good.Consistent with new GR_IFACE_INFO pattern.
modules/ipip/datapath_out.c (1)
55-61: Good guard and edge mapping for admin-down.IFACE_DOWN routing and iface_info_ipip accessor are correct and consistent.
Ensure ipip_input/ip_output nodes also map IFACE_DOWN to "iface_input_admin_down" for symmetry.
Also applies to: 101-107
modules/infra/control/iface.c (10)
28-35: LGTM: const-correctness improvementReturn type correctly changed to
constto reflect that type definitions should not be modified by callers.
148-223: LGTM: improved reconfig with validation and event propagationGood additions:
- VRF overflow validation (line 184)
- Proper VRF rollback on error (lines 191-195)
- Delegation to new
iface_set_*APIs for MTU and flags- Post-reconfig event pushed consistently (line 220)
278-293: LGTM: consistent non-const subinterface handlingSignatures and iteration correctly updated to treat subinterfaces as mutable, aligning with the new pattern where their state is modified (e.g.,
GR_IFACE_S_RUNNINGflag).
295-308: LGTM: new setter follows established patternImplementation correctly validates interface, checks callback availability, and returns
EOPNOTSUPPwhen not supported.
340-357: LGTM: MTU setter with proper validation and fallbackGood validation against
gr_config.max_mtu(line 347) and sensible fallback to direct assignment when type doesn't provide a callback.
359-377: LGTM: up/down setter with correct flag manipulationFallback correctly sets/clears
GR_IFACE_F_UPflag when type doesn't provide a callback.
379-395: LGTM: promisc setter with sensible fallbackFallback correctly rejects enabling promiscuous mode (
EOPNOTSUPP) when unsupported, while allowing disable operations.
397-413: LGTM: allmulti setter matches promisc patternCorrectly rejects enabling all-multicast when unsupported, allows disabling. Consistent with
iface_set_promisc.
415-443: LGTM: VLAN operations properly delegate to type callbacksBoth
iface_add_vlanandiface_del_vlancorrectly validate and returnEOPNOTSUPPwhen the type doesn't support VLAN operations.
521-570: LGTM: event handler now propagates state to subinterfacesGood addition: when parent interface status changes (lines 536-546), the
GR_IFACE_S_RUNNINGflag is correctly propagated to all subinterfaces and events are pushed for each.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
smoke/_init.sh (1)
55-83: Do not flag expected SIGTERM (143) as failure; always restore core_pattern; guard empty PID
- Graceful SIGTERM after kill -15 returns 143; treat 0 and 143 as non-fail. This was previously flagged.
- Restore kernel.core_pattern regardless of grout’s exit result, not only on failure.
- Guard kill/wait when $grout_pid is empty or already gone to avoid noisy errors.
Apply:
- set +x - kill -15 "$grout_pid" - wait "$grout_pid" - ret="$?" - if [ "$ret" -ne 0 ]; then - status="$ret" - if [ "$ret" -gt 128 ]; then - sig=$((ret - 128)) - echo "fail: grout terminated by signal SIG$(kill -l $sig)" - else - echo "fail: grout exited with an error status $ret" - fi >&2 - if [ -n "$core_pattern" ]; then - # core dumps written to files - for core in $tmp/core.*.*; do - [ -f "$core" ] || continue - gdb -c "$core" -batch \ - -ex "info threads" \ - -ex "thread apply all bt" - done - # restore original core pattern - sysctl -w kernel.core_pattern="$core_pattern" - else - # fallback to systemd-coredump, if available - coredumpctl info --no-pager "$grout_pid" - fi - fi - set -x + set +x + if [ -n "${grout_pid:-}" ] && kill -0 "$grout_pid" 2>/dev/null; then + kill -15 "$grout_pid" + wait "$grout_pid" + ret="$?" + case "$ret" in + 0|143) + # normal exit or expected SIGTERM; keep original status + : + ;; + *) + status="$ret" + if [ "$ret" -gt 128 ]; then + sig=$((ret - 128)) + echo "fail: grout terminated by signal SIG$(kill -l "$sig")" >&2 + else + echo "fail: grout exited with an error status $ret" >&2 + fi + if [ -n "$core_pattern" ]; then + # core dumps written to files + for core in "$tmp"/core.*.*; do + [ -f "$core" ] || continue + gdb -c "$core" -batch \ + -ex "info threads" \ + -ex "thread apply all bt" + done + else + # fallback to systemd-coredump, if available + coredumpctl info --no-pager "$grout_pid" + fi + ;; + esac + fi + # restore original core pattern if we changed it + if [ -n "${core_pattern:-}" ]; then + sysctl -w kernel.core_pattern="$core_pattern" >/dev/null 2>&1 || true + fi + set -xmodules/infra/datapath/port_output.c (1)
32-34: Add bounds check before array access.Line 32 indexes
ctx->edges[port->port_id]without validating thatport->port_idis within bounds. The assertion on line 33 may be compiled out in production builds.Add a bounds check:
for (unsigned i = 0; i < nb_objs; i++) { struct rte_mbuf *mbuf = objs[i]; iface = mbuf_data(mbuf)->iface; port = iface_info_port(iface); + if (unlikely(port->port_id >= ctx->n_edges)) { + rte_pktmbuf_free(mbuf); + continue; + } + if (gr_mbuf_is_traced(mbuf)) gr_mbuf_trace_add(mbuf, node, 0); edge = ctx->edges[port->port_id]; assert(edge != RTE_EDGE_ID_INVALID); rte_node_enqueue_x1(graph, node, edge, mbuf); }modules/infra/cli/port.c (1)
51-53: Buffer overflow: use remaining size, not total size.Lines 51 and 53 pass
lentoSAFE_BUFfor the second write, butnbytes were already written at line 48. Onlylen - nbytes remain in the buffer.Apply this diff:
if (iface->mode == GR_IFACE_MODE_L1_XC) { if ((peer = iface_from_id(c, iface->domain_id)) != NULL) - SAFE_BUF(snprintf, len, " xc_peer=%s", peer->name); + SAFE_BUF(snprintf, len - n, " xc_peer=%s", peer->name); else - SAFE_BUF(snprintf, len, " xc_peer=%u", iface->domain_id); + SAFE_BUF(snprintf, len - n, " xc_peer=%u", iface->domain_id); }modules/infra/control/vlan.c (1)
80-81: Return iface_add_vlan's error directly.Returning -errno can report the wrong error if errno was clobbered. Store the return value and propagate it.
Apply this diff:
- if (iface_add_vlan(next->parent_id, next->vlan_id) < 0) - return -errno; + ret = iface_add_vlan(next->parent_id, next->vlan_id); + if (ret < 0) + return ret;modules/infra/control/iface.c (1)
116-146: Critical: type->fini not called on failure after init (past issue still unaddressed).If
type->initsucceeds but any subsequentset_*callback fails (lines 119-128), the code jumps tofailwithout callingtype->fini. This can leak resources allocated byinitor leave hardware in a partially-configured state.The previously suggested fix should be applied:
+ bool inited = false; if (type->init(iface, api_info) < 0) goto fail; + inited = true; if (type->set_mtu != NULL && type->set_mtu(iface, iface->mtu) < 0) @@ fail: if (iface != NULL) { + if (inited && type != NULL && type->fini != NULL) + type->fini(iface); if (vrf_ref) vrf_decref(iface->vrf_id);
🧹 Nitpick comments (4)
modules/infra/control/iface.c (4)
340-357: Consider adding a minimum MTU check.The function validates the upper bound against
gr_config.max_mtubut does not enforce a lower bound. While type-specificset_mtucallbacks can enforce their own limits, a global minimum (e.g., 68 bytes for IPv4, per RFC 791) could prevent misconfigurations.Example:
+ if (mtu < GR_MIN_MTU) + return errno_set(ERANGE); if (mtu > gr_config.max_mtu) return errno_set(ERANGE);
371-374: Simplify flag manipulation logic.The conditional checks before setting/clearing the flag are unnecessary since bitwise operations are idempotent.
- if (!(iface->flags & GR_IFACE_F_UP) && up) - iface->flags |= GR_IFACE_F_UP; - else if ((iface->flags & GR_IFACE_F_UP) && !up) - iface->flags &= ~GR_IFACE_F_UP; + if (up) + iface->flags |= GR_IFACE_F_UP; + else + iface->flags &= ~GR_IFACE_F_UP;
379-395: Consider updating flags even without callback for consistency.Unlike
iface_set_up_down(lines 371-374), this function does not updateiface->flagswhentype->set_promiscis NULL. This could lead to flag state inconsistency if promisc is disabled via API but the flag remains set.type = iface_type_get(iface->type); assert(type != NULL); if (type->set_promisc != NULL) return type->set_promisc(iface, enabled); if (enabled) return errno_set(EOPNOTSUPP); + iface->flags &= ~GR_IFACE_F_PROMISC; return 0;
397-413: Consider updating flags even without callback for consistency.Same as
iface_set_promisc, this function does not updateiface->flagswhentype->set_allmultiis NULL, potentially leavingGR_IFACE_F_ALLMULTIset even after disabling.type = iface_type_get(iface->type); assert(type != NULL); if (type->set_allmulti != NULL) return type->set_allmulti(iface, enabled); if (enabled) return errno_set(EOPNOTSUPP); + iface->flags &= ~GR_IFACE_F_ALLMULTI; return 0;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/check.yml(1 hunks)README.md(1 hunks)modules/infra/api/affinity.c(2 hunks)modules/infra/api/stats.c(4 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/mempool.c(1 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(5 hunks)modules/infra/control/worker.c(3 hunks)modules/infra/control/worker_test.c(5 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(3 hunks)modules/infra/datapath/l1_xconnect.c(1 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)smoke/_init.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- modules/infra/control/mempool.c
- modules/infra/api/stats.c
- modules/ipip/datapath_in.c
- modules/infra/control/worker.c
- modules/infra/control/worker_test.c
- modules/infra/datapath/l1_xconnect.c
🧰 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:
modules/infra/datapath/port_output.cmodules/infra/datapath/eth_output.cmodules/infra/control/loopback.cmodules/ipip/control.cmodules/ipip/ipip_priv.hmodules/infra/control/gr_port.hmodules/infra/cli/port.cmodules/infra/control/gr_vlan.hmodules/infra/api/affinity.cmodules/ipip/datapath_out.cmodules/infra/control/vlan.cmodules/infra/control/port.cmodules/infra/datapath/eth_input.cmodules/infra/control/iface.cmodules/infra/control/gr_iface.h
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: - Never comment about setext vs ATX style headings.
Files:
README.md
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.sh
🧬 Code graph analysis (11)
modules/infra/datapath/port_output.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/eth_output.c (2)
modules/infra/control/worker_test.c (1)
iface_from_id(40-42)modules/infra/control/iface.c (1)
iface_from_id(254-261)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/ipip/control.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/cli/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/vlan.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker_test.c (1)
iface_from_id(40-42)modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_del_subinterface(286-293)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (2)
port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
modules/infra/control/iface.c (6)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)modules/infra/control/worker_test.c (2)
gr_event_push(36-36)iface_from_id(40-42)modules/ip6/control/router_advert.c (1)
iface_event_handler(223-239)main/event.c (1)
gr_event_subscribe(14-16)
modules/infra/control/gr_iface.h (2)
modules/infra/control/worker_test.c (1)
iface_type_register(35-35)modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
⏰ 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 (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-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: deb
- GitHub Check: rpm
🔇 Additional comments (48)
smoke/_init.sh (2)
21-21: Good: capture pre-cleanup statusCapturing "$?" upfront ensures cleanup exits with the correct status baseline. LGTM.
84-85: LGTM: tidy teardownRemoving tmp dir and exiting with captured status is correct.
modules/infra/datapath/port_output.c (1)
27-27: LGTM! Accessor usage is correct.The change from direct casting to
iface_info_port(iface)correctly adopts the new accessor pattern.modules/infra/control/gr_port.h (1)
30-40: LGTM! Macro-based declaration is correct.The
GR_IFACE_INFOmacro correctly declares the per-port info struct and generates the inlineiface_info_port()accessor. New fields (started, pool, devargs) are added while preserving existing fields.modules/ipip/ipip_priv.h (2)
6-6: LGTM! Include necessary for macro.Adding
<gr_iface.h>is required for theGR_IFACE_INFOmacro used on line 16.
16-16: LGTM! Macro usage is correct.The
GR_IFACE_INFOmacro correctly declares the per-ipip info struct and generates the inlineiface_info_ipip()accessor.modules/infra/control/gr_iface.h (6)
24-29: LGTM! Well-designed macro.The
GR_IFACE_INFOmacro generates both the struct declaration and an inline type-checked accessor. The__may_alias__and alignment attributes are appropriate for flexible array usage iniface->info.
37-55: LGTM! API expansion with explicit callbacks.Replacing typedef-based function pointers with explicit named callbacks improves clarity. The new per-attribute setters (set_mtu, set_promisc, etc.) provide fine-grained control over interface configuration.
61-61: LGTM! Const-correctness improved.Returning
const struct iface_type *prevents modification of registered types, which is appropriate since type definitions should be immutable after registration.
74-82: LGTM! New public API surface.New per-attribute setter functions (iface_set_eth_addr, iface_set_mtu, etc.) provide a clean public API for modifying interface attributes. These complement the existing iface_type callbacks.
19-19: Mutablesubinterfacesverified; no caller changes needed.
71-72: No action needed: callers already pass struct iface, and side effects on the subinterfaces list are intentional.*modules/infra/api/affinity.c (2)
48-48: LGTM! Accessor usage is correct.The change from direct casting to
iface_info_port(iface)correctly adopts the new accessor pattern for collecting datapath ports.
89-89: LGTM! Consistent accessor usage.Using
iface_info_port(iface)to retrieve port information is consistent with the new accessor pattern used throughout the codebase.modules/infra/control/gr_vlan.h (1)
14-14: LGTM: Macro-based type declaration.The GR_IFACE_INFO macro usage aligns with the standardized accessor pattern introduced across the codebase.
modules/ipip/datapath_out.c (3)
26-26: LGTM: IFACE_DOWN edge addition.Consistent with the broader pattern of routing downed interfaces to a centralized handler.
55-60: LGTM: Admin-down check and accessor usage.The early UP-flag validation prevents unnecessary encapsulation work, and the iface_info_ipip accessor replaces the direct cast correctly.
106-106: LGTM: Edge routing.Properly routes downed interfaces to iface_input_admin_down.
modules/infra/datapath/eth_input.c (4)
19-19: LGTM: IFACE_DOWN edge addition.Aligns with the centralized admin-down handling pattern.
61-64: LGTM: Admin-down check for main interface.Early exit prevents unnecessary packet processing when the interface is administratively down.
84-87: LGTM: Admin-down check for VLAN interface.Correctly validates the VLAN sub-interface UP status after resolution.
160-160: LGTM: Edge routing and drop registration.Properly routes downed interfaces and registers the drop node.
Also applies to: 180-180
modules/ipip/control.c (3)
54-54: LGTM: Accessor usage.The iface_info_ipip accessor consistently replaces direct casts throughout the file.
Also applies to: 86-86, 113-113
98-99: LGTM: Default MTU initialization.The default MTU of 1480 is appropriate for IPIP tunnels (1500 - 20 byte outer IP header).
95-101: LGTM: Simplified initialization.The plain struct initialization and explicit base assignment clarifies the init flow.
modules/infra/datapath/eth_output.c (3)
52-55: LGTM: Admin-down check.Early validation prevents unnecessary packet processing.
56-66: LGTM: VLAN parent resolution with proper NULL check.The code correctly checks for NULL after iface_from_id and validates parent UP status before proceeding.
123-123: LGTM: Edge routing.Properly routes downed interfaces to iface_input_admin_down.
modules/infra/control/loopback.c (2)
34-38: LGTM: Macro-based type declaration.The GR_IFACE_INFO macro usage aligns with the standardized pattern.
47-47: LGTM: Accessor usage.The iface_info_loopback accessor consistently replaces direct casts throughout the file.
Also applies to: 59-59, 118-118, 192-192, 273-273
modules/infra/control/vlan.c (6)
42-42: LGTM: Accessor usage.The iface_info_vlan accessor consistently replaces direct casts throughout the file.
Also applies to: 107-107, 137-137, 143-143, 152-152, 161-161
51-55: LGTM: MAC filter cleanup on reconfig.Properly removes the old MAC filter when the MAC attribute changes during reconfiguration.
63-63: LGTM: Parent resolution with error handling.The code correctly returns -errno if the parent interface lookup fails.
85-86: LGTM: State and MTU propagation from parent.Correctly synchronizes state and MTU with the new parent interface.
93-100: LGTM: MAC filter handling.The zero-MAC check and error propagation are correct. Uses parent operations via public APIs.
113-117: LGTM: Cleanup operations.Both VLAN and MAC filter removal delegate to public APIs with proper error accumulation.
modules/infra/control/port.c (9)
149-178: LGTM: Port UP/DOWN implementation.Correctly handles link up/down operations with appropriate error handling for ENOSYS/EOPNOTSUPP cases. The flags are properly updated based on the operation outcome.
180-194: LGTM: MAC address setter.Handles both explicit MAC setting and default MAC retrieval correctly.
196-220: LGTM: Promiscuous mode setter.Properly handles enable/disable with appropriate error cases and updates flags based on actual hardware state.
222-246: LGTM: Allmulticast mode setter.Mirrors the promiscuous implementation correctly.
248-272: LGTM: MTU setter with subinterface propagation.Correctly sets MTU on the port and propagates it to all sub-interfaces.
274-286: LGTM: VLAN filter add/del operations.Both operations handle ENOSYS/EOPNOTSUPP gracefully.
Also applies to: 288-300
325-325: LGTM: TXQ size assignment.Correctly uses api->txq_size instead of duplicating rxq_size.
308-308: LGTM: Accessor usage.The iface_info_port accessor consistently replaces direct casts throughout the file.
Also applies to: 345-345, 374-374, 383-383, 421-421, 430-430, 478-478, 484-484, 566-566, 649-649, 689-689
786-792: LGTM: Per-attribute setters in vtable.The new function pointers wire the port-specific operations into the interface type, enabling centralized attribute management.
modules/infra/control/iface.c (3)
28-35: LGTM: const-correctness improvement.Returning a const pointer prevents modification of the type registry by callers, which is appropriate for a getter function.
295-308: LGTM: new setter follows established pattern.The function correctly checks for callback availability and returns
EOPNOTSUPPwhen unsupported.
534-547: LGTM: status propagation to subinterfaces.Correctly propagates interface status changes to all subinterfaces using
gr_vec_foreachand event notifications.
| ret = type->reconfig(iface, set_attrs, conf, api_info); | ||
| if (set_attrs & GR_IFACE_SET_VRF) { | ||
| if (ret == 0) | ||
| vrf_decref(old_vrf_id); | ||
| else | ||
| if (ret < 0) { | ||
| if (set_attrs & GR_IFACE_SET_VRF) | ||
| vrf_decref(conf->vrf_id); | ||
| return ret; | ||
| } | ||
|
|
||
| if (set_attrs & GR_IFACE_SET_VRF) { | ||
| iface->vrf_id = conf->vrf_id; | ||
| vrf_decref(old_vrf_id); | ||
| } | ||
|
|
||
| if (set_attrs & GR_IFACE_SET_MODE) { | ||
| iface->mode = conf->mode; | ||
| } | ||
|
|
||
| if (set_attrs & GR_IFACE_SET_MTU) { | ||
| if ((ret = iface_set_mtu(iface->id, conf->mtu)) < 0) | ||
| return ret; | ||
| } | ||
|
|
||
| if (set_attrs & GR_IFACE_SET_FLAGS) { | ||
| if ((ret = iface_set_promisc(iface->id, conf->flags & GR_IFACE_F_PROMISC)) < 0) | ||
| return ret; | ||
| if ((ret = iface_set_allmulti(iface->id, conf->flags & GR_IFACE_F_ALLMULTI)) < 0) | ||
| return ret; | ||
| if ((ret = iface_set_up_down(iface->id, conf->flags & GR_IFACE_F_UP)) < 0) | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Partial reconfig if common-attribute setters fail after type->reconfig succeeds.
If type->reconfig succeeds (line 190) and VRF/mode changes are committed (lines 197-204), but a subsequent setter (e.g., iface_set_mtu at line 207) fails, the function returns an error without rolling back the VRF/mode changes. This leaves the interface in a partially-reconfigured state.
Consider one of:
- Apply all common-attribute changes before committing VRF/mode updates.
- Add rollback logic to undo VRF/mode changes if common-attribute setters fail.
- Document that reconfig is not atomic and partial application is expected.
🤖 Prompt for AI Agents
In modules/infra/control/iface.c around lines 190–218, the code commits VRF and
mode changes immediately after type->reconfig succeeds but then may return after
a later common-attribute setter (MTU/flags) fails, leaving the iface partially
updated; to fix this, perform the common-attribute setters (MTU, promisc,
allmulti, up/down) before assigning iface->vrf_id and iface->mode and before
calling vrf_decref(old_vrf_id), so that if any setter fails you return without
having mutated iface state, or alternatively add explicit rollback that restores
iface->vrf_id and iface->mode and fixes reference counts when a later setter
fails — choose the former (reorder setters to apply common attributes first,
then commit VRF/mode and drop old_vrf_id) for simpler, atomic behavior.
| ulimit -c unlimited | ||
| core_pattern=$(sysctl -n kernel.core_pattern) | ||
| if ! sysctl -w kernel.core_pattern="$tmp/core.%e.%p"; then | ||
| unset core_pattern | ||
| fi | ||
| export ASAN_OPTIONS=disable_coredump=0 | ||
| taskset -c 0,1 grout -tvvx $grout_extra_options & | ||
| fi |
There was a problem hiding this comment.
Capture grout PID with $! instead of pgrep; remove later pgrep
pgrep can race and may return multiple PIDs; $! is precise and robust here (taskset execs grout). Also guard for unset PID in cleanup (see other comment).
Apply:
if [ "$run_grout" = true ]; then
ulimit -c unlimited
core_pattern=$(sysctl -n kernel.core_pattern)
if ! sysctl -w kernel.core_pattern="$tmp/core.%e.%p"; then
unset core_pattern
fi
export ASAN_OPTIONS=disable_coredump=0
- taskset -c 0,1 grout -tvvx $grout_extra_options &
+ taskset -c 0,1 grout -tvvx $grout_extra_options &
+ grout_pid=$!
fi
socat FILE:/dev/null UNIX-CONNECT:$GROUT_SOCK_PATH,retry=10
-grout_pid=$(pgrep -g0 grout)Also applies to: 143-143
🤖 Prompt for AI Agents
In smoke/_init.sh around lines 134-141 (and also at line 143), the script uses
pgrep to find the grout PID which can race or return multiple PIDs; replace
pgrep usage by capturing the PID immediately after launching grout with
grub_pid=$! (or grout_pid=$!) so you record the exact child PID, remove the
later pgrep invocation(s), and update the cleanup logic to check that the PID
variable is set and non-empty before attempting to kill or wait on it to avoid
errors when the variable is unset.
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 (3)
modules/infra/control/gr_iface.h (1)
6-15: Missing <assert.h> include for macro assert().This header uses assert in inline functions but doesn’t include <assert.h>. Add it here to avoid TU‑order dependency.
#include <rte_ether.h> #include <stddef.h> +#include <assert.h> #include <stdint.h> #include <sys/queue.h>modules/infra/api/affinity.c (1)
34-41: cpusetsize misuse: use sizeof(cpu_set_t), not CPU_SETSIZE.CPU_SETSIZE is a count, but the API expects size in bytes.
- ret = -pthread_setaffinity_np(pthread_self(), CPU_SETSIZE, &req->control_cpus); + ret = -pthread_setaffinity_np(pthread_self(), sizeof(req->control_cpus), &req->control_cpus); @@ - ret = -control_output_set_affinity(CPU_SETSIZE, &req->control_cpus); + ret = -control_output_set_affinity(sizeof(req->control_cpus), &req->control_cpus);modules/ipip/control.c (1)
64-80: Reconfig falsely errors on no-op (self) updates.lookup() flags EADDRINUSE even when the mapping is unchanged for this iface. Use lookup_data and compare to iface; only re-add on actual key change.
- if (rte_hash_lookup(ipip_hash, &next_key) >= 0) - return errno_set(EADDRINUSE); + void *existing; + ret = rte_hash_lookup_data(ipip_hash, &next_key, &existing); + if (ret >= 0 && existing != iface) + return errno_set(EADDRINUSE); @@ - if (memcmp(&cur_key, &next_key, sizeof(cur_key)) != 0) - rte_hash_del_key(ipip_hash, &cur_key); - - if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) - return errno_log(-ret, "rte_hash_add_key_data"); + if (memcmp(&cur_key, &next_key, sizeof(cur_key)) != 0) { + rte_hash_del_key(ipip_hash, &cur_key); + if ((ret = rte_hash_add_key_data(ipip_hash, &next_key, iface)) < 0) + return errno_log(-ret, "rte_hash_add_key_data"); + }
♻️ Duplicate comments (3)
modules/infra/control/vlan.c (1)
80-81: Still returning -errno instead of propagating the actual return value.Despite the past review comment being marked as addressed, the code still returns
-errnoafter callingiface_add_vlan. The globalerrnocan be clobbered between the function call and the check, leading to incorrect error reporting. Other calls in this file (lines 94-95, 113-114, 116-117) correctly capture and propagate the return value.Apply this diff:
- if (iface_add_vlan(next->parent_id, next->vlan_id) < 0) - return -errno; + ret = iface_add_vlan(next->parent_id, next->vlan_id); + if (ret < 0) + return ret;modules/infra/control/iface.c (2)
119-129: Missing type->fini call on post-init setter failure.If
type->initsucceeds (line 116) but any subsequent setter fails (lines 119-128), the code jumps tofailwithout callingtype->fini, potentially leaking resources or leaving hardware configured.Apply this diff to call fini before cleanup:
+ bool inited = false; if (type->init(iface, api_info) < 0) goto fail; + inited = true; if (type->set_mtu != NULL && type->set_mtu(iface, iface->mtu) < 0) @@ fail: if (iface != NULL) { + if (inited && type != NULL && type->fini != NULL) + type->fini(iface); if (vrf_ref) vrf_decref(iface->vrf_id);
197-218: Partial reconfig risk: VRF/mode committed before MTU/flag setters.VRF and mode changes are committed (lines 197-204) before MTU and flag setters are invoked (lines 206-218). If a setter fails, the interface is left in a partially-reconfigured state. The setters for MTU/flags can fail, but VRF/mode assignments cannot be rolled back.
Consider applying common-attribute setters (MTU, promisc, allmulti, up/down) before committing VRF/mode changes, so failures leave the interface unchanged. Alternatively, document that partial reconfiguration is expected behavior.
🧹 Nitpick comments (3)
modules/infra/control/gr_iface.h (2)
24-29: Accessor breaks const-correctness (returns non‑const from const iface).Provide a const‑safe accessor variant to avoid casting away const. Keep existing API for mutating sites.
-#define GR_IFACE_INFO(type_id, type_name, fields) \ - struct type_name fields __attribute__((__may_alias__, aligned(alignof(void *)))); \ - static inline struct type_name *type_name(const struct iface *iface) { \ - assert(iface->type == type_id); \ - return (struct type_name *)iface->info; \ - } +#define GR_IFACE_INFO(type_id, type_name, fields) \ + struct type_name fields __attribute__((__may_alias__, aligned(alignof(void *)))); \ + static inline struct type_name *type_name(struct iface *iface) { \ + assert(iface->type == type_id); \ + return (struct type_name *)iface->info; \ + } \ + static inline const struct type_name *type_name##_c(const struct iface *iface) { \ + assert(iface->type == type_id); \ + return (const struct type_name *)iface->info; \ + }After adding this, switch const call sites (e.g., ipip datapath) to use the
_cvariant.
21-21: Safer alignment for iface->info storage.Pointer alignment may be insufficient on some platforms. Prefer max_align_t.
- alignas(alignof(void *)) uint8_t info[/* size depends on type */]; + alignas(alignof(max_align_t)) uint8_t info[/* size depends on type */];modules/ipip/datapath_out.c (1)
60-61: If adding const-safe accessor, use it here.Swap to iface_info_ipip_c(iface) once available to maintain const-correctness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
modules/infra/api/affinity.c(2 hunks)modules/infra/api/stats.c(4 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/mempool.c(1 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(5 hunks)modules/infra/control/worker.c(3 hunks)modules/infra/control/worker_test.c(5 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(3 hunks)modules/infra/datapath/l1_xconnect.c(1 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- modules/infra/cli/port.c
- modules/infra/control/worker_test.c
- modules/infra/control/gr_vlan.h
- modules/infra/control/loopback.c
- modules/infra/control/mempool.c
- modules/infra/datapath/l1_xconnect.c
- modules/infra/control/port.c
- modules/ipip/datapath_in.c
- modules/infra/datapath/port_output.c
- modules/infra/api/stats.c
- modules/infra/control/gr_port.h
- modules/infra/datapath/eth_input.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
modules/ipip/datapath_out.cmodules/infra/api/affinity.cmodules/ipip/control.cmodules/infra/datapath/eth_output.cmodules/ipip/ipip_priv.hmodules/infra/control/worker.cmodules/infra/control/vlan.cmodules/infra/control/gr_iface.hmodules/infra/control/iface.c
🧬 Code graph analysis (8)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/ipip/control.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/eth_output.c (1)
modules/infra/control/iface.c (1)
iface_from_id(254-261)
modules/infra/control/worker.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/vlan.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_del_subinterface(286-293)
modules/infra/control/gr_iface.h (2)
modules/infra/control/worker_test.c (1)
iface_type_register(35-35)modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
modules/infra/control/iface.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)modules/infra/control/worker_test.c (2)
gr_event_push(36-36)iface_from_id(40-42)main/event.c (1)
gr_event_subscribe(14-16)
⏰ 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-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-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (4)
modules/infra/control/worker.c (1)
171-171: LGTM: accessor usage is correct and consistent.Swapping raw casts for iface_info_port(iface) improves safety/readability without behavior change.
Also applies to: 212-212, 281-281
modules/infra/api/affinity.c (1)
48-49: LGTM: switched to iface_info_port(iface).Consistent with the new API.
modules/ipip/datapath_out.c (1)
26-28: LGTM: admin-down path added and mapped.Clean early routing to iface_input_admin_down; edge table updated accordingly.
Also applies to: 55-58, 101-107
modules/ipip/control.c (1)
98-102: LGTM: default MTU initialization.Setting MTU to 1480 when unset simplifies init.
Push GR_EVENT_IFACE_POST_RECONFIG directly from iface_reconfig() if everything is successful. Signed-off-by: Robin Jarry <[email protected]>
Instead of handling status up/down events in VLAN-specific code, propagate status changes to all sub-interfaces from the central interface event handler. This makes status inheritance work for any sub-interface type, not just VLANs. Remove const qualifiers from subinterface vector since we now modify sub-interface state directly. Simplify MTU propagation in port reconfiguration to iterate subinterfaces directly. Signed-off-by: Robin Jarry <[email protected]>
Enable independent configuration of receive and transmit queue sizes by assigning txq_size from the correct API field instead of reusing rxq_size for both. Signed-off-by: Robin Jarry <[email protected]>
The condition `reserved <= 0` would free the mempool when reserved is negative (underflow). Use exact equality check instead and add assertion to catch underflow bugs earlier. Signed-off-by: Robin Jarry <[email protected]>
Replace typedef declarations with direct function pointer types in iface_type structure fields. The typedefs are only used once and add unnecessary indirection. Signed-off-by: Robin Jarry <[email protected]>
Interface type definitions are registered once at initialization and should never be modified by callers. Enforce this with const return type. Signed-off-by: Robin Jarry <[email protected]>
Set default MTU of 1480 directly on the iface object when mtu is zero, then copy iface->base to initialize the conf structure for reconfig. This avoids ternary operators in structure initializers and eliminates field-by-field reconstruction. Signed-off-by: Robin Jarry <[email protected]>
Extend iface_type with optional callbacks for setting MAC addresses, MTU, up/down state, promiscuous mode, allmulticast mode, and VLAN filtering. This allows interface types to implement hardware-specific configuration while falling back to generic behavior when callbacks are not provided. Implement wrapper functions that validate the interface, check if the callback is available, and invoke it. For operations that cannot be performed without hardware support (like promiscuous mode), return EOPNOTSUPP if the callback is missing and the operation is requested. Signed-off-by: Robin Jarry <[email protected]>
Implement the iface_type callbacks for DPDK ethernet ports, allowing configuration operations to be performed through the interface layer instead of directly on port objects. Ignore ENOSYS and EOPNOTSUPP errors from DPDK drivers when hardware features are unavailable. Update interface flags based on actual hardware state after configuration attempts to reflect what the driver actually supports. Signed-off-by: Robin Jarry <[email protected]>
Use the interface callback system for VLAN operations on parent interfaces instead of calling DPDK functions directly. This makes VLAN interfaces work with any parent that implements the necessary callbacks, not just ethernet ports. Remove get_parent_port_id() which enforced the port-only restriction. Propagate callback failures properly while ignoring EOPNOTSUPP for operations the parent doesn't support. Signed-off-by: Robin Jarry <[email protected]>
Move common configuration logic (MTU, flags, VRF) from type-specific reconfig functions into iface_reconfig() itself. After the type-specific callback returns successfully, apply configuration through the new interface callbacks (set_mtu, set_promisc, set_allmulti, set_up_down) instead of requiring each type to implement it. Call these configuration callbacks during iface_create() as well to ensure initial configuration is applied consistently. This reduces code duplication across interface types and ensures all interfaces handle configuration the same way regardless of their underlying implementation. Signed-off-by: Robin Jarry <[email protected]>
Pass the correct remaining buffer length to the second snprintf() call. This prevents writing past the end of the buffer when formatting the xc_peer information. Signed-off-by: Robin Jarry <[email protected]>
If rte_eth_dev_info_get() returns an error, info.driver_name may be NULL. Check the value before passing it to strcmp to avoid crashes. Signed-off-by: Robin Jarry <[email protected]>
Define a GR_IFACE_INFO() macro that both declares the interface info structure and generates a type-safe inline accessor function. Replace all manual casts of iface->info with calls to these generated accessor functions (iface_info_port, iface_info_vlan, iface_info_loopback). The inline functions include assertions to catch type mismatches in debug builds and make the code more readable by eliminating repetitive cast expressions. Signed-off-by: Robin Jarry <[email protected]>
Check GR_IFACE_F_UP flag in eth_input, eth_output, ipip_input and ipip_output nodes before processing packets. If the interface or its parent (for VLANs) is administratively down, redirect packets to the iface_input_admin_down drop node. This prevents traffic from flowing through interfaces that have been disabled via configuration. Signed-off-by: Robin Jarry <[email protected]>
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)
modules/infra/control/loopback.c (1)
118-146: Fix mbuf leak on EAGAIN/EWOULDBLOCKEarly return skips freeing mbuf.
Apply:
- if ((len = read(lo->fd, data, read_len)) <= 0) { - if (errno == EAGAIN || errno == EWOULDBLOCK) - return; + if ((len = read(lo->fd, data, read_len)) <= 0) { + if (errno == EAGAIN || errno == EWOULDBLOCK) { + rte_pktmbuf_free(mbuf); + return; + }modules/infra/control/vlan.c (1)
66-69: Avoid false EADDRINUSE on no‑op reconfigChecking existence without verifying it’s not the same iface blocks reconfig when parent/vlan unchanged.
Proposed fix: compare found value with iface and only error if different.
- if (rte_hash_lookup(vlan_hash, &next_key) >= 0) - return errno_set(EADDRINUSE); + void *existing = NULL; + if (rte_hash_lookup_data(vlan_hash, &next_key, &existing) >= 0 && existing != iface) + return errno_set(EADDRINUSE);
♻️ Duplicate comments (3)
modules/infra/control/iface.c (2)
116-129: Fix: call type->fini on post-init failures and avoid using uninitialized iface->vrf_id in fail path
- If any set_* fails after init, we leak hardware/resources (missing fini).
- Fail path decrefs VRF via iface->vrf_id which may be unset when init fails.
Apply:
@@ - struct iface *iface = NULL; - bool vrf_ref = false; + struct iface *iface = NULL; + bool vrf_ref = false, inited = false; @@ - if (conf->type != GR_IFACE_TYPE_LOOPBACK) { - vrf_incref(conf->vrf_id); + if (conf->type != GR_IFACE_TYPE_LOOPBACK) { + vrf_incref(conf->vrf_id); vrf_ref = true; } @@ - if (type->init(iface, api_info) < 0) - goto fail; + if (type->init(iface, api_info) < 0) + goto fail; + inited = true; @@ fail: if (iface != NULL) { - if (vrf_ref) - vrf_decref(iface->vrf_id); + if (inited && type != NULL && type->fini != NULL) + type->fini(iface); + if (vrf_ref) + vrf_decref(conf->vrf_id); free(iface->name); }Also applies to: 137-146, 98-101, 109-114
183-195: Make reconfig more atomic: apply MTU/flags before committing VRF/modeCurrently VRF/mode are committed, then MTU/flags can fail leaving partial state. Reorder to apply common-attrs first; commit VRF/mode last.
@@ - if (set_attrs & GR_IFACE_SET_VRF) { - iface->vrf_id = conf->vrf_id; - vrf_decref(old_vrf_id); - } - - if (set_attrs & GR_IFACE_SET_MODE) { - iface->mode = conf->mode; - } - - if (set_attrs & GR_IFACE_SET_MTU) { - if ((ret = iface_set_mtu(iface->id, conf->mtu)) < 0) - return ret; - } - - if (set_attrs & GR_IFACE_SET_FLAGS) { - if ((ret = iface_set_promisc(iface->id, conf->flags & GR_IFACE_F_PROMISC)) < 0) - return ret; - if ((ret = iface_set_allmulti(iface->id, conf->flags & GR_IFACE_F_ALLMULTI)) < 0) - return ret; - if ((ret = iface_set_up_down(iface->id, conf->flags & GR_IFACE_F_UP)) < 0) - return ret; - } + /* Apply common attributes first; then commit VRF/mode */ + if (set_attrs & GR_IFACE_SET_MTU) { + if ((ret = iface_set_mtu(iface->id, conf->mtu)) < 0) + return ret; + } + if (set_attrs & GR_IFACE_SET_FLAGS) { + if ((ret = iface_set_promisc(iface->id, conf->flags & GR_IFACE_F_PROMISC)) < 0) + return ret; + if ((ret = iface_set_allmulti(iface->id, conf->flags & GR_IFACE_F_ALLMULTI)) < 0) + return ret; + if ((ret = iface_set_up_down(iface->id, conf->flags & GR_IFACE_F_UP)) < 0) + return ret; + } + if (set_attrs & GR_IFACE_SET_VRF) { + iface->vrf_id = conf->vrf_id; + vrf_decref(old_vrf_id); + } + if (set_attrs & GR_IFACE_SET_MODE) { + iface->mode = conf->mode; + }Also applies to: 197-218
modules/infra/control/vlan.c (1)
80-82: Return actual error from iface_add_vlan (don’t use -errno)Using -errno may return the wrong code. Propagate ret directly.
- if (iface_add_vlan(next->parent_id, next->vlan_id) < 0) - return -errno; + ret = iface_add_vlan(next->parent_id, next->vlan_id); + if (ret < 0) + return ret;
🧹 Nitpick comments (6)
modules/infra/control/iface.c (3)
340-357: Keep iface->mtu in sync on successful driver set (optional)After a successful type->set_mtu, update iface->mtu to reflect current state.
- if (type->set_mtu != NULL) - return type->set_mtu(iface, mtu); + if (type->set_mtu != NULL) { + int rc = type->set_mtu(iface, mtu); + if (rc < 0) + return rc; + iface->mtu = mtu; + return 0; + } iface->mtu = mtu; return 0;
379-395: Sync GR_IFACE_F_PROMISC bit after successful driver call (optional)Keep flags consistent when promisc toggles succeed.
- if (type->set_promisc != NULL) - return type->set_promisc(iface, enabled); + if (type->set_promisc != NULL) { + int rc = type->set_promisc(iface, enabled); + if (rc < 0) + return rc; + if (enabled) + iface->flags |= GR_IFACE_F_PROMISC; + else + iface->flags &= ~GR_IFACE_F_PROMISC; + return 0; + } - if (enabled) - return errno_set(EOPNOTSUPP); - - return 0; + if (enabled) + return errno_set(EOPNOTSUPP); + iface->flags &= ~GR_IFACE_F_PROMISC; + return 0;
397-413: Sync GR_IFACE_F_ALLMULTI bit after successful driver call (optional)Mirror promisc change for allmulti.
- if (type->set_allmulti != NULL) - return type->set_allmulti(iface, enabled); + if (type->set_allmulti != NULL) { + int rc = type->set_allmulti(iface, enabled); + if (rc < 0) + return rc; + if (enabled) + iface->flags |= GR_IFACE_F_ALLMULTI; + else + iface->flags &= ~GR_IFACE_F_ALLMULTI; + return 0; + } - if (enabled) - return errno_set(EOPNOTSUPP); - - return 0; + if (enabled) + return errno_set(EOPNOTSUPP); + iface->flags &= ~GR_IFACE_F_ALLMULTI; + return 0;modules/ipip/datapath_in.c (1)
65-68: Populate iface in mbuf metadata before routing to IFACE_DOWNiface_input_admin_down may rely on eth_input_mbuf_data->iface. Set it before goto next to keep attribution and tracing consistent.
Proposed diff:
- if (!(ipip->flags & GR_IFACE_F_UP)) { - edge = IFACE_DOWN; - goto next; - } + if (!(ipip->flags & GR_IFACE_F_UP)) { + eth_data = eth_input_mbuf_data(mbuf); + eth_data->iface = ipip; + edge = IFACE_DOWN; + goto next; + }modules/infra/control/loopback.c (1)
47-49: Const‑correctness nitUse a const pointer when only reading fields.
- struct iface_info_loopback *lo = iface_info_loopback(iface); + const struct iface_info_loopback *lo = iface_info_loopback(iface);Apply similarly to other read‑only call sites.
modules/infra/datapath/eth_output.c (1)
98-106: Trace should keep original iface id (not parent)After VLAN resolution, priv->iface points to parent. For consistent tracing, record the original interface (iface variable), not the parent.
- t->iface_id = priv->iface->id; + t->iface_id = iface->id;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
modules/infra/api/affinity.c(2 hunks)modules/infra/api/stats.c(4 hunks)modules/infra/cli/port.c(1 hunks)modules/infra/control/gr_iface.h(2 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/gr_vlan.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(6 hunks)modules/infra/control/mempool.c(1 hunks)modules/infra/control/port.c(12 hunks)modules/infra/control/vlan.c(5 hunks)modules/infra/control/worker.c(3 hunks)modules/infra/control/worker_test.c(5 hunks)modules/infra/datapath/eth_input.c(5 hunks)modules/infra/datapath/eth_output.c(3 hunks)modules/infra/datapath/l1_xconnect.c(1 hunks)modules/infra/datapath/port_output.c(1 hunks)modules/ipip/control.c(4 hunks)modules/ipip/datapath_in.c(3 hunks)modules/ipip/datapath_out.c(3 hunks)modules/ipip/ipip_priv.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- modules/infra/cli/port.c
- modules/infra/api/stats.c
- modules/infra/control/worker_test.c
- modules/infra/datapath/port_output.c
- modules/ipip/control.c
- modules/infra/control/gr_port.h
- modules/infra/datapath/l1_xconnect.c
- modules/infra/control/mempool.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
modules/infra/datapath/eth_input.cmodules/ipip/datapath_in.cmodules/infra/control/port.cmodules/infra/api/affinity.cmodules/infra/control/vlan.cmodules/ipip/ipip_priv.hmodules/infra/control/worker.cmodules/infra/datapath/eth_output.cmodules/ipip/datapath_out.cmodules/infra/control/iface.cmodules/infra/control/loopback.cmodules/infra/control/gr_vlan.hmodules/infra/control/gr_iface.h
🧬 Code graph analysis (9)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/worker.c (2)
port_plug(184-223)worker_count(125-133)modules/infra/control/iface.c (1)
iface_next(237-252)
modules/infra/api/affinity.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/vlan.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/iface.c (8)
iface_from_id(254-261)iface_del_eth_addr(325-338)iface_del_vlan(430-443)iface_add_vlan(415-428)iface_add_subinterface(278-282)iface_get_eth_addr(263-276)iface_add_eth_addr(310-323)iface_del_subinterface(286-293)
modules/infra/control/worker.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/datapath/eth_output.c (2)
modules/infra/control/worker_test.c (1)
iface_from_id(40-42)modules/infra/control/iface.c (1)
iface_from_id(254-261)
modules/ipip/datapath_out.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/iface.c (5)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/vrf.c (2)
vrf_incref(26-42)vrf_decref(44-60)modules/infra/control/worker_test.c (2)
gr_event_push(36-36)iface_from_id(40-42)main/event.c (1)
gr_event_subscribe(14-16)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/gr_iface.h (1)
modules/infra/control/iface.c (14)
iface_type_register(37-43)iface_type_get(28-35)iface_add_subinterface(278-282)iface_del_subinterface(286-293)iface_get_eth_addr(263-276)iface_set_eth_addr(295-308)iface_add_eth_addr(310-323)iface_del_eth_addr(325-338)iface_set_mtu(340-357)iface_set_up_down(359-377)iface_set_promisc(379-395)iface_set_allmulti(397-413)iface_add_vlan(415-428)iface_del_vlan(430-443)
⏰ 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: deb
- GitHub Check: rpm
- 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: 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)
🔇 Additional comments (30)
modules/infra/control/iface.c (2)
28-35: Const-correct iface_type_get looks goodReturning const struct iface_type * is correct and consistent across uses.
521-570: Event propagation to subinterfaces looks goodRUNNING state propagation on STATUS_UP/DOWN and handler wiring is clear.
modules/infra/control/gr_iface.h (3)
19-29: GR_IFACE_INFO macro and accessor are soundAligned alias with type check is clean and avoids scattered casts.
61-61: Const-correct iface_type_get prototype OKMatches implementation in iface.c and improves safety.
49-56: Per-attribute callbacks added appropriatelyAPI surface is coherent with control paths and datapath needs.
Also applies to: 71-82
modules/infra/control/worker.c (1)
170-174: Good move to iface_info_port() accessorRemoves brittle casts and improves readability.
Also applies to: 208-216, 278-283
modules/infra/api/affinity.c (1)
47-50: Accessor migration LGTMUsing iface_info_port(iface) consistently is correct here.
Also applies to: 89-91
modules/ipip/ipip_priv.h (1)
6-6: Adoption of GR_IFACE_INFO for IPIP looks goodConsistent with iface info accessors elsewhere.
Also applies to: 16-16
modules/infra/control/gr_vlan.h (1)
14-15: VLAN info via GR_IFACE_INFO is correctKeeps per-type access uniform.
modules/infra/datapath/eth_input.c (1)
19-21: Admin-down handling and edge wiring look solidProperly routes to iface_input_admin_down and registers the drop.
Also applies to: 61-65, 84-88, 156-163, 177-180
modules/ipip/datapath_in.c (2)
24-26: Enum extension looks goodAdding IFACE_DOWN with EDGE_COUNT update is correct.
101-104: next_nodes wiring is consistentIFACE_DOWN → iface_input_admin_down wiring is aligned with the new edge.
modules/ipip/datapath_out.c (4)
26-28: Enum addition is fineAdds IFACE_DOWN and keeps EDGE_COUNT accurate.
55-61: Early admin-down check is correctShort-circuits non‑UP iface to IFACE_DOWN and preserves ip_data->iface for downstream handling.
60-61: Good: switch to accessorUsing iface_info_ipip(iface) instead of casting improves safety and clarity.
106-107: next_nodes wiring is correctIFACE_DOWN → iface_input_admin_down matches the new edge.
modules/infra/control/loopback.c (2)
34-39: Adoption of GR_IFACE_INFO is goodMacro-based per-type storage + accessor improves safety and encapsulation.
273-276: Fini path looks fineAccessor usage + event_free_finalize is correct.
modules/infra/datapath/eth_output.c (3)
52-55: Early admin‑down path is correctDefensive check avoids building frames for down interfaces.
56-67: VLAN parent resolution and checks look goodNULL and UP checks avoid crashes and misroutes; header push is after validation.
123-124: next_nodes wiring is correctIFACE_DOWN → iface_input_admin_down matches the new edge.
modules/infra/control/vlan.c (4)
127-134: Init flow via reconfig is fineReusing reconfig for IFACE_SET_ALL and propagating errno is OK.
137-140: Accessor usage in get_eth_addr looks goodReads from iface_info_vlan and returns copy; OK.
142-158: Add/Del multicast MAC via parent is correctValidates multicast and delegates to parent using public API.
160-165: to_api uses accessor correctlyCopies base into API struct; OK.
modules/infra/control/port.c (5)
149-300: Well-structured callback implementations.The new per-port operation functions (port_up_down, port_mac_set, port_promisc_set, port_allmulti_set, port_mtu_set, port_vlan_add, port_vlan_del) properly handle -ENOSYS and -EOPNOTSUPP as non-fatal cases. State synchronization is handled correctly—promiscuous and allmulticast modes read back actual hardware state after attempting changes.
325-325: Good catch on txq_size.Correctly uses
api->txq_sizeinstead of duplicatingapi->rxq_size.
376-376: Defensive programming improvements.Initializing
infoto zero at line 376 and adding a NULL check fordriver_nameat line 402 prevent potential crashes in error paths.Also applies to: 402-402
308-308: Accessor pattern consistently applied.All conversions from direct
iface->infocasts toiface_info_port(iface)are correct and align with the new type-safe accessor approach introduced in this PR.Also applies to: 345-345, 374-374, 383-383, 421-421, 430-430, 478-478, 484-484, 566-566, 649-649, 689-689
786-792: Function pointer assignments complete the callback interface.The iface_type_port struct now correctly exposes all per-port operations (set_eth_addr, set_mtu, set_up_down, set_promisc, set_allmulti, add_vlan, del_vlan) to the interface layer.
| static int iface_vlan_fini(struct iface *iface) { | ||
| struct iface_info_vlan *vlan = (struct iface_info_vlan *)iface->info; | ||
| struct iface_info_vlan *vlan = iface_info_vlan(iface); | ||
| struct iface *parent = iface_from_id(vlan->parent_id); | ||
| uint16_t port_id = RTE_MAX_ETHPORTS; | ||
| struct iface_type *parent_type; | ||
| int ret, status = 0; | ||
|
|
||
| if (get_parent_port_id(vlan->parent_id, &port_id) < 0) | ||
| return -errno; | ||
|
|
||
| parent_type = iface_type_get(parent->type); | ||
|
|
||
| rte_hash_del_key(vlan_hash, &(struct vlan_key) {vlan->parent_id, vlan->vlan_id}); | ||
|
|
||
| if ((ret = rte_eth_dev_vlan_filter(port_id, vlan->vlan_id, false)) < 0) | ||
| errno_log(-ret, "rte_eth_dev_vlan_filter disable"); | ||
| if (status == 0 && ret < 0) { | ||
| switch (errno) { | ||
| case ENOSYS: | ||
| case EOPNOTSUPP: | ||
| break; | ||
| default: | ||
| status = ret; | ||
| } | ||
| } | ||
| if ((ret = iface_del_vlan(vlan->parent_id, vlan->vlan_id)) < 0) | ||
| status = ret; | ||
|
|
||
| ret = parent_type->del_eth_addr(parent, &vlan->mac); | ||
| if (status == 0 && ret < 0) { | ||
| switch (errno) { | ||
| case ENOSYS: | ||
| case EOPNOTSUPP: | ||
| break; | ||
| default: | ||
| status = ret; | ||
| } | ||
| } | ||
| if ((ret = iface_del_eth_addr(vlan->parent_id, &vlan->mac)) < 0) | ||
| status = status ?: ret; | ||
|
|
||
| iface_del_subinterface(parent, iface); | ||
|
|
||
| return status; | ||
| } |
There was a problem hiding this comment.
NULL‑deref risk on parent in fini
parent may be NULL (e.g., already removed). Guard before calling iface_del_subinterface.
- struct iface *parent = iface_from_id(vlan->parent_id);
+ struct iface *parent = iface_from_id(vlan->parent_id);
@@
- iface_del_subinterface(parent, iface);
+ if (parent)
+ iface_del_subinterface(parent, iface);📝 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.
| static int iface_vlan_fini(struct iface *iface) { | |
| struct iface_info_vlan *vlan = (struct iface_info_vlan *)iface->info; | |
| struct iface_info_vlan *vlan = iface_info_vlan(iface); | |
| struct iface *parent = iface_from_id(vlan->parent_id); | |
| uint16_t port_id = RTE_MAX_ETHPORTS; | |
| struct iface_type *parent_type; | |
| int ret, status = 0; | |
| if (get_parent_port_id(vlan->parent_id, &port_id) < 0) | |
| return -errno; | |
| parent_type = iface_type_get(parent->type); | |
| rte_hash_del_key(vlan_hash, &(struct vlan_key) {vlan->parent_id, vlan->vlan_id}); | |
| if ((ret = rte_eth_dev_vlan_filter(port_id, vlan->vlan_id, false)) < 0) | |
| errno_log(-ret, "rte_eth_dev_vlan_filter disable"); | |
| if (status == 0 && ret < 0) { | |
| switch (errno) { | |
| case ENOSYS: | |
| case EOPNOTSUPP: | |
| break; | |
| default: | |
| status = ret; | |
| } | |
| } | |
| if ((ret = iface_del_vlan(vlan->parent_id, vlan->vlan_id)) < 0) | |
| status = ret; | |
| ret = parent_type->del_eth_addr(parent, &vlan->mac); | |
| if (status == 0 && ret < 0) { | |
| switch (errno) { | |
| case ENOSYS: | |
| case EOPNOTSUPP: | |
| break; | |
| default: | |
| status = ret; | |
| } | |
| } | |
| if ((ret = iface_del_eth_addr(vlan->parent_id, &vlan->mac)) < 0) | |
| status = status ?: ret; | |
| iface_del_subinterface(parent, iface); | |
| return status; | |
| } | |
| static int iface_vlan_fini(struct iface *iface) { | |
| struct iface_info_vlan *vlan = iface_info_vlan(iface); | |
| struct iface *parent = iface_from_id(vlan->parent_id); | |
| int ret, status = 0; | |
| rte_hash_del_key(vlan_hash, &(struct vlan_key) {vlan->parent_id, vlan->vlan_id}); | |
| if ((ret = iface_del_vlan(vlan->parent_id, vlan->vlan_id)) < 0) | |
| status = ret; | |
| if ((ret = iface_del_eth_addr(vlan->parent_id, &vlan->mac)) < 0) | |
| status = status ?: ret; | |
| if (parent) | |
| iface_del_subinterface(parent, iface); | |
| return status; | |
| } |
🤖 Prompt for AI Agents
In modules/infra/control/vlan.c around lines 106 to 122, the iface_vlan_fini
function uses parent obtained from iface_from_id(vlan->parent_id) without
checking for NULL before calling iface_del_subinterface, risking a NULL
dereference; add a NULL check and only call iface_del_subinterface(parent,
iface) if parent is non-NULL (handle or log the missing parent case as
appropriate), ensuring the rest of cleanup still runs and return status
unchanged.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor