prometheus exporter endpoints#471
Conversation
📝 WalkthroughWalkthroughAdds a new OpenMetrics exporter and runtime (main/gr_metrics.h, main/metrics.c) served via libevent, including APIs to register collectors, start/stop the server, and set thread affinity. Replaces rte_telemetry usage across the codebase with gr_metrics collectors for interfaces, ports, workers, nexthops, IPv4/IPv6 RIBs, and node statistics; removes several telemetry-backed public stats structs and getters. Introduces CLI/config support for metrics (-M, gr_config.metrics_addr/metrics_port), validator functions for nexthop types/origins, and adds libevent_extra as a build dependency. 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: 3
🤖 Fix all issues with AI Agents
In @modules/infra/api/graph.py:
- Line 19: The loop in graph.py iterates over the result of
sock.cmd("/grout/stats/graph") using "for name, stats in sock.cmd(...)" which
will raise a TypeError because the returned dict must be iterated with .items();
change the iteration to use sock.cmd("/grout/stats/graph").items() (preserving
the variables name and stats) so it matches the pattern used in nexthop.py,
rib4.py, and rib6.py.
In @modules/infra/control/port.c:
- Around line 600-616: port_telemetry_info may pass a NULL port->devargs to
rte_tel_data_add_dict_string (risky because iface_port_fini can set devargs
NULL); update port_telemetry_info to defensively check port->devargs (or use a
ternary) and pass a safe string like "" or "unknown" to
rte_tel_data_add_dict_string when devargs is NULL; locate the function
port_telemetry_info and the iface_info_port struct reference to implement the
null-coalescing fix and ensure no other rte_tel_data_add_dict_string calls in
this function receive NULL.
In @modules/infra/control/vlan.c:
- Around line 187-198: vlan_telemetry_info dereferences parent->name after
calling iface_from_id(vlan->parent_id) which can return NULL; update
vlan_telemetry_info to check the return value of iface_from_id and handle NULL
safely (e.g., set a local parent_name = parent ? parent->name : "<unknown>" or
similar) before calling rte_tel_data_add_dict_string, and avoid any dereference
of parent when NULL; ensure the function still returns 0 on success.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
meson.buildmodules/infra/api/gr_infra.hmodules/infra/api/graph.pymodules/infra/api/iface.cmodules/infra/api/iface.pymodules/infra/api/meson.buildmodules/infra/api/stats.cmodules/infra/control/bond.cmodules/infra/control/gr_iface.hmodules/infra/control/meson.buildmodules/infra/control/nexthop.pymodules/infra/control/port.cmodules/infra/control/vlan.cmodules/ip/control/meson.buildmodules/ip/control/rib4.pymodules/ip6/control/meson.buildmodules/ip6/control/rib6.pymodules/ipip/control.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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/api/gr_infra.hmodules/infra/control/gr_iface.hmodules/infra/control/bond.cmodules/infra/control/vlan.cmodules/infra/api/stats.cmodules/ipip/control.cmodules/infra/api/iface.cmodules/infra/control/port.c
🧠 Learnings (7)
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/gr_iface.hmodules/ipip/control.cmodules/infra/api/iface.pymodules/infra/api/iface.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/gr_iface.hmodules/infra/api/iface.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/infra/control/gr_iface.hmodules/infra/api/iface.c
🧬 Code graph analysis (7)
modules/infra/control/bond.c (1)
modules/infra/api/gr_infra.h (2)
gr_bond_mode_name(133-141)gr_bond_algo_name(150-160)
modules/infra/control/vlan.c (2)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/iface.c (1)
iface_from_id(256-263)
modules/ip/control/rib4.py (2)
modules/infra/control/nexthop.py (2)
info(7-10)metrics(13-17)modules/ip6/control/rib6.py (2)
info(7-10)metrics(13-21)
modules/ip6/control/rib6.py (2)
modules/infra/control/nexthop.py (2)
info(7-10)metrics(13-17)modules/ip/control/rib4.py (2)
info(7-10)metrics(13-21)
modules/infra/control/nexthop.py (4)
modules/infra/api/graph.py (2)
info(9-14)metrics(17-26)modules/infra/api/iface.py (2)
info(25-46)metrics(49-96)modules/ip/control/rib4.py (2)
info(7-10)metrics(13-21)modules/ip6/control/rib6.py (2)
info(7-10)metrics(13-21)
modules/infra/api/iface.c (3)
modules/infra/control/gr_iface.h (2)
iface(17-26)iface_get_stats(103-105)modules/infra/control/iface.c (1)
iface_next(239-254)modules/infra/api/gr_infra.h (2)
gr_iface_type_name(428-446)gr_iface_mode_name(449-459)
modules/infra/api/graph.py (4)
modules/infra/api/iface.py (2)
info(25-46)metrics(49-96)modules/infra/control/nexthop.py (2)
info(7-10)metrics(13-17)modules/ip/control/rib4.py (2)
info(7-10)metrics(13-21)modules/ip6/control/rib6.py (2)
info(7-10)metrics(13-21)
🪛 Ruff (0.14.10)
modules/ip/control/rib4.py
7-7: Undefined name Name
(F821)
7-7: Undefined name Description
(F821)
7-7: Undefined name Type
(F821)
13-13: Undefined name TelemetrySocket
(F821)
13-13: Undefined name Name
(F821)
13-13: Undefined name Value
(F821)
13-13: Undefined name Labels
(F821)
modules/ip6/control/rib6.py
7-7: Undefined name Name
(F821)
7-7: Undefined name Description
(F821)
7-7: Undefined name Type
(F821)
13-13: Undefined name TelemetrySocket
(F821)
13-13: Undefined name Name
(F821)
13-13: Undefined name Value
(F821)
13-13: Undefined name Labels
(F821)
modules/infra/control/nexthop.py
7-7: Undefined name Name
(F821)
7-7: Undefined name Description
(F821)
7-7: Undefined name Type
(F821)
13-13: Undefined name TelemetrySocket
(F821)
13-13: Undefined name Name
(F821)
13-13: Undefined name Value
(F821)
13-13: Undefined name Labels
(F821)
modules/infra/api/iface.py
25-25: Undefined name Name
(F821)
25-25: Undefined name Description
(F821)
25-25: Undefined name Type
(F821)
49-49: Undefined name TelemetrySocket
(F821)
49-49: Undefined name Name
(F821)
49-49: Undefined name Value
(F821)
49-49: Undefined name Labels
(F821)
modules/infra/api/graph.py
9-9: Undefined name Name
(F821)
9-9: Undefined name Description
(F821)
9-9: Undefined name Type
(F821)
17-17: Undefined name TelemetrySocket
(F821)
17-17: Undefined name Name
(F821)
17-17: Undefined name Value
(F821)
17-17: Undefined name Labels
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-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: rpm
- GitHub Check: deb
🔇 Additional comments (3)
modules/infra/control/bond.c (1)
411-428: LGTM!The telemetry hook implementation is clean. Correctly uses the helper functions for mode/algo names, handles LACP-specific
bond_algofield conditionally, and properly formats the MAC address with an adequately sized buffer.Also applies to: 446-446
modules/infra/control/nexthop.py (1)
1-17: LGTM!The module follows the established telemetry endpoint pattern. The Ruff warnings about undefined names are false positives—the quoted type hints (
"dict[Name, ...]") are documentation-only annotations consistent with the other telemetry modules in the codebase.modules/infra/api/iface.py (1)
1-96: LGTM!Well-structured telemetry module that properly handles optional fields and follows the established patterns from other telemetry endpoints. The Ruff warnings about undefined names are false positives—the quoted type hints are documentation-only annotations consistent with the codebase conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @modules/infra/api/iface.c:
- Around line 257-266: The telemetry dict container calls use keep=0 so
ownership isn't transferred and child containers leak; update the two calls to
rte_tel_data_add_dict_container (the one adding "statistics" to iface_dict and
the one adding iface_dict to d) to pass keep=1 so the parent dictionary takes
ownership and will free the child containers when freed; ensure existing
error-path frees (rte_tel_data_free(stats)/rte_tel_data_free(iface_dict)) remain
unchanged for failure cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
meson.buildmodules/infra/api/graph.pymodules/infra/api/iface.cmodules/infra/api/iface.pymodules/infra/api/meson.buildmodules/infra/api/stats.cmodules/infra/control/bond.cmodules/infra/control/gr_iface.hmodules/infra/control/meson.buildmodules/infra/control/nexthop.pymodules/infra/control/port.cmodules/infra/control/vlan.cmodules/ip/control/meson.buildmodules/ip/control/rib4.pymodules/ip6/control/meson.buildmodules/ip6/control/rib6.pymodules/ipip/control.c
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/infra/control/port.c
- modules/infra/control/bond.c
- modules/infra/control/meson.build
- modules/ipip/control.c
- modules/ip/control/meson.build
🧰 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.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/api/stats.cmodules/infra/api/iface.cmodules/infra/control/vlan.cmodules/infra/control/gr_iface.h
🧠 Learnings (7)
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/api/iface.pymodules/infra/api/iface.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/api/iface.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/infra/api/iface.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/control/gr_iface.h
🧬 Code graph analysis (4)
modules/ip6/control/rib6.py (2)
modules/infra/control/nexthop.py (2)
info(7-10)metrics(13-17)modules/ip/control/rib4.py (2)
info(7-10)metrics(13-21)
modules/infra/control/vlan.c (1)
modules/infra/control/iface.c (1)
iface_from_id(256-263)
modules/infra/control/nexthop.py (4)
modules/infra/api/graph.py (2)
info(9-14)metrics(17-26)modules/infra/api/iface.py (2)
info(25-46)metrics(49-96)modules/ip/control/rib4.py (2)
info(7-10)metrics(13-21)modules/ip6/control/rib6.py (2)
info(7-10)metrics(13-21)
modules/ip/control/rib4.py (2)
modules/infra/control/nexthop.py (2)
info(7-10)metrics(13-17)modules/ip6/control/rib6.py (2)
info(7-10)metrics(13-21)
🪛 Ruff (0.14.10)
modules/ip6/control/rib6.py
7-7: Undefined name Name
(F821)
7-7: Undefined name Description
(F821)
7-7: Undefined name Type
(F821)
13-13: Undefined name TelemetrySocket
(F821)
13-13: Undefined name Name
(F821)
13-13: Undefined name Value
(F821)
13-13: Undefined name Labels
(F821)
modules/infra/api/iface.py
25-25: Undefined name Name
(F821)
25-25: Undefined name Description
(F821)
25-25: Undefined name Type
(F821)
49-49: Undefined name TelemetrySocket
(F821)
49-49: Undefined name Name
(F821)
49-49: Undefined name Value
(F821)
49-49: Undefined name Labels
(F821)
modules/infra/control/nexthop.py
7-7: Undefined name Name
(F821)
7-7: Undefined name Description
(F821)
7-7: Undefined name Type
(F821)
13-13: Undefined name TelemetrySocket
(F821)
13-13: Undefined name Name
(F821)
13-13: Undefined name Value
(F821)
13-13: Undefined name Labels
(F821)
modules/ip/control/rib4.py
7-7: Undefined name Name
(F821)
7-7: Undefined name Description
(F821)
7-7: Undefined name Type
(F821)
13-13: Undefined name TelemetrySocket
(F821)
13-13: Undefined name Name
(F821)
13-13: Undefined name Value
(F821)
13-13: Undefined name Labels
(F821)
modules/infra/api/graph.py
9-9: Undefined name Name
(F821)
9-9: Undefined name Description
(F821)
9-9: Undefined name Type
(F821)
17-17: Undefined name TelemetrySocket
(F821)
17-17: Undefined name Name
(F821)
17-17: Undefined name Value
(F821)
17-17: Undefined name Labels
(F821)
⏰ 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: rpm
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- 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: deb
🔇 Additional comments (11)
modules/infra/api/meson.build (1)
17-21: LGTM!The telemetry endpoints addition is straightforward and correctly integrates the new Python telemetry modules into the build system.
meson.build (1)
105-105: LGTM!The telemetry endpoints infrastructure is properly initialized and installed. The pattern of accumulating entries across subdirectories is standard Meson practice.
Also applies to: 132-132
modules/ip6/control/rib6.py (1)
1-21: LGTM!The module correctly mirrors the IPv4 rib4.py telemetry pattern for IPv6 routes. The implementation properly queries
/grout/rib6/stats, extracts per-VRF IPv6 origin data, and emits Prometheus-style metric tuples.Note: The static analysis warnings about undefined type hints are false positives—the quoted forward references are a valid Python pattern used consistently across the codebase's telemetry modules.
modules/ip6/control/meson.build (1)
13-15: LGTM!The rib6.py telemetry endpoint is correctly added to the build configuration.
modules/infra/api/stats.c (1)
327-338: LGTM!The modification to always export nodes regardless of zero batch counts is correctly implemented. The error handling properly manages memory—
valownership transfers todon success, or is freed on failure before returning. The DPDK telemetry framework handles cleanup of the parent containerd.modules/infra/control/nexthop.py (1)
1-17: LGTM - Clean telemetry module implementation.The implementation follows the established pattern used in other telemetry modules (rib4.py, rib6.py). The static analysis warnings about undefined names are false positives—these are forward-reference type hints in string quotes, which is valid Python.
modules/ip/control/rib4.py (1)
1-21: LGTM - Consistent with IPv6 counterpart.Implementation is symmetric with rib6.py and follows the same safe patterns. Static analysis warnings are false positives (forward-reference type hints).
modules/infra/control/gr_iface.h (1)
11-11: LGTM - Safe telemetry callback extension.The addition of the telemetry_info callback pointer is a clean extension to the iface_type API. The const qualifier on the iface parameter ensures read-only access.
Also applies to: 57-57
modules/infra/control/vlan.c (1)
187-199: LGTM - Previous NULL dereference issue resolved.The NULL check on lines 195-196 properly handles the case where
iface_from_id()returns NULL, addressing the critical issue raised in the previous review.modules/infra/api/graph.py (1)
17-26: LGTM - Previous .items() issue resolved.The
.items()call on line 19 is now present, fixing the TypeError that would have occurred. The implementation follows the same safe pattern as the other telemetry modules.modules/infra/api/iface.py (1)
1-96: LGTM!The telemetry exporter logic correctly handles optional fields (lines 58-74, 92-95) while failing fast on required fields. The pattern is consistent with other telemetry endpoint modules.
rjarry
force-pushed
the
prometheus
branch
2 times, most recently
from
a479cf7 to
c525e88
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @main/prom.c:
- Around line 61-81: In append_labels_va the new label text is written starting
at buf = ctx->labels (and using n only) which overwrites existing labels; change
buf to start at ctx->labels + ctx->labels_len, track the remaining length (len =
sizeof(ctx->labels) - ctx->labels_len), and after each SAFE_BUF write advance
buf by the number of bytes written and decrement len (and increment n
accordingly), then finally add n to ctx->labels_len; update all
SAFE_BUF(snprintf, ...) calls to write to buf with the adjusted len so
gr_prom_ctx_add appends instead of clobbering existing labels.
In @modules/infra/api/iface.c:
- Around line 213-220: iface_type_get(iface->type) may return NULL and the code
dereferences type->name when calling gr_prom_ctx_init; fix by checking the
result immediately after the call to iface_type_get (or using the established
pattern of assert(type != NULL)) before using type->name in gr_prom_ctx_init, or
provide a safe fallback string (e.g. "<unknown>") for the type name; update the
block around iface_next/iface_type_get so type is validated (or asserted) prior
to passing type->name to gr_prom_ctx_init.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Containerfile.groutGNUmakefiledebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_worker.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.crpm/grout.specsmoke/_init.shsmoke/telemetry_test.sh
💤 Files with no reviewable changes (5)
- GNUmakefile
- rpm/grout.spec
- debian/rules
- debian/grout-prometheus.install
- debian/control
🚧 Files skipped from review as they are similar to previous changes (2)
- meson.build
- smoke/_init.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/dpdk.cmodules/infra/control/gr_worker.hmodules/ip/control/route.cmodules/infra/control/worker_test.cmodules/infra/control/port_test.cmodules/ip6/control/route.cmodules/infra/control/gr_iface.hmain/gr_config.hmodules/infra/api/stats.cmain/main.cmain/prom.cmain/gr_prom.hmodules/infra/api/iface.cmodules/infra/control/worker.cmodules/infra/control/nexthop.cmodules/infra/control/port.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/telemetry_test.sh
🧠 Learnings (16)
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/port.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/gr_iface.hmodules/infra/control/worker.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/ip/control/route.cmodules/infra/control/worker_test.cmodules/infra/control/port_test.cmodules/infra/control/gr_iface.hmain/main.cmodules/infra/api/iface.cmodules/infra/control/nexthop.cmodules/infra/control/port.csmoke/telemetry_test.sh
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/worker_test.cmodules/infra/control/gr_iface.hmodules/infra/api/iface.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
main/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
main/meson.build
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
docs/grout.8.scdocmain/main.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
🧬 Code graph analysis (9)
modules/infra/control/gr_worker.h (1)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)
modules/infra/control/port_test.c (1)
modules/infra/control/worker_test.c (3)
gr_prom_ctx_init(37-37)gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/ip6/control/route.c (3)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)modules/infra/control/worker_test.c (2)
gr_prom_ctx_init(37-37)gr_prom_emit(39-39)main/prom.c (1)
gr_prom_register(20-22)
modules/infra/api/stats.c (3)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(20-22)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/prom.c (2)
gr_prom_start(137-165)gr_prom_stop(167-172)
main/prom.c (1)
api/gr_errno.h (1)
errno_set(10-13)
main/gr_prom.h (1)
main/prom.c (6)
gr_prom_ctx_init(83-93)gr_prom_ctx_add(95-101)gr_prom_emit(103-106)gr_prom_register(20-22)gr_prom_start(137-165)gr_prom_stop(167-172)
modules/infra/api/iface.c (4)
modules/infra/control/gr_iface.h (2)
iface(17-26)iface_get_stats(104-106)modules/infra/control/iface.c (2)
iface_next(243-258)iface_type_get(29-36)modules/infra/control/port_test.c (2)
gr_prom_ctx_init(46-46)gr_prom_emit(48-48)main/prom.c (1)
gr_prom_register(20-22)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 18-18: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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-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: deb
- GitHub Check: rpm
🔇 Additional comments (19)
Containerfile.grout (1)
5-5: LGTM!The removal of
grout-prometheus.*.rpmaligns with the consolidation of Prometheus functionality into the main grout package. The multi-stage build pattern and cleanup steps are appropriate.main/meson.build (1)
10-10: LGTM!Build configuration correctly adds the new Prometheus exporter module.
main/dpdk.c (1)
140-140: LGTM!Disabling DPDK telemetry is appropriate since the Prometheus exporter now handles metrics export.
docs/grout.8.scdoc (1)
79-83: LGTM!Documentation clearly describes the new Prometheus endpoint configuration with appropriate default and disable instructions.
smoke/telemetry_test.sh (1)
1-38: LGTM!Good test coverage for the Prometheus endpoint, exercising multiple interface types (port, bond, vlan, ipip), nexthops, and routes across VRFs.
main/gr_prom.h (1)
1-63: LGTM!Clean Prometheus metrics API with appropriate abstractions for collectors, contexts, and metric emission.
modules/ip/control/route.c (1)
462-490: LGTM!Prometheus collector implementation correctly iterates VRFs, filters valid origins, and emits route count metrics with appropriate labels.
modules/ip6/control/route.c (1)
504-532: LGTM!IPv6 route collector implementation mirrors the IPv4 pattern correctly.
modules/infra/control/nexthop.c (1)
706-730: LGTM!The Prometheus collector cleanly iterates by type, correctly skips zero-count and unknown types, and properly registers during initialization.
modules/infra/api/stats.c (1)
260-315: LGTM!The graph and CPU collectors correctly retrieve stats and emit Prometheus metrics with appropriate labels. Registration is properly placed in the init function.
modules/infra/control/gr_iface.h (1)
8-8: LGTM!Clean extension to support per-interface-type Prometheus collection callbacks.
Also applies to: 57-58
modules/infra/control/gr_worker.h (1)
81-81: LGTM!Declaration properly exposes the stats aggregation function for Prometheus collectors.
main/gr_config.h (1)
27-28: LGTM!Config fields properly documented and typed for Prometheus exporter configuration.
main/main.c (1)
115-134: LGTM!The Prometheus CLI option parsing handles both IPv4 and IPv6 addresses correctly (using
strrchrfor the last:). Lifecycle management is properly integrated into startup and shutdown paths with appropriate error handling.Also applies to: 336-339, 360-360
modules/infra/control/worker_test.c (1)
37-39: LGTM!Test stubs for Prometheus context helpers are correctly implemented as no-ops, consistent with the mocking pattern used elsewhere in this file.
modules/infra/control/port_test.c (1)
46-48: LGTM!Test stubs correctly satisfy linker requirements for Prometheus functions, consistent with
worker_test.c.modules/infra/control/worker.c (1)
425-499: LGTM!The
find_stathelper andworker_dump_statsfunction correctly aggregate per-worker statistics. Atomic loads ensure visibility of stats updated by datapath threads. Per coding guidelines,gr_vec_*()functions cannot fail, so the usage is correct.modules/infra/control/port.c (1)
608-625: LGTM!Port metrics collection properly validates return values from
rte_eth_dev_info_getandrte_eth_stats_getbefore using their output.modules/infra/api/iface.c (1)
258-270: LGTM!Collector registration during
RTE_INITis correctly placed alongside other API handler registrations.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @main/prom.c:
- Around line 157-164: The bind failure handler for evhttp_bind_socket wrongly
returns success (0) after logging; update the error path in the block that calls
evhttp_bind_socket so it returns a non-zero error code (or propagate an
appropriate failure) instead of 0, and ensure the LOG call level is adjusted if
you intend a soft failure (e.g., NOTICE/WARN) — modify the block around
evhttp_bind_socket, LOG, evhttp_free and http_server so the function exits with
failure when bind fails or downgrades the log level if soft-fail semantics are
desired.
- Around line 61-81: In append_labels_va the buffer pointer is set to the start
(buf = ctx->labels) so appended labels overwrite existing content and
ctx->labels_len isn't updated because n never accumulates written bytes; change
buf to start at the current end (buf = ctx->labels + ctx->labels_len) and adjust
len to sizeof(ctx->labels) - ctx->labels_len, and after each SAFE_BUF/snprintf
call capture its return (e.g., int r = snprintf(...)) and add r to n (or
otherwise accumulate bytes written) so that ctx->labels_len += n correctly
advances the length.
In @modules/infra/control/worker.c:
- Around line 489-496: The subtraction assigning .cycles in the gr_infra_stat
initializer can underflow if node_cycles > loop_cycles; change the assignment so
cycles is computed defensively (e.g., if loop_cycles >= node_cycles use
loop_cycles - node_cycles, otherwise set cycles to 0) before building the struct
(refer to variables loop_cycles, node_cycles and the struct gr_infra_stat stat),
then use that clamped value in the initializer before calling gr_vec_add(stats,
stat).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Containerfile.groutGNUmakefiledebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_worker.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.crpm/grout.specsmoke/_init.shsmoke/telemetry_test.sh
💤 Files with no reviewable changes (5)
- debian/rules
- debian/grout-prometheus.install
- GNUmakefile
- rpm/grout.spec
- debian/control
🚧 Files skipped from review as they are similar to previous changes (9)
- main/gr_config.h
- Containerfile.grout
- modules/infra/control/nexthop.c
- modules/ip/control/route.c
- main/meson.build
- smoke/_init.sh
- main/dpdk.c
- docs/grout.8.scdoc
- main/gr_prom.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/worker_test.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/gr_worker.hmain/main.cmodules/ip6/control/route.cmain/prom.cmodules/infra/control/worker.cmodules/infra/api/stats.cmodules/infra/api/iface.cmodules/infra/control/gr_iface.h
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/telemetry_test.sh
🧠 Learnings (15)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/worker_test.cmodules/infra/control/port.cmodules/infra/control/port_test.csmoke/telemetry_test.shmain/main.cmodules/infra/api/iface.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/worker_test.cmodules/infra/api/iface.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/control/port.cmodules/ip6/control/route.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip6/control/route.cmodules/infra/control/worker.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.build
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/api/iface.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
🧬 Code graph analysis (7)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_prom_ctx_add(47-47)gr_prom_emit(48-48)modules/infra/control/worker_test.c (2)
gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/infra/control/gr_worker.h (1)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/prom.c (2)
gr_prom_start(137-169)gr_prom_stop(171-176)
modules/ip6/control/route.c (3)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)modules/infra/control/worker_test.c (2)
gr_prom_ctx_init(37-37)gr_prom_emit(39-39)main/prom.c (1)
gr_prom_register(20-22)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
modules/infra/api/stats.c (3)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(20-22)
modules/infra/api/iface.c (5)
modules/infra/control/gr_iface.h (2)
iface(17-26)iface_get_stats(104-106)modules/infra/control/iface.c (2)
iface_next(243-258)iface_type_get(29-36)modules/infra/control/port_test.c (3)
gr_prom_ctx_init(46-46)gr_prom_emit(48-48)gr_register_api_handler(21-21)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(20-22)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 18-18: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: rpm
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
🔇 Additional comments (13)
modules/ip6/control/route.c (3)
498-502: LGTM!The bounds validation via
get_rib6()ensuresvrf_id < GR_MAX_VRFSbefore accessingstats[vrf_id], and additionally only returns stats for VRFs with an actual RIB allocated.
504-532: LGTM!The Prometheus collector implementation is correct: proper bounds checking via
rib6_get_stats(), safe buffer sizing for VRF ID strings, and appropriate filtering of zero counts and unknown origins.
603-603: LGTM!Static collector registration follows the established pattern.
meson.build (1)
77-77: LGTM!The
libevent_extradependency is correctly declared and properly linked into thegrout_exetarget. This is needed for the new Prometheus HTTP exporter functionality.Also applies to: 116-116
modules/infra/control/gr_worker.h (1)
81-81: LGTM!The function declaration is correct and consistent with the implementation in
worker.c. Thegr_vecreturn type follows existing codebase conventions.modules/infra/control/port_test.c (1)
46-48: LGTM!No-op stubs for the Prometheus API functions are appropriate for unit test isolation.
modules/infra/control/worker_test.c (1)
37-39: LGTM!Prometheus API stubs mirror those in
port_test.c, maintaining consistent test scaffolding across the test suite.smoke/telemetry_test.sh (1)
1-38: LGTM!The test script properly exercises the new Prometheus endpoint. The topology setup covers various interface types (port, bond, vlan, ipip) and routing configurations, providing good coverage for the telemetry export functionality.
modules/infra/control/gr_iface.h (1)
57-58: LGTM!Clean addition of the optional
prom_collectcallback with appropriate documentation.main/main.c (1)
116-135: LGTM!The Prometheus address parsing correctly handles the ADDR:PORT format, validates the port range, and integrates cleanly with existing argument parsing patterns.
modules/infra/control/port.c (1)
608-625: LGTM!Port metrics collection properly handles potential DPDK API failures by conditionally emitting metrics only when data is available.
modules/infra/api/stats.c (1)
260-276: LGTM!Graph metrics collection correctly obtains stats, emits metrics for each node, and properly frees the allocated vector.
modules/infra/api/iface.c (1)
208-256: LGTM!The interface collector properly aggregates per-core statistics and delegates to type-specific collectors when available. The iteration pattern is consistent with other parts of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @main/prom.c:
- Around line 103-106: The format specifier for the uint64_t value in
gr_prom_emit is non-portable; include <inttypes.h> and change the
evbuffer_add_printf call in gr_prom_emit to use the PRIu64 macro (e.g. use "%"
PRIu64) for the value argument, ensuring the argument remains the uint64_t value
from gr_prom_emit; no other behavior changes.
- Around line 61-81: append_labels_va writes labels into ctx->labels using a
local buf and n, but never advances buf or accumulates n from SAFE_BUF/snprintf
results, causing overwrites; change the writes to use an offset based on
ctx->labels_len + n (e.g., write into ctx->labels + ctx->labels_len + n) or
advance buf after each write, and after every SAFE_BUF/snprintf call add the
returned byte count to n (and handle errors as SAFE_BUF already does), then at
the end update ctx->labels_len += n so subsequent calls (e.g., gr_prom_ctx_add
after gr_prom_ctx_init) append correctly; adjust uses of buf, n, and
ctx->labels_len in append_labels_va accordingly and keep the existing err
handling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/grout.8.scdocmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.csmoke/_init.shsmoke/telemetry_test.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/infra/control/gr_iface.h
- main/gr_config.h
- docs/grout.8.scdoc
- main/meson.build
- modules/ip/control/route.c
- smoke/_init.sh
- modules/infra/control/worker_test.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/port_test.cmodules/infra/control/port.cmodules/infra/control/nexthop.cmain/main.cmain/prom.cmodules/infra/api/iface.cmain/gr_prom.hmodules/infra/api/stats.cmodules/ip6/control/route.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/telemetry_test.sh
🧠 Learnings (14)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/port_test.cmodules/infra/control/port.cmodules/infra/control/nexthop.cmain/main.cmodules/infra/api/iface.csmoke/telemetry_test.sh
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/control/port.cmodules/ip6/control/route.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/nexthop.cmodules/infra/api/iface.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/api/iface.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.build
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
🧬 Code graph analysis (5)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_prom_ctx_add(47-47)gr_prom_emit(48-48)modules/infra/control/worker_test.c (2)
gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/infra/control/nexthop.c (2)
modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/prom.c (1)
gr_prom_register(20-22)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/prom.c (2)
gr_prom_start(140-175)gr_prom_stop(177-182)
modules/infra/api/iface.c (4)
modules/infra/control/gr_iface.h (2)
iface(17-26)iface_get_stats(104-106)modules/infra/control/iface.c (2)
iface_next(243-258)iface_type_get(29-36)modules/infra/control/port_test.c (2)
gr_prom_ctx_init(46-46)gr_prom_emit(48-48)main/prom.c (1)
gr_prom_register(20-22)
modules/infra/api/stats.c (3)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(20-22)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 18-18: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (11)
meson.build (1)
77-77: LGTM!The
libevent_extradependency is correctly added for the Prometheus HTTP server functionality and properly integrated into the grout executable dependencies.Also applies to: 116-116
smoke/telemetry_test.sh (1)
1-38: LGTM!The test script correctly exercises the new Prometheus exporter by configuring various network elements (ports, bonds, VLANs, nexthops, routes, IPIP tunnel) and verifying the metrics endpoint responds successfully.
modules/infra/api/stats.c (1)
260-316: LGTM!The Prometheus collectors for graph and CPU statistics are well-implemented. The
worker_dump_statsvector is properly freed after use, and the collectors correctly iterate workers and emit metrics with appropriate labels.modules/infra/api/iface.c (1)
179-271: LGTM!The interface Prometheus collector is well-structured. It properly aggregates per-core statistics, emits standard interface metrics, and delegates to type-specific collectors when available.
main/prom.c (1)
163-170: Bind failure returns success, allowing silent startup failure.When
evhttp_bind_socketfails, the function returns0(success) after logging the error. This means the application will continue running without the Prometheus exporter, and callers won't know it failed to start. If this is intentional (graceful degradation), consider adding a comment. Otherwise, it should return an error.Is silent failure on bind intentional? If not:
if (evhttp_bind_socket(http_server, addr, port) < 0) { errno = errno ?: EADDRNOTAVAIL; LOG(ERR, "bind http://%s:%u failed: %s", addr, port, strerror(errno)); evhttp_free(http_server); http_server = NULL; - return 0; + return -errno; }modules/infra/control/port_test.c (1)
46-48: LGTM!Test stubs for Prometheus API are appropriate for unit testing.
main/main.c (1)
337-361: LGTM!Prometheus lifecycle integration is properly sequenced - started after API socket and stopped during shutdown before resource cleanup.
modules/ip6/control/route.c (1)
506-527: LGTM!The Prometheus collection logic properly validates VRF stats, skips zero counts, and uses bounded string formatting.
modules/infra/control/port.c (1)
608-625: LGTM!Port Prometheus collection properly checks return values from DPDK APIs before emitting metrics.
modules/infra/control/nexthop.c (1)
708-723: LGTM!Nexthop Prometheus collection properly filters zero counts and unknown types before emission.
main/gr_prom.h (1)
1-63: LGTM!Well-structured Prometheus API with clear separation between metric definitions, context management, and collector registration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @main/prom.c:
- Around line 62-82: In append_labels_va fix the inverted buffer pointer:
compute buf as ctx->labels + ctx->labels_len and len as sizeof(ctx->labels) -
ctx->labels_len (not the other way around), use buf and len for each
SAFE_BUF(snprintf, ...) call, increment n or track bytes written after each
snprintf by advancing buf and decrementing len, and finally update
ctx->labels_len by the number of bytes appended (ctx->labels_len +=
total_written) before returning; keep references to append_labels_va,
ctx->labels_len, buf, len, SAFE_BUF and snprintf to locate the changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/grout.8.scdocmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.csmoke/_init.shsmoke/telemetry_test.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- main/meson.build
- smoke/_init.sh
- modules/infra/control/gr_iface.h
- modules/infra/control/worker_test.c
- modules/infra/api/iface.c
- docs/grout.8.scdoc
- main/gr_prom.h
- modules/infra/api/stats.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/port_test.cmodules/infra/api/affinity.cmodules/ip6/control/route.cmain/main.cmodules/infra/control/port.cmain/gr_config.hmodules/ip/control/route.cmodules/infra/control/nexthop.cmain/prom.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/telemetry_test.sh
🧠 Learnings (16)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/port_test.cmain/main.cmodules/infra/control/port.cmodules/ip/control/route.cmodules/infra/control/nexthop.csmoke/telemetry_test.sh
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/api/affinity.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/api/affinity.cmodules/ip6/control/route.cmodules/infra/control/port.cmodules/ip/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.build
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/prom.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/prom.c
🧬 Code graph analysis (6)
modules/infra/control/port_test.c (1)
modules/infra/control/worker_test.c (3)
gr_prom_ctx_init(37-37)gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/ip6/control/route.c (3)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)modules/infra/control/worker_test.c (1)
gr_prom_ctx_init(37-37)main/prom.c (1)
gr_prom_register(22-24)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/prom.c (2)
gr_prom_start(201-208)gr_prom_stop(210-217)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_prom_ctx_add(47-47)gr_prom_emit(48-48)modules/infra/control/worker_test.c (2)
gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/ip/control/route.c (3)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)modules/infra/control/worker_test.c (1)
gr_prom_ctx_init(37-37)main/prom.c (1)
gr_prom_register(22-24)
modules/infra/control/nexthop.c (2)
modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/prom.c (1)
gr_prom_register(22-24)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: deb
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-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-cross-aarch64
- GitHub Check: rpm
🔇 Additional comments (15)
smoke/telemetry_test.sh (1)
1-38: LGTM!The test script correctly sets up a comprehensive network topology (ports, bond, VLANs, nexthops, routes, IPIP tunnel) and validates the Prometheus metrics endpoint. The use of
curl --failensures the test fails appropriately if the endpoint returns an HTTP error.Note: The shellcheck warning about unquoted
$(dirname $0)is ignored per coding guidelines.modules/infra/api/affinity.c (1)
44-47: LGTM!The Prometheus affinity integration follows the established error-handling pattern used for
control_output_set_affinity.main/gr_config.h (1)
27-28: LGTM!The new fields use appropriate types for address string and port number.
modules/infra/control/port_test.c (1)
46-48: LGTM!Test stubs correctly provide no-op implementations matching the Prometheus API surface.
meson.build (1)
77-77: LGTM!The
libevent_extradependency is correctly added for HTTP server support needed by the Prometheus exporter.Also applies to: 116-116
modules/ip6/control/route.c (2)
498-502: LGTM!Using
get_rib6()for validation is safer than a raw range check—it validates both bounds and RIB existence in one call.
506-527: LGTM!The Prometheus collector correctly iterates within bounds, skips uninitialized VRFs, and filters out zero-count and unknown origins.
main/main.c (2)
116-135: LGTM!The parsing logic using
strrchrcorrectly handles IPv4 and bracketed IPv6 addresses (e.g.,[::1]:2015). Bare IPv6 addresses without brackets are ambiguous by design—the standard convention requires brackets when specifying a port.
337-338: LGTM!Prometheus lifecycle is correctly integrated—started after the API socket and stopped before resource cleanup during shutdown.
Also applies to: 358-358
modules/infra/control/port.c (2)
601-625: LGTM!Port metrics collection handles DPDK API failures gracefully and emits appropriate metric types (gauges for configuration, counters for cumulative stats).
794-794: LGTM!The
prom_collectcallback is correctly wired into the interface type definition.modules/ip/control/route.c (1)
462-490: LGTM!The Prometheus collector implementation correctly iterates over VRFs and origins, skipping empty entries and unknown origin types. The pattern aligns with the project-wide Prometheus integration approach.
main/prom.c (2)
109-137: LGTM!The metrics handler properly allocates and frees resources, iterates collectors, and sends the HTTP response. Error handling for
evbuffer_newfailure is appropriate.
148-199: LGTM!Thread lifecycle is well-managed with proper affinity configuration, error handling for each libevent resource, and clean shutdown via the
end:label cleanup path.modules/infra/control/nexthop.c (1)
706-723: LGTM!The Prometheus collector correctly filters out zero counts and unknown nexthop types before emitting metrics. The implementation is consistent with the pattern used in other collectors (rib4, rib6).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @main/prom.c:
- Around line 104-107: The format specifier for uint64_t in gr_prom_emit is
non-portable (using %lu); update the evbuffer_add_printf call in gr_prom_emit to
use the PRIu64 format macro (e.g. evbuffer_add_printf(ctx->w->buf, "grout_%s{%s}
%" PRIu64 "\n", m->name, ctx->labels, (uint64_t)value)), add #include
<inttypes.h> if missing, and keep emit_help_type(ctx->w, m) unchanged so the
metric name/labels remain correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/grout.8.scdocmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.csmoke/_init.shsmoke/telemetry_test.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/infra/control/worker_test.c
- docs/grout.8.scdoc
- main/gr_config.h
- modules/infra/control/gr_iface.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/api/affinity.cmodules/ip6/control/route.cmain/main.cmodules/infra/control/port_test.cmodules/infra/control/nexthop.cmain/gr_prom.hmodules/infra/control/port.cmodules/infra/api/stats.cmodules/ip/control/route.cmodules/infra/api/iface.cmain/prom.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/telemetry_test.sh
🧠 Learnings (18)
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/api/affinity.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/api/affinity.cmodules/ip6/control/route.cmodules/infra/control/port.cmodules/ip/control/route.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.buildmain/meson.build
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/infra/control/nexthop.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/infra/control/nexthop.cmodules/ip/control/route.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
main/main.cmodules/infra/control/port_test.cmodules/infra/control/nexthop.cmodules/infra/control/port.csmoke/_init.shmodules/ip/control/route.cmodules/infra/api/iface.csmoke/telemetry_test.sh
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/nexthop.cmodules/infra/api/iface.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/nexthop.cmodules/infra/api/iface.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
main/meson.build
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/prom.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/prom.c
🧬 Code graph analysis (7)
modules/infra/api/affinity.c (1)
main/prom.c (1)
gr_prom_set_affinity(142-146)
modules/infra/control/port_test.c (1)
modules/infra/control/worker_test.c (3)
gr_prom_ctx_init(37-37)gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/infra/control/nexthop.c (2)
modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/prom.c (1)
gr_prom_register(22-24)
main/gr_prom.h (1)
main/prom.c (7)
gr_prom_ctx_init(84-94)gr_prom_ctx_add(96-102)gr_prom_emit(104-107)gr_prom_register(22-24)gr_prom_start(201-208)gr_prom_stop(210-217)gr_prom_set_affinity(142-146)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_prom_ctx_add(47-47)gr_prom_emit(48-48)modules/infra/control/worker_test.c (2)
gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/infra/api/stats.c (4)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)modules/infra/control/worker_test.c (3)
gr_prom_ctx_init(37-37)gr_prom_emit(39-39)gr_register_api_handler(34-34)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(22-24)
modules/ip/control/route.c (3)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)modules/infra/control/worker_test.c (2)
gr_prom_ctx_init(37-37)gr_prom_emit(39-39)main/prom.c (1)
gr_prom_register(22-24)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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 (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: rpm
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: deb
- GitHub Check: build-cross-aarch64
🔇 Additional comments (10)
smoke/_init.sh (1)
12-15: LGTM!Loopback setup logic is correct - brings up
loand conditionally adds the address only if missing.meson.build (1)
77-77: LGTM!The
libevent_extradependency is correctly added to support theevhttpfunctionality used in the Prometheus exporter.smoke/telemetry_test.sh (1)
1-38: LGTM!Test script correctly sets up a comprehensive topology and validates the Prometheus endpoint. The static analysis warning about unquoted
$(dirname $0)can be ignored per coding guidelines.main/prom.c (2)
20-24: LGTM!The Cppcheck syntax error on line 20 is a false positive -
STAILQ_HEADis a valid BSD queue macro. Collector registration is straightforward.
201-217: LGTM!Lifecycle management looks correct -
gr_prom_startconditionally creates the thread, andgr_prom_stopusesevent_base_loopbreakto cleanly exit the event loop before joining.main/meson.build (1)
10-10: LGTM!Build file correctly includes the new Prometheus exporter source.
main/gr_prom.h (1)
34-38: Thelabelsbuffer is properly protected against overflow. Theappend_labels_vafunction calculates the remaining space (len = sizeof(ctx->labels) - ctx->labels_len) and uses theSAFE_BUFmacro, which enforces bounds checking by detecting ifsnprintfwould exceed the available space. If an overflow condition is detected, the error handler is invoked. No buffer overflow vulnerability exists.Likely an incorrect or invalid review comment.
modules/infra/api/iface.c (1)
179-270: LGTM!The Prometheus integration is well-structured: metric definitions are clear, per-interface stat aggregation across lcores is implemented correctly, and the type-specific collector dispatch is properly guarded. Registration follows the established pattern.
modules/infra/control/port_test.c (1)
46-48: LGTM!Standard no-op test stubs matching the pattern in
worker_test.c. Required for test compilation with the new Prometheus integration.modules/infra/control/port.c (1)
601-625: LGTM!The port-specific Prometheus collector correctly retrieves driver info and stats, guards usage behind successful DPDK API calls, and wires into the interface type properly.
Also applies to: 794-794
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/infra/control/worker.c`:
- Around line 485-486: The subtraction "w_stats->loop_cycles -
w_stats->sleep_cycles" can underflow if sleep_cycles > loop_cycles; change
accumulation in the loop where loop_cycles and n_loops are updated (referencing
loop_cycles, n_loops and w_stats->loop_cycles / w_stats->sleep_cycles) to guard
the subtraction: compute the delta as zero when sleep_cycles >= loop_cycles (or
use a signed/extended difference and clamp to >=0) and only add a non-negative
delta to loop_cycles, leaving n_loops unchanged; this prevents unsigned
wraparound while preserving correct accumulated loop time.
♻️ Duplicate comments (3)
main/prom.c (2)
178-183: Bind failure silently leaves exporter non-functional.When
evhttp_bind_socketfails, the thread logs an error and exits viagoto end, but the application continues without a working Prometheus exporter. If this is intentional soft-fail behavior, consider downgrading the log level. Otherwise, consider signaling failure togr_prom_start.
104-107: Portability bug:%luforuint64_tis non-portable.On platforms where
uint64_tisunsigned long long, this causes format warnings or incorrect output. UsePRIu64from<inttypes.h>.🔧 Proposed fix
+#include <inttypes.h> + void gr_prom_emit(struct gr_prom_ctx *ctx, const struct gr_prom_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value); }modules/infra/control/worker.c (1)
489-496: Potential unsigned underflow in overhead cycles calculation.If
node_cyclesexceedsloop_cycles, the subtraction will underflow.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
Containerfile.groutGNUmakefiledebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.hmodules/infra/control/gr_worker.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/gr_ip4_control.hmodules/ip/control/route.cmodules/ip6/control/gr_ip6_control.hmodules/ip6/control/route.crpm/grout.specsmoke/_init.shsmoke/telemetry_test.sh
💤 Files with no reviewable changes (8)
- GNUmakefile
- debian/control
- debian/rules
- rpm/grout.spec
- modules/ip/control/gr_ip4_control.h
- debian/grout-prometheus.install
- modules/infra/control/gr_nh_control.h
- modules/ip6/control/gr_ip6_control.h
🚧 Files skipped from review as they are similar to previous changes (8)
- meson.build
- main/dpdk.c
- modules/infra/api/affinity.c
- docs/grout.8.scdoc
- main/meson.build
- modules/infra/api/iface.c
- modules/infra/control/port.c
- main/gr_config.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/gr_worker.hmain/main.cmodules/ip/control/route.cmodules/infra/control/worker.cmodules/ip6/control/route.cmodules/infra/control/worker_test.cmodules/infra/api/stats.cmain/gr_prom.hmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port_test.cmain/prom.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/telemetry_test.sh
🧠 Learnings (16)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
main/main.cmodules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/worker_test.csmoke/_init.shmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port_test.csmoke/telemetry_test.sh
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip/control/route.cmodules/infra/control/worker.cmodules/ip6/control/route.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/worker_test.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/prom.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/prom.c
🧬 Code graph analysis (7)
modules/infra/control/gr_worker.h (1)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/prom.c (2)
gr_prom_start(201-208)gr_prom_stop(210-217)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
modules/infra/api/stats.c (3)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(22-24)
main/gr_prom.h (1)
main/prom.c (7)
gr_prom_ctx_init(84-94)gr_prom_ctx_add(96-102)gr_prom_emit(104-107)gr_prom_register(22-24)gr_prom_start(201-208)gr_prom_stop(210-217)gr_prom_set_affinity(142-146)
modules/infra/control/nexthop.c (2)
modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/prom.c (1)
gr_prom_register(22-24)
modules/infra/control/port_test.c (1)
modules/infra/control/worker_test.c (3)
gr_prom_ctx_init(37-37)gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (14)
Containerfile.grout (1)
1-13: LGTM!Container build changes align with the removal of the separate grout-prometheus package. The cleanup step is a good practice for reducing image size.
smoke/_init.sh (1)
12-15: LGTM!Loopback interface setup is necessary for the Prometheus endpoint tests that curl
127.0.0.1:2015. The conditional address addition prevents errors on systems where it's already configured.modules/infra/control/gr_iface.h (1)
57-58: LGTM!Clean extension of the
iface_typestruct for per-type Prometheus collectors. The comment documenting that ctx already has base iface labels is helpful.smoke/telemetry_test.sh (1)
1-38: LGTM!Good smoke test coverage for the Prometheus metrics endpoint. The
--failflag on curl ensures the test fails appropriately on HTTP errors.main/main.c (2)
116-135: LGTM!The address parsing correctly uses
strrchrto find the last colon, properly handles port validation, and the previous IPv6 bracket notation concern has been addressed.
337-338: LGTM!Prometheus lifecycle properly integrated - started after API socket is ready, and
gr_prom_stop()is correctly placed in the shutdown path to ensure cleanup regardless of how we reach shutdown.main/gr_prom.h (1)
1-65: LGTM!The Prometheus API surface is well-designed with clear documentation comments. The struct definitions, macros, and function declarations provide a clean interface for metric collection and emission.
modules/infra/control/worker_test.c (1)
37-39: LGTM!The no-op stub functions for Prometheus APIs are appropriate for the test harness, satisfying linker requirements without introducing actual Prometheus dependencies in tests.
modules/infra/control/port_test.c (1)
46-48: LGTM!Stub implementations consistent with
worker_test.c, properly satisfying test harness requirements.modules/ip6/control/route.c (1)
484-526: LGTM!The Prometheus collector for IPv6 RIB routes is well-implemented. The per-origin counting with VRF labels follows the same pattern as the IPv4 implementation, and the bounds are properly handled via
UINT_NUM_VALUES(gr_nh_origin_t).modules/infra/control/nexthop.c (1)
690-717: LGTM!The Prometheus collector implementation correctly iterates through nexthops, aggregates counts by type, and emits metrics. The use of
UINT_NUM_VALUESensures the counts array is properly sized for all possiblegr_nh_type_tvalues.modules/infra/control/gr_worker.h (1)
81-81: LGTM!The function declaration aligns with the implementation in worker.c.
modules/ip/control/route.c (1)
445-488: LGTM!The Prometheus collector correctly iterates VRFs, aggregates route counts by origin, and emits per-VRF metrics. The implementation follows the same pattern as other collectors in this PR.
modules/infra/api/stats.c (1)
260-317: LGTM!The Prometheus collectors are well-implemented:
graph_collectorproperly aggregates worker stats and frees the vector after usecpu_collectorcorrectly emits per-CPU idle/busy cycle metrics with appropriate labels- Both collectors are registered during module initialization
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/infra/control/port.c`:
- Around line 608-625: In port_prom_collect ensure the "driver" label is always
added so label sets are stable: call gr_prom_ctx_add(ctx, "driver", ...)
unconditionally—use dev_info.driver_name when
rte_eth_dev_info_get(port->port_id, &dev_info) returns 0 and a stable sentinel
like "" or "unknown" when it fails (also guard against dev_info.driver_name
being NULL) instead of only adding the label on success.
♻️ Duplicate comments (6)
modules/infra/control/worker.c (2)
485-486: Unsigned underflow risk whensleep_cycles > loop_cycles.Due to timing jitter,
w_stats->sleep_cyclesmay exceedw_stats->loop_cycles, causing unsigned underflow and corrupted metrics.
489-496: Unsigned underflow risk whennode_cycles > loop_cycles.If accumulated
node_cyclesexceedsloop_cycles, the subtraction at line 492 underflows, producing an incorrect overhead metric.main/main.c (1)
116-135:--prometheus ADDR:PORTparsing breaks IPv6 (and[v6]:portneeds bracket stripping).
strrchr(addr_port_str, ':')can’t unambiguously parse bare IPv6, and if users pass[::1]:2015you currently keep brackets ingr_config.prom_addr, which libevent bind calls typically won’t accept.Proposed fix
static int parse_prom_addr(char *addr_port_str) { - char *port_str; - char *colon; + char *addr = addr_port_str; + char *port_str = NULL; + char *colon; - colon = strrchr(addr_port_str, ':'); - if (!colon) - return perr("--prometheus: missing ':' (expected ADDR:PORT)"); - - *colon = '\0'; - port_str = colon + 1; + /* Support [IPv6]:PORT and reject bare IPv6 (ambiguous). */ + if (addr[0] == '[') { + char *end = strchr(addr, ']'); + if (end == NULL || end[1] != ':') + return perr("--prometheus: invalid IPv6 format (expected [ADDR]:PORT)"); + *end = '\0'; + addr++; /* skip '[' */ + port_str = end + 2; + } else { + colon = strrchr(addr, ':'); + if (!colon) + return perr("--prometheus: missing ':' (expected ADDR:PORT)"); + /* If there are multiple colons, this looks like bare IPv6 -> require brackets. */ + if (strchr(addr, ':') != colon) + return perr("--prometheus: IPv6 must use bracket notation: [ADDR]:PORT"); + *colon = '\0'; + port_str = colon + 1; + } if (parse_uint(&port, port_str, 10, 1, 65535) < 0) return perr("--prometheus: invalid port: %s", strerror(errno)); - gr_config.prom_addr = addr_port_str; + gr_config.prom_addr = addr; gr_config.prom_port = port; return 0; }main/prom.c (2)
62-82: VerifySAFE_BUFis actually appending (not overwriting) label content.This function depends on
SAFE_BUF(...)writing atbuf + nand incrementingn; otherwise label concatenation will be corrupted.To verify:
#!/bin/bash set -euo pipefail rg -n --no-heading '^\s*#\s*define\s+SAFE_BUF\b' -S rg -n --no-heading 'SAFE_BUF\(' -S main/prom.c
104-107:uint64_tprinted with%luis not portable (and can misprint / warn).Use
PRIu64.Proposed fix
`#include` <pthread.h> +#include <inttypes.h> `#include` <stdarg.h> `#include` <stdbool.h> `#include` <string.h> @@ void gr_prom_emit(struct gr_prom_ctx *ctx, const struct gr_prom_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf( + ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value + ); }modules/infra/api/iface.c (1)
213-220: Potential NULL pointer dereference fromiface_type_get().
iface_type_get()can returnNULL, buttype->nameis used on line 219 without a NULL check. Add an assertion or check consistent with the pattern used elsewhere in this file (e.g., lines 18-19, 84-85).while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { const struct iface_type *type = iface_type_get(iface->type); + assert(type != NULL); snprintf(vrf, sizeof(vrf), "%u", iface->vrf_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
Containerfile.groutGNUmakefileapi/gr_net_types.hcli/ec_node_dyn.ccli/ecoli.ccli/exec.ccli/interact.cdebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/cli/affinity.cmodules/infra/cli/bond.cmodules/infra/cli/iface.cmodules/infra/cli/nexthop.cmodules/infra/cli/port.cmodules/infra/cli/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.hmodules/infra/control/gr_worker.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/gr_ip4_control.hmodules/ip/control/route.cmodules/ip6/control/gr_ip6_control.hmodules/ip6/control/route.cmodules/srv6/cli/localsid.crpm/grout.specsmoke/_init.shsmoke/telemetry_test.sh
💤 Files with no reviewable changes (7)
- debian/grout-prometheus.install
- modules/ip6/control/gr_ip6_control.h
- debian/rules
- rpm/grout.spec
- GNUmakefile
- debian/control
- modules/ip/control/gr_ip4_control.h
✅ Files skipped from review due to trivial changes (4)
- modules/infra/cli/nexthop.c
- cli/ecoli.c
- modules/infra/cli/affinity.c
- cli/ec_node_dyn.c
🚧 Files skipped from review as they are similar to previous changes (6)
- main/gr_config.h
- modules/infra/control/worker_test.c
- meson.build
- modules/infra/control/gr_iface.h
- smoke/_init.sh
- main/meson.build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/srv6/cli/localsid.cmodules/infra/cli/iface.ccli/interact.capi/gr_net_types.hmodules/infra/control/worker.cmodules/infra/control/gr_worker.hmodules/infra/cli/stats.ccli/exec.cmain/main.cmodules/infra/cli/port.cmain/prom.cmodules/infra/control/port_test.cmain/gr_prom.hmodules/ip6/control/route.cmodules/infra/cli/bond.cmain/dpdk.cmodules/ip/control/route.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/port.cmodules/infra/control/gr_nh_control.hmodules/infra/control/nexthop.cmodules/infra/api/affinity.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/telemetry_test.sh
🧠 Learnings (18)
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/cli/iface.cmodules/infra/api/iface.cmodules/infra/control/nexthop.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
api/gr_net_types.h
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
api/gr_net_types.hmodules/infra/control/worker.cmodules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
api/gr_net_types.hmodules/ip6/control/route.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
main/main.cmodules/infra/control/port_test.cmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/api/iface.cmodules/infra/control/port.cmodules/infra/control/nexthop.csmoke/telemetry_test.sh
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/prom.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/prom.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/port.cmodules/infra/api/affinity.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/gr_nh_control.hmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/gr_nh_control.hmodules/infra/control/nexthop.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/api/iface.cmodules/infra/control/gr_nh_control.hmodules/infra/control/nexthop.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_nh_control.h
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/api/affinity.c
🧬 Code graph analysis (8)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
modules/infra/control/gr_worker.h (1)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/prom.c (2)
gr_prom_start(201-208)gr_prom_stop(210-217)
modules/ip/control/route.c (5)
modules/ip6/control/route.c (1)
count_origin_cb(486-496)modules/infra/control/nexthop.c (1)
nexthop_origin_valid(306-343)modules/infra/control/worker_test.c (2)
gr_prom_ctx_init(37-37)gr_prom_emit(39-39)modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)main/prom.c (1)
gr_prom_register(22-24)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_prom_ctx_add(47-47)gr_prom_emit(48-48)modules/infra/control/worker_test.c (2)
gr_prom_ctx_add(38-38)gr_prom_emit(39-39)
modules/infra/control/gr_nh_control.h (1)
modules/infra/control/nexthop.c (2)
nexthop_type_valid(292-304)nexthop_origin_valid(306-343)
modules/infra/control/nexthop.c (4)
api/gr_net_types.h (2)
gr_af_valid(45-53)gr_af_name(32-42)api/gr_errno.h (2)
errno_set_null(16-19)errno_set(10-13)modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/prom.c (1)
gr_prom_register(22-24)
modules/infra/api/affinity.c (1)
main/prom.c (1)
gr_prom_set_affinity(142-146)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: lint
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (14)
api/gr_net_types.h (1)
44-53: LGTM!Simple and correct validity check for address family values, mirroring the switch pattern used in
gr_af_name().Containerfile.grout (1)
5-9: LGTM!The changes correctly exclude the prometheus RPM from the container build (consistent with the PR's shift to native Prometheus collectors) and add a sensible cleanup step to reduce image size.
modules/infra/cli/stats.c (1)
14-14: LGTM!Required include for
strncmpandstrcmpusage in this file.modules/infra/cli/port.c (1)
15-15: LGTM!Required include for
memccpyandstrlenusage in this file.modules/infra/cli/iface.c (1)
17-17: LGTM!Required include for the various string functions (
strcmp,memccpy,strlen,memcpy) used throughout this file.modules/srv6/cli/localsid.c (1)
16-16: LGTM!Required include for
strcmpusage instr_to_behaviorand flavor parsing.cli/interact.c (1)
15-15: LGTM!Required include for
strerrorusage in error handling throughout this file.cli/exec.c (1)
13-13: No issues.Standard include addition for
strcmpusage.main/dpdk.c (1)
140-140: LGTM.Disabling DPDK's built-in telemetry in favor of the new Prometheus-based metrics is a reasonable architectural choice.
modules/infra/cli/bond.c (1)
15-15: No issues.Necessary include for
strcmpusage in parsing functions.modules/infra/control/gr_worker.h (1)
81-81: LGTM.Clean API addition. Callers are expected to
gr_vec_free()the returned vector per established conventions.modules/infra/api/affinity.c (1)
44-47: LGTM.Error handling follows the same pattern as existing affinity calls. The
gr_prom_set_affinitygracefully handles the case when the Prometheus thread isn't running.main/prom.c (1)
4-24: No issue here.gr_prom.hincludes<sys/queue.h>at line 10, so STAILQ macros are available tomain/prom.cvia the transitive include. No additional include needed.Likely an incorrect or invalid review comment.
modules/infra/control/nexthop.c (1)
179-206: LGTM!The Prometheus metric declarations and collector registration follow the established pattern used across other modules. The new validators add robust input checking.
Also applies to: 258-261, 270-270
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
rjarry
force-pushed
the
prometheus
branch
2 times, most recently
from
9948d70 to
4dd07a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main/main.c`:
- Around line 337-338: The call to gr_prom_start() in main currently ignores its
return value; change main to check gr_prom_start()'s return (or error code) and
handle failure by either logging a clear warning/error (using the existing
logging facility) or returning a non-zero exit status so the daemon doesn't
continue silently; on failure you can also call cleanup functions such as
gr_prom_stop() (or other shutdown helpers) before exit to keep behavior
consistent.
♻️ Duplicate comments (3)
main/prom.c (2)
104-107: Portability bug:%luforuint64_tis not portable.On platforms where
uint64_tisunsigned long long, this causes format string warnings or incorrect output.Proposed fix
+#include <inttypes.h> + void gr_prom_emit(struct gr_prom_ctx *ctx, const struct gr_prom_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value); }
140-146: Thread tracking relies on non-portablepthread_tcomparison.Comparing
pthread_tto0is not portable—pthread_tis an opaque type that may be a struct on some platforms. Consider using a separateboolflag to track whether the thread was started.Proposed fix
static struct event_base *ev_base; static pthread_t thread_id; +static bool prom_thread_started; int gr_prom_set_affinity(size_t set_size, const cpu_set_t *affinity) { - if (thread_id == 0) + if (!prom_thread_started) return 0; return pthread_setaffinity_np(thread_id, set_size, affinity); }And update
gr_prom_startandgr_prom_stopaccordingly to set/checkprom_thread_started.modules/infra/api/iface.c (1)
213-220: Potential NULL pointer dereference.
iface_type_get()can returnNULL, but there's no check before dereferencingtype->nameon line 219. The established pattern in this file (lines 19, 85, 102) usesassert(type != NULL)after callingiface_type_get().Proposed fix
while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { const struct iface_type *type = iface_type_get(iface->type); + assert(type != NULL); snprintf(vrf, sizeof(vrf), "%u", iface->vrf_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/grout.8.scdocmain/gr_config.hmain/gr_prom.hmain/main.cmain/meson.buildmain/prom.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.csmoke/_init.shsmoke/telemetry_test.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/infra/control/worker_test.c
- main/gr_prom.h
- modules/infra/control/port_test.c
- meson.build
- modules/infra/api/affinity.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/telemetry_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/api/iface.cmodules/infra/api/stats.cmain/prom.cmodules/infra/control/gr_iface.hmodules/infra/control/port.cmain/gr_config.hmodules/infra/control/nexthop.cmain/main.c
🧠 Learnings (18)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
smoke/_init.shmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/api/iface.cmodules/infra/control/gr_iface.hmodules/infra/control/port.cmodules/infra/control/nexthop.cmain/main.csmoke/telemetry_test.sh
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/port.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/gr_iface.h
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/api/iface.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/api/iface.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/prom.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/prom.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
main/meson.build
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
🧬 Code graph analysis (3)
modules/infra/api/stats.c (4)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)modules/infra/control/worker_test.c (3)
gr_prom_ctx_init(37-37)gr_prom_emit(39-39)gr_register_api_handler(34-34)main/module.c (1)
gr_register_api_handler(19-33)main/prom.c (1)
gr_prom_register(22-24)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_prom_labels_add(47-47)gr_prom_emit(48-48)modules/infra/control/worker_test.c (2)
gr_prom_labels_add(38-38)gr_prom_emit(39-39)
modules/infra/control/nexthop.c (4)
api/gr_net_types.h (2)
gr_af_valid(45-53)gr_af_name(32-42)api/gr_errno.h (2)
errno_set_null(16-19)errno_set(10-13)modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/prom.c (1)
gr_prom_register(22-24)
🪛 Cppcheck (2.19.0)
main/prom.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/telemetry_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- 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-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (13)
main/meson.build (1)
10-10: LGTM!Source file addition for Prometheus exporter is correct.
smoke/_init.sh (1)
12-15: LGTM!Loopback setup is idempotent and correctly ensures localhost networking for Prometheus exporter tests.
main/gr_config.h (1)
27-28: LGTM!Configuration fields for Prometheus exporter are well-typed and appropriately documented.
main/prom.c (1)
20-24: LGTM!Collector registration using STAILQ is a standard and correct pattern. The static analysis hint is a false positive—
STAILQ_HEAD_INITIALIZERis valid BSD queue macro syntax.modules/infra/control/gr_iface.h (1)
57-58: LGTM!Clean extension point for per-interface-type Prometheus metrics collection.
docs/grout.8.scdoc (1)
79-84: LGTM!Documentation accurately describes the new Prometheus option including the disable syntax.
smoke/telemetry_test.sh (1)
1-38: LGTM!Good coverage of interface types (port, bond, vlan, ipip), nexthops, addresses, and routes. The
curl --failproperly catches HTTP errors from the Prometheus endpoint.modules/infra/control/port.c (1)
601-627: LGTM!The Prometheus metrics are well-defined and the driver label instability issue from prior review is now addressed with the
"?"fallback. The conditional stats emission is acceptable sincerte_eth_stats_getfailure indicates device-level problems.modules/ip6/control/route.c (1)
484-523: LGTM!The Prometheus collector properly iterates VRFs, tallies routes by origin, and emits labeled metrics. The stack-allocated counts array is correctly zeroed per-VRF and the zero-count filter avoids unnecessary metric emission.
modules/infra/control/nexthop.c (1)
724-756: LGTM!The Prometheus collector implementation is sound. The array bounds match the iteration, counts are properly zeroed, and validity is checked before emission.
modules/ip/control/route.c (1)
445-484: LGTM!The rib4 Prometheus collector properly handles per-VRF iteration, validates origins before emission, and correctly aggregates route counts by origin.
modules/infra/api/iface.c (1)
258-270: LGTM!The Prometheus collector registration and type-specific dispatch pattern look correct.
modules/infra/api/stats.c (1)
260-317: LGTM!The Prometheus collectors for graph and CPU stats are well-implemented. Proper NULL checks on worker stats, memory is freed after use, and the skip_stat filtering is correctly applied.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Looks good, but as a general comment, shouldn't we rename |
Yes, I can do that. It will be more generic. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/control/nexthop.c (1)
398-439: Potential issue: nexthop ID may be lost on update failure.If
nexthop_updatefails after line 403 (wherenexthop_id_putreleases the existing ID), the nexthop permanently loses its ID even thoughnh->baseis restored. For nexthops withref_count > 0that were previously accessible by ID, this could orphan them fromnexthop_lookup_by_id.Consider deferring
nexthop_id_putuntil after validation succeeds, or saving/restoring the old ID on failure:Suggested approach
int nexthop_update(struct nexthop *nh, const struct gr_nexthop_base *base, const void *info) { const struct nexthop_type_ops *ops = type_ops[base->type]; struct gr_nexthop_base backup = nh->base; + uint32_t old_id = nh->nh_id; int ret; if (!nexthop_type_valid(base->type)) return errno_set(ESOCKTNOSUPPORT); if (!nexthop_origin_valid(base->origin)) return errno_set(EPFNOSUPPORT); nexthop_id_put(nh); // ... rest of function ... err: - if (nh->ref_count == 0) - nexthop_id_put(nh); // nexthop was just created, release the ID + if (nh->ref_count == 0) { + nexthop_id_put(nh); + } else if (old_id != 0) { + // Restore original ID for existing nexthops + nh->nh_id = old_id; + rte_hash_add_key_data(hash_by_id, &nh->nh_id, nh); + if (old_id <= nh_conf.max_count) + gr_id_pool_book(pool_id, old_id); + } nh->base = backup; return ret; }
♻️ Duplicate comments (1)
main/metrics.c (1)
104-107: Non-portable format specifier foruint64_t.Using
%luforuint64_tis not portable—on 32-bit systemsunsigned longis 32 bits whileuint64_tis always 64 bits, causing incorrect output.Use
PRIu64from<inttypes.h>:+#include <inttypes.h> ... void gr_metric_emit(struct gr_metrics_ctx *ctx, const struct gr_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/grout.8.scdocmain/gr_config.hmain/gr_metrics.hmain/main.cmain/meson.buildmain/metrics.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.csmoke/_init.shsmoke/metrics_test.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- main/gr_metrics.h
- modules/infra/control/gr_iface.h
- main/meson.build
- main/gr_config.h
- modules/infra/control/port.c
- modules/infra/api/iface.c
- modules/infra/api/affinity.c
- modules/infra/control/port_test.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/worker_test.cmodules/ip6/control/route.cmodules/ip/control/route.cmain/main.cmain/metrics.cmodules/infra/control/nexthop.cmodules/infra/api/stats.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/metrics_test.sh
🧠 Learnings (20)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.build
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
docs/grout.8.scdocmain/main.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/worker_test.cmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.csmoke/_init.shsmoke/metrics_test.sh
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/ip/control/route.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
main/main.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
main/main.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:26.686Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:458-472
Timestamp: 2025-09-09T12:08:26.686Z
Learning: DPDK's rte_hash_iterate function supports safe deletion during iteration. Unlike many hash table implementations, rte_hash is specifically designed to handle concurrent modifications including deletions via rte_hash_del_key without corrupting the iterator state.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:36.790Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:621-637
Timestamp: 2025-09-09T12:08:36.790Z
Learning: DPDK's rte_hash supports safe deletion during iteration via rte_hash_iterate, unlike many other hash table implementations that require two-phase deletion patterns.
Applied to files:
modules/infra/control/nexthop.c
🧬 Code graph analysis (3)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/metrics.c (2)
gr_metrics_start(201-210)gr_metrics_stop(212-219)
modules/infra/control/nexthop.c (4)
api/gr_net_types.h (2)
gr_af_valid(45-53)gr_af_name(32-42)api/gr_errno.h (2)
errno_set_null(16-19)errno_set(10-13)modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/api/stats.c (4)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)modules/infra/control/worker_test.c (3)
gr_metrics_ctx_init(37-37)gr_metric_emit(39-39)gr_register_api_handler(34-34)main/module.c (1)
gr_register_api_handler(19-33)main/metrics.c (1)
gr_metrics_register(22-24)
🪛 Cppcheck (2.19.0)
main/metrics.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/metrics_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- 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-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (8)
smoke/_init.sh (1)
12-15: LGTM!Loopback setup is correctly conditional—only adds the address if not already present. This ensures the metrics endpoint is reachable.
smoke/metrics_test.sh (1)
1-38: LGTM!Good coverage of the new metrics endpoint—sets up a realistic topology with ports, bonds, VLANs, nexthops, tunnels, and routes before validating the exporter responds.
meson.build (1)
77-77: LGTM!Build configuration correctly adds
libevent_extradependency needed for the HTTP-based metrics exporter.Also applies to: 116-116
modules/infra/control/worker_test.c (1)
37-39: LGTM!Test stubs for the new metrics API allow worker tests to link successfully without pulling in the full metrics implementation.
main/metrics.c (1)
148-199: LGTM!Thread lifecycle is well-structured with proper cleanup on all error paths. The bind failure now correctly goes to cleanup via
goto endrather than silently returning success.modules/infra/api/stats.c (3)
260-278: LGTM!The graph metrics collector properly dumps stats, iterates through them, emits metrics, and correctly frees the vector. Memory management is sound.
288-310: LGTM!The CPU metrics collector correctly uses
atomic_loadfor thread-safe access to worker stats, performs proper NULL checking before use, and uses boundedsnprintffor string formatting.
316-317: LGTM!Both collectors are registered during module initialization, consistent with the pattern used for API handlers above.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
rjarry
force-pushed
the
prometheus
branch
2 times, most recently
from
09e0393 to
14f43e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main/metrics.c`:
- Around line 62-82: In append_labels_va, n is never updated so ctx->labels_len
is not advanced; modify the loop in append_labels_va (the va_arg loop using
SAFE_BUF and snprintf) to capture the number of bytes written on each SAFE_BUF
call, accumulate that into n (or directly increment ctx->labels_len), and adjust
buf and len after each successful write (buf += written; len -= written) so
subsequent writes append at the correct offset; ensure error handling still
jumps to err on failures and that you guard against overruns before updating
ctx->labels_len.
♻️ Duplicate comments (2)
main/metrics.c (1)
104-107: Non-portable format specifier foruint64_t.Using
%luforuint64_tis incorrect on 32-bit systems whereunsigned longis 32 bits. UsePRIu64from<inttypes.h>.Suggested fix
+#include <inttypes.h> + void gr_metric_emit(struct gr_metrics_ctx *ctx, const struct gr_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value); }modules/infra/api/iface.c (1)
205-212: Potential NULL pointer dereference.
iface_type_get()can returnNULL(permodules/infra/control/iface.clines 28-35), but there's no check before dereferencingtype->nameon line 211. The established pattern in this file (lines 84-85, 100-102) usesassert(type != NULL)after callingiface_type_get().🐛 Proposed fix
while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { const struct iface_type *type = iface_type_get(iface->type); + assert(type != NULL); snprintf(vrf, sizeof(vrf), "%u", iface->vrf_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/grout.8.scdocmain/gr_config.hmain/gr_metrics.hmain/main.cmain/meson.buildmain/metrics.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker_test.cmodules/ip/control/route.cmodules/ip6/control/route.csmoke/_init.shsmoke/metrics_test.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/infra/control/port_test.c
- main/gr_config.h
- smoke/_init.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/gr_iface.hmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.cmodules/infra/control/port.cmodules/infra/control/worker_test.cmodules/infra/api/stats.cmain/metrics.cmain/gr_metrics.hmodules/infra/api/affinity.cmain/main.cmodules/infra/api/iface.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/metrics_test.sh
🧠 Learnings (23)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
main/meson.build
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/gr_iface.hmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.cmodules/infra/control/worker_test.cmodules/infra/api/iface.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.hmodules/ip/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/infra/control/gr_iface.hmodules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/gr_iface.hmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.cmodules/infra/api/iface.csmoke/metrics_test.sh
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.h
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/nexthop.cmodules/infra/api/iface.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/nexthop.cmodules/infra/control/port.c
📚 Learning: 2025-09-09T12:08:26.686Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:458-472
Timestamp: 2025-09-09T12:08:26.686Z
Learning: DPDK's rte_hash_iterate function supports safe deletion during iteration. Unlike many hash table implementations, rte_hash is specifically designed to handle concurrent modifications including deletions via rte_hash_del_key without corrupting the iterator state.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:36.790Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:621-637
Timestamp: 2025-09-09T12:08:36.790Z
Learning: DPDK's rte_hash supports safe deletion during iteration via rte_hash_iterate, unlike many other hash table implementations that require two-phase deletion patterns.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
docs/grout.8.scdocmain/main.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/metrics.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/api/affinity.cmain/main.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/api/affinity.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
main/main.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/infra/api/iface.cmeson.build
🧬 Code graph analysis (6)
modules/infra/control/nexthop.c (4)
api/gr_net_types.h (2)
gr_af_valid(45-53)gr_af_name(32-42)api/gr_errno.h (2)
errno_set_null(16-19)errno_set(10-13)modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/metrics.c (1)
gr_metrics_register(22-24)
modules/ip6/control/route.c (3)
modules/infra/control/nexthop.c (1)
nexthop_origin_valid(306-343)modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/control/worker_test.c (1)
modules/infra/control/port_test.c (3)
gr_metrics_ctx_init(46-46)gr_metrics_labels_add(47-47)gr_metric_emit(48-48)
modules/infra/api/stats.c (4)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)modules/infra/control/worker_test.c (3)
gr_metrics_ctx_init(37-37)gr_metric_emit(39-39)gr_register_api_handler(34-34)main/module.c (1)
gr_register_api_handler(19-33)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/api/affinity.c (1)
main/metrics.c (1)
gr_metrics_set_affinity(142-146)
modules/infra/api/iface.c (6)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/iface.c (2)
iface_next(243-258)iface_type_get(29-36)modules/infra/control/port_test.c (3)
gr_metrics_ctx_init(46-46)gr_metric_emit(48-48)gr_register_api_handler(21-21)main/module.c (1)
gr_register_api_handler(19-33)main/event.c (1)
gr_event_register_serializer(37-57)main/metrics.c (1)
gr_metrics_register(22-24)
🪛 Cppcheck (2.19.0)
main/metrics.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/metrics_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-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-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (17)
smoke/metrics_test.sh (1)
1-38: LGTM!The test script correctly sets up a comprehensive topology and validates the metrics endpoint with
curl --fail. The shellcheck warning about word splitting on line 5 is explicitly acceptable per coding guidelines for shell scripts in this project.meson.build (1)
77-77: LGTM!The
libevent_extradependency is correctly added for the HTTP server functionality required by the metrics exporter.Also applies to: 116-116
docs/grout.8.scdoc (2)
14-37: LGTM!The documentation correctly describes the new
-Mmetrics option with theADDR:PORTformat, disablement via:0, and the default127.0.0.1:9111binding.
79-83: LGTM!Good addition documenting the
--separator for advanced EAL arguments with appropriate warnings about potential instability.main/meson.build (1)
9-9: LGTM!Build configuration correctly includes the new
metrics.csource file.modules/infra/api/affinity.c (1)
44-47: LGTM!The metrics thread affinity is correctly set alongside other control plane threads, following the established error handling pattern. The
gr_metrics_set_affinityimplementation gracefully handles the case where the metrics thread hasn't started yet.modules/infra/control/gr_iface.h (1)
8-8: LGTM!The new
metrics_collectcallback follows the established pattern for interface type callbacks, and the include is appropriately added for thestruct gr_metrics_ctxtype.Also applies to: 57-57
modules/infra/control/worker_test.c (1)
37-39: LGTM!Test stubs for the metrics API are consistent with
port_test.cand allow the worker tests to compile without the full metrics runtime.main/main.c (1)
108-127: LGTM!The metrics address parsing and lifecycle integration look correct. The start/stop calls are properly ordered in the main initialization/shutdown sequence.
Also applies to: 192-193, 212-215, 306-307, 327-327
modules/infra/control/port.c (1)
601-627: LGTM!The port metrics collection is well-implemented with proper fallback for the driver label (avoiding label-set instability), and the metrics are only emitted when stats retrieval succeeds.
Also applies to: 796-796
modules/ip/control/route.c (1)
445-486: LGTM!The metrics collection implementation is sound. The
countsarray is properly sized usingUINT_NUM_VALUES(gr_nh_origin_t), and the validation withnexthop_origin_valid(o)before emission prevents any invalid origin values from being processed.modules/ip6/control/route.c (1)
484-525: LGTM!Consistent implementation with the IPv4 counterpart. The metrics collection is correctly bounded and validated.
modules/infra/control/nexthop.c (2)
292-363: LGTM!The new validation functions
nexthop_type_validandnexthop_origin_validare comprehensive, covering all enum values. The registration functions now properly validate inputs and prevent duplicate registrations with clear error messages viaABORT().
724-749: LGTM!The metrics collection is correctly implemented with proper bounds checking via
nexthop_type_valid(t)before accessing type names.modules/infra/api/iface.c (1)
179-253: LGTM on metrics definitions and collector registration.The metric declarations and aggregation logic for per-core stats are well-structured. The type-specific dispatch at lines 245-246 correctly handles optional
metrics_collectcallbacks.modules/infra/api/stats.c (1)
260-317: LGTM!The metrics collectors are properly implemented:
graph_metrics_collectcorrectly frees the stats vector after use (line 277)cpu_collect_metricsproperly checks for NULL stats (line 295-296) before accessing worker statistics- Both collectors are registered during initialization
main/gr_metrics.h (1)
1-65: LGTM!Clean API design for the metrics framework. The type definitions, collector registration mechanism, and lifecycle functions follow well-established patterns. The varargs approach with NULL termination for labels is idiomatic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
rjarry
force-pushed
the
prometheus
branch
2 times, most recently
from
67297cc to
783ad3b
Compare
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/control/nexthop.c (1)
393-401: Validatebase->typebefore indexingtype_ops.
type_ops[base->type]is accessed beforenexthop_type_valid()runs. An invalid type can read out of bounds.✅ Proposed fix
-int nexthop_update(struct nexthop *nh, const struct gr_nexthop_base *base, const void *info) { - const struct nexthop_type_ops *ops = type_ops[base->type]; +int nexthop_update(struct nexthop *nh, const struct gr_nexthop_base *base, const void *info) { + const struct nexthop_type_ops *ops = NULL; @@ if (!nexthop_type_valid(base->type)) return errno_set(ESOCKTNOSUPPORT); if (!nexthop_origin_valid(base->origin)) return errno_set(EPFNOSUPPORT); + + ops = type_ops[base->type];
🤖 Fix all issues with AI agents
In `@modules/infra/api/stats.c`:
- Around line 264-271: The graph_metrics_collect function is excluding nodes
with zero packets via the conditional skip_stat(s, 0); remove or alter that
check so zero‑packet nodes are not filtered out — e.g., delete the if
(skip_stat(s, 0)) continue; line in graph_metrics_collect (which iterates over
worker_dump_stats results) so gr_metrics_ctx_init still runs for nodes with zero
packets; ensure no other code relies on skip_stat(s, 0) being applied here.
In `@modules/ip/control/route.c`:
- Around line 173-181: The code writes to route_counts[vrf_id][origin] without
validating origin; add a check using nexthop_origin_valid(origin) before any
read/write to route_counts in both the insert path (around rte_rib_set_nh /
rte_rib_get_ext where *o = origin and route_counts[vrf_id][origin]++) and the
delete path (where route_counts[vrf_id][*o]-- is performed). If
nexthop_origin_valid(origin) returns false, avoid indexing route_counts (log or
return an error such as -EINVAL) and skip the increment/decrement to prevent
out-of-bounds writes.
♻️ Duplicate comments (4)
modules/infra/control/worker.c (1)
436-499: LGTM!The
worker_dump_statsfunction provides clean aggregation of per-worker statistics with proper handling of CPU filtering via thecpu_idparameter.Note: The potential unsigned underflow at line 492 (
.cycles = loop_cycles - node_cycles) was already flagged in a previous review.main/metrics.c (2)
62-77: Labels buffer offset never advances; labels overwrite each other.
nis never updated, soctx->labels_lenstays unchanged and subsequent labels overwrite earlier ones. Track bytes written and advance the cursor.🛠️ Proposed fix
static void append_labels_va(struct gr_metrics_ctx *ctx, va_list ap) { - const size_t len = sizeof(ctx->labels) - ctx->labels_len; - char *buf = ctx->labels + ctx->labels_len; + size_t len = sizeof(ctx->labels) - ctx->labels_len; + char *buf = ctx->labels + ctx->labels_len; const char *key, *val; - size_t n = 0; + size_t total = 0; + int written; for (key = va_arg(ap, const char *); key != NULL; key = va_arg(ap, const char *)) { val = va_arg(ap, const char *); - if (n > 0 || ctx->labels_len > 0) - SAFE_BUF(snprintf, len, ","); + if (total > 0 || ctx->labels_len > 0) { + written = snprintf(buf, len, ","); + if (written < 0 || (size_t)written >= len) + goto err; + buf += written; + len -= written; + total += written; + } - SAFE_BUF(snprintf, len, "%s=\"%s\"", key, val ?: ""); + written = snprintf(buf, len, "%s=\"%s\"", key, val ?: ""); + if (written < 0 || (size_t)written >= len) + goto err; + buf += written; + len -= written; + total += written; } - ctx->labels_len += n; + ctx->labels_len += total;
104-107: Use PRIu64 foruint64_toutput.
%luis not portable foruint64_t(breaks on 32‑bit). UsePRIu64.🔧 Proposed fix
+#include <inttypes.h> @@ - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf( + ctx->w->buf, + "grout_%s{%s} %" PRIu64 "\n", + m->name, + ctx->labels, + value + );modules/infra/api/iface.c (1)
205-212: Potential NULL pointer dereference ontype.
iface_type_get()can returnNULL, buttype->nameis dereferenced on line 211 without validation. The established pattern in this file (see lines 84-85, 100-102, 159-160) is to asserttype != NULLafter the call.Proposed fix
while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { const struct iface_type *type = iface_type_get(iface->type); + assert(type != NULL); snprintf(vrf, sizeof(vrf), "%u", iface->vrf_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
Containerfile.groutGNUmakefileapi/gr_net_types.hcli/ec_node_dyn.ccli/ecoli.ccli/exec.ccli/interact.cdebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_metrics.hmain/main.cmain/meson.buildmain/metrics.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/gr_infra.hmodules/infra/api/gr_nexthop.hmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/cli/affinity.cmodules/infra/cli/bond.cmodules/infra/cli/iface.cmodules/infra/cli/nexthop.cmodules/infra/cli/port.cmodules/infra/cli/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.hmodules/infra/control/gr_worker.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/gr_ip4_control.hmodules/ip/control/route.cmodules/ip6/control/gr_ip6_control.hmodules/ip6/control/route.cmodules/srv6/cli/localsid.crpm/grout.specsmoke/_init.shsmoke/metrics_test.sh
💤 Files with no reviewable changes (7)
- modules/ip/control/gr_ip4_control.h
- rpm/grout.spec
- modules/ip6/control/gr_ip6_control.h
- debian/control
- GNUmakefile
- debian/rules
- debian/grout-prometheus.install
✅ Files skipped from review due to trivial changes (1)
- main/meson.build
🚧 Files skipped from review as they are similar to previous changes (17)
- modules/srv6/cli/localsid.c
- modules/infra/cli/affinity.c
- cli/exec.c
- modules/infra/cli/iface.c
- cli/ec_node_dyn.c
- meson.build
- smoke/_init.sh
- modules/infra/api/gr_infra.h
- Containerfile.grout
- modules/infra/api/affinity.c
- modules/infra/control/worker_test.c
- cli/interact.c
- modules/infra/cli/bond.c
- modules/infra/control/port_test.c
- modules/infra/cli/stats.c
- main/dpdk.c
- modules/infra/cli/port.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
api/gr_net_types.hmodules/infra/control/gr_iface.hmodules/ip6/control/route.cmain/metrics.cmodules/ip/control/route.cmodules/infra/control/port.ccli/ecoli.cmain/main.cmodules/infra/control/worker.cmodules/infra/cli/nexthop.cmain/gr_metrics.hmodules/infra/api/iface.cmodules/infra/control/nexthop.cmodules/infra/api/stats.cmodules/infra/api/gr_nexthop.hmodules/infra/control/gr_worker.hmodules/infra/control/gr_nh_control.hmain/gr_config.h
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/metrics_test.sh
🧠 Learnings (22)
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
api/gr_net_types.hmodules/infra/control/gr_iface.hmodules/ip/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
api/gr_net_types.hmodules/infra/control/gr_iface.hmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/worker.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
api/gr_net_types.hmodules/ip6/control/route.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/gr_iface.hmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/api/iface.cmodules/infra/control/nexthop.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/gr_iface.hmodules/ip/control/route.cmodules/infra/api/iface.cmodules/infra/control/nexthop.csmoke/metrics_test.sh
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/port.cmodules/infra/control/nexthop.c
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/ecoli.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
main/main.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
main/main.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.cdocs/grout.8.scdoc
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/cli/nexthop.cmodules/infra/api/iface.cmodules/infra/control/nexthop.cmodules/infra/api/gr_nexthop.hmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/infra/api/iface.c
📚 Learning: 2025-09-09T12:08:26.686Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:458-472
Timestamp: 2025-09-09T12:08:26.686Z
Learning: DPDK's rte_hash_iterate function supports safe deletion during iteration. Unlike many hash table implementations, rte_hash is specifically designed to handle concurrent modifications including deletions via rte_hash_del_key without corrupting the iterator state.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:36.790Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:621-637
Timestamp: 2025-09-09T12:08:36.790Z
Learning: DPDK's rte_hash supports safe deletion during iteration via rte_hash_iterate, unlike many other hash table implementations that require two-phase deletion patterns.
Applied to files:
modules/infra/control/nexthop.c
🧬 Code graph analysis (6)
modules/ip6/control/route.c (4)
modules/infra/control/nexthop.c (1)
nexthop_origin_valid(306-343)modules/infra/control/worker_test.c (2)
gr_metrics_ctx_init(37-37)gr_metric_emit(39-39)modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/control/port.c (3)
modules/infra/control/gr_iface.h (1)
iface(17-26)modules/infra/control/port_test.c (2)
gr_metrics_labels_add(47-47)gr_metric_emit(48-48)modules/infra/control/worker_test.c (2)
gr_metrics_labels_add(38-38)gr_metric_emit(39-39)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
modules/infra/api/stats.c (4)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)modules/infra/control/worker_test.c (3)
gr_metrics_ctx_init(37-37)gr_metric_emit(39-39)gr_register_api_handler(34-34)main/module.c (1)
gr_register_api_handler(19-33)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/control/gr_worker.h (1)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)
modules/infra/control/gr_nh_control.h (1)
modules/infra/control/nexthop.c (2)
nexthop_type_valid(292-304)nexthop_origin_valid(306-343)
🪛 Cppcheck (2.19.0)
main/metrics.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/metrics_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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 (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (16)
modules/infra/api/gr_nexthop.h (1)
194-195: LGTM!Returning
"UNSPEC"instead of an empty string is a sensible fix—consistent with how"INTERNAL"is handled and avoids ambiguous output in logs or metrics.cli/ecoli.c (1)
10-10: LGTM!Necessary include for
strcmp()usage in the file.modules/infra/cli/nexthop.c (1)
18-18: LGTM!Necessary include for
strcmp()usage in the file.api/gr_net_types.h (1)
44-53: LGTM!Clean validation function that correctly covers all defined
addr_family_tenum values and safely returnsfalsefor any invalid input.modules/infra/control/gr_worker.h (1)
81-81: LGTM!Declaration matches the implementation and follows the established
gr_vecpattern for returning dynamically-sized results.main/gr_config.h (1)
27-28: LGTM!Configuration fields for the OpenMetrics exporter are well-defined with clear disable semantics documented in comments.
modules/infra/control/gr_nh_control.h (1)
90-94: LGTM!The new validator functions provide a clean public API for runtime validation of nexthop type and origin values. The implementations in the relevant snippets correctly enumerate all valid enum cases.
smoke/metrics_test.sh (1)
1-38: LGTM!The test script provides good coverage for the new metrics endpoint, exercising a variety of configuration options (bonds, VLANs, nexthops, routes, tunnels) before validating the metrics export.
modules/infra/control/worker.c (1)
425-434: LGTM!The
find_stathelper correctly useserrno_set_null(ENOENT)to propagate error state when no matching stat is found.docs/grout.8.scdoc (1)
33-37: LGTM!Documentation clearly explains the new metrics option with default value and disable syntax.
main/gr_metrics.h (2)
1-65: LGTM!Clean OpenMetrics API design with proper separation between metric definitions, emission context, and collector registration. The variadic label functions with NULL termination follow common C conventions.
34-38: No action needed. The implementation already has proper bounds checking inappend_labels_va()via theSAFE_BUFmacro, which validates thatsnprintfdoes not overflow the buffer and jumps to an error handler if overflow is detected.Likely an incorrect or invalid review comment.
modules/infra/control/port.c (1)
601-627: LGTM!The metrics collection is well-structured. The driver label stability concern from the previous review has been addressed by always adding the "driver" label with a fallback value of "?" when
rte_eth_dev_info_getfails. The stats retrieval is properly guarded.modules/ip6/control/route.c (1)
495-521: LGTM!The metrics collection properly validates origins via
nexthop_origin_valid()before accessingroute_counts, and the route count tracking in insert/delete is protected by assertions against underflow.modules/infra/control/gr_iface.h (1)
57-57: LGTM!Clean extension of the interface type structure to support per-type metrics collection.
modules/infra/api/iface.c (1)
219-243: LGTM!The per-core stats aggregation loop correctly iterates all possible lcores, and the metrics emission pattern is consistent with the rest of the file.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main/main.c`:
- Around line 128-130: The code currently uses strlen(addr_port_str) and
strcpy(addr_port_str, "::"), which can overflow if addr_port_str is empty or
smaller than 3 bytes; instead, avoid writing a 3-byte literal into a potentially
too-small buffer — either use a safe bounded copy (e.g., snprintf/strncpy with
the buffer size) or switch to using a static default string and point to it
(e.g., use const char *default_addr = "::" when strlen(addr_port_str) == 0) so
you don't overwrite adjacent memory; update the logic around
addr_port_str/strcpy in main (or the function that contains addr_port_str)
accordingly.
♻️ Duplicate comments (4)
modules/infra/control/worker.c (1)
489-496: Potential unsigned underflow in overhead cycles calculation.If
node_cyclesexceedsloop_cyclesdue to timing jitter during stats collection, line 492 will underflow. The similar concern at line 485 was dismissed by the author as "cannot happen" - if the same guarantee applies here, this can be ignored.main/metrics.c (2)
104-107: Non-portable format specifier foruint64_t.Using
%luforuint64_tis not portable. On 32-bit systems,unsigned longis 32 bits whileuint64_tis always 64 bits.🐛 Proposed fix
+#include <inttypes.h> + void gr_metric_emit(struct gr_metrics_ctx *ctx, const struct gr_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value); }
62-82:ctx->labels_lenis never updated - labels will overwrite each other.Variable
nis initialized to0at line 66 and never modified. TheSAFE_BUFmacro advancesbufbut doesn't updaten. At line 77,ctx->labels_len += nadds zero, so subsequent calls togr_metrics_labels_addwill overwrite previously written labels.🐛 Proposed fix
Track the bytes written by computing the difference between the final and initial buffer positions:
static void append_labels_va(struct gr_metrics_ctx *ctx, va_list ap) { const size_t len = sizeof(ctx->labels) - ctx->labels_len; char *buf = ctx->labels + ctx->labels_len; + char *start = buf; const char *key, *val; - size_t n = 0; for (key = va_arg(ap, const char *); key != NULL; key = va_arg(ap, const char *)) { val = va_arg(ap, const char *); - if (n > 0 || ctx->labels_len > 0) + if (buf > start || ctx->labels_len > 0) SAFE_BUF(snprintf, len, ","); SAFE_BUF(snprintf, len, "%s=\"%s\"", key, val ?: ""); } - ctx->labels_len += n; + ctx->labels_len += (buf - start); return; err: LOG(ERR, "snprintf: %s", strerror(errno)); }modules/infra/api/iface.c (1)
205-212: Potential NULL pointer dereference.
iface_type_get()can returnNULL(permodules/infra/control/iface.c), but there's no check before dereferencingtype->nameon line 211. The established pattern in this file (lines 84-85, 100-102) usesassert(type != NULL)after callingiface_type_get().🐛 Proposed fix
while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { const struct iface_type *type = iface_type_get(iface->type); + assert(type != NULL); snprintf(vrf, sizeof(vrf), "%u", iface->vrf_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
Containerfile.groutGNUmakefileapi/gr_net_types.hcli/ec_node_dyn.ccli/ecoli.ccli/exec.ccli/interact.cdebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_metrics.hmain/main.cmain/meson.buildmain/metrics.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/cli/affinity.cmodules/infra/cli/bond.cmodules/infra/cli/iface.cmodules/infra/cli/nexthop.cmodules/infra/cli/port.cmodules/infra/cli/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.hmodules/infra/control/gr_worker.hmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/gr_ip4_control.hmodules/ip/control/route.cmodules/ip6/control/gr_ip6_control.hmodules/ip6/control/route.cmodules/srv6/cli/localsid.crpm/grout.specsmoke/_init.shsmoke/metrics_test.sh
💤 Files with no reviewable changes (7)
- debian/rules
- debian/control
- modules/ip/control/gr_ip4_control.h
- rpm/grout.spec
- debian/grout-prometheus.install
- GNUmakefile
- modules/ip6/control/gr_ip6_control.h
✅ Files skipped from review due to trivial changes (1)
- modules/infra/cli/bond.c
🚧 Files skipped from review as they are similar to previous changes (16)
- modules/infra/api/affinity.c
- cli/ecoli.c
- smoke/_init.sh
- modules/infra/cli/affinity.c
- main/gr_config.h
- main/meson.build
- docs/grout.8.scdoc
- modules/infra/cli/port.c
- api/gr_net_types.h
- modules/infra/control/port.c
- modules/infra/cli/stats.c
- modules/infra/control/gr_worker.h
- cli/interact.c
- modules/infra/cli/nexthop.c
- cli/ec_node_dyn.c
- modules/srv6/cli/localsid.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/dpdk.cmodules/infra/control/port_test.cmodules/infra/control/gr_iface.hmodules/infra/control/worker_test.cmain/metrics.cmain/main.cmodules/ip6/control/route.ccli/exec.cmodules/ip/control/route.cmodules/infra/cli/iface.cmodules/infra/api/stats.cmain/gr_metrics.hmodules/infra/control/nexthop.cmodules/infra/control/gr_nh_control.hmodules/infra/control/worker.cmodules/infra/api/iface.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/metrics_test.sh
🧠 Learnings (22)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/port_test.cmodules/infra/control/gr_iface.hmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/api/iface.csmoke/metrics_test.sh
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/control/gr_iface.hmodules/infra/control/worker_test.cmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/api/iface.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/infra/control/gr_iface.hmodules/ip/control/route.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/infra/control/gr_iface.hmodules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/worker.c
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.buildmodules/infra/api/iface.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/metrics.c
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
main/main.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
main/main.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
modules/ip6/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/control/gr_nh_control.h
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/route.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
Applied to files:
modules/ip/control/route.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/infra/control/gr_nh_control.hmodules/infra/api/iface.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:26.686Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:458-472
Timestamp: 2025-09-09T12:08:26.686Z
Learning: DPDK's rte_hash_iterate function supports safe deletion during iteration. Unlike many hash table implementations, rte_hash is specifically designed to handle concurrent modifications including deletions via rte_hash_del_key without corrupting the iterator state.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:36.790Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:621-637
Timestamp: 2025-09-09T12:08:36.790Z
Learning: DPDK's rte_hash supports safe deletion during iteration via rte_hash_iterate, unlike many other hash table implementations that require two-phase deletion patterns.
Applied to files:
modules/infra/control/nexthop.c
🧬 Code graph analysis (5)
modules/infra/control/port_test.c (1)
modules/infra/control/worker_test.c (3)
gr_metrics_ctx_init(37-37)gr_metrics_labels_add(38-38)gr_metric_emit(39-39)
modules/infra/control/worker_test.c (1)
modules/infra/control/port_test.c (3)
gr_metrics_ctx_init(46-46)gr_metrics_labels_add(47-47)gr_metric_emit(48-48)
main/gr_metrics.h (1)
main/metrics.c (7)
gr_metrics_ctx_init(84-94)gr_metrics_labels_add(96-102)gr_metric_emit(104-107)gr_metrics_register(22-24)gr_metrics_start(205-214)gr_metrics_stop(216-223)gr_metrics_set_affinity(142-146)
modules/infra/control/nexthop.c (4)
api/gr_net_types.h (2)
gr_af_valid(45-53)gr_af_name(32-42)api/gr_errno.h (2)
errno_set_null(16-19)errno_set(10-13)modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
🪛 Cppcheck (2.19.0)
main/metrics.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/metrics_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- 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-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (21)
main/dpdk.c (1)
137-140: No action needed. The--no-telemetryflag has been available since DPDK 20.05, so it is definitely supported in version 25.11 and later.modules/infra/control/port_test.c (1)
46-48: LGTM!The stub functions correctly match the signatures declared in
main/gr_metrics.hand align with identical stubs inworker_test.c, allowing tests to link without the full metrics implementation.modules/infra/control/gr_nh_control.h (1)
90-95: LGTM!Clean public API additions for validating nexthop type and origin values. The declarations are consistent with the existing API style in this header.
smoke/metrics_test.sh (1)
1-38: LGTM!Comprehensive smoke test that configures a realistic topology (ports, bonds, VLANs, nexthops, routes, tunnel) and validates the metrics endpoint is functional. The
--failflag ensures the test fails on HTTP errors.main/gr_metrics.h (2)
1-65: LGTM!Well-designed OpenMetrics framework API with clear separation between metric definitions, context management, and collector registration. The lifecycle functions (
gr_metrics_start/gr_metrics_stop/gr_metrics_set_affinity) provide clean thread management.
34-38: No action needed. The labels buffer is properly protected against overflow. TheSAFE_BUFmacro (api/gr_macro.h:37-47) enforces bounds checking on allsnprintfwrites to the 512-byte buffer, detecting overflows and raisingENOBUFSerrors before any write exceeds available space.modules/infra/control/worker_test.c (1)
37-39: LGTM!Stub functions correctly match the metrics API signatures, consistent with the stubs in
port_test.c.modules/infra/control/gr_iface.h (1)
8-8: LGTM!The new
metrics_collecthook integrates cleanly with the existingiface_typestructure, following the same callback pattern as other operations.Also applies to: 57-57
main/main.c (1)
315-316: LGTM!Metrics lifecycle is properly integrated - started after API socket and stopped during shutdown in the correct order.
Also applies to: 336-336
modules/infra/api/iface.c (1)
219-243: LGTM!Per-core statistics aggregation is correctly implemented, iterating over all lcores and summing the counters before emission.
modules/infra/control/worker.c (1)
425-434: LGTM!The
worker_dump_statsfunction correctly aggregates per-worker and per-node statistics, with proper handling of the "idle" and "overhead" synthetic stats.Also applies to: 436-499
main/metrics.c (1)
205-223: LGTM!The metrics start/stop lifecycle is properly implemented with appropriate thread management and cleanup.
modules/infra/api/stats.c (2)
264-276: LGTM - metrics collection for graph nodes.The graph metrics collector correctly iterates over all worker stats and emits packets, batches, and cycles for each node without inappropriate filtering.
286-308: LGTM - CPU metrics collection.The CPU collector properly handles the atomic load of worker stats, null-checks the stats pointer, and emits both idle and busy cycle counts with appropriate CPU and NUMA labels.
modules/infra/control/nexthop.c (3)
292-343: LGTM - nexthop type and origin validators.The validators correctly enumerate all valid enum cases and return false for any unrecognized values, providing safe bounds checking for array indexing operations.
370-371: Good defensive addition - NULL parameter validation.The NULL checks for
baseandinfoparameters prevent potential crashes from invalid API calls.
724-749: LGTM - nexthop metrics collection.The collector safely iterates using
nexthop_type_valid()to guard array bounds, andcount_typesonly increments for nexthops with valid types (ensured by validation in nexthop_new/nexthop_update).modules/ip6/control/route.c (2)
170-173: LGTM - origin validation on insert path.The origin is validated before any array indexing into
route_counts, preventing out-of-bounds writes from invalid API inputs.
501-520: LGTM - metrics collection with proper validation.The metrics collector correctly uses
nexthop_origin_valid()to guard the iteration, ensuring only valid origins are accessed inroute_counts.modules/ip/control/route.c (2)
156-159: Past critical issue addressed - origin validation now present.The origin parameter is validated before any indexing into
route_counts, preventing the out-of-bounds write vulnerability identified in the previous review.
461-480: LGTM - metrics collection with validation guards.The metrics collector properly validates origin values via
nexthop_origin_valid()before accessingroute_counts, consistent with the IPv6 implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // If address is empty, default to "::" | ||
| if (strlen(addr_port_str) == 0) | ||
| strcpy(addr_port_str, "::"); |
There was a problem hiding this comment.
Buffer overflow when address portion is empty or short.
If the user provides an input like [:]:9111 or :9111, after bracket stripping the addr_port_str buffer may be empty or very short. Writing "::" (3 bytes including null) via strcpy can overflow into adjacent memory.
Consider using a static default string instead of writing into the potentially too-small buffer:
🐛 Proposed fix
- // If address is empty, default to "::"
- if (strlen(addr_port_str) == 0)
- strcpy(addr_port_str, "::");
-
- gr_config.metrics_addr = addr_port_str;
+ // If address is empty, default to "::"
+ if (strlen(addr_port_str) == 0)
+ gr_config.metrics_addr = "::";
+ else
+ gr_config.metrics_addr = addr_port_str;🤖 Prompt for AI Agents
In `@main/main.c` around lines 128 - 130, The code currently uses
strlen(addr_port_str) and strcpy(addr_port_str, "::"), which can overflow if
addr_port_str is empty or smaller than 3 bytes; instead, avoid writing a 3-byte
literal into a potentially too-small buffer — either use a safe bounded copy
(e.g., snprintf/strncpy with the buffer size) or switch to using a static
default string and point to it (e.g., use const char *default_addr = "::" when
strlen(addr_port_str) == 0) so you don't overwrite adjacent memory; update the
logic around addr_port_str/strcpy in main (or the function that contains
addr_port_str) accordingly.
The vrf_id field was not validated against GR_MAX_VRFS before interface creation. This could lead to out-of-bounds array accesses in the RIB statistics or other VRF-indexed structures. Return EOVERFLOW early to reject invalid VRF identifiers. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
The empty string was filtered out by telemetry handlers and would not produce valid dictionary keys. Return "UNSPEC" to make the origin visible in telemetry output. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Introduce gr_af_valid(), nexthop_type_valid(), and nexthop_origin_valid() functions to centralize validation of address family, nexthop type, and origin enum values. These helpers can now be used both internally and by external callers. Refactor nexthop_af_ops_register() and nexthop_type_ops_register() to use the new helpers instead of inline switch statements. This removes code duplication and improves error messages. Add input validation in nexthop_update() to reject invalid type or origin values with appropriate errno codes before modifying the nexthop. Also add a null pointer check in nexthop_new() for the base and info parameters. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
The string.h header is not used in gr_net_types.h itself. Remove it from the public header and add it explicitly to the CLI source files that actually require it. This reduces unnecessary includes in translation units that only need the network type definitions. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
The DPDK telemetry library requires a python script to export metrics in prometheus format. This adds complexity and an external dependency that is hard to package and maintain. Remove all rte_telemetry_register_cmd() calls and their associated handler functions. Disable the DPDK telemetry socket by passing --no-telemetry to rte_eal_init(). Remove the lcore usage callback registration as well. Remove the now unused nexthop and RIB statistics. Drop the grout-prometheus package from both RPM and Debian packaging since it only contained the DPDK telemetry exporter script. A builtin openmetrics exporter will be added in following commits. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Move the graph_stats() function from the API layer to the worker module and rename it to worker_dump_stats(). This allows other components to aggregate per-worker node statistics without going through the API. The function will be used by the openmetrics exporter to collect graph node metrics. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Instead of white listing which EAL arguments we support, do not pollute grout command line flags with them and allow users to specify any EAL argument after grout options and a "--" marker. Clearly mention that this feature is for debugging/development purposes. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Implement an openmetrics exporter using libevent HTTP server. The exporter listens on 127.0.0.1:9111 by default and can be configured via the -M/--metrics option or disabled with -M :0. The HTTP server runs in a dedicated thread with its own event loop, keeping it isolated from the main control plane. The thread inherits control_cpus affinity and can be updated at runtime via the affinity API. The implementation provides a collector registration mechanism where modules can register callbacks to emit their metrics. The API uses a context structure to manage labels and emit metric values with proper HELP and TYPE annotations in the openmetrics exposition format. Startup failures (bind errors, resource allocation) are logged but do not prevent grout from starting. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Register a metrics collector for interfaces including administrative and operational state, MTU, promiscuous mode, and packet/byte counters for both datapath and control plane. The interface statistics are aggregated across all CPU cores. Extend the iface_type structure with an optional metrics_collect callback to allow interface types to export type-specific metrics. Implement this callback for port interfaces to expose queue configuration and hardware statistics such as missed packets and TX errors. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Register a metrics collector for rte_graph node statistics. Each node exposes packets processed, batch count and CPU cycles consumed. The stats are aggregated across all workers. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Register a metrics collector for worker CPU utilization. Each worker exports total cycles and busy cycles with cpu and numa labels to allow computing utilization percentages. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Register a metrics collector for nexthop statistics. The collector exports the number of nexthops grouped by type (l3, blackhole, reject, group, etc.). Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Register a metrics collector for the IPv4 RIB. The collector exports the number of routes grouped by VRF and origin (static, kernel, ospf, etc.). Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Register a metrics collector for the IPv6 RIB. The collector exports the number of routes grouped by VRF and origin (static, kernel, ospf, etc.). Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
Configure loopback in the test namespace to allow the metrics exporter to bind its HTTP socket. Add a new test that configures various interfaces, nexthops, addresses and routes. Fetch all metrics via HTTP and verify nothing failed. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modules/ip/control/route.c`:
- Around line 461-478: rib4_metrics_collect reads shared array route_counts from
the metrics thread without synchronization, causing a data race with route
insert/delete updates; protect route_counts with a synchronization primitive
(preferably a mutex) shared between the metrics collector and the route update
code: add a global mutex (e.g., route_counts_lock), acquire it in
rib4_metrics_collect before reading route_counts and release after emitting
metrics, and also acquire/release it in the route insert/delete code paths that
modify route_counts (the functions that update route_counts at the existing
update sites). Ensure lightweight locking (hold only while reading/updating
route_counts) and use the same lock for all accesses so
gr_metrics_start/metrics_thread and the route update functions coordinate
safely.
♻️ Duplicate comments (4)
main/metrics.c (2)
62-82:ctx->labels_lenis never updated - labels will be overwritten.The variable
nis initialized to 0 at line 66 and never modified. TheSAFE_BUFmacro appears to advancebufbut doesn't updaten. At line 77,ctx->labels_len += nadds zero, so subsequent calls togr_metrics_labels_addwill overwrite previously written labels instead of appending.🐛 Proposed fix - track bytes written
static void append_labels_va(struct gr_metrics_ctx *ctx, va_list ap) { - const size_t len = sizeof(ctx->labels) - ctx->labels_len; + size_t remaining = sizeof(ctx->labels) - ctx->labels_len; char *buf = ctx->labels + ctx->labels_len; const char *key, *val; - size_t n = 0; + size_t total = 0; + int written; for (key = va_arg(ap, const char *); key != NULL; key = va_arg(ap, const char *)) { val = va_arg(ap, const char *); - if (n > 0 || ctx->labels_len > 0) - SAFE_BUF(snprintf, len, ","); - - SAFE_BUF(snprintf, len, "%s=\"%s\"", key, val ?: ""); + if (total > 0 || ctx->labels_len > 0) { + written = snprintf(buf, remaining, ","); + if (written < 0 || (size_t)written >= remaining) + goto err; + buf += written; + remaining -= written; + total += written; + } + + written = snprintf(buf, remaining, "%s=\"%s\"", key, val ?: ""); + if (written < 0 || (size_t)written >= remaining) + goto err; + buf += written; + remaining -= written; + total += written; } - ctx->labels_len += n; + ctx->labels_len += total; return; err: LOG(ERR, "snprintf: %s", strerror(errno)); }
104-107: Non-portable format specifier foruint64_t.Using
%luforuint64_tis not portable across architectures. UsePRIu64from<inttypes.h>instead.Suggested fix
+#include <inttypes.h> + void gr_metric_emit(struct gr_metrics_ctx *ctx, const struct gr_metric *m, uint64_t value) { emit_help_type(ctx->w, m); - evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %lu\n", m->name, ctx->labels, value); + evbuffer_add_printf(ctx->w->buf, "grout_%s{%s} %" PRIu64 "\n", m->name, ctx->labels, value); }main/main.c (1)
128-132: Potential buffer overflow when address is empty after bracket stripping.If the user provides an input like
[]:9111, after bracket strippingaddr_port_strpoints to a 2-byte region ([]with null), butstrcpy(addr_port_str, "::")writes 3 bytes. Use a static default instead of writing into the potentially too-small buffer.🐛 Proposed fix
// If address is empty, default to "::" - if (strlen(addr_port_str) == 0) - strcpy(addr_port_str, "::"); - - gr_config.metrics_addr = addr_port_str; + if (strlen(addr_port_str) == 0) + gr_config.metrics_addr = "::"; + else + gr_config.metrics_addr = addr_port_str;modules/infra/api/iface.c (1)
205-211: Potential NULL pointer dereference — add assert.
iface_type_get()can returnNULL, buttype->nameis used at line 211 without checking. The established pattern in this file (see lines 84-85, 100-102) usesassert(type != NULL)after this call.Proposed fix
while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { const struct iface_type *type = iface_type_get(iface->type); + assert(type != NULL); snprintf(vrf, sizeof(vrf), "%u", iface->vrf_id);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
Containerfile.groutGNUmakefileapi/gr_net_types.hcli/ec_node_dyn.ccli/ecoli.ccli/exec.ccli/interact.cdebian/controldebian/grout-prometheus.installdebian/rulesdocs/grout.8.scdocmain/dpdk.cmain/gr_config.hmain/gr_metrics.hmain/main.cmain/meson.buildmain/metrics.cmeson.buildmodules/infra/api/affinity.cmodules/infra/api/gr_infra.hmodules/infra/api/gr_nexthop.hmodules/infra/api/iface.cmodules/infra/api/stats.cmodules/infra/cli/affinity.cmodules/infra/cli/bond.cmodules/infra/cli/iface.cmodules/infra/cli/nexthop.cmodules/infra/cli/port.cmodules/infra/cli/stats.cmodules/infra/control/gr_iface.hmodules/infra/control/gr_nh_control.hmodules/infra/control/gr_worker.hmodules/infra/control/iface.cmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/worker.cmodules/infra/control/worker_test.cmodules/ip/control/gr_ip4_control.hmodules/ip/control/route.cmodules/ip6/control/gr_ip6_control.hmodules/ip6/control/route.cmodules/srv6/cli/localsid.crpm/grout.specsmoke/_init.shsmoke/metrics_test.sh
💤 Files with no reviewable changes (7)
- modules/ip6/control/gr_ip6_control.h
- GNUmakefile
- debian/rules
- modules/ip/control/gr_ip4_control.h
- rpm/grout.spec
- debian/control
- debian/grout-prometheus.install
✅ Files skipped from review due to trivial changes (1)
- modules/infra/cli/bond.c
🚧 Files skipped from review as they are similar to previous changes (20)
- modules/srv6/cli/localsid.c
- modules/infra/api/gr_nexthop.h
- modules/infra/cli/iface.c
- modules/infra/control/worker_test.c
- main/meson.build
- cli/interact.c
- modules/infra/cli/port.c
- modules/infra/control/gr_iface.h
- modules/infra/control/port_test.c
- smoke/_init.sh
- modules/infra/api/gr_infra.h
- modules/infra/cli/nexthop.c
- modules/infra/control/iface.c
- modules/infra/cli/affinity.c
- modules/infra/cli/stats.c
- cli/exec.c
- cli/ecoli.c
- meson.build
- modules/infra/control/gr_nh_control.h
- docs/grout.8.scdoc
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/gr_worker.hmain/main.cmodules/infra/api/iface.cmodules/ip/control/route.capi/gr_net_types.hmodules/infra/api/stats.cmodules/infra/control/nexthop.cmodules/ip6/control/route.cmodules/infra/api/affinity.cmodules/infra/control/port.cmain/gr_metrics.hmain/dpdk.cmodules/infra/control/worker.cmain/gr_config.hcli/ec_node_dyn.cmain/metrics.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/metrics_test.sh
🧠 Learnings (22)
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
main/main.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
main/main.cmodules/infra/api/affinity.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
main/main.c
📚 Learning: 2025-12-17T17:32:21.746Z
Learnt from: christophefontaine
Repo: DPDK/grout PR: 466
File: modules/srv6/datapath/l2_encap.c:26-32
Timestamp: 2025-12-17T17:32:21.746Z
Learning: The GR_MBUF_PRIV_DATA_TYPE macro in modules/infra/datapath/gr_mbuf.h automatically adds `const struct iface *iface` as the first field to any structure defined with it. All types defined using this macro (e.g., srv6_dx2_mbuf_data, mbuf_data, queue_mbuf_data) will have the iface field available, followed by any custom fields provided as the second macro argument.
Applied to files:
modules/infra/api/iface.cmodules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/api/iface.cmodules/ip/control/route.cmodules/infra/control/nexthop.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
modules/infra/api/iface.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/api/iface.cmodules/ip/control/route.cmodules/infra/control/nexthop.csmoke/metrics_test.sh
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
Applied to files:
modules/ip/control/route.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-08-27T15:33:22.299Z
Learnt from: rjarry
Repo: DPDK/grout PR: 305
File: modules/ip6/control/route.c:407-413
Timestamp: 2025-08-27T15:33:22.299Z
Learning: DPDK rte_rib6_get_nxt() with RTE_RIB6_GET_NXT_ALL flag does not yield the default route ::/0 if configured. The explicit rte_rib6_lookup_exact() call for the default route is necessary to ensure complete route enumeration.
Applied to files:
modules/ip/control/route.cmodules/ip6/control/route.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-11-24T12:50:09.337Z
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
Applied to files:
modules/ip/control/route.capi/gr_net_types.h
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip/control/route.capi/gr_net_types.hmodules/ip6/control/route.cmodules/infra/control/worker.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip/control/route.cmodules/infra/control/nexthop.cmodules/ip6/control/route.c
📚 Learning: 2025-09-09T06:02:47.703Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.
Applied to files:
api/gr_net_types.hmodules/ip6/control/route.c
📚 Learning: 2025-09-05T08:58:53.047Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:533-539
Timestamp: 2025-09-05T08:58:53.047Z
Learning: DPDK functions rte_mempool_free() and rte_hash_free() are NULL-safe - they explicitly handle NULL pointers and do nothing when passed NULL, unlike many C standard library functions.
Applied to files:
modules/infra/control/nexthop.cmodules/infra/control/port.c
📚 Learning: 2025-09-09T12:08:26.686Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:458-472
Timestamp: 2025-09-09T12:08:26.686Z
Learning: DPDK's rte_hash_iterate function supports safe deletion during iteration. Unlike many hash table implementations, rte_hash is specifically designed to handle concurrent modifications including deletions via rte_hash_del_key without corrupting the iterator state.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-09-09T12:08:36.790Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/control/conntrack.c:621-637
Timestamp: 2025-09-09T12:08:36.790Z
Learning: DPDK's rte_hash supports safe deletion during iteration via rte_hash_iterate, unlike many other hash table implementations that require two-phase deletion patterns.
Applied to files:
modules/infra/control/nexthop.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 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:
modules/infra/api/affinity.c
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
main/metrics.c
📚 Learning: 2025-09-16T08:04:58.998Z
Learnt from: rjarry
Repo: DPDK/grout PR: 310
File: modules/infra/api/affinity.c:17-17
Timestamp: 2025-09-16T08:04:58.998Z
Learning: In Grout codebase, do not suggest adding parameter names to unnamed parameters in function definitions or suggest void casting for unused parameters. The codebase uses `-std=gnu2x` and C23 unnamed parameters in function definitions are valid and preferred when parameters are intentionally unused.
Applied to files:
main/metrics.c
🧬 Code graph analysis (7)
modules/infra/control/gr_worker.h (1)
modules/infra/control/worker.c (1)
worker_dump_stats(436-499)
main/main.c (2)
api/gr_errno.h (1)
errno_set(10-13)main/metrics.c (2)
gr_metrics_start(205-214)gr_metrics_stop(216-223)
modules/ip/control/route.c (4)
modules/infra/control/nexthop.c (1)
nexthop_origin_valid(306-343)modules/infra/control/worker_test.c (2)
gr_metrics_ctx_init(37-37)gr_metric_emit(39-39)modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/api/stats.c (2)
main/module.c (1)
gr_register_api_handler(19-33)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/control/nexthop.c (4)
api/gr_net_types.h (2)
gr_af_valid(45-53)gr_af_name(32-42)api/gr_errno.h (2)
errno_set_null(16-19)errno_set(10-13)modules/infra/api/gr_nexthop.h (1)
gr_nh_type_name(171-189)main/metrics.c (1)
gr_metrics_register(22-24)
modules/ip6/control/route.c (4)
modules/infra/control/nexthop.c (1)
nexthop_origin_valid(306-343)modules/infra/control/worker_test.c (2)
gr_metrics_ctx_init(37-37)gr_metric_emit(39-39)modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(192-260)main/metrics.c (1)
gr_metrics_register(22-24)
modules/infra/api/affinity.c (1)
main/metrics.c (1)
gr_metrics_set_affinity(142-146)
🪛 Cppcheck (2.19.0)
main/metrics.c
[error] 20-20: syntax error
(syntaxError)
🪛 Shellcheck (0.11.0)
smoke/metrics_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (21)
cli/ec_node_dyn.c (1)
11-11: LGTM!The
<string.h>include is required forstrcmp()used inec_node_dyn_desc()on line 102.main/dpdk.c (1)
140-140: LGTM!Disabling DPDK's built-in telemetry via
--no-telemetryis consistent with the migration to the new OpenMetrics-based metrics framework introduced in this PR.Containerfile.grout (1)
5-9: LGTM!The COPY directive update and cleanup step are consistent with removing the separate prometheus package and follow good container hygiene practices.
api/gr_net_types.h (1)
44-53: LGTM!The
gr_af_valid()function correctly validates address family values and mirrors the style of the existinggr_af_name()function.modules/infra/control/gr_worker.h (1)
81-81: LGTM!The declaration correctly exposes the
worker_dump_stats()function for per-CPU statistics collection, aligning with the metrics infrastructure changes.main/gr_config.h (1)
27-28: LGTM!The new configuration fields for the OpenMetrics exporter are well-documented and appropriately typed.
modules/infra/control/worker.c (1)
425-499: LGTM - Statistics aggregation logic is sound.The
worker_dump_statsfunction correctly aggregates per-worker and per-node statistics. The potential underflow concerns at lines 485-486 and 492 have been previously discussed, and the author has confirmed the architectural guarantee thatsleep_cyclescannot exceedloop_cycles.main/gr_metrics.h (1)
1-65: Clean metrics API design.The header provides a well-structured OpenMetrics framework with proper encapsulation via opaque writer type and clear separation between metric definitions, context management, and collector registration.
modules/infra/api/affinity.c (1)
44-46: Correct integration of metrics affinity.The metrics thread affinity update follows the same error-handling pattern as the existing
control_output_set_affinitycall, and gracefully handles the case when the metrics thread hasn't started yet (returns 0 permain/metrics.cline 143).main/metrics.c (1)
109-223: HTTP handler and thread lifecycle look correct.The metrics handler properly allocates/frees resources, the thread lifecycle is correctly managed with affinity setting, and the start/stop functions handle edge cases appropriately.
main/main.c (1)
315-336: Metrics lifecycle integration is correct.The
gr_metrics_start()andgr_metrics_stop()calls are properly placed in the startup/shutdown sequence, matching the pattern of other subsystems.modules/infra/control/port.c (1)
608-627: LGTM!The metrics collector correctly handles the driver label fallback case (line 616) ensuring label set stability, and conditionally emits stats only when available. The implementation looks solid.
modules/infra/api/iface.c (1)
250-262: LGTM!The collector definition and registration follow the established pattern used by other collectors in this PR.
modules/infra/api/stats.c (2)
264-276: LGTM!The collector now emits all node stats without filtering, correctly addressing the prior concern about skipping zero-packet nodes.
286-308: LGTM!The CPU metrics collector properly guards against NULL worker stats before dereferencing.
modules/infra/control/nexthop.c (3)
292-343: LGTM!The new
nexthop_type_validandnexthop_origin_validvalidators are comprehensive and provide consistent validation across the codebase.
370-401: LGTM!Good defensive checks: NULL validation for base/info parameters and type/origin validation before proceeding with nexthop creation or update.
726-749: LGTM!The metrics collector properly validates types before indexing and emitting, preventing out-of-bounds access.
modules/ip6/control/route.c (3)
170-198: LGTM!Origin validation ensures only valid values are stored, and the assert guards protect against count underflow during replacement.
277-279: LGTM!Assert guard properly protects against underflow when decrementing route counts.
501-525: LGTM!The rib6 metrics collector properly validates origins before using them as array indices.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| static void rib4_metrics_collect(struct gr_metrics_writer *w) { | ||
| struct gr_metrics_ctx ctx; | ||
| char vrf[16]; | ||
|
|
||
| for (uint16_t vrf_id = 0; vrf_id < GR_MAX_VRFS; vrf_id++) { | ||
| const struct rib4_stats *vrf_stats = rib4_get_stats(vrf_id); | ||
|
|
||
| if (vrf_id != 0 && (vrf_stats == NULL || vrf_stats->total_routes == 0)) | ||
| if (vrf_ribs[vrf_id] == NULL) | ||
| continue; | ||
|
|
||
| char vrf_key[32]; | ||
| snprintf(vrf_key, sizeof(vrf_key), "%u", vrf_id); | ||
|
|
||
| struct rte_tel_data *vrf_data = rte_tel_data_alloc(); | ||
| if (vrf_data == NULL) | ||
| continue; | ||
|
|
||
| rte_tel_data_start_dict(vrf_data); | ||
| rte_tel_data_add_dict_uint(vrf_data, "vrf_id", vrf_id); | ||
|
|
||
| struct rte_tel_data *ipv4_data = rte_tel_data_alloc(); | ||
| if (ipv4_data != NULL) { | ||
| rte_tel_data_start_dict(ipv4_data); | ||
| rte_tel_data_add_dict_uint(ipv4_data, "total", vrf_stats->total_routes); | ||
| for (unsigned o = 0; o < ARRAY_DIM(vrf_stats->by_origin); o++) { | ||
| const char *name = gr_nh_origin_name(o); | ||
| uint32_t count = vrf_stats->by_origin[o]; | ||
| if (count > 0 && strcmp(name, "") != 0 && strcmp(name, "?") != 0) | ||
| rte_tel_data_add_dict_uint(ipv4_data, name, count); | ||
| } | ||
| rte_tel_data_add_dict_container(vrf_data, "ipv4", ipv4_data, 1); | ||
| } | ||
| snprintf(vrf, sizeof(vrf), "%u", vrf_id); | ||
|
|
||
| if (rte_tel_data_add_dict_container(d, vrf_key, vrf_data, 0) != 0) { | ||
| rte_tel_data_free(vrf_data); | ||
| continue; | ||
| for (unsigned o = 0; o < UINT_NUM_VALUES(gr_nh_origin_t); o++) { | ||
| if (!nexthop_origin_valid(o)) | ||
| continue; | ||
| gr_metrics_ctx_init( | ||
| &ctx, w, "vrf", vrf, "origin", gr_nh_origin_name(o), NULL | ||
| ); | ||
| gr_metric_emit(&ctx, &m_routes, route_counts[vrf_id][o]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the structure and threading model in main/metrics.c
wc -l main/metrics.c
head -100 main/metrics.cRepository: DPDK/grout
Length of output: 2395
🏁 Script executed:
#!/bin/bash
# Search for threading-related patterns and metrics registration
rg -n "gr_metrics|event_base|thread|evthread|mutex|lock|atomic" main/metrics.c --max-count=50Repository: DPDK/grout
Length of output: 1893
🏁 Script executed:
#!/bin/bash
# Check the route.c file to see if route_counts has any protection
rg -n "route_counts" modules/ip/control/route.cRepository: DPDK/grout
Length of output: 396
🏁 Script executed:
#!/bin/bash
# Look for any mutex or locking around route updates and metrics
rg -n "pthread_mutex|mtx_lock|lock|atomic" modules/ip/control/route.cRepository: DPDK/grout
Length of output: 36
🏁 Script executed:
#!/bin/bash
# Search for how gr_metrics_register is used and when collectors are called
rg -n "gr_metrics_register|gr_metrics_ctx_init|gr_metric_emit" modules/ip/control/route.cRepository: DPDK/grout
Length of output: 189
Unsynchronized access to route_counts between metrics thread and route updates creates a data race.
The metrics collector rib4_metrics_collect runs on a dedicated metrics thread (created by gr_metrics_start() and executing metrics_thread() in main/metrics.c), while route insert/delete operations update route_counts from a different execution context. The unsynchronized reads at line 477 and writes at lines 180-181 and 254-255 are a data race resulting in undefined behavior. Protect route_counts with synchronization (e.g., atomic operations or a mutex shared between the update and collection paths).
Also applies to: 180-181, 254-255
🤖 Prompt for AI Agents
In `@modules/ip/control/route.c` around lines 461 - 478, rib4_metrics_collect
reads shared array route_counts from the metrics thread without synchronization,
causing a data race with route insert/delete updates; protect route_counts with
a synchronization primitive (preferably a mutex) shared between the metrics
collector and the route update code: add a global mutex (e.g.,
route_counts_lock), acquire it in rib4_metrics_collect before reading
route_counts and release after emitting metrics, and also acquire/release it in
the route insert/delete code paths that modify route_counts (the functions that
update route_counts at the existing update sites). Ensure lightweight locking
(hold only while reading/updating route_counts) and use the same lock for all
accesses so gr_metrics_start/metrics_thread and the route update functions
coordinate safely.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Removed Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.