prometheus exporter endpoints #471
Conversation
📝 WalkthroughWalkthroughThis PR replaces legacy rte_telemetry with a new OpenMetrics HTTP exporter and collector framework (main/gr_metrics.h, main/metrics.c) served via libevent. It instruments infra (interfaces, ports, workers, nexthops, RIBs) with collectors and metrics, registers collectors at init, and removes telemetry handlers and telemetry-based stats APIs. Adds CLI/config support (-M, gr_config.metrics_*), starts/stops the metrics thread, adds libevent_extra dependency, and removes Prometheus packaging and telemetry installer files. Several files add string.h includes. 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.
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>
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>
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.
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>
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>
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.
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. |
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>
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>
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>
rjarry
force-pushed
the
prometheus
branch
2 times, most recently
from
d905953 to
cce8ef9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@main/metrics.c`:
- Around line 104-107: The format specifier in gr_metric_emit is non-portable:
change the evbuffer_add_printf call to print value using the PRIu64 macro from
<inttypes.h> instead of "%lu", and include <inttypes.h> where metrics.c includes
headers so PRIu64 is available; update the format string to use "%" PRIu64 and
pass value (or (uint64_t)value) to match the macro. Ensure the rest of the
function (emit_help_type, ctx->w, ctx->labels) remains unchanged.
♻️ Duplicate comments (2)
modules/infra/control/worker.c (1)
436-499: LGTM on the stats aggregation logic.The
worker_dump_statsfunction properly iterates workers, aggregates per-node statistics, and handles idle/overhead metrics. The underflow concerns on lines 485-486 and 489-496 were previously discussed and dismissed by the author based on architecture guarantees.modules/infra/api/iface.c (1)
205-212: Missing NULL check foriface_type_get()result.
iface_type_get()can returnNULL, but the result is used on line 211 without validation. The established pattern in this file (lines 18-19, 84-85, 100-102) is to assert after 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 (31)
Containerfile.groutGNUmakefiledebian/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/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/metrics_test.sh
💤 Files with no reviewable changes (7)
- debian/rules
- debian/grout-prometheus.install
- debian/control
- modules/ip6/control/gr_ip6_control.h
- modules/ip/control/gr_ip4_control.h
- GNUmakefile
- rpm/grout.spec
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/infra/control/worker_test.c
- modules/infra/control/gr_worker.h
- main/meson.build
- main/main.c
- modules/infra/control/gr_iface.h
- main/gr_config.h
- main/dpdk.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.cmodules/ip/control/route.cmodules/infra/control/gr_nh_control.hmain/metrics.cmodules/ip6/control/route.cmodules/infra/control/nexthop.cmodules/infra/control/port.cmodules/infra/api/iface.cmain/gr_metrics.hmodules/infra/api/stats.cmodules/infra/api/affinity.cmodules/infra/control/port_test.c
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/metrics_test.shsmoke/_init.sh
🧠 Learnings (19)
📚 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/worker.cmodules/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 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/gr_nh_control.hmodules/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/infra/control/gr_nh_control.hmodules/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-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-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/ip/control/route.cmodules/ip6/control/route.cmodules/infra/control/nexthop.cmodules/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/ip/control/route.csmoke/metrics_test.shmodules/ip6/control/route.cmodules/infra/control/nexthop.cmodules/infra/api/iface.csmoke/_init.shmodules/infra/control/port_test.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/ip6/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:
meson.buildmodules/infra/api/iface.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/gr_nh_control.hmodules/infra/control/nexthop.cmodules/infra/api/iface.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-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.scdoc
📚 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-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.c
🧬 Code graph analysis (9)
modules/infra/control/worker.c (1)
api/gr_errno.h (1)
errno_set_null(16-19)
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/ip6/control/route.c (5)
modules/ip/control/route.c (1)
count_origin_cb(447-457)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/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/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)
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(201-210)gr_metrics_stop(212-219)gr_metrics_set_affinity(142-146)
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/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/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)
🪛 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-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: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (31)
Containerfile.grout (1)
5-9: LGTM!The removal of the
grout-prometheus.*.rpmpackage from the COPY statement aligns with the PR's shift to the new OpenMetrics framework. The added cleanup step is a good practice for reducing container image size.smoke/_init.sh (1)
12-15: LGTM!Necessary setup for the metrics endpoint to bind to
127.0.0.1:2015within the network namespace. The conditional check before adding the address prevents duplicate-address errors.modules/infra/api/affinity.c (1)
44-46: LGTM!The metrics thread affinity handling follows the same pattern as
control_output_set_affinity, with proper error propagation. The implementation ingr_metrics_set_affinitygracefully handles the case when the metrics thread isn't running.smoke/metrics_test.sh (1)
1-38: LGTM!The test script properly exercises the new metrics infrastructure by setting up a realistic network topology and verifying the metrics endpoint responds. The shellcheck warning about word splitting on line 5 can be ignored per the coding guidelines for shell scripts.
meson.build (1)
77-77: LGTM!The
libevent_extradependency is required for the HTTP server functionality in the new metrics framework. Properly added to both the dependency declaration and the grout executable build.Also applies to: 116-116
modules/infra/control/worker.c (1)
425-434: LGTM!The
find_statfunction correctly useserrno_set_null(from the relevant code snippets) for error handling when no matching stat is found.modules/ip6/control/route.c (2)
484-525: LGTM on the metrics collection implementation.The
rib6_metrics_collectfunction correctly iterates VRFs, counts routes by origin using the callback pattern consistent with the IPv4 route module, and emits metrics with propervrfandoriginlabels.
596-596: LGTM on collector registration.The
rib6_collectoris properly registered during module initialization.main/gr_metrics.h (1)
1-65: LGTM on the metrics API design.The header provides a clean and well-documented OpenMetrics framework interface with appropriate types, macros, and function declarations.
modules/infra/control/port_test.c (1)
46-48: LGTM on the metrics stubs.The stub implementations correctly match the signatures in
worker_test.cand provide no-op behavior for tests that don't require actual metrics functionality.main/metrics.c (1)
1-219: LGTM on overall metrics implementation.The OpenMetrics HTTP exporter is well-structured with proper error handling, resource cleanup, and lifecycle management. The static analysis hint about a syntax error on line 20 is a false positive -
STAILQ_HEAD_INITIALIZERis a valid BSD queue macro.docs/grout.8.scdoc (1)
1-91: LGTM!Documentation accurately reflects the new metrics endpoint configuration and EAL arguments feature.
modules/ip/control/route.c (3)
445-481: LGTM!The metrics collection logic correctly iterates VRFs, aggregates per-origin route counts, and emits via the Prometheus writer. The bounds check via
nexthop_origin_valid(o)in the emit loop provides appropriate validation.
483-486: LGTM!Collector struct properly defined with name and callback.
550-557: LGTM!Metrics collector registration correctly added to module initialization.
modules/infra/control/port.c (2)
601-627: LGTM!Metrics collection properly handles potential failures from
rte_eth_dev_info_get(fallback to "?") and conditionally emits stats only whenrte_eth_stats_getsucceeds. This addresses the label stability concern from previous reviews.
780-797: LGTM!The
metrics_collectcallback is correctly wired into the interface type structure.modules/infra/api/iface.c (2)
219-233: LGTM!Per-core stats aggregation loop is correctly bounded by
RTE_MAX_LCOREand accumulates values safely.
244-247: LGTM!Type-specific metrics collector dispatch properly checks for NULL before calling.
modules/infra/api/stats.c (4)
42-45: LGTM!The transition to
worker_dump_stats()correctly handles thecpu_idparameter, returning NULL for invalid CPU IDs.
260-283: LGTM!Graph metrics collection correctly aggregates stats across all workers and properly frees the vector after use.
285-310: LGTM!CPU metrics collection properly checks for NULL stats before dereferencing and correctly calculates idle cycles.
312-318: LGTM!Both collectors are properly registered during initialization.
modules/infra/control/gr_nh_control.h (1)
90-95: LGTM!New validator declarations properly exposed for type and origin validation.
modules/infra/control/nexthop.c (7)
292-304: LGTM!Type validator correctly covers all defined nexthop types.
306-343: LGTM!Origin validator comprehensively covers all defined origin values.
345-363: LGTM!Registration functions properly validate inputs and fail fast on programmer errors.
370-371: LGTM!Defensive NULL checks prevent potential null pointer dereference.
398-403: LGTM!Validation correctly happens before any state modifications, preserving nexthop integrity on invalid input.
724-749: LGTM!Metrics collection properly counts nexthops by type and filters invalid types before emission.
1053-1059: LGTM!Metrics collector properly registered alongside event serializer and module.
✏️ 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: 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.
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>
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>
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>
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>
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>
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>
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>
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>
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.
| 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; | ||
| 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) | ||
| SAFE_BUF(snprintf, len, ","); | ||
|
|
||
| SAFE_BUF(snprintf, len, "%s=\"%s\"", key, val ?: ""); | ||
| } | ||
|
|
||
| ctx->labels_len += n; | ||
|
|
||
| return; | ||
| err: | ||
| LOG(ERR, "snprintf: %s", strerror(errno)); | ||
| } |
There was a problem hiding this comment.
ctx->labels_len is never updated with actual bytes written.
Variable n is initialized to 0 at line 66 and never modified within the loop. At line 77, ctx->labels_len += n adds zero, leaving labels_len unchanged. Subsequent calls to gr_metrics_labels_add will overwrite previously written labels since the offset isn't tracked.
Proposed fix
The SAFE_BUF macro likely updates buf but n needs to track the total bytes written. Assuming SAFE_BUF returns or updates a count, you need to accumulate it:
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;
const char *key, *val;
- size_t n = 0;
+ int written;
+ size_t total = 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)
- SAFE_BUF(snprintf, len, ",");
+ if (total > 0 || ctx->labels_len > 0) {
+ written = snprintf(buf, len - total, ",");
+ if (written < 0 || (size_t)written >= len - total)
+ goto err;
+ buf += written;
+ total += written;
+ }
- SAFE_BUF(snprintf, len, "%s=\"%s\"", key, val ?: "");
+ written = snprintf(buf, len - total, "%s=\"%s\"", key, val ?: "");
+ if (written < 0 || (size_t)written >= len - total)
+ goto err;
+ buf += written;
+ total += written;
}
- ctx->labels_len += n;
+ ctx->labels_len += total;
return;
err:
LOG(ERR, "snprintf: %s", strerror(errno));
}Alternatively, if SAFE_BUF is intended to track bytes via n, ensure the macro updates n correctly.
🤖 Prompt for AI Agents
In `@main/metrics.c` around lines 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.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Removed Features
✏️ Tip: You can customize this high-level summary in your review settings.