cli ctx rework #329
cli ctx rework #329
Conversation
📝 WalkthroughWalkthroughReworks the CLI from verb-first to noun-first commands (e.g., address, route, interface, nexthop, stats, trace, conntrack, dnat44, snat44), renames many public CLI symbols from gr_cli_* → cli_*, and renames STAILQ linkage fields from entries → next across multiple headers/modules. Introduces L3 operation registries (struct cli_route_ops, struct cli_addr_ops) and registration APIs, and changes list handlers to accept vrf/iface + libscols_table. Adds a trace boolean to exec APIs and updates completion to use ec_node_sh_lex_expand. Tightens an IP-net regex anchor, bumps the ecoli dependency, updates Makefile/meson targets and docs/README, and adjusts many smoke tests and init scripts to the new CLI grammar. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
smoke/_init_frr.sh (1)
53-60: Fix address verification loop
grcli address show iface ${p}now prints the VRF column first, so lines look like0 tap0 198.51.100.1/24. The pattern^${p}[[:space:]]\+${ip_cidr}$can never match, the loop spins and eventually aborts even though FRR/grcli already programmed the address. Anchor on the VRF + IFACE (or drop the^${p}) so the grep actually detects the configured address.- local grep_pattern="^${p}[[:space:]]\+${ip_cidr}$" + local grep_pattern="^[[:digit:]]\+[[:space:]]\+${p}[[:space:]]\+${ip_cidr}$"smoke/nexthop_ageing_test.sh (1)
56-59: Update the heredoc commands to the new CLI order.These still use the legacy
show …form. With the verb-first swap, they now fail, so the smoke test breaks. Switch to the new noun-first order.grcli <<EOF -show nexthop -show ip address +nexthop show +address show EOFmodules/ip6/cli/address.c (1)
59-68: Restore iface filter when listing IPv6 addressesThe updated loop no longer skips entries whose
addr->iface_iddiffers from the requestediface_id, sogrcli address iface X shownow dumps every interface’s addresses. The IPv4 implementation still enforces this check (modules/ip/cli/address.c), and we need the same guard here to keep per-interface listings accurate.gr_api_client_stream_foreach (addr, ret, c, GR_IP6_ADDR_LIST, sizeof(req), &req) { + if (iface_id != GR_IFACE_ID_UNDEF && addr->iface_id != iface_id) + continue; struct libscols_line *line = scols_table_new_line(table, NULL);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(4 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/api.c(1 hunks) -
main/gr_module.h(2 hunks) -
main/grout.init(1 hunks) -
main/module.c(4 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(3 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(5 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks) -
subprojects/ecoli.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/ip/cli/ip.h
- modules/ip6/cli/ip.h
- modules/policy/cli/policy.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
PR: DPDK/grout#294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/infra/cli/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
PR: DPDK/grout#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/cli/meson.build
🧬 Code graph analysis (41)
smoke/vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/ip/cli/icmp.c (2)
modules/infra/cli/nexthop.c (1)
constructor(485-491)cli/exec.c (1)
cli_context_register(16-18)
main/api.c (1)
main/gr_module.h (1)
api_out(20-23)
modules/infra/cli/gr_cli_event.h (1)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/trace.c (1)
cli/exec.c (1)
cli_context_register(16-18)
cli/log.h (1)
cli/log.c (4)
tty_init(16-20)is_tty(22-30)need_quote(50-63)trace_cmd(65-78)
modules/infra/cli/affinity.c (3)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/api/affinity.c (1)
rxq_list(62-79)cli/exec.c (1)
cli_context_register(16-18)
smoke/cross_vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/infra/cli/graph.c (3)
modules/infra/cli/events.c (1)
ctx_init(55-62)cli/quit.c (1)
ctx_init(11-13)cli/exec.c (1)
cli_context_register(16-18)
smoke/graph_svg_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/ipip/cli.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/srv6/cli/route.c (2)
modules/infra/cli/nexthop.c (2)
ctx_init(346-443)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/trace.c (1)
constructor(187-189)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/stats.c (2)
cli/ecoli.c (1)
arg_str(99-109)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (2)
arg_ip_net(199-212)arg_str(99-109)modules/infra/cli/iface.c (3)
iface_from_name(92-105)complete_iface_names(70-90)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip6/cli/router_advert.c (3)
modules/infra/cli/iface.c (1)
complete_iface_names(70-90)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/dnat44.c (3)
modules/infra/cli/nexthop.c (2)
ctx_init(346-443)cli_nexthop_formatter_register(22-27)modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)cli/exec.c (1)
cli_context_register(16-18)
modules/srv6/cli/localsid.c (2)
modules/infra/cli/nexthop.c (3)
ctx_init(346-443)constructor(485-491)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/port.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/route.c (5)
cli/ecoli.c (3)
arg_ip_net(199-212)arg_ip(180-189)with_help(15-24)cli/gr_cli.h (1)
arg_u16(64-70)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/icmp6.c (2)
modules/infra/cli/address.c (1)
ctx_init(90-133)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
modules/infra/cli/vlan.c (4)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)modules/infra/cli/port.c (2)
ctx_init(188-218)constructor(225-228)modules/ipip/cli.c (2)
ctx_init(127-156)constructor(163-166)cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/policy/cli/snat44.c (4)
modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)modules/policy/cli/dnat44.c (1)
ctx_init(102-137)modules/policy/api/snat44.c (1)
snat44_list(41-52)cli/exec.c (1)
cli_context_register(16-18)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
cli/quit.c (2)
modules/infra/cli/nexthop.c (1)
constructor(485-491)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (4)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip6/cli/address.c (3)
addr_list(54-71)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip6/cli/route.c (6)
modules/ip6/control/route.c (4)
route6_list(454-461)route6_add(286-339)route6_del(341-358)route6_get(360-375)cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/main.c (3)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)cli/log.c (1)
is_tty(22-30)cli/interact.c (1)
interact(36-114)
modules/infra/cli/gr_cli_l3.h (2)
modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/address.c (1)
cli_addr_ops_register(19-28)
smoke/affinity_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/infra/cli/events.c (3)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/iface.c (1)
constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/route.c (4)
modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/nexthop.c (3)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
🪛 Shellcheck (0.11.0)
smoke/graph_svg_test.sh
[warning] 17-17: tmp is referenced but not assigned.
(SC2154)
smoke/nexthop_ageing_test.sh
[error] 66-66: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 67-67: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 81-81: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 82-82: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ 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-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-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-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (53)
api/gr_net_types.h (1)
140-140: LGTM! Regex anchor fix.The anchor now correctly applies to the entire alternation, preventing partial matches on the IPv4 branch. This aligns with the pattern used in
IP_ANY_REat line 139.smoke/vrf_forward_test.sh (1)
12-23: CLI command updates look consistent.Syntax now matches the verb-first CLI and keeps the same VRF wiring, so the smoke test should still exercise the expected paths.
cli/complete.c (5)
17-38: LGTM!The new
bash_complete_nodefunction correctly constructs the completion tree with proper nesting and includes the new trace-commands flag mentioned in the PR objectives.
65-65: LGTM!Proper NULL initialization for cleanup safety.
87-91: LGTM!Correct error handling for the new function call.
100-100: LGTM!Simplified completion call using buf directly.
117-117: LGTM!Correct extraction of the completion word from the unified vec.
smoke/cross_vrf_forward_frr_test.sh (1)
26-27: Update aligns with new signature.
set_ip_addressnow only needs iface and prefix; these call sites are in sync with the helper change.main/api.c (1)
218-218: Approve error code change. EBADMSG more precisely signals a version‐mismatch and no client code explicitly checks for EPROTO in the handshake.subprojects/ecoli.wrap (1)
3-3: LGTM! Dependency version bump.The libecoli revision bump to v0.8.0 aligns with the PR objectives and supports the CLI refactor.
modules/ip/cli/icmp.c (1)
207-213: LGTM! Context type and registration updated.The changes from
gr_cli_contexttocli_contextandregister_contexttocli_context_registerare consistent with the broader CLI refactor.cli/quit.c (1)
15-21: LGTM! Context refactor applied.Consistent with the CLI context API updates across the codebase.
GNUmakefile (1)
39-40: LGTM! CLI syntax updated.The command reordering (
interface addandgraph show) aligns with the new verb-noun CLI structure.modules/ip6/cli/icmp6.c (2)
210-227: LGTM! Interface alias added.The addition of
interface|ifacesyntax allows both forms interchangeably, consistent with the PR objectives and patterns used elsewhere (e.g.,modules/infra/cli/address.c).
241-247: LGTM! Context refactor applied.The context type and registration function updates are consistent with the broader CLI refactor.
smoke/vlan_forward_test.sh (1)
12-17: LGTM! Test updated for new CLI syntax.The command invocations now use the updated verb-noun structure (
interface add,address add) consistent with the CLI refactor.smoke/graph_svg_test.sh (1)
12-18: LGTM! Test updated for new CLI syntax.The command invocations now use
interface addandgraph show brief, consistent with the CLI refactor.smoke/affinity_test.sh (1)
8-28: LGTM! Test updated for new affinity CLI syntax.All command invocations now use the updated structure (
affinity cpus set,affinity qmap set,interface add, etc.) consistent with the CLI refactor.docs/grout-frr.7.md (1)
123-140: Documentation aligns with CLI refactor.The updated command syntax (
route showinstead ofshow ip route) and richer output format match the broader CLI restructuring in this PR.main/module.c (2)
26-26: STAILQ field rename applied consistently.The change from
entriestonextaligns with the project-wide STAILQ field standardization mentioned in the PR objectives.Also applies to: 32-32, 38-38, 54-54, 59-59, 139-139, 161-161
43-43: Explicit return improves clarity.Making the NULL return path explicit when no matching handler is found is a minor readability improvement.
modules/infra/cli/gr_cli_event.h (1)
9-16: Public API migration applied consistently.The renaming of
gr_cli_event_printertocli_event_printer, the STAILQ field fromentriestonext, and the registration function tocli_event_printer_registerall align with the project-wide CLI refactor.smoke/_init.sh (3)
4-4: Robust error handling with pipefail.Adding
-o pipefailensures that pipeline failures are caught, improving test reliability.
24-34: Cleanup commands updated to new CLI syntax.The migration from
showtointerface showand deletion tointerface delaligns with the CLI refactor.
96-100: Test infrastructure updated to new CLI syntax.Banner generation, trace initialization, and event monitoring commands now use the updated CLI subcommand structure.
Also applies to: 114-114, 118-118
modules/ip6/cli/router_advert.c (4)
87-88: Context macro follows refactor pattern.The
RA_CTXmacro wrapping aligns with the new CLI context definition approach seen across the codebase.
93-100: Set command updated with enhanced help text.The command registration now uses the
RA_CTXmacro and includes help text for interval and lifetime parameters.
114-122: Show subcommand with flexible syntax.The optional
showkeyword and acceptance of bothinterfaceandifacealign with the PR objectives to simplify CLI usage and support aliases.
127-133: Context type migrated to new API.The change from
gr_cli_contexttocli_contextand the updated registration function align with the project-wide CLI refactor.modules/infra/cli/trace.c (3)
110-111: Context macros for trace and logging.The separate
TRACE_CTXandLOGGING_CTXmacros follow the established CLI context pattern and provide clear separation of concerns.
117-175: Trace and logging commands updated consistently.All commands now use the appropriate context macros, support
interface/ifacealiases, and maintain clear help text. The optionalcountparameter in the show command improves usability.
182-188: Context migrated to new registration API.The change from
gr_cli_contexttocli_contextand updated registration function maintain consistency with the broader refactor.cli/exec.h (2)
10-10: Include added for bool type.The
stdbool.hinclude is necessary for theboolparameters added to the execution functions.
27-36: Execution API extended with tracing support.The addition of the
bool traceparameter to bothexec_argsandexec_lineenables command execution tracing, aligning with the PR's CLI enhancement objectives.cli/ecoli.c (3)
26-34: Callback parameter changed from pointer to value.The signature change from
cmd_cb_t *cbtocmd_cb_t cbsimplifies the callback interface. The implementation at line 30 correctly passescbdirectly toec_dict_set.
67-67: Context node construction updated.The change from
EC_NODE_STRtoEC_NODE_CMDfor context names likely provides improved command-specific behavior in the CLI tree structure.
180-180: IP parsing helpers promoted to public API.Making
arg_ipandarg_ip_netpublic allows other CLI modules to reuse these common IP parsing utilities, reducing code duplication across the codebase.Also applies to: 199-199
smoke/ip6_builtin_icmp_test.sh (1)
10-13: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor. Functional semantics preserved.
smoke/ip6_forward_test.sh (1)
10-16: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (interface/address/route/nexthop). Functional semantics preserved.
smoke/ipip_encap_test.sh (1)
11-16: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (interface/address). Functional semantics preserved.
smoke/ip6_add_del_test.sh (1)
9-18: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (interface/address). Functional semantics preserved.
modules/infra/cli/vlan.c (4)
162-162: Context macro updated consistently.Usage of INTERFACE_ADD_CTX aligns with the standardized interface context helpers used in other modules (iface.c, port.c, ipip/cli.c).
177-177: Context macro updated consistently.Usage of INTERFACE_SET_CTX aligns with the standardized interface context helpers used in other modules (iface.c, port.c, ipip/cli.c).
199-199: Type updated consistently.Renamed from
gr_cli_contexttocli_context, matching the standardized context type used across CLI modules.
205-205: Registration function updated consistently.Changed from
register_contexttocli_context_register, matching the standardized registration function used in other CLI modules (iface.c, port.c, ipip/cli.c).smoke/ip_forward_test.sh (1)
10-16: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (interface/address/route/nexthop). Functional semantics preserved.
smoke/srv6_test.sh (3)
11-14: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (interface/address). Functional semantics preserved.
51-54: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (nexthop/route). Functional semantics preserved.
66-67: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (nexthop/route). Functional semantics preserved.
smoke/dnat44_test.sh (2)
10-16: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (interface/address/dnat44/nexthop). Functional semantics preserved.
33-34: CLI syntax updated consistently.Command surface changes align with the unified CLI context refactor (dnat44/nexthop show). Functional semantics preserved.
modules/ip/cli/address.c (1)
54-73: List handler fits shared table contractThe refactored list hook cleanly appends to the shared table while preserving interface/name resolution—looks good.
modules/infra/cli/nexthop.c (1)
333-335: Nice reuse of the formatterLeaning on
cli_nexthop_formatkeeps list output aligned with event printing. Solid refactor.
rjarry
force-pushed
the
cli-ctx-rework
branch
3 times, most recently
from
3de4c86 to
27365cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/ecoli.c (2)
26-35: Replace void storage of callback with heap‐allocated wrapper*
- In with_callback (cli/ecoli.c:26–35), allocate a
cmd_cb_tslot (malloc(sizeof *slot)), store*slot = cb, and callec_dict_set(attrs, CALLBACK_ATTR, slot, free)instead of passing the function pointer directly.- In cli/exec.c:45–47, change
cmd_cb_t cb = ec_dict_get(attrs, CALLBACK_ATTR);to retrieve acmd_cb_t *slot = ec_dict_get(attrs, CALLBACK_ATTR);then docmd_cb_t cb = slot ? *slot : NULL;.
180-189: Add#include <arpa/inet.h>to cli/ecoli.c to declareinet_ptonand avoid implicit‐declaration warnings.modules/srv6/cli/route.c (1)
11-12: Add missing header for route ctx macros.
NEXTHOP_ADD_CTXlives in<gr_cli_l3.h>; without that include this file won’t compile. Please add the header alongside the other CLI includes.#include <gr_cli_nexthop.h> +#include <gr_cli_l3.h> #include <gr_net_types.h>
♻️ Duplicate comments (3)
cli/main.c (1)
192-195: Restore EXIT handling in non-interactive loop.
EXEC_CMD_EXITnow yields 0 fromcheck_status, so the batch reader plows ahead and scripts cannot stop themselves. Break before checking status.- status = exec_line(client, cmdlist, buf, opts.trace_commands); - if (check_status(status) < 0 && opts.err_exit) + status = exec_line(client, cmdlist, buf, opts.trace_commands); + if (status == EXEC_CMD_EXIT) + break; + if (check_status(status) < 0 && opts.err_exit) goto end;cli/log.h (1)
6-6:<stdbool.h>include still missing after swapping headers.The previous review flagged that
boolis used but may not be reliably defined after replacingexec.hwithecoli.h. Add#include <stdbool.h>directly to ensure the type is always available.cli/exec.c (1)
53-116: Set errno when no suggestions, and drop unused variableWhen no suggestions are found, return NULL with errno=ENOENT to simplify callers; also remove the unused attrs variable.
static struct ec_strvec * get_suggestions(const struct ec_node *cmdlist, const char *cmdline, unsigned *pos) { - struct ec_strvec *args = NULL; - struct ec_strvec *sug = NULL; - struct ec_dict *attrs = NULL; + struct ec_strvec *args = NULL; + struct ec_strvec *sug = NULL; struct ec_comp *c = NULL; @@ out: ec_comp_free(c); ec_strvec_free(args); - ec_dict_free(attrs); + if (sug == NULL && errno == 0) + errno = ENOENT; return sug; }
🧹 Nitpick comments (6)
modules/infra/cli/gr_cli_nexthop.h (1)
11-16: Header hygiene: add ssize_t and forward decl for clarity.To make this header self-sufficient and avoid relying on incidental includes:
- Include sys/types.h for ssize_t.
- Optionally forward-declare struct gr_api_client to decouple from gr_api.h.
Apply outside-this-hunk:
#pragma once @@ #include <gr_nexthop.h> -#include <stdio.h> +#include <stdio.h> +#include <sys/types.h> /* ssize_t */ #include <sys/queue.h> + +struct gr_api_client; /* forward decl */modules/ip6/cli/route.c (1)
51-66: route6_list refactor LGTM; minor IP6_F param consistency.Consider using &route->dest.ip (as done elsewhere in this file) for clarity:
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &route->dest, route->dest.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &route->dest.ip, route->dest.prefixlen);modules/infra/cli/events.c (1)
14-16: Printer registration OKTail insert on printers with next linkage is fine. Consider asserting p->print and p->ev_count > 0 for early validation.
cli/exec.c (1)
117-166: Minor: print_suggestions formatting nits
- fprintf width for %.*s expects int; cast pos to (int) to avoid sign/width warnings.
- SAFE_BUF appears to rely on a variable named n; you defined size_t n. If SAFE_BUF expects ssize_t, consider using ssize_t to avoid signed/unsigned mix.
- fprintf(stderr, "%s* %.*s%s", YELLOW_SGR, pos, buf, RESET_SGR); + fprintf(stderr, "%s* %.*s%s", YELLOW_SGR, (int)pos, buf, RESET_SGR); - fprintf(stderr, "%s%s%s\n", BOLD_RED_SGR, buf + pos, RESET_SGR); + fprintf(stderr, "%s%s%s\n", BOLD_RED_SGR, buf + (int)pos, RESET_SGR); - fprintf(stderr, "* %*s^\n", pos, ""); + fprintf(stderr, "* %*s^\n", (int)pos, "");modules/ip/cli/address.c (1)
54-73: addr_list refactor OK; optional redundant filterThe iface_id filter mirrors server-side filtering; you can drop the client-side check for tiny savings, but it’s fine as-is.
modules/infra/cli/nexthop.c (1)
20-27: Formatter registry OK; consider duplicate guardTail insert with basic asserts is fine. Optionally check for duplicate .type or .name to prevent collisions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(4 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/api.c(1 hunks) -
main/gr_module.h(2 hunks) -
main/grout.init(1 hunks) -
main/module.c(4 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(3 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(5 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks) -
subprojects/ecoli.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/ip/cli/ip.h
- modules/policy/cli/policy.h
- modules/ip6/cli/ip.h
🚧 Files skipped from review as they are similar to previous changes (24)
- smoke/ip_forward_test.sh
- meson.build
- smoke/ip6_add_del_test.sh
- smoke/srv6_test.sh
- smoke/affinity_test.sh
- smoke/graph_svg_test.sh
- smoke/vrf_forward_frr_test.sh
- modules/infra/cli/route.c
- GNUmakefile
- cli/interact.c
- smoke/ip_builtin_icmp_test.sh
- modules/infra/cli/trace.c
- modules/ip6/cli/icmp6.c
- modules/ipip/cli.c
- modules/infra/cli/address.c
- smoke/ip6_same_peer_test.sh
- api/gr_net_types.h
- modules/ip/cli/icmp.c
- modules/infra/cli/graph.c
- modules/infra/cli/gr_cli_l3.h
- smoke/ip6_forward_test.sh
- smoke/cross_vrf_forward_test.sh
- main/gr_module.h
- smoke/snat44_test.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
PR: DPDK/grout#294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/infra/cli/meson.build
🧬 Code graph analysis (29)
modules/infra/cli/stats.c (2)
cli/ecoli.c (1)
arg_str(99-109)cli/exec.c (1)
cli_context_register(16-18)
main/api.c (1)
main/gr_module.h (1)
api_out(20-23)
cli/log.h (1)
cli/log.c (4)
tty_init(16-20)is_tty(22-30)need_quote(50-63)trace_cmd(65-78)
modules/infra/cli/affinity.c (3)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/trace.c (1)
constructor(187-189)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
modules/srv6/cli/localsid.c (3)
modules/infra/cli/nexthop.c (3)
ctx_init(346-443)constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/route.c (2)
ctx_init(136-181)constructor(188-191)cli/exec.c (1)
cli_context_register(16-18)
cli/main.c (3)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)cli/log.c (1)
is_tty(22-30)cli/interact.c (1)
interact(36-114)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
smoke/cross_vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/quit.c (1)
cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/events.c (7)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/iface.c (1)
constructor(589-592)modules/ip/cli/address.c (1)
constructor(113-116)modules/ip6/cli/address.c (1)
constructor(111-114)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/dnat44.c (3)
modules/infra/cli/nexthop.c (2)
ctx_init(346-443)cli_nexthop_formatter_register(22-27)modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/router_advert.c (4)
modules/infra/cli/iface.c (1)
complete_iface_names(70-90)cli/ecoli.c (1)
with_help(15-24)cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (5)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip6/cli/address.c (3)
addr_list(54-71)addr_add(18-34)addr_del(36-52)modules/ip/control/address.c (3)
addr_list(149-175)addr_add(63-115)addr_del(117-147)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
modules/policy/cli/snat44.c (4)
modules/infra/cli/address.c (2)
ctx_init(90-133)constructor(140-142)modules/ip6/cli/icmp6.c (2)
ctx_init(203-239)constructor(246-248)modules/policy/cli/dnat44.c (2)
ctx_init(102-137)constructor(144-147)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/nexthop.c (3)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/route.c (4)
modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
modules/infra/cli/gr_cli_event.h (1)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip/cli/route.c (5)
modules/ip/control/route.c (4)
route4_list(416-423)route4_add(262-306)route4_del(308-321)route4_get(323-338)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/nexthop.c (1)
ctx_init(346-443)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/port.c (2)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/infra/cli/vlan.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 66-66: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 67-67: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 81-81: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 82-82: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ 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-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (81)
main/grout.init (4)
8-8: LGTM!The updated affinity command syntax follows the new verb-noun pattern consistently.
11-12: LGTM!The interface creation commands correctly follow the new
interface add portsyntax.
15-16: LGTM!The address assignment commands correctly use the new
address addsyntax.
19-19: LGTM!The route command correctly follows the new
route addsyntax.subprojects/ecoli.wrap (1)
3-3: Approve version bump to v0.8.0
v0.8.0 tag exists in libecoli; wrap file update is valid.modules/infra/cli/meson.build (1)
5-5: Approve additions of address.c and route.c
Both files exist in modules/infra/cli and align with the CLI refactor objectives.main/module.c (4)
26-32: LGTM: Mechanical field rename.The STAILQ macro calls correctly reference the new
nextfield name. Logic unchanged.
38-38: LGTM: Mechanical field rename.The STAILQ_FOREACH macro correctly uses the new
nextfield name.
54-59: LGTM: Mechanical field rename.Both STAILQ macros correctly reference the new
nextfield name. Logic unchanged.
139-161: Field rename verified. All STAILQ macros now usenext; no residual references toentrieswere found.main/api.c (1)
218-218: LGTM: Better error semantics.
EBADMSGmore accurately describes a version mismatch thanEPROTO.smoke/vrf_forward_test.sh (1)
12-23: LGTM: CLI syntax updated consistently.Commands updated to new verb-noun surface (
interface add,address add,route add). Test logic unchanged.smoke/vlan_forward_test.sh (1)
12-17: LGTM: Consistent CLI modernization.Test updated to new command syntax. Functionally equivalent.
modules/infra/cli/vlan.c (2)
162-177: LGTM: Context macros standardized.Updated to use
INTERFACE_ADD_CTXandINTERFACE_SET_CTX, consistent with the broader context refactor.
199-205: LGTM: Type and registration updated.
gr_cli_context→cli_contextandregister_context→cli_context_registeralign with the PR's unified API surface.smoke/ipip_encap_test.sh (1)
11-16: LGTM: CLI commands updated.Commands aligned to new syntax. Test setup expanded appropriately.
README.md (3)
102-108: LGTM: Startup examples updated.Configuration examples reflect new CLI syntax (
interface add,address add,route add).
152-227: LGTM: CLI reference updated comprehensively.Help text and command taxonomy accurately document the reworked CLI surface.
251-251: LGTM: Graph command updated.
grcli graph briefaligns with new verb-noun structure.smoke/_init_frr.sh (5)
14-14: LGTM: Interface creation updated.Command syntax aligned with new CLI surface.
53-53: LGTM: Address verification updated.Uses
grcli address show ifaceinstead of family-specific commands.
116-116: LGTM: Route verification updated.Switched to
grcli route show vrffor consistent verification.
166-170: LGTM: Verification pattern updated.Comments and pattern reflect new
grcli route showoutput format.
222-233: LGTM: SRv6 route verification updated.Pattern adjusted for new CLI output. Variable cleanup (gr_ip removal) improves clarity.
cli/exec.h (2)
10-10: LGTM: Include for bool type.Required for new
bool traceparameters.
27-36: LGTM: Tracing support added.Both
exec_argsandexec_linenow accept abool traceparameter for command execution tracing. Signatures match implementations in cli/exec.c.smoke/dnat44_test.sh (2)
10-16: LGTM! Test updated to new CLI syntax.Commands correctly migrated to verb-noun format (
grcli dnat44 add,grcli address add).
33-34: LGTM! Show commands updated with new syntax.
type dnatqualifier correctly added to filter nexthop output.smoke/nexthop_ageing_test.sh (4)
12-12: LGTM! Nexthop show command updated.Correctly uses new syntax
grcli nexthop show internal.
31-35: LGTM! Config and interface setup updated.Commands correctly migrated to new verb-noun syntax.
52-52: LGTM! Show command updated.
66-67: LGTM! Address verification updated.
grcli address showcorrectly replaces old syntax. Static analysis warnings are false positives—$p0and$p1are simple variables, not arrays.Also applies to: 81-82
cli/complete.c (3)
17-38: LGTM! Completion node structure refactored.Clearer hierarchy: options wrapped in
EC_NODE_SUBSETwithinEC_NODE_SEQ. Renamingadd_flags→bash_complete_nodeimproves clarity.
65-103: LGTM! Simplified to single-vector approach.Removed
vec_cmdand associated duplication. Usingec_complete(cmdlist, buf)directly is cleaner.
117-121: LGTM! comp_word derived from unified vec.Consistent with single-vector refactor.
modules/infra/cli/stats.c (3)
39-42: LGTM! Flag logic inverted.Now checks hardware first, defaults to software. Behavior preserved.
113-131: LGTM! Stats context consolidated.
STATS_CTX(root)macro unifies context. Commands correctly registered under it.
138-144: LGTM! Context type and registration updated.Correctly migrated to
cli_contextandcli_context_register.modules/ip6/cli/router_advert.c (3)
87-87: LGTM! RA_CTX macro introduced.Follows established pattern for context definition.
93-122: LGTM! Commands wired to RA_CTX.Set, clear, and show commands correctly registered. New show command includes optional interface filter.
127-133: LGTM! Context migration complete.Type and registration updated to
cli_contextandcli_context_register.cli/log.c (2)
50-63: LGTM! need_quote now public.Matches header export. Logic unchanged.
65-78: LGTM! trace_cmd refactored to use ec_strvec.Eliminates local allocation and operates directly on provided vector. Cleaner and more efficient.
modules/policy/cli/snat44.c (3)
87-87: LGTM! SNAT_CTX macro introduced.Mirrors DNAT44 approach for consistency.
93-126: LGTM! Commands migrated to SNAT_CTX.Add, del, and show commands correctly registered under new context.
133-139: LGTM! Context type and registration updated.Correctly uses
cli_contextandcli_context_register.modules/infra/cli/gr_cli_iface.h (1)
48-54: Interface context helpers look good and align with aliasing "interface|iface".Macro names, help strings, and usage pattern are consistent with the new CLI context style.
modules/infra/cli/affinity.c (4)
111-114: Nice consolidation of affinity subcontexts.AFFINITY_ARG/CPU_CTX/QMAP_CTX read well and reduce repetition.
119-121: Optional show and verb reordering are consistent.CPU affinity set/show wiring matches the PR objectives (“allow omitting the show arguments”).
Also applies to: 134-134
138-148: QMAP set/show wiring LGTM.Good help text and completion for IFACE via complete_iface_names.
Also applies to: 151-151
158-158: Context type/registration rename is correct.cli_context + cli_context_register are used as intended.
Also applies to: 164-164
modules/srv6/cli/localsid.c (4)
124-128: Formatter type rename is correct.Matches cli_nexthop_formatter_register API.
200-207: NEXTHOP_ADD_CTX usage is consistent with new macros.Command grammar and help look right.
214-217: Context type rename LGTM.No behavior change; improves consistency.
220-222: Registration path updated properly.Context + formatter registrations align with infra/nexthop pattern.
modules/infra/cli/gr_cli_nexthop.h (2)
18-27: API renames to cli_ look correct.*cli_nexthop_formatter_register and cli_nexthop_format signatures align with nexthop.c implementation.
28-32: Context macros for nexthop LGTM.Alias “nexthop|nh” matches the PR goal; ADD helper reads well.
cli/ecoli.c (2)
199-212: arg_ip_net exposure looks good; same inet_pton/include caveat applies.Parsing dispatch for AF_INET/AF_INET6 is clear.
Please run the arpa/inet.h check above to ensure prototypes are visible.
67-67: Completion and help behavior verified: dynamichelpwithEC_NODE_CMD(...)is consistent and unique to this context; tab-completion andshowplumbing remain unchanged.modules/ip6/cli/route.c (5)
74-75: DEST now mandatory for get; confirm this matches CLI UX expectations.If you intend to support partial args, consider allowing DEST omission with a sensible default. Otherwise, current strictness is fine.
86-86: Switch to cli_nexthop_format is correct.Consistent with nexthop formatter changes.
92-98: cli_route_ops wiring looks sound.af/add/del/list/get populated as expected.
118-118: Event formatting updated to cli_nexthop_format.Matches infra/nexthop API. Good.
137-139: Constructor registration flow is correct.cli_route_ops_register + cli_event_printer_register aligned with infra/route/events.
modules/infra/cli/events.c (4)
12-12: STAILQ head/type rename looks correctSwitch to cli_event_printer with next linkage is consistent.
18-26: Printer lookup logic LGTMIterates over printers and matches ev_type; returns const printer. Simple and correct.
55-59: CLI command wiring matches new root/verb style"events [show]" under root is aligned with the new CLI. Nice.
64-71: Context migration approvedcli_context/cli_context_register usage is consistent with the refactor.
cli/exec.c (2)
14-18: Context registry migration approvedSTAILQ over cli_context with next linkage and head insert is consistent with the new API.
244-249: Trace plumbing propagation
Trace parameter is cleanly passed; verify noexec_lineorexec_argscalls omit the newtraceargument.modules/ip/cli/address.c (3)
27-28: IPv4 net parsing switch LGTMUsing arg_ip4_net("ADDR", ...) aligns with the unified L3 helpers.
45-46: Consistency in del pathSame addr parser in del; consistent.
75-80: Ops/printer registration approvedcli_addr_ops and cli_event_printer wiring matches the new registration model.
Also applies to: 104-116
modules/ip/cli/route.c (3)
51-66: route4_list refactor approvedSignature change and use of cli_nexthop_format are correct; returns ret for upstream handling.
74-87: route4_get uses cli_nexthop_format appropriatelyGood switch to cli_nexthop_format; buffer handling matches usage.
93-99: Ops/printer registration migration LGTMcli_route_ops and cli_event_printer wiring is consistent with the new API.
Also applies to: 128-140
modules/infra/cli/nexthop.c (4)
40-76: cli_nexthop_format looks goodBase-info + type-specific formatter chaining is clean. Error paths and SAFE_BUF usage are consistent.
126-156: Completions/type resolution LGTMIteration over formatters for completion and name→type mapping is straightforward.
445-491: Init wiring approvedcli_context_register, cli_event_printer_register, and formatter registrations align with the new API.
343-345: NEXTHOP_ macros are already defined*
The symbols NEXTHOP_ARG, NEXTHOP_CTX, and NEXTHOP_ADD_CTX live in modules/infra/cli/gr_cli_nexthop.h, so there’s no missing definition here.Likely an incorrect or invalid review comment.
cli/gr_cli.h (1)
32-37: Ignore proposed API change; storing &cb yields a dangling pointer.Likely an incorrect or invalid review comment.
rjarry
force-pushed
the
cli-ctx-rework
branch
2 times, most recently
from
b5a792a to
3290dca
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
modules/infra/cli/trace.c (3)
16-27: Zero‑init request before setting fields (trace_set)req is stack‑allocated and partially filled; other bits remain garbage. Zero‑init to avoid undefined flags being sent.
Apply:
-static struct gr_infra_packet_trace_set_req req; +static struct gr_infra_packet_trace_set_req req = {0};
36-46: Zero‑init request before setting fields (trace_del)Same issue as enable path; ensure deterministic flags.
Apply:
-static struct gr_infra_packet_trace_set_req req; +static struct gr_infra_packet_trace_set_req req = {0};
56-61: Zero‑init dump request (trace_show)req may contain garbage except max_packets; zero it first.
Apply:
-struct gr_infra_packet_trace_dump_req req; +struct gr_infra_packet_trace_dump_req req = (struct gr_infra_packet_trace_dump_req){0};modules/ip6/cli/router_advert.c (2)
17-18: Zero‑init show requestreq is uninitialized; only iface_id is set. Zero‑init to avoid garbage fields.
Apply:
-struct gr_ip6_ra_show_req req; +struct gr_ip6_ra_show_req req = {0};
74-81: Zero‑init clear requestSame here; ensure deterministic payload.
Apply:
-struct gr_ip6_ra_clear_req req; +struct gr_ip6_ra_clear_req req = (struct gr_ip6_ra_clear_req){0};modules/ip6/cli/address.c (1)
54-71: Fix IPv6 formatter argument.Line 67:
IP6_Fexpects astruct in6_addr *, but&addr->addrpasses anip6_net. This will print incorrect data. Use&addr->addr.ipas done in other modules.Apply this diff:
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);modules/srv6/cli/route.c (1)
153-159: Help/grammar mismatch: VRF documented but not accepted nor usedCommand string is “set SRC”, but help adds “VRF” and srv6_tunsrc_set ignores it. Drop the VRF help (or add grammar and handle it).
Apply this diff to remove the misleading help:
ret = CLI_COMMAND( TUNSRC_CTX(root), "set SRC", srv6_tunsrc_set, "Set Segment Routing SRv6 source address", with_help("Ipv6 address to use as source.", ec_node_re("SRC", IPV6_RE)), - with_help("L3 routing domain ID.", ec_node_uint("VRF", 0, UINT16_MAX - 1, 10)) );
♻️ Duplicate comments (4)
cli/main.c (2)
183-183: Honor -x tracing for positional commands.Passing
falsehardcodes trace-off for argv execution. Feedopts.trace_commandsthrough.Apply this diff:
- status = exec_args(client, cmdlist, argc, (const char *const *)argv, false); + status = exec_args(client, cmdlist, argc, (const char *const *)argv, opts.trace_commands);
191-195: Handle EXEC_CMD_EXIT when replaying scripts.If
exec_linereturnsEXEC_CMD_EXIT, the loop continues reading the rest of the file, so a script can no longer stop itself. Break before callingcheck_status.Apply this diff:
while (fgets(buf, sizeof(buf), opts.cmds_file)) { status = exec_line(client, cmdlist, buf, opts.trace_commands); + if (status == EXEC_CMD_EXIT) + break; if (check_status(status) < 0 && opts.err_exit) goto end; }cli/exec.c (2)
40-51: Non-portable function pointer storage remains unaddressed.The previous review comment about non-portable function pointer storage via void* has not been addressed. Line 45 still casts void* directly to cmd_cb_t, which violates ISO C (function pointers and object pointers are incompatible).
110-115: Missing errno setting before returning NULL remains unaddressed.The previous review comment about setting errno before returning NULL from get_suggestions has not been addressed. When sug is NULL and errno is 0, callers will incorrectly report "Success".
🧹 Nitpick comments (6)
modules/ipip/cli.c (2)
127-155: Cache INTERFACE_ contexts and NULL-check before use (avoid repeated macro evaluation).*Create the add/set context nodes once, check for NULL, then reuse in CLI_COMMAND. This mirrors modules/infra/cli/iface.c and is safer.
static int ctx_init(struct ec_node *root) { - int ret; - - ret = CLI_COMMAND( - INTERFACE_ADD_CTX(root), + int ret; + struct ec_node *add_ctx = INTERFACE_ADD_CTX(root); + if (add_ctx == NULL) + return -1; + struct ec_node *set_ctx = INTERFACE_SET_CTX(root); + if (set_ctx == NULL) + return -1; + + ret = CLI_COMMAND( + add_ctx, "ipip NAME local LOCAL remote REMOTE [" IFACE_ATTRS_CMD "]", ipip_add, "Create a new IPIP tunnel interface.", with_help("Interface name.", ec_node("any", "NAME")), IPIP_ATTRS_ARGS ); if (ret < 0) return ret; - ret = CLI_COMMAND( - INTERFACE_SET_CTX(root), + ret = CLI_COMMAND( + set_ctx, "ipip NAME (name NEW_NAME),(local LOCAL),(remote REMOTE)," IFACE_ATTRS_CMD, ipip_set, "Modify ipip parameters.", with_help( "Interface name.", ec_node_dyn("NAME", complete_iface_names, INT2PTR(GR_IFACE_TYPE_IPIP)) ), with_help("New interface name.", ec_node("any", "NEW_NAME")), IPIP_ATTRS_ARGS ); if (ret < 0) return ret; return 0; }As seen in modules/infra/cli/iface.c (lines 485-535).
158-165: Context type/registration update is correct; naming nit.struct cli_context + cli_context_register is correct. Optional: align ctx.name with port.c style (e.g., "infra ipip") for consistency.
static struct cli_context ctx = { - .name = "ipip", + .name = "infra ipip", .init = ctx_init, };modules/infra/cli/port.c (1)
188-214: Precompute interface contexts and NULL-check before CLI_COMMAND.Avoid calling INTERFACE_* macros inline; cache and reuse. Matches iface.c and is safer.
static int ctx_init(struct ec_node *root) { int ret; + struct ec_node *add_ctx = INTERFACE_ADD_CTX(root); + if (add_ctx == NULL) + return -1; + struct ec_node *set_ctx = INTERFACE_SET_CTX(root); + if (set_ctx == NULL) + return -1; - ret = CLI_COMMAND( - INTERFACE_ADD_CTX(root), + ret = CLI_COMMAND( + add_ctx, "port NAME devargs DEVARGS [" PORT_ATTRS_CMD "]", port_add, "Create a new port.", with_help("Interface name.", ec_node("any", "NAME")), with_help("DPDK device args.", ec_node("devargs", "DEVARGS")), PORT_ATTRS_ARGS ); if (ret < 0) return ret; - ret = CLI_COMMAND( - INTERFACE_SET_CTX(root), + ret = CLI_COMMAND( + set_ctx, "port NAME (name NEW_NAME)," PORT_ATTRS_CMD, port_set, "Modify port parameters.",As seen in modules/infra/cli/iface.c (lines 485-535).
modules/ip6/cli/router_advert.c (2)
97-100: Consistent iface completion argOther modules pass INT2PTR(GR_IFACE_TYPE_UNDEF) to complete_iface_names. Consider doing the same for consistency.
Apply:
-with_help("Interface name.", ec_node_dyn("IFACE", complete_iface_names, NULL)), +with_help("Interface name.", ec_node_dyn("IFACE", complete_iface_names, INT2PTR(GR_IFACE_TYPE_UNDEF))),
99-99: Nit: wording"Life time" → "Lifetime".
Apply:
-with_help("Life time", ec_node_uint("LT", 0, UINT16_MAX - 1, 10)) +with_help("Lifetime", ec_node_uint("LT", 0, UINT16_MAX - 1, 10))modules/infra/cli/iface.c (1)
519-531: Show wiring looks right; consider returning 0 from ctx_init for consistencyAll other modules’ ctx_init end with return 0 on success. Here ctx_init ends with “return ret;” which may propagate a non‑error value. Align to 0 for success.
Outside-selected-line change (function tail):
// at the end of ctx_init(...) return 0;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(4 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/api.c(1 hunks) -
main/gr_module.h(2 hunks) -
main/grout.init(1 hunks) -
main/module.c(4 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(3 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(5 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks) -
subprojects/ecoli.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/ip6/cli/ip.h
- modules/policy/cli/policy.h
- modules/ip/cli/ip.h
🚧 Files skipped from review as they are similar to previous changes (26)
- smoke/ip6_add_del_test.sh
- smoke/cross_vrf_forward_frr_test.sh
- api/gr_net_types.h
- smoke/cross_vrf_forward_test.sh
- modules/infra/cli/vlan.c
- smoke/graph_svg_test.sh
- smoke/ip_builtin_icmp_test.sh
- smoke/vrf_forward_test.sh
- smoke/ip6_same_peer_test.sh
- smoke/ip_add_del_test.sh
- modules/ip6/cli/icmp6.c
- cli/ecoli.c
- modules/srv6/cli/localsid.c
- GNUmakefile
- modules/ip/cli/icmp.c
- main/grout.init
- meson.build
- smoke/vlan_forward_test.sh
- main/api.c
- smoke/ip_forward_test.sh
- smoke/_init_frr.sh
- smoke/affinity_test.sh
- cli/log.h
- smoke/snat44_test.sh
- main/gr_module.h
- smoke/config_test.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
PR: DPDK/grout#294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/infra/cli/meson.build
🧬 Code graph analysis (32)
cli/quit.c (2)
modules/infra/cli/nexthop.c (1)
constructor(485-491)cli/exec.c (1)
cli_context_register(16-18)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/infra/cli/graph.c (3)
modules/ip6/cli/router_advert.c (2)
ctx_init(89-125)constructor(132-134)modules/infra/cli/events.c (2)
ctx_init(55-62)constructor(69-71)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (3)
arg_ip_net(199-212)arg_str(99-109)with_help(15-24)modules/infra/cli/iface.c (2)
iface_from_name(92-105)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
modules/infra/cli/port.c (2)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/gr_cli_event.h (1)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip/cli/route.c (3)
modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ipip/cli.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
cli/main.c (2)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)cli/interact.c (1)
interact(36-114)
modules/policy/cli/snat44.c (4)
modules/infra/cli/address.c (1)
ctx_init(90-133)modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)modules/policy/cli/dnat44.c (1)
ctx_init(102-137)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/dnat44.c (4)
modules/infra/cli/address.c (2)
ctx_init(90-133)constructor(140-142)modules/infra/cli/nexthop.c (3)
ctx_init(346-443)constructor(485-491)cli_nexthop_formatter_register(22-27)modules/policy/cli/snat44.c (2)
ctx_init(89-131)constructor(138-140)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/stats.c (3)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)modules/infra/api/stats.c (2)
stats_reset(196-220)stats_get(79-194)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/affinity.c (3)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/trace.c (1)
constructor(187-189)cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/ip6/cli/router_advert.c (2)
modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/route.c (6)
modules/ip6/control/route.c (2)
route6_list(454-461)route6_get(360-375)cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/trace.c (2)
modules/infra/cli/address.c (1)
ctx_init(90-133)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (5)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip6/cli/address.c (3)
addr_list(54-71)addr_add(18-34)addr_del(36-52)modules/ip/control/address.c (3)
addr_list(149-175)addr_add(63-115)addr_del(117-147)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/route.c (5)
cli/ecoli.c (3)
arg_ip_net(199-212)arg_ip(180-189)with_help(15-24)cli/gr_cli.h (1)
arg_u16(64-70)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
modules/infra/cli/events.c (7)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/iface.c (1)
constructor(589-592)modules/ip/cli/address.c (1)
constructor(113-116)modules/ip6/cli/address.c (1)
constructor(111-114)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/nexthop.c (1)
constructor(485-491)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/gr_cli_l3.h (2)
modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/address.c (1)
cli_addr_ops_register(19-28)
smoke/vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/infra/cli/nexthop.c (2)
modules/infra/cli/events.c (2)
ctx_init(55-62)cli_event_printer_register(14-16)cli/exec.c (1)
cli_context_register(16-18)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 66-66: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 67-67: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 81-81: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 82-82: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (88)
main/module.c (4)
26-32: LGTM—field rename applied correctly.The
entries→nextsubstitution in both traversal and insertion is consistent with the struct field change described in the PR.
38-43: LGTM—field rename and explicit return.Line 38:
entries→nextis consistent with struct changes.
Line 43: Explicitreturn NULL;is redundant (STAILQ_FOREACH already leaveshandlerNULL after exhaustion) but harmless—defensive style.
54-59: LGTM—field rename applied correctly.Both traversal (line 54) and insertion (line 59) updated from
entriestonext, consistent with struct field change.
139-161: LGTM—field rename applied correctly.Both module iteration sites (lines 139, 161) updated from
entriestonext, consistent with struct field change. Logic unchanged.modules/infra/cli/affinity.c (1)
118-152: Context refactor looks solid. Consolidating the affinity contexts through the new macros keeps the command layout tidy and aligns with the broader CLI changes.subprojects/ecoli.wrap (1)
3-3: v0.8.0 tag verified; review upstream changelog
The libecoli v0.8.0 tag exists; ensure no breaking changes or security advisories affect this bump.cli/quit.c (1)
15-15: Migration to cli_context/cli_context_register looks good.Type and registration updates align with exec.c and the tree-wide refactor.
Also applies to: 21-21
modules/infra/cli/gr_cli_iface.h (1)
48-54: Interface-context macros are clear and consistent.Alias "interface|iface" and ADD/SET helpers look good and match usage patterns across modules.
modules/infra/cli/port.c (1)
220-227: Context migration looks good.struct cli_context + cli_context_register aligns with the new API.
modules/infra/cli/meson.build (1)
5-5: CLI sources validated—no duplicates
address.candroute.cexist under modules/infra/cli and are each referenced exactly once in its meson.build.modules/infra/cli/trace.c (1)
110-112: New TRACE/LOGGING context macros look goodNames and help strings are clear; aligns with new CLI_CONTEXT pattern.
cli/complete.c (1)
17-37: Completion node construction refactor LGTMUsing EC_NODE_SEQ with a prog placeholder + option subset and ec_node_sh_lex_expand is cleaner and works with ec_complete(cmdlist, buf).
modules/infra/cli/stats.c (4)
61-65: Default to software unless 'hardware' explicitly setFlag logic matches synopsis/help and avoids accidental mixed modes.
152-153: STATS context macro adoption LGTMAligns with the new cli_context model.
157-163: Reset + unified show registration LGTMGood split of reset vs show, and richer show synopsis.
200-207: cli_context migration is correctType change and cli_context_register() usage are consistent treewide.
modules/ip6/cli/router_advert.c (1)
114-121: Show registration LGTMContext, synopsis, and help align with the new CLI structure.
smoke/ip6_builtin_icmp_test.sh (1)
10-13: Test updates match new CLI surfaceinterface add and address add syntax look correct (iface alias used).
smoke/ip6_forward_test.sh (1)
10-16: LGTM! CLI commands correctly updated to verb-noun syntax.The test commands have been properly migrated to the new CLI surface (
interface add,address add,route add,nexthop add). The test logic remains intact.smoke/ip6_rs_ra_test.sh (1)
11-12: LGTM! Commands migrated correctly.CLI invocations updated to match the new verb-noun syntax.
modules/infra/cli/graph.c (2)
36-41: LGTM! Graph command updated to new CLI surface.The command syntax now uses explicit
[show]subcommand and improved help text for thefulloption. Consistent with the PR-wide CLI refactor.
49-55: LGTM! Context type and registration updated correctly.Properly migrated from
gr_cli_context→cli_contextandregister_context→cli_context_register.modules/infra/cli/gr_cli_event.h (1)
9-16: LGTM! Event printer API renamed consistently.Type and function names updated from
gr_cli_*→cli_*prefix, and STAILQ field renamed fromentries→next, consistent with the PR-wide refactor.smoke/dnat44_test.sh (2)
10-16: LGTM! DNAT44 commands migrated to new CLI surface.Commands correctly updated to use
interface add,address add, anddnat44 add/showsyntax.
33-34: LGTM! Show commands updated correctly.Verification commands now use
dnat44 showandnexthop show type dnatsyntax.smoke/_init.sh (4)
4-4: LGTM! Added pipefail for robust error handling.Adding
-o pipefailensures that errors in pipeline commands are properly caught.
24-34: LGTM! Cleanup logic updated to new CLI surface.Interface deletion now uses
grcli interface showandgrcli interface delcommands correctly.
96-100: LGTM! Banner commands updated consistently.All show commands migrated to new subcommand syntax (
stats show software,interface show, etc.).
114-118: LGTM! Trace and events commands updated.Commands correctly changed to
grcli trace enable allandgrcli events.smoke/ipip_encap_test.sh (1)
11-16: LGTM! IPIP test commands migrated correctly.Interface, address, and IPIP tunnel commands updated to the new CLI syntax.
cli/exec.h (2)
10-10: LGTM! Added stdbool.h for bool type.Required for the new
bool traceparameter in function signatures.
27-36: LGTM! Exec functions updated with trace parameter.Both
exec_argsandexec_linenow accept abool traceparameter, enabling tracing capability. Signatures match the implementations in exec.c.README.md (1)
102-251: Documentation accurately reflects the new CLI surface.All command examples and help text have been updated to match the verb-noun CLI structure (e.g.,
interface add,address add,route show). The changes are consistent and clear.cli/log.c (2)
50-63: LGTM: need_quote is now public.Exposing
need_quoteallows other modules to reuse the quoting logic.
65-78: LGTM: trace_cmd refactored to eliminate vector duplication.The new implementation directly uses
ec_strvecAPIs, removing unnecessary allocation and cleanup.modules/policy/cli/snat44.c (1)
87-139: LGTM: snat44 CLI migrated to new API.The context macro, command syntax, and registration follow the same pattern as other policy modules (dnat44, address, etc.). Consistent with the broader CLI refactor.
modules/infra/cli/address.c (5)
19-28: LGTM: provider registration with runtime checks.Assertions ensure that providers are well-formed and address families are unique. Good defensive programming.
30-42: LGTM: address add dispatches to the first matching provider.The iteration logic and error handling (ENOPROTOOPT) are appropriate.
44-56: LGTM: address del follows the same pattern as add.Consistent dispatch logic.
58-86: LGTM: address list aggregates results from all providers.The table-building and error-handling logic are sound.
88-142: LGTM: context and command registration follow established patterns.The ADDR_CTX macro and command definitions are consistent with other CLI modules (snat44, dnat44).
smoke/srv6_test.sh (1)
11-67: LGTM: test script updated to new CLI surface.All command invocations now use the verb-noun structure (
interface add,address add,nexthop add,route add). Test semantics are unchanged.cli/main.c (1)
126-140: LGTM: check_status centralizes status interpretation.The mapping is clear and consistent.
cli/gr_cli.h (1)
14-54: LGTM: CLI API migration complete.All type and function renames (gr_cli_* → cli_*) are consistent with the broader refactor. The new signatures and forward declarations align with usage in related files.
smoke/nexthop_ageing_test.sh (1)
12-82: LGTM: test script updated to new CLI surface.All
grcliinvocations now use the verb-noun structure (nexthop show,address show,interface add). Test logic is unchanged.modules/infra/cli/gr_cli_l3.h (3)
13-20: LGTM!Struct definition is clear and consistent with the PR's STAILQ unification (
nextfield).
22-28: LGTM!Struct definition is clear and consistent with the PR's STAILQ unification (
nextfield).
30-31: LGTM!Registration function declarations are consistent with their implementations in route.c and address.c.
modules/infra/cli/gr_cli_nexthop.h (3)
11-16: LGTM!Struct rename and STAILQ_ENTRY field (
next) align with the PR's cli_* unification.
18-26: LGTM!Function renames align with the PR's cli_* unification and match their implementations in nexthop.c.
28-31: LGTM!Macros provide convenient context builders and support the "nh" alias as mentioned in PR objectives.
cli/interact.c (3)
73-73: LGTM!Replacement of
ec_node_sh_lexwithec_node_sh_lex_expandaligns with the PR's ecoli completion API refactor.
92-92: LGTM!Addition of
falseparameter aligns with the updatedexec_linesignature (trace parameter).
99-100: LGTM!Streamlined exit handling with jump to
exit_okpath.modules/infra/cli/route.c (7)
18-28: LGTM!Registration function enforces non-null callbacks and unique AF values. STAILQ_ENTRY
nextaligns with PR refactor.
30-42: LGTM!Handler dispatches to registered operation based on AF match. ENOPROTOOPT correctly signals unsupported protocol.
44-56: LGTM!Handler dispatches to registered operation based on AF match. ENOPROTOOPT correctly signals unsupported protocol.
58-81: LGTM!Handler aggregates routes from all registered operations into a single table. Error handling is correct.
83-95: LGTM!Handler dispatches to registered operation based on AF match. ENOPROTOOPT correctly signals unsupported protocol.
99-145: LGTM!Context initialization wires four subcommands with clear help text. Error handling propagates registration failures.
152-154: LGTM!Constructor registers the route context at load time, following standard initialization pattern.
modules/ip6/cli/address.c (6)
8-8: LGTM!Inclusion of
gr_cli_l3.hprovides thecli_addr_opsdefinition required for the new L3 API.
18-34: LGTM!Argument name change from "IP6_NET" to "ADDR" aligns with the unified CLI surface across address modules.
36-52: LGTM!Argument name change from "IP6_NET" to "ADDR" aligns with the unified CLI surface across address modules.
73-78: LGTM!
addr_opsstruct correctly defines IPv6 address operations for the new L3 API.
102-109: LGTM!Event printer struct rename to
cli_event_printeraligns with the PR's cli_* unification.
111-114: LGTM!Registration calls updated to use
cli_addr_ops_registerandcli_event_printer_register, aligning with the new L3 API.modules/policy/cli/conntrack.c (1)
143-147: CLI context migration and registrations look goodMacros, command wiring, and context registration align with the new cli_* infra and “[show]” convention. No issues spotted.
Also applies to: 152-155, 158-192, 197-201, 205-208, 212-215, 218-219
modules/srv6/cli/route.c (1)
128-132: SRv6 CLI wiring and formatter registration are consistentFormatter, contexts, and command wiring follow the new patterns. Looks correct.
Also applies to: 134-134, 140-149, 164-167, 172-176, 183-186, 189-191
modules/infra/cli/iface.c (3)
489-494: Defensive guards are fineEnsuring INTERFACE_*_CTX roots exist early is good; immediate -1 keeps init predictable.
508-511: New stats/rates commands: OKCommands and bindings look correct and match handlers.
Also applies to: 513-517
538-541: Context and event printer migration look goodType renames and registration via cli_* APIs are consistent.
Also applies to: 577-587, 590-592
modules/policy/cli/dnat44.c (1)
94-98: DNAT44 CLI migration and formatter registration LGTMContext macro, commands, and registrations are aligned with the new cli_* infra. No issues.
Also applies to: 100-100, 106-113, 117-123, 127-132, 139-142, 145-147
modules/infra/cli/events.c (1)
12-16: Events printer/type migration and command wiring look correctSTAILQ linkage, types, and “events [show]” registration align with the new API.
Also applies to: 18-26, 32-32, 55-61, 64-67, 70-71
modules/ip6/cli/route.c (1)
8-8: IPv6 route ops integration and formatting LGTM
- route_ops registration conforms to infra expectations.
- cli_nexthop_format usage in list/get/event is consistent.
No issues spotted.Also applies to: 51-66, 74-76, 85-86, 92-98, 118-119, 127-134, 137-139
cli/exec.c (6)
14-18: LGTM!Context type and registration API correctly updated to new naming convention.
20-38: LGTM!Context initialization correctly updated to use new cli_context type and next field for traversal.
117-166: LGTM!Suggestions printing logic is well-structured with proper error handling and TTY-aware formatting.
191-242: LGTM!Trace parameter and argument expansion correctly integrated into execution flow.
244-272: LGTM!Trace parameter propagated correctly and error messages enhanced with context.
274-297: LGTM!Trace parameter propagated correctly and error message enhanced.
modules/infra/cli/nexthop.c (7)
20-27: LGTM!Formatter registration correctly updated to new naming convention and next field.
29-38: LGTM!Formatter lookup correctly updated to use new type and next field.
40-76: LGTM!Function correctly renamed and type updated; implementation remains sound.
104-124: LGTM!Formatter instances correctly updated to new type.
126-156: LGTM!Completion and type conversion functions correctly updated to new type and next field.
343-443: LGTM!CLI commands correctly updated with new context macros, optional show arguments, interface alias support, and verb-noun syntax reordering.
445-491: LGTM!Context and printer registration correctly updated to new types and registration functions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/policy/cli/dnat44.c (1)
106-113: Document ‘iface’ alias alongside ‘interface’
The README and help text currently use both keywords but don’t state that they are interchangeable—add a note in the user guide or CLI help.
♻️ Duplicate comments (4)
cli/main.c (2)
183-184: Honor-xtracing for argv execution.The trace parameter is hardcoded to
false, disabling tracing for positional commands. Useopts.trace_commandsinstead.Apply this diff:
- status = exec_args(client, cmdlist, argc, (const char *const *)argv, false); + status = exec_args( + client, cmdlist, argc, (const char *const *)argv, opts.trace_commands + );
192-194: Handle EXEC_CMD_EXIT when replaying scripts.If
exec_linereturnsEXEC_CMD_EXIT, the loop continues reading remaining lines. Scripts can no longer stop themselves. Break immediately.Apply this diff:
status = exec_line(client, cmdlist, buf, opts.trace_commands); + if (status == EXEC_CMD_EXIT) + break; if (check_status(status) < 0 && opts.err_exit) goto end;modules/ip6/cli/address.c (1)
67-68: Fix IPv6 formatter argument.
IP6_Fneeds astruct in6_addr *. Passing&addr->addrfeeds the wholeip6_netand prints junk. Point it at&addr->addr.ip.- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);cli/exec.c (1)
40-47: Stop casting callbacks through void*
Still pulling the callback out as a barevoid *and casting tocmd_cb_t. That is undefined on platforms where data and function pointers differ (e.g. many BSD targets). Please store acmd_cb_t *in the attrs and dereference it here to keep this code portable.- cmd_cb_t cb = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR); - if (cb != NULL) - return cb; + cmd_cb_t *cbp = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR); + if (cbp != NULL) + return *cbp;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(4 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/api.c(1 hunks) -
main/gr_module.h(2 hunks) -
main/grout.init(1 hunks) -
main/module.c(4 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(3 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(5 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks) -
subprojects/ecoli.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/ip6/cli/ip.h
- modules/policy/cli/policy.h
- modules/ip/cli/ip.h
✅ Files skipped from review due to trivial changes (1)
- subprojects/ecoli.wrap
🚧 Files skipped from review as they are similar to previous changes (28)
- GNUmakefile
- main/module.c
- smoke/ip_builtin_icmp_test.sh
- smoke/ip_forward_test.sh
- smoke/vrf_forward_frr_test.sh
- smoke/srv6_test.sh
- modules/infra/cli/gr_cli_iface.h
- smoke/ip6_rs_ra_test.sh
- modules/infra/cli/trace.c
- cli/quit.c
- modules/infra/cli/meson.build
- smoke/_init_frr.sh
- modules/infra/cli/route.c
- modules/infra/cli/graph.c
- smoke/ip6_forward_test.sh
- modules/ip6/cli/icmp6.c
- smoke/dnat44_test.sh
- smoke/_init.sh
- modules/infra/cli/gr_cli_event.h
- meson.build
- smoke/ip_add_del_test.sh
- smoke/ip6_add_del_test.sh
- cli/log.h
- cli/log.c
- smoke/cross_vrf_forward_test.sh
- main/grout.init
- modules/policy/cli/snat44.c
- main/api.c
🧰 Additional context used
🧬 Code graph analysis (32)
smoke/vlan_forward_test.sh (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/srv6/cli/localsid.c (2)
modules/infra/cli/nexthop.c (3)
ctx_init(346-443)constructor(485-491)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/ipip/cli.c (4)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)modules/infra/cli/port.c (2)
ctx_init(188-218)constructor(225-228)modules/infra/cli/vlan.c (2)
ctx_init(158-197)constructor(204-207)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/vlan.c (2)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (3)
arg_ip_net(199-212)arg_str(99-109)with_help(15-24)modules/infra/cli/iface.c (4)
iface_from_name(92-105)ctx_init(486-536)complete_iface_names(70-90)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/trace.c (1)
constructor(187-189)cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
modules/infra/cli/gr_cli_l3.h (2)
modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/address.c (1)
cli_addr_ops_register(19-28)
modules/infra/cli/affinity.c (3)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/trace.c (1)
constructor(187-189)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/dnat44.c (2)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/route.c (6)
modules/ip/control/route.c (1)
route4_list(416-423)cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
modules/ip/cli/address.c (5)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip6/cli/address.c (3)
addr_list(54-71)addr_add(18-34)addr_del(36-52)modules/ip/control/address.c (3)
addr_list(149-175)addr_add(63-115)addr_del(117-147)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/port.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/stats.c (3)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)modules/infra/api/stats.c (2)
stats_reset(196-220)stats_get(79-194)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/route.c (4)
modules/ip6/control/route.c (1)
route6_list(454-461)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/ip/cli/icmp.c (1)
cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/nexthop.c (3)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
smoke/affinity_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
cli/main.c (3)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)cli/log.c (1)
is_tty(22-30)cli/interact.c (1)
interact(36-114)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/ip6/cli/router_advert.c (4)
modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)modules/infra/cli/iface.c (2)
ctx_init(486-536)complete_iface_names(70-90)cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/events.c (7)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/iface.c (1)
constructor(589-592)modules/ip/cli/address.c (1)
constructor(113-116)modules/ip6/cli/address.c (1)
constructor(111-114)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
smoke/cross_vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 66-66: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 67-67: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 81-81: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 82-82: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ 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). (1)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (75)
smoke/graph_svg_test.sh (1)
16-16: Command switch aligns with new CLI verb order. Keeps the SVG comparison flow intact.api/gr_net_types.h (1)
140-140: Approve IP_ANY_NET_RE anchoringAll usages in CLI input validation now correctly enforce full-match anchoring; no further action needed.
cli/ecoli.c (4)
67-67: LGTM!Wrapping the context name with
EC_NODE_CMDinstead ofEC_NODE_STRaligns with the CLI refactor objectives for command parsing behavior.
26-35: with_callback call sites updated
Only occurrences (prototype and CLI_COMMAND macro) pass cb by value; signature change is fully propagated.
180-189: Approve publicarg_ipchange
External usage confirmed in modules/infra/cli/route.c (line 88).
199-212: arg_ip_net visibility change validated
External calls in modules/infra/cli/route.c and modules/infra/cli/address.c rely on this function and the updated declaration in cli/gr_cli.h aligns with its usage.smoke/ip6_builtin_icmp_test.sh (1)
10-13: LGTM! CLI surface migration correctly applied.The commands now use the new noun-verb syntax (
interface add,address add) consistently with the PR's CLI refactor.main/gr_module.h (2)
41-41: LGTM! Field name aligns with STAILQ conventions.Renaming
entries→nextfollows standard linked list naming and matches the broader migration across the codebase.
51-51: LGTM! Consistent field renaming.Same
entries→nextmigration as ingr_api_handler, maintaining consistency.smoke/vlan_forward_test.sh (1)
12-17: LGTM! Consistent CLI surface migration.All commands correctly use the new
interface addandaddress addsyntax, preserving test functionality.smoke/cross_vrf_forward_frr_test.sh (1)
26-27: LGTM! Function arity corrected.The calls now match the 2-parameter signature of
set_ip_addressinsmoke/_init_frr.sh.smoke/nexthop_ageing_test.sh (4)
12-12: LGTM! Command updated to new surface.Correctly uses
grcli nexthop show internal.
31-35: LGTM! Consistent CLI surface migration.All commands (
nexthop config set,interface add,address add) follow the new noun-verb pattern correctly.
52-52: LGTM! Command surface updated.Correctly uses
grcli nexthop show.
66-67: Shellcheck warnings are false positives.Shellcheck incorrectly flags
$p0and$p1as arrays (SC1087). These are simple string variables, not arrays, so the warnings can be safely ignored.Also applies to: 81-82
cli/gr_cli.h (7)
14-14: LGTM! Typedef refactored to pointer form.Changing from
int (*cli_ctx_init_t)(...)toint (*cli_ctx_init_t)(...)is more idiomatic for function pointer types.
16-19: LGTM! Context struct renamed and modernized.The struct rename (
gr_cli_context→cli_context), member type update, and field rename (entries→next) align with the codebase-wide CLI refactor.
22-22: LGTM! Registration function renamed consistently.
register_context→cli_context_registerfollows the new naming convention and is used consistently in cli/exec.c and module files.
32-32: LGTM! Callback typedef uses pointer syntax.The typedef now correctly declares
cmd_cb_tas a function pointer type.
36-36: LGTM! Signature follows typedef change.
with_callbacknow takescmd_cb_t cb(pointer) instead ofcmd_cb_t *cb, matching the typedef.
39-40: LGTM! Context arg extended with help text.Adding the
helpfield toctx_argsupports better documentation of CLI contexts.
49-49: LGTM! New IP parsing utilities added.
arg_ipandarg_ip_netprovide family-agnostic IP parsing, complementing the existingarg_ip4/arg_ip6functions. Implementations in cli/ecoli.c look correct.Also applies to: 52-52
modules/infra/cli/affinity.c (3)
111-113: LGTM! Context macros standardize CLI structure.The new macros (
AFFINITY_ARG,CPU_CTX,QMAP_CTX) provide consistent context construction for affinity commands, matching patterns in other CLI modules.
119-120: LGTM! Commands use new context macros.All command registrations now use
CPU_CTX(root)andQMAP_CTX(root), maintaining consistency with the new CLI surface.Also applies to: 134-139, 151-151
158-164: LGTM! Context type and registration updated.The context struct type change (
gr_cli_context→cli_context) and registration call update (register_context→cli_context_register) align with the API migration.smoke/snat44_test.sh (2)
10-17: LGTM! CLI commands updated consistently.All commands (
interface add,address add,snat44 add/show,conntrack show/config set) follow the new noun-verb CLI surface correctly.
44-54: LGTM! Conntrack commands use new surface.Commands (
conntrack show,conntrack config show,conntrack flush) are updated correctly.modules/policy/cli/dnat44.c (3)
94-98: LGTM: Type change aligns with API refactor.The formatter type change from
gr_cli_nexthop_formattertocli_nexthop_formatteris consistent with the broader CLI API refactoring across the codebase.
100-101: LGTM: Context macro follows established pattern.The
DNAT_CTXmacro usage is consistent with other context macros in the codebase (e.g.,INTERFACE_ADD_CTX,INTERFACE_SET_CTX).
139-146: LGTM: Registration calls match new API.The context type change and registration function updates are consistent with the CLI refactoring (from
gr_cli_context/register_contexttocli_context/cli_context_register).smoke/ipip_encap_test.sh (1)
11-16: LGTM: Test updated for new CLI surface.The command syntax changes from "grcli add interface" to "grcli interface add" and "grcli add ip address" to "grcli address add" correctly reflect the noun-then-verb CLI refactoring described in the PR objectives.
smoke/ip6_same_peer_test.sh (1)
10-14: LGTM: IPv6 test updated consistently.Command syntax updates match the CLI refactoring pattern seen across the codebase.
modules/ip/cli/icmp.c (1)
207-214: LGTM: Standard refactoring applied.The context type and registration function changes are consistent with the CLI API refactoring pattern applied throughout the codebase.
modules/ipip/cli.c (2)
130-153: LGTM: Interface context macros applied correctly.The usage of
INTERFACE_ADD_CTXandINTERFACE_SET_CTXmacros is consistent with the pattern established in other interface type modules (port.c, vlan.c).
158-166: LGTM: Standard refactoring applied.Context type and registration changes match the CLI API refactoring pattern.
modules/infra/cli/port.c (1)
188-228: LGTM: Port module refactored consistently.The context macro usage and registration changes follow the established pattern for interface modules. The changes are consistent with the broader CLI refactoring.
docs/grout-frr.7.md (1)
123-140: LGTM: Documentation updated for new CLI surface.The command syntax change from "show ip route" to "route show" and the updated output format align with the CLI refactoring. The per-entry field-based presentation matches the new route display semantics described in the PR.
modules/infra/cli/vlan.c (1)
158-207: LGTM: VLAN module refactored consistently.The context macro usage and registration changes are consistent with the CLI refactoring pattern applied across all interface modules.
modules/ip6/cli/router_advert.c (3)
87-88: LGTM! Context macro follows new pattern.The
RA_CTXmacro correctly wraps the CLI context with help text, consistent with the broader CLI refactor.
93-122: LGTM! Command registrations updated correctly.All three commands (set, clear, show) now use the
RA_CTXmacro consistently. The new show command properly supports optional arguments with[show] [interface|iface IFACE]syntax, aligning with the PR's goal of allowing omitted show arguments and interface/iface aliases.
127-133: LGTM! API migration complete.Correctly updated from
struct gr_cli_contexttostruct cli_contextand fromregister_context(&ctx)tocli_context_register(&ctx), consistent with the broader API renaming across the PR.smoke/affinity_test.sh (1)
8-28: LGTM! Test commands updated to new CLI surface.All commands correctly migrated to the new noun-then-verb pattern:
grcli set affinity cpus→grcli affinity cpus setgrcli show affinity qmap→grcli affinity qmap showgrcli add interface port→grcli interface add portConsistent with the PR's CLI reorganization objectives.
cli/complete.c (2)
17-38: LGTM! Function restructured cleanly.The rename from
add_flagstobash_complete_nodeis clear, and the restructured node tree with explicitEC_NODE_SEQandEC_NODE_SUBSETnesting improves readability. The use ofec_node_sh_lex_expandis consistent with the broader CLI refactor.
87-137: LGTM! Completion flow simplified.The removal of
vec_cmdduplication and direct usage ofvecforcomp_wordextraction simplifies the logic. The error message correctly references the new function namebash_complete_node.modules/infra/cli/address.c (4)
17-28: LGTM! Registry implementation is sound.The static STAILQ and
cli_addr_ops_registerfunction properly maintain the list of address operation providers. Assertions ensure non-null ops and unique address families, preventing registration errors.
30-56: LGTM! Dispatching logic handles multiple address families.Both
addr_addandaddr_deliterate registered ops and dispatch to the first matching address family based onarg_ip_netparsing. The fallback toENOPROTOOPTis appropriate when no ops match.
58-86: LGTM! List command aggregates across providers.The
addr_listfunction correctly creates a single table, iterates all registered ops, and aggregates results. Error handling breaks on first failure, which is appropriate.
88-142: LGTM! Context wiring follows new pattern.The
ADDR_CTXmacro and command registrations follow the established pattern from other modules. Commands support bothinterfaceandifacealiases and optional[show]prefix, consistent with PR objectives. Registration usescli_context_register.smoke/config_test.sh (1)
7-48: LGTM! Test commands comprehensively updated.All commands correctly migrated to noun-then-verb pattern:
grcli add interface→grcli interface addgrcli add nexthop→grcli nexthop addgrcli add address→grcli address addgrcli add route→grcli route add- Show/delete operations use
grcli <component> show|delpatternTest semantics preserved while aligning with new CLI surface.
cli/main.c (1)
126-140: LGTM! Status checking centralized.The
check_statushelper properly returns 0 for success cases (EXEC_SUCCESS,EXEC_CMD_EMPTY,EXEC_CMD_EXIT) and -1 for errors, centralizing status interpretation logic.cli/exec.h (2)
10-10: LGTM! Required include added.The
#include <stdbool.h>is necessary for the newbool traceparameters in function signatures.
27-36: LGTM! Function signatures updated for tracing.Both
exec_argsandexec_linenow accept abool traceparameter, enabling trace-aware execution. Signatures match the implementations provided in relevant code snippets fromcli/exec.c.README.md (4)
102-108: LGTM! Startup configuration updated.Commands in the example
/etc/grout.initfile correctly use the new CLI surface:
affinity cpus setinterface add portaddress addroute add
131-139: LGTM! Examples reflect new CLI surface.Service startup logs and interactive help output correctly demonstrate the reorganized command structure with noun-then-verb ordering and the new contexts (address, affinity, conntrack, etc.). The
interface show,address show,route show, andnexthop showexamples are accurate.Also applies to: 151-194
201-243: LGTM! Bash completion and one-shot examples updated.Tab completion examples and one-shot command invocations correctly demonstrate the new CLI surface, including the updated
stats show softwarepath and the restructured command hierarchy.
251-252: LGTM! Graph command updated.The graph dump command correctly uses
grcli graph briefinstead of the old syntax, consistent with the CLI reorganization.cli/interact.c (3)
73-73: LGTM: ecoli v0.8.0 API update.The switch from
ec_node_sh_lextoec_node_sh_lex_expandaligns with the ecoli dependency upgrade to v0.8.0 mentioned in the PR objectives.
92-92: Verify the trace parameter default.The
exec_linecall now passesfalsefor the trace parameter. Ensure this aligns with interactive mode behavior where tracing is typically disabled by default.
99-100: LGTM: simplified exit handling.The EXEC_CMD_EXIT case now directly jumps to
exit_ok, streamlining the exit path.modules/infra/cli/gr_cli_l3.h (3)
13-20: LGTM: well-structured route operations registry.The
cli_route_opsstruct provides a clean pluggable interface for route providers with all necessary callbacks (add/del/get/list) and proper STAILQ linkage.
22-28: LGTM: consistent address operations registry.The
cli_addr_opsstruct mirrors the design ofcli_route_opswith appropriate callbacks for address management.
30-31: Registration functions validated in implementations.Based on the snippets from
modules/infra/cli/route.candmodules/infra/cli/address.c, these registration functions properly validate all required function pointers and prevent duplicate registrations by address family.Based on learnings
modules/srv6/cli/localsid.c (4)
124-128: LGTM: formatter type rename.The nexthop formatter type is correctly updated from
gr_cli_nexthop_formattertocli_nexthop_formatter, consistent with the CLI API migration.
200-200: LGTM: context helper macro adoption.The switch from
CLI_CONTEXT(root, CTX_ADD, CTX_ARG(...))toNEXTHOP_ADD_CTX(root)improves consistency and reduces boilerplate.
214-217: LGTM: context type and registration updated.The context type and registration call are correctly updated to use the new
cli_contextandcli_context_registerAPIs.
220-221: LGTM: registration calls updated.Both context and formatter registration calls are correctly updated to the new API surface.
modules/infra/cli/stats.c (6)
152-152: LGTM: context macro definition.The
STATS_CTXmacro provides a clean way to reference the stats context consistently across command registrations.
157-159: LGTM: reset command properly scoped.The reset command is correctly registered under the
STATS_CTXcontext.
161-193: LGTM: show command context migration.The show command is properly migrated to use
STATS_CTXand the synopsis is broadened with optionalshowkeyword, consistent with the "allow omitting the show arguments" objective.
200-203: LGTM: context type updated.The context type is correctly updated from
gr_cli_contexttocli_context.
206-206: LGTM: registration function updated.The registration call is correctly updated to
cli_context_register.
61-64: Verify default stats flag logic No tests or docs specify the intended default; please confirm that inverting the logic—now defaulting to software whenhardwareisn’t passed—is intentional.modules/infra/cli/gr_cli_nexthop.h (3)
11-16: LGTM: formatter struct renamed and linkage unified.The struct rename from
gr_cli_nexthop_formattertocli_nexthop_formatterand the STAILQ field rename fromentriestonextalign with the PR objectives for API unification and consistent linkage naming.
18-26: LGTM: public function renames.Both registration and formatting functions are correctly renamed to use the
cli_prefix, maintaining API consistency.
28-31: LGTM: context helper macros reduce boilerplate.The new
NEXTHOP_ARG,NEXTHOP_CTX, andNEXTHOP_ADD_CTXmacros provide convenient shorthands for nexthop context creation, supporting the alias "nh" mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cli/log.h (1)
6-9: Add<stdbool.h>include.
boolis used here, but after swappingexec.hforecoli.hthere's no guarantee every consumer still gets the definition. Include it directly.+#include <stdbool.h> #include <ecoli.h>cli/main.c (2)
183-184: Honor -x tracing for argv execution.The hardcoded
falsedisables tracing for positional commands, sogrcli -x ...won't echo the command.Apply this diff to pass the user's trace preference:
- status = exec_args(client, cmdlist, argc, (const char *const *)argv, false); + status = exec_args( + client, cmdlist, argc, (const char *const *)argv, opts.trace_commands + );
191-195: Handle EXEC_CMD_EXIT when replaying scripts.If
exec_linereturnsEXEC_CMD_EXIT, the loop continues reading the rest of the file instead of stopping, unlike the interactive loop which exits immediately.Apply this diff to break out of the loop before calling
check_status:status = exec_line(client, cmdlist, buf, opts.trace_commands); + if (status == EXEC_CMD_EXIT) + break; if (check_status(status) < 0 && opts.err_exit) goto end;cli/exec.c (1)
180-181: Guard glibc-only strerror helpers
strerrordesc_np/strerrorname_npare glibc extensions; this won’t even compile on musl, FreeBSD, etc. Wrap them in a glibc feature check and fall back tostrerror(errno)(or similar) when unavailable.- case EXEC_CMD_FAILED: - errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); - break; + case EXEC_CMD_FAILED: +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 36) + errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); +#else + errorf("command failed: %s", strerror(errno)); +#endif + break;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(4 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/grout.init(1 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(3 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(5 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (3)
- modules/ip6/cli/ip.h
- modules/policy/cli/policy.h
- modules/ip/cli/ip.h
🚧 Files skipped from review as they are similar to previous changes (19)
- smoke/cross_vrf_forward_test.sh
- modules/infra/cli/meson.build
- modules/infra/cli/port.c
- modules/infra/cli/gr_cli_iface.h
- smoke/ip6_rs_ra_test.sh
- modules/infra/cli/vlan.c
- smoke/affinity_test.sh
- modules/ip6/cli/address.c
- smoke/ip6_forward_test.sh
- smoke/ip6_same_peer_test.sh
- smoke/ip6_add_del_test.sh
- smoke/config_test.sh
- smoke/ip_add_del_test.sh
- meson.build
- smoke/vlan_forward_test.sh
- smoke/ip_builtin_icmp_test.sh
- smoke/ip6_builtin_icmp_test.sh
- modules/infra/cli/gr_cli_l3.h
- modules/ip/cli/icmp.c
🧰 Additional context used
🧬 Code graph analysis (33)
cli/quit.c (2)
modules/infra/cli/trace.c (1)
constructor(187-189)cli/exec.c (1)
cli_context_register(16-18)
cli/log.h (1)
cli/log.c (4)
tty_init(16-20)is_tty(22-30)need_quote(50-63)trace_cmd(65-78)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/ipip/cli.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
modules/srv6/cli/localsid.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/route.c (1)
constructor(188-191)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/infra/cli/gr_cli_event.h (1)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/stats.c (3)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)modules/infra/api/stats.c (2)
stats_reset(196-220)stats_get(79-194)cli/exec.c (1)
cli_context_register(16-18)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/ip6/cli/router_advert.c (5)
modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)modules/infra/cli/iface.c (2)
ctx_init(486-536)complete_iface_names(70-90)cli/ecoli.c (1)
with_help(15-24)cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
smoke/vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
smoke/cross_vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/ip6/cli/route.c (6)
modules/ip6/control/route.c (1)
route6_list(454-461)cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (2)
arg_ip_net(199-212)arg_str(99-109)modules/infra/cli/iface.c (2)
iface_from_name(92-105)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/snat44.c (3)
modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)modules/policy/cli/dnat44.c (1)
ctx_init(102-137)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/affinity.c (6)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/trace.c (1)
constructor(187-189)modules/infra/cli/iface.c (1)
constructor(589-592)modules/infra/cli/port.c (1)
constructor(225-228)modules/infra/api/affinity.c (1)
rxq_list(62-79)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/events.c (6)
modules/infra/cli/nexthop.c (1)
constructor(485-491)modules/infra/cli/iface.c (1)
constructor(589-592)modules/ip/cli/address.c (1)
constructor(113-116)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
modules/policy/cli/dnat44.c (3)
modules/infra/cli/nexthop.c (3)
ctx_init(346-443)constructor(485-491)cli_nexthop_formatter_register(22-27)modules/ip6/cli/icmp6.c (2)
ctx_init(203-239)constructor(246-248)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/route.c (5)
cli/ecoli.c (3)
arg_ip_net(199-212)arg_ip(180-189)with_help(15-24)cli/gr_cli.h (1)
arg_u16(64-70)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/nexthop.c (3)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (5)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(58-86)addr_add(30-42)addr_del(44-56)cli_addr_ops_register(19-28)modules/ip6/cli/address.c (3)
addr_list(54-71)addr_add(18-34)addr_del(36-52)modules/ip/control/address.c (3)
addr_list(149-175)addr_add(63-115)addr_del(117-147)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/main.c (3)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)cli/log.c (1)
is_tty(22-30)cli/interact.c (1)
interact(36-114)
modules/infra/cli/graph.c (4)
modules/infra/cli/nexthop.c (2)
ctx_init(346-443)constructor(485-491)modules/infra/cli/events.c (2)
ctx_init(55-62)constructor(69-71)cli/quit.c (2)
ctx_init(11-13)constructor(20-22)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/route.c (5)
modules/ip/control/route.c (1)
route4_list(416-423)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/nexthop.c (2)
ctx_init(346-443)constructor(485-491)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/trace.c (2)
modules/infra/cli/address.c (2)
ctx_init(90-133)constructor(140-142)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/icmp6.c (4)
modules/infra/cli/address.c (2)
ctx_init(90-133)constructor(140-142)modules/infra/cli/trace.c (2)
ctx_init(113-180)constructor(187-189)modules/ip/cli/icmp.c (2)
ctx_init(177-205)constructor(212-214)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 66-66: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 67-67: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 81-81: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 82-82: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (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-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (86)
api/gr_net_types.h (1)
140-140: LGTM! Regex anchor fix.The old pattern anchored only the IPv6 alternative, allowing the IPv4 branch to match partial strings. Moving
$outside the alternation ensures both IPv4 and IPv6 prefixes are fully validated.smoke/vrf_forward_frr_test.sh (1)
28-45: set_ip_address calls updated correctlyDropping the VRF argument matches the new helper signature; test flow stays coherent.
smoke/cross_vrf_forward_frr_test.sh (1)
26-27: Arity update matches helper signature.Good catch trimming the VRF arg now that
set_ip_address()only takes iface+CIDR. Keeps the smoke test in sync.main/grout.init (5)
8-8: LGTM!The reordering to noun-first syntax (
affinity cpus set ...) aligns with the CLI refactor described in the PR objectives.
11-12: LGTM!The interface creation syntax now follows the noun-first form (
interface add port ...), consistent with the broader CLI refactor.
15-16: LGTM!The address commands now use the simplified noun-first syntax (
address add ...), dropping theipkeyword as part of the unified address/address6 context refactor.
19-19: LGTM!The route command now uses the simplified noun-first syntax (
route add ...), consistent with the unified route/route6 context refactor.
8-19: Verify CLI example commands (main/grout.init:8-19)
The noun-first syntax examples (affinity cpus set,interface add port,address add iface,route add via) don’t match any command registrations—confirm support in the implementation or update the examples accordingly.GNUmakefile (1)
39-39: LGTM!Command updated to match the new CLI surface (
graph show briefvs.show graph brief).modules/ipip/cli.c (3)
131-131: LGTM!Updated to use
INTERFACE_ADD_CTXmacro, consistent with the migration.
141-141: LGTM!Updated to use
INTERFACE_SET_CTXmacro, consistent with the migration.
158-164: LGTM!Context type and registration API updated (
gr_cli_context→cli_context,register_context→cli_context_register).smoke/srv6_test.sh (3)
11-14: LGTM!Commands updated to new CLI surface (
interface add,address add).
51-53: LGTM!Commands updated to new CLI surface (
nexthop add,route add).
66-67: LGTM!Commands updated to new CLI surface (
nexthop add,route add).smoke/ipip_encap_test.sh (1)
11-16: LGTM!Commands updated to new CLI surface (
interface add,address add).cli/log.h (3)
12-12: LGTM!New color macro added.
20-20: LGTM!New public function for quote detection.
22-22: LGTM!Signature updated to use
ec_strvec, aligning with vector-based command handling.cli/complete.c (3)
17-38: LGTM!Refactored to use
ec_node_sh_lex_expandfor clearer option nesting.
65-103: LGTM!Simplified vector handling by removing
vec_cmdand usingec_completedirectly.
117-121: LGTM!
comp_wordnow derived from the simplifiedvec.modules/ip6/cli/router_advert.c (4)
87-87: LGTM!New
RA_CTXmacro for router-advert context registration.
93-122: LGTM!Commands updated to use
RA_CTXand new CLI surface (set IFACE ...,clear IFACE,[show] ...).
127-127: LGTM!Context type updated to
cli_context.
133-133: LGTM!Registration updated to
cli_context_register.cli/log.c (2)
50-63: LGTM!
need_quotemade public, consistent with header declaration.
65-78: LGTM!
trace_cmdrefactored to useec_strvecdirectly, removing redundant vector allocation.cli/ecoli.c (3)
26-26: LGTM: Callback parameter type simplified.The parameter change from
cmd_cb_t *cbtocmd_cb_t cbis correct becausecmd_cb_tis now a function pointer type per the updated typedef incli/gr_cli.h(line 32).
180-180: LGTM: arg_ip and arg_ip_net made public.Exposing these functions is consistent with the new declarations in
cli/gr_cli.h(lines 49, 52) and enables unified IP parsing across address families.Also applies to: 199-199
67-67: Confirm no context matching relies on EC_NODE_STR. No directnode->typechecks found—ensure parsing/completion still matches commands correctly.modules/policy/cli/snat44.c (4)
87-87: LGTM: Context macro introduced.The
SNAT_CTXmacro follows the same pattern as other modules (e.g.,DNAT_CTXinmodules/policy/cli/dnat44.c).
94-94: LGTM: "interface|iface" alternative syntax.Allowing both
interfaceandifaceimproves usability by accepting the common abbreviation.Also applies to: 111-111
126-126: LGTM: Optional "show" argument.Wrapping
showin brackets[show]makes the argument optional, allowing users to omit it (consistent with other modules per the PR objectives).
133-139: LGTM: Context type and registration updated.The change from
struct gr_cli_contexttostruct cli_contextand fromregister_contexttocli_context_registeris consistent with the broader refactor (seecli/exec.clines 15-17).modules/infra/cli/trace.c (3)
110-111: LGTM: Context macros introduced.
LOGGING_CTXandTRACE_CTXmacros follow the same pattern as other modules.
157-157: LGTM: Trailing comma issues resolved.The CLI_COMMAND invocations at lines 157, 165, and 174 no longer have trailing commas after the description string, addressing the past review comments.
Based on past review comments.
Also applies to: 165-165, 174-174
182-188: LGTM: Context type and registration updated.Consistent with the refactor across all CLI modules.
modules/infra/cli/affinity.c (3)
111-113: LGTM: Context macros introduced.The hierarchical context macros (
AFFINITY_ARG,CPU_CTX,QMAP_CTX) follow the same pattern as other modules.
134-134: LGTM: Optional "show" argument.Consistent with the PR objective to allow omitting show arguments.
Also applies to: 151-151
158-164: LGTM: Context type and registration updated.Consistent with the broader refactor.
modules/policy/cli/conntrack.c (4)
143-146: LGTM: Context macros introduced.The
CONNTRACK_ARG,CONNTRACK_CTX, andCONNTRACK_CONFIG_CTXmacros follow the established pattern.
158-191: LGTM: Expanded help for config set command.The detailed
with_helpentries for each timeout parameter improve usability by clearly documenting the configuration options.
198-198: LGTM: Optional "show" argument.Consistent with the PR objective.
Also applies to: 205-205
212-218: LGTM: Context type and registration updated.Consistent with the broader refactor.
cli/gr_cli.h (4)
14-20: LGTM: Public API types renamed and simplified.The renaming from
gr_cli_contexttocli_context, typedef simplification to function pointers, and STAILQ field rename fromentriestonextare consistent throughout the codebase.
32-36: LGTM: Callback typedef changed to function pointer.The change from
typedef cmd_status_t(cmd_cb_t)(...)totypedef cmd_status_t(*cmd_cb_t)(...)simplifies usage by makingcmd_cb_titself a pointer type, eliminating the need forcmd_cb_t *in function signatures.
38-41: LGTM: ctx_arg expanded with help field.Adding the
helpfield tostruct ctx_argenables inline documentation for context arguments.
49-49: LGTM: arg_ip and arg_ip_net exposed.Exposing these functions enables unified IP address parsing across IPv4 and IPv6, consistent with their implementation in
cli/ecoli.c.Also applies to: 52-52
smoke/graph_svg_test.sh (1)
16-16: LGTM: CLI invocation updated.The command change to
grcli graph show briefaligns with the new noun-first CLI syntax introduced in this PR.smoke/vrf_forward_test.sh (1)
12-23: LGTM: CLI invocations updated to new syntax.All
grclicommands have been updated to the new noun-first syntax (e.g.,interface add,address add,route add), consistent with the CLI refactor. Test logic and topology remain unchanged.cli/quit.c (1)
15-21: LGTM!Type rename and registration API update are consistent with the PR-wide migration to
cli_context.smoke/ip_forward_test.sh (1)
10-16: LGTM!Command syntax correctly migrated to noun-first style (
grcli <resource> add).smoke/_init.sh (3)
4-4: Good addition!Adding
pipefailimproves error detection in pipeline sequences.
24-34: LGTM!Cleanup commands correctly migrated to new syntax (
grcli interface show/del).
96-118: LGTM!Startup and configuration commands correctly updated to new CLI surface.
docs/grout-frr.7.md (1)
123-140: LGTM!Documentation correctly reflects the new per-entry route output format.
modules/infra/cli/graph.c (2)
32-47: LGTM!Command syntax correctly updated to support optional
showargument. Help text addition improves usability.
49-56: LGTM!Type and registration API consistently updated to
cli_context.modules/ip6/cli/icmp6.c (2)
203-239: LGTM!Command syntax correctly updated to support
interface|ifacealiases. Help text additions improve usability.
241-248: LGTM!Type and registration API consistently updated to
cli_context.modules/infra/cli/gr_cli_event.h (1)
9-16: LGTM!Public API consistently renamed and STAILQ field unified to
next.smoke/dnat44_test.sh (2)
10-16: LGTM!Commands correctly migrated to noun-first syntax (
grcli dnat44 add/show,grcli nexthop show).
33-34: LGTM!Verification commands correctly use new syntax.
smoke/_init_frr.sh (1)
14-240: LGTM! Command syntax updates are consistent.The script correctly adopts the new CLI surface (
grcli interface add,grcli address show,grcli route show) while preserving all verification logic and retry flows.smoke/nexthop_ageing_test.sh (1)
12-83: LGTM! Command syntax updates are correct.All CLI invocations properly updated to the new surface (
grcli nexthop show,grcli address show,grcli interface add, etc.). Test logic preserved.Note: Shellcheck SC1087 warnings on lines 66, 67, 81, 82 are false positives—
$p0and$p1are scalar variables, not arrays.cli/main.c (1)
126-140: LGTM! Status helper is correct.The
check_statushelper correctly maps success/empty/exit to 0 and errors to -1.modules/infra/cli/address.c (4)
19-28: LGTM! Registration logic is sound.The validation ensures non-null providers, required function pointers, and unique address families. The STAILQ_INSERT_TAIL ordering is appropriate.
30-56: LGTM! Add/del handlers are correct.Both handlers iterate registered providers, use
arg_ip_netto probe for family match, and returnENOPROTOOPTif no provider handles the request. Logic is clean and follows the pluggable pattern.
58-86: LGTM! List handler is well-structured.Interface filtering, table construction, and provider iteration are all correct. Error propagation from
ops->listis properly handled.
88-142: LGTM! CLI wiring is consistent.The context initialization, command registration, and constructor-based registration follow the new CLI patterns correctly.
README.md (1)
102-251: LGTM! Documentation updates are comprehensive.All examples, logs, and interactive sessions correctly reflect the new CLI surface (
interface add,address show,route show,nexthop show, etc.). The documentation is consistent with the code changes across the PR.modules/srv6/cli/localsid.c (2)
124-128: LGTM! Type rename is correct.The formatter type correctly updated from
gr_cli_nexthop_formattertocli_nexthop_formatter, consistent with the broader API migration.
199-222: LGTM! Context and registration updates are correct.The macro change to
NEXTHOP_ADD_CTX, type rename tocli_context, and updated registration calls (cli_context_register,cli_nexthop_formatter_register) all align with the new CLI patterns.cli/interact.c (2)
73-73: LGTM! Shell lex update is correct.The change from
ec_node_sh_lextoec_node_sh_lex_expandaligns with the broader refactor to standardize shell lex construction.
92-104: LGTM! Execution flow updates are correct.The updated
exec_linesignature with the trace parameter (set tofalsefor interactive mode) and the cleaner exit handling viaexit_okare both correct.modules/infra/cli/gr_cli_nexthop.h (2)
11-26: LGTM! API renames are correct.The struct, field, and function renames from
gr_cli_*tocli_*are consistent with the broader API migration. The STAILQ field rename fromentriestonextstandardizes the linkage field name.
28-31: LGTM! New macros centralize nexthop wiring.The
NEXTHOP_ARG,NEXTHOP_CTX, andNEXTHOP_ADD_CTXmacros provide clean, consistent wiring for nexthop contexts across modules.modules/ip/cli/route.c (5)
93-99: LGTM!The
route_opsregistration structure correctly maps the AF and handlers for the new CLI route operations pattern.
119-119: LGTM!Consistent with the rename from
gr_cli_format_nexthoptocli_nexthop_format.
137-140: LGTM!Initialization correctly adopts the new registration pattern with
cli_route_ops_registerandcli_event_printer_register.
51-66: Signature matches cli_route_ops.list definition The route4_list prototype aligns exactly with the function pointer in gr_cli_l3.h.
74-76: DEST parsing is correct
GR_IP4_ROUTE_GETrequest takes only anip4_addr_t dest, so replacingarg_ip4_netwitharg_ip4is appropriate.modules/infra/cli/nexthop.c (3)
20-27: LGTM!The STAILQ and registration function correctly adopt the
cli_nexthop_formatternaming andnextfield convention.
485-491: LGTM!Initialization correctly registers the CLI context, event printer, and all three nexthop formatters using the new naming conventions.
396-440: Macros defined NEXTHOP_ADD_CTX and NEXTHOP_CTX are declared in modules/infra/cli/gr_cli_nexthop.h (lines 28–30).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/ip6/cli/router_advert.c (1)
61-66: Correct the arg_u16 flag checks
arg_u16()returns non-zero on success, so the!inversion flips the meaning and sets theset_*flags when the argument is absent/invalid. This makes us request an update even when no value was provided. Drop the negation.- if (!arg_u16(p, "IT", &req.interval)) - req.set_interval = 1; + if (arg_u16(p, "IT", &req.interval)) + req.set_interval = 1; - if (!arg_u16(p, "LT", &req.lifetime)) - req.set_lifetime = 1; + if (arg_u16(p, "LT", &req.lifetime)) + req.set_lifetime = 1;modules/ip6/cli/address.c (1)
54-71: Honor iface filter in IPv6 list (parity with IPv4)Currently lists all entries even when iface_id is specified. Add the filter to match modules/ip/cli/address.c behavior.
gr_api_client_stream_foreach (addr, ret, c, GR_IP6_ADDR_LIST, sizeof(req), &req) { - struct libscols_line *line = scols_table_new_line(table, NULL); + if (iface_id != GR_IFACE_ID_UNDEF && addr->iface_id != iface_id) + continue; + struct libscols_line *line = scols_table_new_line(table, NULL);
♻️ Duplicate comments (3)
modules/infra/cli/route.c (1)
30-41: Don’t clobber parse errors; preserve first meaningful errnoIf no ops match, you overwrite parser errno with ENOPROTOOPT, hiding the real cause (e.g., EINVAL for bad DEST). Keep the first non-ENOENT errno and only fall back to ENOPROTOOPT if nothing else was set. This affects add/del/get.
static cmd_status_t route_add(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_route_ops *ops; uint8_t net[64]; + int saved_errno = ENOPROTOOPT; STAILQ_FOREACH (ops, &route_ops, next) { - if (arg_ip_net(p, "DEST", net, false, ops->af) == 0) - return ops->add(c, p); + if (arg_ip_net(p, "DEST", net, false, ops->af) == 0) + return ops->add(c, p); + if (errno != ENOENT) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno; return CMD_ERROR; } static cmd_status_t route_del(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_route_ops *ops; uint8_t net[64]; + int saved_errno = ENOPROTOOPT; STAILQ_FOREACH (ops, &route_ops, next) { - if (arg_ip_net(p, "DEST", net, false, ops->af) == 0) - return ops->del(c, p); + if (arg_ip_net(p, "DEST", net, false, ops->af) == 0) + return ops->del(c, p); + if (errno != ENOENT) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno; return CMD_ERROR; } static cmd_status_t route_get(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_route_ops *ops; uint8_t ip[64]; + int saved_errno = ENOPROTOOPT; STAILQ_FOREACH (ops, &route_ops, next) { - if (arg_ip(p, "DEST", ip, ops->af) == 0) - return ops->get(c, p); + if (arg_ip(p, "DEST", ip, ops->af) == 0) + return ops->get(c, p); + if (errno != ENOENT) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno; return CMD_ERROR; }Also applies to: 43-54, 81-92
modules/ip6/cli/address.c (1)
67-68: Fix IPv6 formatter argument (prints junk otherwise)IP6_F expects struct in6_addr*. Pass &addr->addr.ip, not &addr->addr (ip6_net).
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);cli/exec.c (1)
168-189: Use portable error functions.Lines 180 use
strerrordesc_npandstrerrorname_np, which are GNU-specific and will break on non-glibc platforms (FreeBSD, musl, macOS, etc.).Apply this diff to use portable
strerror:case EXEC_CMD_FAILED: - errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); +#if defined(__GLIBC__) && defined(_GNU_SOURCE) + errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); +#else + errorf("command failed: %s", strerror(errno)); +#endif break;
🧹 Nitpick comments (8)
smoke/config_test.sh (1)
47-48: Make emptiness checks robust.
wc -lcan be non-zero due to headers/blank lines. Grep for any non-empty output instead.Apply this diff:
-if [ "$(grcli nexthop show | wc -l)" -ne 0 ]; then fail "Nexthop list is not empty" ; fi -if [ "$(grcli route show | wc -l)" -ne 0 ]; then fail "route list is not empty" ; fi +if grcli nexthop show | grep -q .; then fail "Nexthop list is not empty"; fi +if grcli route show | grep -q .; then fail "Route list is not empty"; ficli/complete.c (1)
118-122: Clamp width after stripping colon prefix.Avoid negative field widths when choices are shorter than the prefix.
- comp_width -= colon_prefix; + comp_width -= colon_prefix; + if (comp_width < 0) + comp_width = 0;modules/infra/cli/affinity.c (1)
158-165: Nit: context name consistency.Consider using .name = "affinity" (single token) for parity with other contexts (graph, address), unless the space is intentional for grouping.
modules/infra/cli/address.c (1)
70-82: Optional: guard table allocation failures.Handle scols_new_table() returning NULL to avoid deref on OOM.
- struct libscols_table *table = scols_new_table(); + struct libscols_table *table = scols_new_table(); + if (!table) + return CMD_ERROR;README.md (1)
250-252: Doc clarity: show is optional now.Consider noting both forms: “grcli graph brief” and “grcli graph show brief” to match the new “graph [show] …” grammar.
cli/interact.c (1)
72-75: Guard shlex creation failureCheck ec_node_clone/ec_node_sh_lex_expand return. If shlex is NULL, set_node(edit, shlex) may degrade completion silently. Log and fallback to cmdlist to keep completion usable.
- // required for command completion in ec_editline_gets - shlex = ec_node_sh_lex_expand(EC_NO_ID, ec_node_clone(cmdlist)); - ec_editline_set_node(edit, shlex); + // required for command completion in ec_editline_gets + struct ec_node *clone = ec_node_clone(cmdlist); + shlex = ec_node_sh_lex_expand(EC_NO_ID, clone); + if (shlex) + ec_editline_set_node(edit, shlex); + else { + errorf("failed to init shlex completion"); + ec_node_free(clone); + ec_editline_set_node(edit, cmdlist); + }modules/infra/cli/route.c (2)
18-28: Avoid relying on assert for AF uniqueness in productionasserts vanish in NDEBUG builds; duplicate AFs would slip in. Prefer a runtime guard with a clear error/log.
void cli_route_ops_register(struct cli_route_ops *ops) { assert(ops != NULL); assert(ops->add != NULL); assert(ops->del != NULL); assert(ops->get != NULL); assert(ops->list != NULL); struct cli_route_ops *o; - STAILQ_FOREACH (o, &route_ops, next) - assert(ops->af != o->af); + STAILQ_FOREACH (o, &route_ops, next) { + if (ops->af == o->af) { + errno = EEXIST; + errorf("route ops already registered for AF=%d", ops->af); + return; + } + } STAILQ_INSERT_TAIL(&route_ops, ops, next); }
64-69: Check table allocation and guard scols usagescols_new_table can fail; add a NULL-check to avoid deref. Also propagate failure.
- struct libscols_table *table = scols_new_table(); + struct libscols_table *table = scols_new_table(); + if (!table) + return CMD_ERROR;Also applies to: 75-79
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
README.md(5 hunks)api/gr_net_types.h(1 hunks)cli/complete.c(4 hunks)cli/ecoli.c(3 hunks)cli/exec.c(5 hunks)cli/exec.h(2 hunks)cli/gr_cli.h(3 hunks)cli/interact.c(2 hunks)cli/log.c(2 hunks)cli/log.h(1 hunks)cli/main.c(2 hunks)meson.build(1 hunks)modules/infra/cli/address.c(1 hunks)modules/infra/cli/affinity.c(3 hunks)modules/infra/cli/events.c(3 hunks)modules/infra/cli/gr_cli_l3.h(1 hunks)modules/infra/cli/graph.c(2 hunks)modules/infra/cli/iface.c(5 hunks)modules/infra/cli/meson.build(1 hunks)modules/infra/cli/nexthop.c(14 hunks)modules/infra/cli/route.c(1 hunks)modules/infra/cli/stats.c(3 hunks)modules/ip/cli/address.c(7 hunks)modules/ip/cli/route.c(7 hunks)modules/ip6/cli/address.c(7 hunks)modules/ip6/cli/route.c(7 hunks)modules/ip6/cli/router_advert.c(2 hunks)modules/policy/cli/conntrack.c(2 hunks)modules/policy/cli/dnat44.c(4 hunks)modules/policy/cli/snat44.c(3 hunks)modules/srv6/cli/route.c(4 hunks)smoke/_init.sh(4 hunks)smoke/_init_frr.sh(7 hunks)smoke/config_test.sh(1 hunks)smoke/cross_vrf_forward_frr_test.sh(1 hunks)smoke/ip6_add_del_test.sh(1 hunks)smoke/ip6_builtin_icmp_test.sh(1 hunks)smoke/ip6_forward_test.sh(1 hunks)smoke/ip6_rs_ra_test.sh(1 hunks)smoke/ip6_same_peer_test.sh(1 hunks)smoke/srv6_test.sh(3 hunks)smoke/vrf_forward_frr_test.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- meson.build
- modules/infra/cli/meson.build
- smoke/ip6_forward_test.sh
- cli/ecoli.c
- smoke/srv6_test.sh
- smoke/ip6_builtin_icmp_test.sh
- modules/infra/cli/nexthop.c
- modules/infra/cli/events.c
- modules/policy/cli/snat44.c
- modules/infra/cli/gr_cli_l3.h
- cli/log.c
- smoke/ip6_same_peer_test.sh
- modules/infra/cli/iface.c
- smoke/_init.sh
- smoke/ip6_add_del_test.sh
- cli/log.h
- cli/main.c
- modules/policy/cli/conntrack.c
🧰 Additional context used
🧬 Code graph analysis (20)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
smoke/cross_vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/infra/cli/graph.c (3)
modules/infra/cli/affinity.c (2)
ctx_init(115-156)constructor(163-165)modules/infra/cli/events.c (2)
ctx_init(55-62)constructor(69-71)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/affinity.c (2)
modules/infra/api/affinity.c (1)
rxq_list(62-79)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/dnat44.c (2)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/route.c (5)
cli/ecoli.c (3)
arg_ip_net(199-212)arg_ip(180-189)with_help(15-24)cli/gr_cli.h (1)
arg_u16(64-70)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (5)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/ip6/cli/address.c (3)
addr_list(54-71)addr_add(18-34)addr_del(36-52)modules/ip/control/address.c (3)
addr_list(149-175)addr_add(63-115)addr_del(117-147)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip/cli/route.c (5)
modules/ip/control/route.c (4)
route4_list(416-423)route4_add(262-306)route4_del(308-321)route4_get(323-338)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/stats.c (3)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)modules/infra/cli/affinity.c (1)
ctx_init(115-156)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (2)
arg_ip_net(199-212)arg_str(99-109)modules/infra/cli/iface.c (3)
iface_from_name(92-105)complete_iface_names(70-90)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
smoke/vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
modules/ip6/cli/router_advert.c (4)
modules/infra/cli/iface.c (1)
complete_iface_names(70-90)cli/ecoli.c (1)
with_help(15-24)cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/route.c (4)
modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (27)
api/gr_net_types.h (1)
140-140: Anchoring fix for IPv4 network validation.Previously, the
$anchor was only applied to the IPv6 alternative, allowing IPv4 network strings to match with trailing garbage. Moving it outside the group now enforces end-of-string matching for both address families consistently.modules/infra/cli/stats.c (2)
61-64: Default-to-software and explicit hardware flag handling look correct.
152-153: New stats context and command wiring LGTM.Context macro, reset/show commands, and registration align with the new CLI framework.
Also applies to: 157-159, 161-163, 165-165, 200-200, 206-206
modules/ip6/cli/route.c (1)
51-53: IPv6 route ops migration and formatting look good.List/get handlers, ops registration, and event printing align with the new L3 ops API.
Also applies to: 61-66, 74-75, 86-86, 92-98, 118-118, 127-134, 137-139
modules/ip/cli/route.c (1)
51-53: IPv4 route ops refactor LGTM.Handlers, ops registration, and event formatting are consistent with the unified L3 routing interface.
Also applies to: 61-66, 74-76, 86-86, 93-99, 119-119, 128-135, 137-139
cli/exec.h (1)
10-11: API change: manually verify exec_line/exec_args usages
Automated grep filters matched no files—ensure every call site is updated to pass the newtraceflag.modules/infra/cli/graph.c (1)
35-42: LGTM: context migration and grammar update look correct.
- Optional [show] keeps backward compatibility with existing "graph brief".
- Flag handling for "full" aligns with help text.
Also applies to: 49-56
modules/infra/cli/affinity.c (1)
118-131: CLI context wiring and [show] optionality look good.Grammar aligns with examples (affinity cpus/qmap). API calls and helpers are correct.
Also applies to: 134-136, 137-149, 151-153
modules/infra/cli/address.c (1)
17-28: Well-structured addr ops registry; unified CLI looks solid.
- AF uniqueness assertions and required callbacks are good.
- add/del/list dispatching is clean; IFACE filter handled correctly.
Also applies to: 30-41, 43-54, 56-84, 86-101, 104-131, 133-140
cli/interact.c (1)
92-104: exec_line(trace=false) integration LGTMNew status handling and explicit exit path align with exec.c’s consolidated error logging.
modules/policy/cli/dnat44.c (1)
94-99: API migration to cli_ looks correct*Formatter type rename, context registration, and command wiring match the new cli_* APIs. Grammar/help parameters look consistent.
Please run smoke tests for dnat44 to confirm the top-level noun after fixing the mismatch.
Also applies to: 145-147, 105-113, 117-123, 127-133
modules/ip6/cli/address.c (1)
8-8: CLI L3 ops/event migration looks goodSwitch to cli_addr_ops, registration, and event printer aligns with the new framework.
Also applies to: 27-28, 45-46, 73-78, 102-109, 111-114
modules/srv6/cli/route.c (2)
128-132: SRv6 formatter and registrations LGTMcli_nexthop_formatter and cli_context registrations align with the new infra.
Also applies to: 189-190
134-135: CLI contexts/commands read wellNew TUNSRC_CTX and the set/clear/show wiring are consistent; grammar/help are coherent.
Confirm NEXTHOP_ADD_CTX(root) is included here (or via headers) to avoid missing-symbol build errors.
Also applies to: 153-159, 163-167, 172-175
cli/gr_cli.h (1)
14-52: LGTM! API surface refactoring is consistent.The typedef/struct renames (gr_cli_* → cli_*), callback signature changes, and new IP parsing helpers align with the PR's CLI redesign objectives. The changes maintain consistency across the codebase.
modules/ip/cli/address.c (4)
8-8: LGTM! New L3 header included for registration pattern.The addition of
gr_cli_l3.hprovides thecli_addr_opsregistration API used below.
27-27: LGTM! Argument name unified.The change from
IP_NETtoADDRaligns with the unified CLI grammar across IPv4/IPv6 contexts.Also applies to: 45-45
54-73: LGTM! addr_list refactored for new registration pattern.The signature change to return
intand acceptiface_id/tableparameters simplifies the function. The filtering logic at lines 60-61 correctly skips non-matching interfaces, and the function now integrates cleanly with the centralized address listing inmodules/infra/cli/address.c.
75-80: LGTM! Registration pattern adopted.The
cli_addr_opsstruct and registration calls replace the old context init pattern, aligning with the broader CLI refactoring. This matches the IPv6 address module structure (seemodules/ip6/cli/address.c).Also applies to: 114-115
cli/exec.c (8)
14-18: LGTM! Context management updated.The type rename to
cli_contextand linkage field change tonextalign with the API refactoring ingr_cli.h. The registration function is now properly exported.
20-38: LGTM! Context traversal updated.The
STAILQ_FOREACHnow uses thenextlinkage field consistently with the type changes.
40-51: LGTM! Callback retrieval corrected.The callback type usage is now consistent with the pointer-form typedef in
gr_cli.h.
53-115: LGTM! Suggestion helper added.The
get_suggestionsfunction provides completion suggestions for invalid arguments. The logic correctly handles trailing spaces and empty arguments to retrieve all available options.
117-166: LGTM! Print suggestions helper added.The
print_suggestionsfunction provides clear visual feedback for invalid arguments with TTY-aware color formatting.
191-242: LGTM! exec_strvec updated with trace support.The function now expands arguments to full names (line 208), traces commands when requested (lines 211-212), and delegates error reporting to
print_status.
244-272: LGTM! exec_line updated with trace parameter.The signature change adds trace support and improves error messages for lexing failures.
274-297: LGTM! exec_args updated with trace parameter.The signature change adds trace support consistently with
exec_line.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
smoke/nexthop_ageing_test.sh (1)
56-59: Update interactive commands to noun-first CLI.
show nexthop/show ip addressstill use the old verb-first grammar; after this refactor they no longer resolve, so the smoke test will fail. Switch them tonexthop showandaddress show.grcli <<EOF -show nexthop -show ip address +nexthop show +address show EOFmodules/ip6/cli/address.c (1)
59-68: Filter by iface_id and fix IP6_F argument type.
- Add iface_id filter (parity with IPv4).
- IP6_F expects struct in6_addr*; pass &addr->addr.ip.
gr_api_client_stream_foreach (addr, ret, c, GR_IP6_ADDR_LIST, sizeof(req), &req) { - struct libscols_line *line = scols_table_new_line(table, NULL); + if (iface_id != GR_IFACE_ID_UNDEF && addr->iface_id != iface_id) + continue; + struct libscols_line *line = scols_table_new_line(table, NULL); struct gr_iface *iface = iface_from_id(c, addr->iface_id); if (iface != NULL) scols_line_sprintf(line, 0, "%s", iface->name); else scols_line_sprintf(line, 0, "%u", addr->iface_id); free(iface); - scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen); }modules/srv6/cli/route.c (1)
153-159: Remove unused VRF option from srv6 tunsrc CLI command
The CLI grammar exposesvrf VRF, butsrv6_tunsrc_setdoesn’t parse or send it (novrf_idingr_srv6_tunsrc_set_req). Remove the VRF argument or implement full VRF support.modules/ip/cli/address.c (1)
54-73: Initialize ret to avoid UB; optional: drop redundant client-side filter
retis returned uninitialized if the stream yields no rows. Initialize to 0. Theiface_idcheck is redundant sincereq.iface_idalready filters server-side.static int addr_list(struct gr_api_client *c, uint16_t iface_id, struct libscols_table *table) { struct gr_ip4_addr_list_req req = {.vrf_id = GR_VRF_ID_ALL, .iface_id = iface_id}; const struct gr_ip4_ifaddr *addr; - int ret; + int ret = 0; gr_api_client_stream_foreach (addr, ret, c, GR_IP4_ADDR_LIST, sizeof(req), &req) { - if (iface_id != GR_IFACE_ID_UNDEF && addr->iface_id != iface_id) - continue; + /* optional: server already filters by iface_id */ + /* if (iface_id != GR_IFACE_ID_UNDEF && addr->iface_id != iface_id) continue; */modules/infra/cli/nexthop.c (2)
16-21: Include <errno.h> for errno usage
format_nexthop_info_l3returns-errnobut this file doesn’t include<errno.h>.#include <assert.h> #include <stdio.h> #include <unistd.h> +#include <errno.h>
78-103: Declare loop variable used by gr_nh_flags_foreach
fis used but not declared, causing a compile error.static ssize_t format_nexthop_info_l3(char *buf, size_t len, const void *info) { const struct gr_nexthop_info_l3 *l3 = info; ssize_t n = 0; + gr_nh_flag_t f; SAFE_BUF(snprintf, len, "af=%s", gr_af_name(l3->af)); if (l3->af != GR_AF_UNSPEC) {If
gr_nh_flag_tisn’t available in your headers, use an appropriate unsigned integer type.
♻️ Duplicate comments (8)
cli/complete.c (1)
87-138: Stop clobbering the caller’s cmdlist.Still reassigning
cmdlistand freeing it later; if wrapping fails you’ve lost the original pointer, and on success you free the caller’s tree. Please keep the wrapper separate exactly as already suggested earlier.Apply:
- struct ec_strvec *vec = NULL; + struct ec_strvec *vec = NULL; + struct ec_node *root = NULL; @@ - if ((cmdlist = bash_complete_node(cmdlist)) == NULL) { + if ((root = bash_complete_node(cmdlist)) == NULL) { errorf("bash_complete_node: %s", strerror(errno)); goto end; } @@ - if ((cmpl = ec_complete(cmdlist, buf)) == NULL) { + if ((cmpl = ec_complete(root, buf)) == NULL) { errorf("ec_complete: %s", strerror(errno)); goto end; } @@ - ec_node_free(cmdlist); + ec_node_free(root);cli/main.c (2)
183-185: Respect -x for non-interactive commands.Hard-coding
falsestill drops tracing for argv execution; feed throughopts.trace_commandslike before.- status = exec_args(client, cmdlist, argc, (const char *const *)argv, false); + status = exec_args( + client, cmdlist, argc, (const char *const *)argv, opts.trace_commands + );
192-195: Break on EXEC_CMD_EXIT when replaying files.Scripts still can’t stop themselves; bail out of the loop as soon as exit is returned, then run the existing err-exit check.
- while (fgets(buf, sizeof(buf), opts.cmds_file)) { - status = exec_line(client, cmdlist, buf, opts.trace_commands); - if (check_status(status) < 0 && opts.err_exit) + while (fgets(buf, sizeof(buf), opts.cmds_file)) { + status = exec_line(client, cmdlist, buf, opts.trace_commands); + if (status == EXEC_CMD_EXIT) + break; + if (check_status(status) < 0 && opts.err_exit) goto end; }modules/infra/cli/route.c (1)
34-91: Stop replacing parser errno with ENOPROTOOPTSame as earlier review: when
arg_ip_net/arg_ipfails (typoed DEST), we stomp its errno and surfaceENOPROTOOPT. Retain the first real errno (skip ENOENT) and only default toENOPROTOOPTif no AF matched.cli/exec.c (3)
168-181: Guard glibc‑specific strerror helpers; add portable fallback.strerrordesc_np/strerrorname_np are glibc‑only.
- case EXEC_CMD_FAILED: - errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); - break; + case EXEC_CMD_FAILED: +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 36) + errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); +#else + errorf("command failed: %s", strerror(errno)); +#endif + break;
110-115: Set errno when no suggestions are found.Avoid reporting “Success” or a silent path; set a meaningful errno.
out: ec_comp_free(c); ec_strvec_free(args); - ec_dict_free(attrs); - return sug; + ec_dict_free(attrs); + if (sug == NULL && errno == 0) + errno = ENOENT; + return sug;
40-51: Portable callback retrieval: read a cmd_cb_t and dereference.*Stop casting void* to function pointer. Pair with with_callback change to store cmd_cb_t*.
-static cmd_cb_t find_cmd_callback(struct ec_pnode *parsed) { +static cmd_cb_t find_cmd_callback(struct ec_pnode *parsed) { const struct ec_pnode *p; for (p = parsed; p != NULL; p = EC_PNODE_ITER_NEXT(parsed, p, true)) { const struct ec_node *node = ec_pnode_get_node(p); - cmd_cb_t cb = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR); - if (cb != NULL) - return cb; + cmd_cb_t *cbp = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR); + if (cbp != NULL) + return *cbp; } return errno_set_null(EOPNOTSUPP); }modules/policy/cli/dnat44.c (1)
100-101: Fix context noun mismatch: “dnat44” vs “nat44”.DNAT_CTX is "dnat44" while ctx.name is "nat44", yielding “nat44 dnat44 …”. Align them.
static struct cli_context ctx = { - .name = "nat44", + .name = "dnat44", .init = ctx_init, };Also applies to: 139-142
🧹 Nitpick comments (3)
cli/gr_cli.h (1)
32-37: Avoid storing function pointers as void. Use pointer-to-callback.*Casting function pointers to/from void* is non‑portable C. Store a cmd_cb_t* in attrs and dereference on read. No signature change needed.
Implement in with_callback (cli/ecoli.c):
struct ec_node *with_callback(cmd_cb_t cb, struct ec_node *node) { if (node == NULL) return NULL; struct ec_dict *attrs = ec_node_attrs(node); cmd_cb_t *cbp = malloc(sizeof(*cbp)); if (cbp == NULL) { ec_node_free(node); return NULL; } *cbp = cb; if (attrs == NULL || ec_dict_set(attrs, CALLBACK_ATTR, cbp, free) < 0) { free(cbp); ec_node_free(node); node = NULL; } return node; }And in exec.c (see comment there), fetch cmd_cb_t* and deref.
cli/exec.c (1)
55-59: Remove unused local attrs in get_suggestions.Minor cleanup.
- struct ec_dict *attrs = NULL; ... - ec_dict_free(attrs);Also applies to: 111-114
modules/infra/cli/nexthop.c (1)
22-27: Guard against duplicate formatter registrationsOptional, but helpful: assert uniqueness on
typeto avoid ambiguous formatters.void cli_nexthop_formatter_register(struct cli_nexthop_formatter *f) { assert(f->name != NULL); assert(f->type != GR_NH_T_ALL); assert(f->format != NULL); + struct cli_nexthop_formatter *o; + STAILQ_FOREACH (o, &formatters, next) + assert(f->type != o->type); STAILQ_INSERT_TAIL(&formatters, f, next); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(3 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/api.c(1 hunks) -
main/gr_module.h(2 hunks) -
main/grout.init(1 hunks) -
main/module.c(4 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(1 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(5 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks) -
subprojects/ecoli.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/policy/cli/policy.h
- modules/ip6/cli/ip.h
- modules/ip/cli/ip.h
✅ Files skipped from review due to trivial changes (1)
- docs/grout-frr.7.md
🚧 Files skipped from review as they are similar to previous changes (27)
- modules/ip6/cli/icmp6.c
- smoke/ip6_forward_test.sh
- modules/infra/cli/gr_cli_l3.h
- meson.build
- smoke/graph_svg_test.sh
- cli/log.h
- smoke/ip_add_del_test.sh
- modules/ip/cli/icmp.c
- modules/infra/cli/gr_cli_event.h
- smoke/cross_vrf_forward_frr_test.sh
- modules/srv6/cli/localsid.c
- smoke/vrf_forward_frr_test.sh
- modules/infra/cli/port.c
- cli/quit.c
- smoke/ip_builtin_icmp_test.sh
- smoke/_init.sh
- smoke/ip6_rs_ra_test.sh
- main/grout.init
- smoke/ip6_same_peer_test.sh
- smoke/ip_forward_test.sh
- subprojects/ecoli.wrap
- cli/ecoli.c
- smoke/cross_vrf_forward_test.sh
- smoke/srv6_test.sh
- smoke/affinity_test.sh
- modules/infra/cli/events.c
- smoke/ipip_encap_test.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
PR: DPDK/grout#294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Applied to files:
modules/infra/cli/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
PR: DPDK/grout#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/cli/meson.build
🧬 Code graph analysis (28)
main/api.c (1)
main/gr_module.h (1)
api_out(20-23)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/infra/cli/vlan.c (3)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)modules/infra/cli/port.c (2)
ctx_init(188-218)constructor(225-228)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/affinity.c (1)
cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/router_advert.c (3)
cli/ecoli.c (1)
with_help(15-24)cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/trace.c (2)
modules/infra/cli/affinity.c (1)
constructor(163-165)cli/exec.c (1)
cli_context_register(16-18)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/ipip/cli.c (2)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (2)
arg_ip_net(199-212)arg_str(99-109)modules/infra/cli/iface.c (2)
iface_from_name(92-105)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/route.c (5)
modules/ip/control/route.c (4)
route4_list(416-423)route4_add(262-306)route4_del(308-321)route4_get(323-338)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/graph.c (3)
modules/infra/cli/events.c (1)
ctx_init(55-62)cli/quit.c (1)
ctx_init(11-13)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/route.c (5)
cli/ecoli.c (3)
arg_ip_net(199-212)arg_ip(180-189)with_help(15-24)cli/gr_cli.h (1)
arg_u16(64-70)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/affinity.c (1)
ctx_init(115-156)cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
modules/infra/cli/nexthop.c (3)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (3)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip6/cli/route.c (5)
modules/ip6/control/route.c (1)
route6_list(454-461)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/main.c (3)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)cli/log.c (1)
is_tty(22-30)cli/interact.c (1)
interact(36-114)
modules/policy/cli/dnat44.c (2)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/infra/cli/stats.c (2)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
modules/policy/cli/snat44.c (4)
modules/infra/cli/affinity.c (1)
ctx_init(115-156)modules/policy/cli/dnat44.c (1)
ctx_init(102-137)modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 66-66: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 67-67: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 81-81: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 82-82: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ 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-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (25)
main/api.c (1)
218-218: LGTM: More precise error semantics.EBADMSG better represents a version string validation failure than EPROTO.
smoke/dnat44_test.sh (2)
10-16: Updated CLI grammar looks solid.These noun-first commands line up with the reworked contexts and cover the full setup path cleanly.
33-34: Nice targeted nexthop check.Using
type dnatkeeps the smoke assertion focused on the DNAT entries after the ping.GNUmakefile (1)
39-39: LGTM!The update-graph target correctly reflects the new noun-first CLI surface (
graph show brief).smoke/vrf_forward_test.sh (1)
12-23: LGTM!Test commands correctly updated to the new noun-first CLI surface.
main/gr_module.h (1)
41-41: LGTM!STAILQ linkage field renamed to
nextfor consistency across the codebase.Also applies to: 51-51
smoke/vlan_forward_test.sh (1)
12-17: LGTM!VLAN test commands correctly migrated to noun-first CLI syntax.
main/module.c (2)
26-43: LGTM!Handler registration and lookup correctly updated to use the
nextlinkage field. The explicitreturn NULLat line 43 is clear and correct.
54-59: LGTM!Module registration and initialization consistently use the
nextfield for STAILQ operations.Also applies to: 139-161
smoke/config_test.sh (1)
7-48: LGTM!Configuration test comprehensively exercises the new noun-first CLI surface with correct syntax across all command types (interface, nexthop, address, route, graph, stats).
api/gr_net_types.h (1)
140-140: Fix regex end-anchoring.Corrects the IP_ANY_NET_RE pattern to properly anchor both IPv4 and IPv6 alternatives. The old form only anchored the IPv6 alternative, potentially allowing IPv4 patterns with trailing garbage to match incorrectly.
README.md (3)
102-107: LGTM!Quickstart configuration examples correctly demonstrate the new noun-first CLI syntax.
152-230: LGTM!CLI documentation comprehensively covers the expanded noun-first command surface with accurate help text and tab-completion examples.
170-251: LGTM!Interactive CLI examples accurately demonstrate the new command structure and output formats, including the updated graph command invocation.
modules/policy/cli/conntrack.c (3)
143-147: CLI context macro usage looks consistentCONNTRACK_ARG/CTX/CONFIG_CTX wrappers align with the new CLI scaffolding; good reuse and clearer help strings.
205-207: Nice: optional[show]for listAllowing
[show]keeps noun-first grammar flexible and consistent with other modules.
212-213: Context type/registration migration LGTMSwitch to struct cli_context and cli_context_register matches the new core API.
Also applies to: 218-219
smoke/ip6_builtin_icmp_test.sh (1)
10-13: Command migration to noun-first CLI looks correctinterface add port and address add iface forms align with the new grammar.
smoke/ip6_add_del_test.sh (1)
9-11: Good coverage of add/del/show under new namespaces
- interface add/del sequence is clear.
- address show without args matches the new “[show] args optional” behavior.
Also applies to: 13-18
modules/ipip/cli.c (1)
158-161: Context type/registration migration LGTMcli_context and cli_context_register usage matches the new API.
Also applies to: 164-165
modules/infra/cli/graph.c (2)
36-42: Grammar tweak is consistent and clearerUsing root with "graph [show] [brief|full]" and adding help for "full" is consistent with other top-level commands.
49-56: Context type/registration migration LGTMSwitch to cli_context + cli_context_register is aligned with the new framework.
modules/infra/cli/meson.build (1)
5-5: Verified new CLI sources are present and included — modules/infra/cli/address.c and route.c exist and are listed in cli_src in meson.build.modules/infra/cli/iface.c (1)
489-494: Init guards + noun‑first commands look good.Context guards and unified INTERFACE_CTX wiring are consistent and robust.
Also applies to: 495-531
modules/srv6/cli/route.c (1)
128-135: Formatter/context registration LGTM.Renames to cli_nexthop_formatter and cli_context_register are correct and consistent.
Also applies to: 189-191
rjarry
force-pushed
the
cli-ctx-rework
branch
2 times, most recently
from
fee7a40 to
02825f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/cli/trace.c (1)
16-46: Initialize trace set/del request structs.
reqis stack garbage unless you zero it first. On the iface pathreq.allstays indeterminate, so the API call can behave as “all” even when you meant a single interface. Zero both structs before filling them.- struct gr_infra_packet_trace_set_req req; + struct gr_infra_packet_trace_set_req req = {0};Apply the same fix to
trace_del.
♻️ Duplicate comments (7)
smoke/config_test.sh (1)
7-48: Add fail-fast (set -euo pipefail)Script still runs without fail-fast—commands can fail silently (e.g. plain
grcli interface add ...). Please addset -euo pipefailafter the shebang.#!/bin/bash +set -euo pipefailcli/log.h (1)
6-21: Include<stdbool.h>
boolis exposed in this header but no longer guaranteed via other includes after the swap to<ecoli.h>. Add the explicit include to keep dependents compiling.+#include <stdbool.h> #include <ecoli.h>modules/ip6/cli/address.c (1)
67-68: Fix IPv6 printf argument.
IP6_Fexpects astruct in6_addr *; passing&addr->addr(anip6_net) emits garbage. Point it at the embedded address.- scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr, addr->addr.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &addr->addr.ip, addr->addr.prefixlen);cli/exec.c (3)
43-47: Stop casting callbacks throughvoid *.
We’re still pulling the callback straight out ofec_dict_get()as avoid *, which is non-portable for function pointers. Please store a stablecmd_cb_tobject, keep acmd_cb_t *in the dict, and dereference it here (adjustingwith_callback()accordingly).- cmd_cb_t cb = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR); - if (cb != NULL) - return cb; + cmd_cb_t *cbp = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR); + if (cbp != NULL) + return *cbp;
110-115: Set errno when no suggestions are found.
get_suggestions()still returns NULL with errno left untouched, so the caller prints “Success”. Please set a meaningful errno (e.g.ENOENT) before returning.out: ec_comp_free(c); ec_strvec_free(args); ec_dict_free(attrs); + if (sug == NULL && errno == 0) + errno = ENOENT; return sug;
177-186: Guard glibc-only strerror helpers.
strerrordesc_np/strerrorname_npbreak the build on non-glibc libc. Wrap them with a glibc check and fall back tostrerror(errno).case EXEC_CMD_FAILED: - errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); + errorf( +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 36) + "command failed: %s (%s)", + strerrordesc_np(errno), + strerrorname_np(errno) +#else + "command failed: %s", + strerror(errno) +#endif + ); break;modules/ip6/cli/route.c (1)
58-62: Format IPv6 prefixes with the address field.
IP6_Fexpects the address pointer; passing&route->destrelies on struct layout and prints garbage on some builds. Use&route->dest.iplike the event printer does.- scols_line_sprintf(line, 1, IP6_F "/%hhu", &route->dest, route->dest.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &route->dest.ip, route->dest.prefixlen);
🧹 Nitpick comments (2)
smoke/nexthop_ageing_test.sh (1)
63-79: Add braces around$p0/$p1before character classes.
$p0[[:space:]]and$p1[[:space:]]are parsed as array subscripts; ShellCheck (SC1087) flags them. Wrap the vars in braces so we match the intended literal:-grcli address show | grep -E "^$p0[[:space:]]+172\.16\.0\.1/24$" || fail "addresses were destroyed" -grcli address show | grep -E "^$p1[[:space:]]+172\.16\.1\.1/24$" || fail "addresses were destroyed" +grcli address show | grep -E "^${p0}[[:space:]]+172\.16\.0\.1/24$" || fail "addresses were destroyed" +grcli address show | grep -E "^${p1}[[:space:]]+172\.16\.1\.1/24$" || fail "addresses were destroyed"Repeat the same adjustment later in the script.
modules/policy/cli/conntrack.c (1)
62-72: Add default branch to proto switch for completeness.The proto switch lacks a default case, which could result in an empty PROTO cell if conn->proto holds an unexpected value.
switch (conn->proto) { case IPPROTO_ICMP: scols_line_set_data(fwd, 6, "ICMP"); break; case IPPROTO_TCP: scols_line_set_data(fwd, 6, "TCP"); break; case IPPROTO_UDP: scols_line_set_data(fwd, 6, "UDP"); break; + default: + scols_line_sprintf(fwd, 6, "%u", conn->proto); + break; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(3 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/grout.init(1 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(1 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(4 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks)
💤 Files with no reviewable changes (3)
- modules/ip6/cli/ip.h
- modules/ip/cli/ip.h
- modules/policy/cli/policy.h
✅ Files skipped from review due to trivial changes (1)
- docs/grout-frr.7.md
🚧 Files skipped from review as they are similar to previous changes (31)
- smoke/_init.sh
- smoke/ip6_same_peer_test.sh
- modules/ip/cli/icmp.c
- smoke/ip6_forward_test.sh
- smoke/srv6_test.sh
- smoke/cross_vrf_forward_test.sh
- smoke/ip6_add_del_test.sh
- modules/ip6/cli/icmp6.c
- GNUmakefile
- smoke/vrf_forward_test.sh
- modules/infra/cli/address.c
- modules/infra/cli/meson.build
- smoke/vlan_forward_test.sh
- modules/infra/cli/port.c
- modules/infra/cli/events.c
- modules/infra/cli/gr_cli_nexthop.h
- modules/srv6/cli/localsid.c
- cli/quit.c
- main/grout.init
- modules/infra/cli/gr_cli_event.h
- modules/infra/cli/gr_cli_l3.h
- smoke/cross_vrf_forward_frr_test.sh
- modules/srv6/cli/route.c
- modules/infra/cli/route.c
- smoke/snat44_test.sh
- api/gr_net_types.h
- cli/main.c
- cli/ecoli.c
- smoke/ipip_encap_test.sh
- modules/infra/cli/affinity.c
- smoke/ip_forward_test.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-03T13:42:52.138Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.138Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.c
📚 Learning: 2025-10-03T13:34:31.774Z
Learnt from: rjarry
PR: DPDK/grout#329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.774Z
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:
modules/ip/cli/route.cmodules/ip6/cli/route.c
🧬 Code graph analysis (25)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
cli/interact.c (1)
cli/exec.c (1)
exec_line(244-272)
modules/infra/cli/vlan.c (2)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
cli/log.h (1)
cli/log.c (4)
tty_init(16-20)is_tty(22-30)need_quote(50-63)trace_cmd(65-78)
modules/ipip/cli.c (2)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/policy/cli/dnat44.c (3)
modules/infra/cli/affinity.c (1)
constructor(163-165)modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/graph.c (2)
modules/infra/cli/events.c (1)
ctx_init(55-62)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/address.c (3)
cli/ecoli.c (1)
arg_ip4_net(214-216)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/ip6/cli/router_advert.c (2)
cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/nexthop.c (3)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/ecoli.c (1)
with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/stats.c (2)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/ip6/cli/address.c (4)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/ip/cli/address.c (3)
addr_list(54-73)addr_add(18-34)addr_del(36-52)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/policy/cli/snat44.c (3)
modules/infra/cli/affinity.c (1)
ctx_init(115-156)modules/policy/cli/dnat44.c (1)
ctx_init(102-137)cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
smoke/vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/ip/cli/route.c (6)
modules/ip/control/route.c (4)
route4_list(416-423)route4_add(262-306)route4_del(308-321)route4_get(323-338)cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/infra/cli/trace.c (1)
cli/exec.c (1)
cli_context_register(16-18)
cli/exec.h (1)
cli/exec.c (2)
exec_args(274-297)exec_line(244-272)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/affinity.c (2)
ctx_init(115-156)constructor(163-165)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/route.c (6)
modules/ip6/control/route.c (2)
route6_list(454-461)route6_get(360-375)cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
smoke/affinity_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
smoke/nexthop_ageing_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 63-63: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 64-64: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 78-78: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 79-79: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (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-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (2)
smoke/ip_add_del_test.sh (1)
9-16: Commands align with rewritten CLI grammar. Verb–noun order and address namespace usage match the CLI rework; no issues spotted.meson.build (1)
81-92: LGTM!The ecoli dependency version constraint bump from
>= 0.5.0to>= 0.8.0aligns with the PR objectives.
The new version includes multiple improvements: complete: order completion items and groups alphabetically sh_lex: store metadata about token positions in original string complete: add function to expand strvec shlex: add support for partial argument parsing editline: fix non ambiguous completion space Link: https://github.com/rjarry/libecoli/releases/tag/v0.8.0 Signed-off-by: Robin Jarry <[email protected]>
Use next everywhere. Signed-off-by: Robin Jarry <[email protected]>
Instead of "Protocol error" which may be mistaken for something related to network configuration, use "Bad message" which seems more appropriate. Signed-off-by: Robin Jarry <[email protected]>
Replace `<verb> <object> ...` with `<object> <verb> ...`. This feels more natural compared to other linux commands, such as iproute2. Also, it is simpler to fit new verbs that are specific to only one type of objects (e.g. `route get <dest>`). Get rid of the unnecessary `ip` and `ip6` intermediate prefixes and put their children directly at the root (suffixing the IPv6 variants with a 6). Update all examples, smoke tests and documentation accordingly. Signed-off-by: Robin Jarry <[email protected]>
The CLI headers are not exported. Replace gr_cli_*, with cli_*. Rename register_context to cli_context_register. Signed-off-by: Robin Jarry <[email protected]>
Consolidate IPv4 and IPv6 route command line interfaces into a unified route context that automatically detects address family from the input syntax. Replace separate route and route6 CLI commands with a single route interface that accepts both IPv4 and IPv6 destinations using the IP_ANY_NET_RE regex pattern. Create a registration mechanism for route operations that allows address family-specific implementations to register their handlers while presenting a unified user interface. Update documentation, smoke tests, and FRR integration scripts to use the single route command instead of separate IPv4 and IPv6 variants. Fix regex syntax error in IP_ANY_NET_RE pattern by adding missing closing parenthesis. Export IP parsing functions from CLI utilities to support the unified route parsing logic. Signed-off-by: Robin Jarry <[email protected]>
Merge separate IPv4 and IPv6 address management into a unified address CLI context that automatically detects address family from the input syntax. Replace the distinct address and address6 commands with a single address interface that accepts both IPv4 and IPv6 addresses using the IP_ANY_NET_RE regex pattern. Create a registration system for address operations that allows address family-specific implementations to register their handlers while presenting a cohesive user interface. Update documentation, smoke tests, and FRR integration to use the unified address command instead of separate protocols. Remove redundant CLI context initialization code and convert address family modules to register their operations with the common framework. Consolidate table creation and display logic into the unified address listing function. Signed-off-by: Robin Jarry <[email protected]>
In all contexts, make the "show" argument implicit, when nothing is specified. This allows the following commands to be equivalents (list is not exhaustive): interface -> interface show nexthop -> nexthop show route -> route show conntrack -> conntrack show conntrack config -> conntrack config show ... This is made possible by adding the optional "show" tokens *after all* possible alternatives in the command tree. Signed-off-by: Robin Jarry <[email protected]>
When using grcli in interactive mode, the error reporting routine displays where the command was found invalid and the expected tokens, if any. Update the batch and one-shot modes to use the same error reporting. We cannot use ec_editline_get_suggestions which is only available in interactive mode. Reimplement it ourselves. Signed-off-by: Robin Jarry <[email protected]>
Update the ec_node passed to ec_complete_strvec to include an extra "any" node that will match the binary name instead of duplicating the vector without the first argument. Signed-off-by: Robin Jarry <[email protected]>
libecoli now has functions to expand strings in a vector using non-ambiguous command completion. Use ec_node_sh_lex_expand() instead of ec_node_sh_lex() which takes care of this transparently. When validating a command, call ec_complete_strvec_expand() explicitly so that we can display useful error messages to the user, e.g.: grout# i a porc x devargs net_null * interface add porc x devargs net_null * ^ error: invalid arguments expected: ipip, port, vlan Signed-off-by: Robin Jarry <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smoke/srv6_test.sh (1)
1-6: Add fail-fast guards (set -euo pipefail).Without
set -euo pipefail, the script can keep going after a failedgrclicommand, masking regressions. Enable fail-fast right after the shebang.modules/ip6/cli/router_advert.c (1)
17-24: Zero-initialize the show request.
reqgoes on the wire without being cleared, so any padding/fields you don't touch carry stack garbage intoGR_IP6_IFACE_RA_SHOW. Please zero it first.Apply this diff:
- const struct gr_ip6_ra_show_req req; + struct gr_ip6_ra_show_req req = {0};
♻️ Duplicate comments (8)
smoke/config_test.sh (1)
1-6: Still missingset -euo pipefail.Same concern as previously noted: add fail-fast guards right after the shebang so the test stops on errors instead of drifting onward.
modules/ip6/cli/route.c (1)
60-60: Fix IPv6 address formatting.Line 60 passes
&route->destbut should pass&route->dest.ipto be consistent with the event printer at line 122. The IP6_F macro expects a pointer tostruct rte_ipv6_addr, notstruct gr_ip6_net.Apply this diff:
- scols_line_sprintf(line, 1, IP6_F "/%hhu", &route->dest, route->dest.prefixlen); + scols_line_sprintf(line, 1, IP6_F "/%hhu", &route->dest.ip, route->dest.prefixlen);modules/infra/cli/address.c (1)
34-53: Keep the first parse errno before falling back to ENOPROTOOPT.If every provider rejects ADDR (bad input), we drop the original arg_ip_net errno and report “protocol not available,” masking the real failure—same problem we flagged earlier. Record the first non-ENOENT errno during the walk and only fall back to ENOPROTOOPT if none was set.
static cmd_status_t addr_add(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_addr_ops *ops; uint8_t addr[64]; + int saved_errno = 0; STAILQ_FOREACH (ops, &addr_ops, next) { - if (arg_ip_net(p, "ADDR", addr, false, ops->af) == 0) - return ops->add(c, p); + if (arg_ip_net(p, "ADDR", addr, false, ops->af) == 0) + return ops->add(c, p); + if (errno != ENOENT && saved_errno == 0) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno ? saved_errno : ENOPROTOOPT; return CMD_ERROR; } static cmd_status_t addr_del(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_addr_ops *ops; uint8_t addr[64]; + int saved_errno = 0; STAILQ_FOREACH (ops, &addr_ops, next) { - if (arg_ip_net(p, "ADDR", addr, false, ops->af) == 0) - return ops->del(c, p); + if (arg_ip_net(p, "ADDR", addr, false, ops->af) == 0) + return ops->del(c, p); + if (errno != ENOENT && saved_errno == 0) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno ? saved_errno : ENOPROTOOPT; return CMD_ERROR; }cli/main.c (2)
183-185: Honor-xtracing for argv execution.We still hardwire
false, so positional commands ignore--trace-commands. Passopts.trace_commands.- status = exec_args(client, cmdlist, argc, (const char *const *)argv, false); + status = exec_args( + client, cmdlist, argc, (const char *const *)argv, opts.trace_commands + );
191-194: Break onexitwhen replaying scripts.If
exec_linereturnsEXEC_CMD_EXITwe loop anyway, so scripts can’t stop themselves—exact regression already noted. Break out beforecheck_status.while (fgets(buf, sizeof(buf), opts.cmds_file)) { status = exec_line(client, cmdlist, buf, opts.trace_commands); + if (status == EXEC_CMD_EXIT) + break; if (check_status(status) < 0 && opts.err_exit) goto end; }modules/infra/cli/route.c (1)
34-91: Preserve parse errno before defaulting to ENOPROTOOPT.When every provider rejects DEST, we overwrite arg_ip{,_net}’s errno with ENOPROTOOPT, so users see “protocol not available” instead of the real parse failure—exactly the regression called out earlier. Cache the first non-ENOENT errno for
add,del, andget, and only fall back to ENOPROTOOPT if none was set.static cmd_status_t route_add(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_route_ops *ops; uint8_t net[64]; + int saved_errno = 0; STAILQ_FOREACH (ops, &route_ops, next) { if (arg_ip_net(p, "DEST", net, false, ops->af) == 0) return ops->add(c, p); + if (errno != ENOENT && saved_errno == 0) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno ? saved_errno : ENOPROTOOPT; return CMD_ERROR; } static cmd_status_t route_del(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_route_ops *ops; uint8_t net[64]; + int saved_errno = 0; STAILQ_FOREACH (ops, &route_ops, next) { if (arg_ip_net(p, "DEST", net, false, ops->af) == 0) return ops->del(c, p); + if (errno != ENOENT && saved_errno == 0) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno ? saved_errno : ENOPROTOOPT; return CMD_ERROR; } static cmd_status_t route_get(struct gr_api_client *c, const struct ec_pnode *p) { struct cli_route_ops *ops; uint8_t ip[64]; + int saved_errno = 0; STAILQ_FOREACH (ops, &route_ops, next) { if (arg_ip(p, "DEST", ip, ops->af) == 0) return ops->get(c, p); + if (errno != ENOENT && saved_errno == 0) + saved_errno = errno; } - errno = ENOPROTOOPT; + errno = saved_errno ? saved_errno : ENOPROTOOPT; return CMD_ERROR; }cli/exec.c (2)
178-179: Critical: GNU-specific error functions break portability.
strerrordesc_np()andstrerrorname_np()are glibc-only (since 2.32) and will cause compilation failures on FreeBSD, musl, macOS, and other non-glibc platforms.Apply this diff to use portable
strerror():case EXEC_CMD_FAILED: - errorf("command failed: %s (%s)", strerrordesc_np(errno), strerrorname_np(errno)); + errorf("command failed: %s", strerror(errno)); break;
40-51: Non-portable function pointer cast.Casting between function pointers and
void*viaec_dict_getis not guaranteed by ISO C and may fail on architectures with different representations (e.g., some embedded targets, old architectures).Store a pointer-to-function-pointer (
cmd_cb_t*) in the dict instead, then dereference on retrieval. Coordinate with thewith_callbackAPI update incli/ecoli.c.
🧹 Nitpick comments (2)
smoke/nexthop_ageing_test.sh (1)
63-64: Brace the interface name in the regexWrap
$p0/$p1in braces before the character class so the pattern is parsed unambiguously and ShellCheck (SC1087) stays green.-grcli address show | grep -E "^$p0[[:space:]]+172\.16\.0\.1/24$" || fail "addresses were destroyed" -grcli address show | grep -E "^$p1[[:space:]]+172\.16\.1\.1/24$" || fail "addresses were destroyed" +grcli address show | grep -E "^${p0}[[:space:]]+172\.16\.0\.1/24$" || fail "addresses were destroyed" +grcli address show | grep -E "^${p1}[[:space:]]+172\.16\.1\.1/24$" || fail "addresses were destroyed"Also applies to: 78-79
modules/infra/cli/stats.c (1)
61-64: Simplify the software flag logic.The
elseclause at line 63 is unnecessary since software stats are already the default when hardware is not specified.Consider simplifying:
if (arg_str(p, "hardware") != NULL) req.flags |= GR_INFRA_STAT_F_HW; - else - req.flags |= GR_INFRA_STAT_F_SW; + else if (arg_str(p, "software") != NULL) + req.flags |= GR_INFRA_STAT_F_SW;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
-
GNUmakefile(1 hunks) -
README.md(5 hunks) -
api/gr_net_types.h(1 hunks) -
cli/complete.c(4 hunks) -
cli/ecoli.c(3 hunks) -
cli/exec.c(5 hunks) -
cli/exec.h(2 hunks) -
cli/gr_cli.h(3 hunks) -
cli/interact.c(2 hunks) -
cli/log.c(2 hunks) -
cli/log.h(1 hunks) -
cli/main.c(2 hunks) -
cli/quit.c(1 hunks) -
docs/grout-frr.7.md(1 hunks) -
main/api.c(1 hunks) -
main/gr_module.h(2 hunks) -
main/grout.init(1 hunks) -
main/module.c(4 hunks) -
meson.build(1 hunks) -
modules/infra/cli/address.c(1 hunks) -
modules/infra/cli/affinity.c(3 hunks) -
modules/infra/cli/events.c(3 hunks) -
modules/infra/cli/gr_cli_event.h(1 hunks) -
modules/infra/cli/gr_cli_iface.h(1 hunks) -
modules/infra/cli/gr_cli_l3.h(1 hunks) -
modules/infra/cli/gr_cli_nexthop.h(1 hunks) -
modules/infra/cli/graph.c(2 hunks) -
modules/infra/cli/iface.c(5 hunks) -
modules/infra/cli/meson.build(1 hunks) -
modules/infra/cli/nexthop.c(14 hunks) -
modules/infra/cli/port.c(3 hunks) -
modules/infra/cli/route.c(1 hunks) -
modules/infra/cli/stats.c(3 hunks) -
modules/infra/cli/trace.c(4 hunks) -
modules/infra/cli/vlan.c(3 hunks) -
modules/ip/cli/address.c(7 hunks) -
modules/ip/cli/icmp.c(1 hunks) -
modules/ip/cli/ip.h(0 hunks) -
modules/ip/cli/route.c(7 hunks) -
modules/ip6/cli/address.c(7 hunks) -
modules/ip6/cli/icmp6.c(1 hunks) -
modules/ip6/cli/ip.h(0 hunks) -
modules/ip6/cli/route.c(7 hunks) -
modules/ip6/cli/router_advert.c(2 hunks) -
modules/ipip/cli.c(3 hunks) -
modules/policy/cli/conntrack.c(2 hunks) -
modules/policy/cli/dnat44.c(4 hunks) -
modules/policy/cli/policy.h(0 hunks) -
modules/policy/cli/snat44.c(3 hunks) -
modules/srv6/cli/localsid.c(3 hunks) -
modules/srv6/cli/route.c(4 hunks) -
smoke/_init.sh(4 hunks) -
smoke/_init_frr.sh(7 hunks) -
smoke/affinity_test.sh(1 hunks) -
smoke/config_test.sh(1 hunks) -
smoke/cross_vrf_forward_frr_test.sh(1 hunks) -
smoke/cross_vrf_forward_test.sh(1 hunks) -
smoke/dnat44_test.sh(2 hunks) -
smoke/graph_svg_test.sh(1 hunks) -
smoke/ip6_add_del_test.sh(1 hunks) -
smoke/ip6_builtin_icmp_test.sh(1 hunks) -
smoke/ip6_forward_test.sh(1 hunks) -
smoke/ip6_rs_ra_test.sh(1 hunks) -
smoke/ip6_same_peer_test.sh(1 hunks) -
smoke/ip_add_del_test.sh(1 hunks) -
smoke/ip_builtin_icmp_test.sh(1 hunks) -
smoke/ip_forward_test.sh(1 hunks) -
smoke/ipip_encap_test.sh(1 hunks) -
smoke/nexthop_ageing_test.sh(4 hunks) -
smoke/snat44_test.sh(2 hunks) -
smoke/srv6_test.sh(3 hunks) -
smoke/vlan_forward_test.sh(1 hunks) -
smoke/vrf_forward_frr_test.sh(2 hunks) -
smoke/vrf_forward_test.sh(1 hunks) -
subprojects/ecoli.wrap(1 hunks)
💤 Files with no reviewable changes (3)
- modules/policy/cli/policy.h
- modules/ip6/cli/ip.h
- modules/ip/cli/ip.h
✅ Files skipped from review due to trivial changes (1)
- modules/ip/cli/address.c
🚧 Files skipped from review as they are similar to previous changes (27)
- modules/ip6/cli/icmp6.c
- cli/ecoli.c
- docs/grout-frr.7.md
- cli/interact.c
- smoke/graph_svg_test.sh
- cli/quit.c
- smoke/cross_vrf_forward_frr_test.sh
- modules/infra/cli/events.c
- main/grout.init
- meson.build
- modules/ipip/cli.c
- modules/infra/cli/gr_cli_iface.h
- smoke/ipip_encap_test.sh
- smoke/vrf_forward_test.sh
- GNUmakefile
- modules/infra/cli/gr_cli_event.h
- modules/infra/cli/meson.build
- modules/ip/cli/icmp.c
- smoke/ip6_add_del_test.sh
- smoke/ip_add_del_test.sh
- cli/log.h
- smoke/dnat44_test.sh
- smoke/ip6_builtin_icmp_test.sh
- modules/infra/cli/gr_cli_l3.h
- smoke/ip_forward_test.sh
- subprojects/ecoli.wrap
- modules/infra/cli/graph.c
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-03T13:42:52.138Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.138Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.c
📚 Learning: 2025-10-03T13:34:31.774Z
Learnt from: rjarry
PR: DPDK/grout#329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.774Z
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:
modules/ip6/cli/route.cmodules/ip/cli/route.c
🧬 Code graph analysis (29)
main/api.c (1)
main/gr_module.h (1)
api_out(20-23)
cli/complete.c (1)
cli/log.c (1)
errorf(32-48)
modules/infra/cli/route.c (5)
cli/ecoli.c (3)
arg_ip_net(199-212)arg_ip(180-189)with_help(15-24)cli/gr_cli.h (1)
arg_u16(64-70)modules/ip/cli/route.c (1)
constructor(137-140)modules/ip6/cli/route.c (1)
constructor(136-139)cli/exec.c (1)
cli_context_register(16-18)
cli/main.c (3)
cli/exec.c (2)
exec_args(272-295)exec_line(242-270)cli/log.c (1)
is_tty(22-30)cli/interact.c (1)
interact(36-114)
modules/infra/cli/stats.c (2)
cli/ecoli.c (2)
arg_str(99-109)with_help(15-24)cli/exec.c (1)
cli_context_register(16-18)
smoke/ip_builtin_icmp_test.sh (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
smoke/affinity_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/srv6/cli/localsid.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/route.c (1)
constructor(188-191)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/iface.c (2)
cli/exec.c (1)
cli_context_register(16-18)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/srv6/cli/route.c (3)
modules/infra/cli/nexthop.c (2)
constructor(485-491)cli_nexthop_formatter_register(22-27)modules/srv6/cli/localsid.c (1)
constructor(219-222)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/vlan.c (2)
modules/infra/cli/iface.c (1)
ctx_init(486-536)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/port.c (4)
modules/infra/cli/iface.c (2)
ctx_init(486-536)constructor(589-592)modules/infra/cli/vlan.c (2)
ctx_init(158-197)constructor(204-207)modules/ipip/cli.c (2)
ctx_init(127-156)constructor(163-166)cli/exec.c (1)
cli_context_register(16-18)
cli/exec.h (1)
cli/exec.c (2)
exec_args(272-295)exec_line(242-270)
smoke/vrf_forward_frr_test.sh (1)
smoke/_init_frr.sh (1)
set_ip_address(30-61)
modules/ip6/cli/address.c (3)
cli/ecoli.c (1)
arg_ip6_net(218-220)modules/infra/cli/address.c (4)
addr_list(56-84)addr_add(30-41)addr_del(43-54)cli_addr_ops_register(19-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
modules/policy/cli/snat44.c (1)
cli/exec.c (1)
cli_context_register(16-18)
cli/gr_cli.h (2)
cli/exec.c (1)
cli_context_register(16-18)cli/ecoli.c (6)
with_help(15-24)with_callback(26-35)arg_ip(180-189)arg_ip4(191-193)arg_ip6(195-197)arg_ip_net(199-212)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(60-63)
modules/infra/cli/trace.c (1)
cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/gr_cli_nexthop.h (1)
modules/infra/cli/nexthop.c (2)
cli_nexthop_formatter_register(22-27)cli_nexthop_format(40-76)
modules/infra/cli/affinity.c (2)
modules/infra/cli/events.c (1)
ctx_init(55-62)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/address.c (5)
modules/ip/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-73)constructor(113-116)modules/ip6/cli/address.c (4)
addr_add(18-34)addr_del(36-52)addr_list(54-71)constructor(111-114)cli/ecoli.c (2)
arg_ip_net(199-212)arg_str(99-109)modules/infra/cli/iface.c (2)
iface_from_name(92-105)constructor(589-592)cli/exec.c (1)
cli_context_register(16-18)
modules/infra/cli/nexthop.c (2)
modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/route.c (5)
cli/table.c (1)
scols_line_sprintf(10-23)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip6(195-197)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
cli/exec.c (2)
api/gr_errno.h (1)
errno_set_null(14-17)cli/log.c (4)
need_quote(50-63)errorf(32-48)is_tty(22-30)trace_cmd(65-78)
modules/policy/cli/conntrack.c (2)
modules/infra/cli/affinity.c (1)
constructor(163-165)cli/exec.c (1)
cli_context_register(16-18)
modules/policy/cli/dnat44.c (3)
modules/infra/cli/nexthop.c (3)
ctx_init(346-443)constructor(485-491)cli_nexthop_formatter_register(22-27)modules/policy/cli/snat44.c (2)
ctx_init(89-131)constructor(138-140)cli/exec.c (1)
cli_context_register(16-18)
modules/ip6/cli/router_advert.c (3)
modules/ip6/cli/icmp6.c (1)
ctx_init(203-239)cli/ec_node_dyn.c (1)
ec_node_dyn(117-133)cli/exec.c (1)
cli_context_register(16-18)
modules/ip/cli/route.c (5)
modules/ip/control/route.c (2)
route4_list(416-423)route4_get(323-338)modules/infra/cli/nexthop.c (1)
cli_nexthop_format(40-76)cli/ecoli.c (1)
arg_ip4(191-193)modules/infra/cli/route.c (1)
cli_route_ops_register(18-28)modules/infra/cli/events.c (1)
cli_event_printer_register(14-16)
🪛 Shellcheck (0.11.0)
smoke/nexthop_ageing_test.sh
[error] 63-63: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 64-64: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 78-78: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
[error] 79-79: Use braces when expanding arrays, e.g. ${array[idx]} (or ${var}[.. to quiet).
(SC1087)
⏰ 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-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (40)
api/gr_net_types.h (1)
140-140: LGTM! Correctness improvement.Moving the end anchor outside the alternation ensures both IPv4 and IPv6 network patterns are uniformly end-anchored, preventing false matches on strings with trailing garbage. This aligns with
IP_ANY_REon line 139.modules/policy/cli/conntrack.c (7)
143-146: LGTM! Well-structured context macros.The hierarchical macro pattern (CONNTRACK_ARG → CONNTRACK_CTX → CONNTRACK_CONFIG_CTX) cleanly establishes the CLI namespace and aligns with the noun-first command structure.
152-152: LGTM! Consistent context migration.The flush command correctly adopts the new CONNTRACK_CTX wrapper without altering functionality.
158-161: LGTM! Proper config context and subset syntax.The multi-line command string is correctly concatenated, and the ecoli subset notation ensures at least one configuration parameter is required (addressing previous review concerns).
197-201: LGTM! Optional show syntax correctly implemented.The [show] notation properly implements the PR objective to allow omitting show arguments, making both
conntrack config showandconntrack configvalid.
205-208: LGTM! Consistent optional show pattern.Mirrors the config show approach, allowing both
conntrack showandconntrackto display tracked connections.
212-212: LGTM! Type updated to new CLI API.The context struct type correctly migrates from
gr_cli_contexttocli_contextas part of the broader API unification.
218-218: LGTM! Registration call updated correctly.The registration function name aligns with the new CLI API and matches the pattern used in other modules.
smoke/ip6_forward_test.sh (1)
10-16: LGTM! Test commands updated consistently.Commands correctly migrated from verb-first (
add interface) to noun-first (interface add) pattern, matching the PR's CLI refactor objectives.smoke/vlan_forward_test.sh (1)
12-17: LGTM! Consistent command migration.Test commands correctly updated to noun-first syntax, consistent with other smoke tests in the PR.
README.md (3)
102-107: LGTM! Init script examples updated correctly.Configuration examples now use noun-first commands (
interface add,address add,route add), matching the refactored CLI surface.
152-170: LGTM! Help text accurately reflects new CLI structure.Command list correctly shows noun-based top-level commands (
address,affinity,conntrack, etc.), aligning with the PR's CLI reorganization.
171-194: LGTM! Interactive examples demonstrate new command patterns.Session output correctly shows noun-first commands (
interface show,affinity qmap show,address show, etc.), providing accurate usage examples.cli/complete.c (2)
17-38: LGTM! Completion wrapper correctly implemented.The
bash_complete_nodefunction properly usesec_node_sh_lex_expandto wrap the command list with options and program name. The nested structure (EC_NODE_SEQ→EC_NODE_SUBSETfor options) follows ecoli library conventions.Based on learnings: The ownership semantics are correct—ecoli library functions take ownership of child nodes, so the reassignment pattern at lines 87-91 is safe.
87-103: LGTM! Completion flow simplified correctly.Direct use of
ec_complete(cmdlist, buf)streamlines the completion path. The previous concern about node ownership was resolved—the library handles it correctly.smoke/ip_builtin_icmp_test.sh (1)
10-14: LGTM! Test commands consistently updated.Commands correctly migrated to noun-first syntax, matching the pattern across all smoke tests.
cli/log.c (2)
50-63: LGTM! Function exposed for reuse.Making
need_quotepublic allows its use in other modules. The implementation correctly identifies characters requiring quoting in shell contexts.
65-78: LGTM! Command tracing simplified.The refactored signature accepting
const struct ec_strvec *directly eliminates string parsing overhead. Early return for empty commands and direct iteration over the vector improves efficiency.modules/infra/cli/gr_cli_nexthop.h (2)
11-26: LGTM! Public API consistently renamed.Type and function renames follow the PR's consistent pattern:
gr_cli_*→cli_*. The STAILQ entry field standardization (entries→next) aligns with the treewide unification noted in PR objectives.
28-31: LGTM! Context macros added for CLI wiring.New macros (
NEXTHOP_CTX,NEXTHOP_ADD_CTX) provide consistent command context construction, matching the pattern used in other CLI modules across the PR.modules/policy/cli/dnat44.c (3)
94-98: LGTM! Formatter type updated correctly.The nexthop formatter now uses the renamed
cli_nexthop_formattertype, consistent with the public API changes across the PR.
100-137: LGTM! Context and commands consistently aligned.The
DNAT_CTXmacro uses"dnat44"(line 100), matchingctx.name = "dnat44"(line 140), resolving the previous review concern. All commands correctly useDNAT_CTX(root).
145-146: LGTM! Registration calls updated correctly.Function calls updated to use renamed APIs:
cli_context_registerandcli_nexthop_formatter_register, aligning with the PR's public API migration.modules/policy/cli/snat44.c (1)
87-139: LGTM!The refactoring correctly:
- Defines SNAT_CTX macro for context creation
- Updates all command registrations to use the new context
- Changes struct type from
gr_cli_contexttocli_context- Updates registration call to
cli_context_registermodules/srv6/cli/route.c (1)
128-190: LGTM!The SRv6 route context refactoring is correct:
- Formatter type updated to
cli_nexthop_formatter- TUNSRC_CTX macro properly defined
- All command registrations use appropriate context macros
- Registration calls updated to
cli_context_registerandcli_nexthop_formatter_registermodules/ip6/cli/route.c (2)
74-75: Verify arg_ip6 vs arg_ip6_net usage.The code changed from
arg_ip6_net(p, "DEST", &req.dest, true)toarg_ip6(p, "DEST", &req.dest). This drops the network flag which might be intentional, but verify thatreq.dest(likely astruct gr_ip6_net) is correctly populated byarg_ip6instead ofarg_ip6_net.
51-98: LGTM on route_ops refactoring!The refactoring to use
cli_route_opsstructure and registration is well done:
- Proper signature updates for
route6_listto accept external table- Correct usage of
cli_nexthop_format- Appropriate registration calls
Also applies to: 127-138
main/gr_module.h (1)
41-41: LGTM!STAILQ_ENTRY field renaming from
entriestonextis consistent with the broader refactoring. Verified that main/module.c has been updated to match.Also applies to: 51-51
modules/infra/cli/port.c (1)
192-226: LGTM!Port CLI correctly migrated to new context system:
- Uses INTERFACE_ADD_CTX and INTERFACE_SET_CTX macros
- Context type updated to
cli_context- Registration call updated to
cli_context_registermodules/infra/cli/vlan.c (1)
162-205: LGTM!VLAN CLI correctly updated:
- Uses INTERFACE_ADD_CTX and INTERFACE_SET_CTX macros
- Context type updated to
cli_context- Registration updated to
cli_context_registermain/module.c (1)
27-44: LGTM!STAILQ traversal correctly updated to use
nextfield throughout, matching the header changes in main/gr_module.h. The explicit NULL return at line 44 is good defensive programming.Also applies to: 55-60, 77-77, 100-100
modules/infra/cli/affinity.c (1)
111-164: LGTM!Affinity CLI refactoring is well-structured:
- Nested context macros (AFFINITY_ARG, CPU_CTX, QMAP_CTX) provide clear hierarchy
- All commands use appropriate contexts
- Type updated to
cli_context- Registration updated to
cli_context_registermodules/srv6/cli/localsid.c (1)
124-128: LGTM!The formatter type migration from
gr_cli_nexthop_formattertocli_nexthop_formatteris correct and consistent with the broader API migration.modules/ip6/cli/address.c (1)
54-71: LGTM!The signature change to accept
uint16_t iface_idandstruct libscols_table *tableproperly aligns with the new operations-based API pattern. The filtering byiface_idis correctly implemented.modules/infra/cli/iface.c (1)
489-493: Verify the necessity of NULL checks for context macros.These NULL checks suggest that
INTERFACE_ADD_CTXandINTERFACE_SET_CTXmacros can fail. If these are expected to always succeed during initialization, the checks may be masking an issue in the macro definitions or calling context.cli/gr_cli.h (2)
14-14: LGTM!The typedef change from
typedef int(cli_ctx_init_t)(...)totypedef int(*cli_ctx_init_t)(...)correctly makescli_ctx_init_ta function pointer type, simplifying usage.
32-32: LGTM!The typedef change to
typedef cmd_status_t(*cmd_cb_t)(...)(pointer form) is consistent with thecli_ctx_init_tpattern and simplifies function pointer handling.modules/ip/cli/route.c (1)
51-66: LGTM!The signature change and ops-based pattern are correct. The
retvariable does not require initialization asgr_api_client_stream_foreachalways sets it during first iteration evaluation.Based on learnings.
modules/infra/cli/nexthop.c (2)
20-27: LGTM!The migration from
gr_cli_nexthop_formattertocli_nexthop_formatterand STAILQ field rename tonextare correct and consistent with the broader API migration.
343-344: LGTM!The
NEXTHOP_CONFIG_CTXmacro definition properly encapsulates the CLI context creation for nexthop configuration commands.
|
|
||
| if (strncmp(req->version, GROUT_VERSION, sizeof(req->version)) != 0) | ||
| return api_out(EPROTO, 0, NULL); | ||
| return api_out(EBADMSG, 0, NULL); |
There was a problem hiding this comment.
🧩 Analysis chain
Better error semantics; verify client handling.
EBADMSG more accurately represents version incompatibility than EPROTO. However, this changes the API contract.
Run the following to check if grout-config or other clients handle the new error code:
🏁 Script executed:
#!/bin/bash
# Verify that clients handle EBADMSG for version mismatch
# Check for EPROTO references that might need updating
rg -n '\bEPROTO\b' --type=c --type=py
# Check grout-config for version mismatch error handling
gh pr view 8 --repo dci-labs/grout-config --json body,commits | \
jq -r '.body, .commits[].commit.message'Length of output: 158
Propagate EBADMSG and verify client updates.
- Change main/sd_notify.c:83 to return EBADMSG instead of EPROTO.
- Confirm all dependent clients handle EBADMSG (grout-config PR link is broken; please verify repo and error-handling updates).
2 participants
build-depends: https://github.com/dci-labs/grout-config/pull/8
Summary by CodeRabbit