cli: generate man pages dynamically from command tree #368
Conversation
📝 WalkthroughWalkthroughAdds Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used📓 Path-based instructions (1)**/*.{c,h}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-09-05T07:06:51.554ZApplied to files:
📚 Learning: 2025-09-05T07:06:51.554ZApplied to files:
📚 Learning: 2025-10-13T08:37:56.819ZApplied to files:
📚 Learning: 2025-10-03T13:42:52.180ZApplied to files:
🧬 Code graph analysis (2)cli/man_main.c (2)
cli/man_commands.c (2)
⏰ 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)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (8)
cli/man.h (1)
8-10: API surface is clearPrototypes match the intended call-sites. Consider extern "C" guards only if C++ consumers are expected; otherwise OK.
cli/complete.c (1)
17-35: Make --man argument optional in the grammar (reflects -m [COMMAND])The help advertises an optional COMMAND, but the grammar makes it mandatory. Wrap COMMAND in a subset to render [COMMAND] in synopsis/completion.
Apply:
- OPT("-m|--man COMMAND", - "Show man page for _COMMAND_ or list all commands if no argument.", - ec_node("str", "COMMAND")) + OPT("-m|--man COMMAND", + "Show man page for _COMMAND_ or list all commands if no argument.", + EC_NODE_SUBSET(EC_NO_ID, ec_node("str", "COMMAND")))Please confirm main.c dispatches print_main_man_page() when no COMMAND is provided to keep behavior consistent.
docs/meson.build (2)
44-63: Guard running grcli during builds (cross-builds/headless builders)custom_target runs grcli_exe at build time. This can fail under cross-compilation or restricted builders. Suggest gating dynamic docs with meson.is_cross_build() and allowing docs=false fallback.
Example:
+if meson.is_cross_build() + warning('Cross-build detected; disabling dynamic grcli manpage generation. Use -Ddocs=false or pre-generated pages.') +else grcli_md = custom_target( 'grcli.1.md', output: 'grcli.1.md', command: [grcli_exe, '--man'], capture: true, ) custom_target( 'grcli.1', input: grcli_md, output: 'grcli.1', command: md2man, install: true, feed: true, capture: true, install_dir: '@0@/man1'.format(mandir), install_tag: 'man', ) +endifConfirm your builders are native (not cross) where docs are enabled.
64-91: Hardcoded command list can drift; add a check to keep it in syncThe static grcli_commands list may desync from the CLI. Add a CI check that compares this list to the commands enumerated by grcli --man (main page). We can provide a script if useful.
Would you like a small script to diff the generated “AVAILABLE COMMANDS” against this list and fail CI on mismatch?
cli/man_commands.c (3)
36-54: Cover all collected argument types in help (missing "re", consider "file")collect_arguments adds ids for dyn/re but print_argument_help doesn’t handle "re" (and "file" shows up in options elsewhere). Add fallbacks to avoid empty sections.
} else if (strcmp(type, "dyn") == 0) { printf("Dynamic value.\n\n"); + } else if (strcmp(type, "re") == 0) { + printf("Value matching a regular expression.\n\n"); + } else if (strcmp(type, "file") == 0) { + printf("Path to a file.\n\n"); + } else { + printf("%s.\n\n", type); }
162-200: SYNOPSIS ordering: show syntax before per-variant help (readability)Minor: printing per-variant help before the syntax line feels inverted. Consider printing the syntax first, then the explanatory sentence.
271-356: Behavior when no COMMAND is given relies on main.cprint_man_page() is a no-op when argc<3, which is fine if main.c calls print_main_man_page() in that case. Please confirm the dispatch path.
If needed, I can scan main.c and add a guard.
cli/man_main.c (1)
145-147: Minor: Unused parameter.The
cmdlistparameter is received but not used (line 146 void-cast suppresses warning). The function gets its options fromgrcli_options_node()instead. Consider removing the parameter if it's not needed for API consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man.hcli/complete.ccli/main.ccli/complete.hcli/man_main.ccli/man_commands.c
🧠 Learnings (4)
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.h
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
debian/grout.install
📚 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:
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:
cli/meson.build
🧬 Code graph analysis (5)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(271-356)cli/man_main.c (1)
print_main_man_page(191-267)
cli/main.c (2)
cli/man_main.c (1)
print_main_man_page(191-267)cli/man_commands.c (1)
print_man_page(271-356)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
cli/man_commands.c (1)
cli/main.c (1)
help(34-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (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 (12)
cli/meson.build (1)
13-14: man sources correctly added to cli_srcThis integrates cleanly with the global cli_src aggregator used by the top-level executable(). LGTM. Based on learnings
debian/grout.install (1)
5-5: Wildcard covers per-command man pagesgrcli*.1 is appropriate for Debian (compression happens later). LGTM.
cli/complete.h (1)
9-9: Public API exposure looks goodSignature is minimal and consistent with usage elsewhere. LGTM.
cli/complete.c (1)
43-43: Completion integration looks rightUsing ec_node_option(..., grcli_options_node()) keeps options centralized and reused. LGTM.
rpm/grout.spec (1)
108-108: Broader manpage glob is correctCatches main and per-command pages, including compressed forms. LGTM.
docs/meson.build (2)
15-16: Rename to static_man_src is fineNo behavioral change; improves clarity.
21-21: Loop update aligns with renameConsistent with static_man_src.
meson.build (1)
129-131: LGTM - Correct build dependency order.Moving
subdir('docs')aftergrcli_exeensures the executable is available for dynamic man page generation viagrcli --man.cli/main.c (3)
8-8: LGTM - Header inclusion for man page support.
31-31: LGTM - Usage documentation updated.The help text accurately describes the --man option behavior.
Also applies to: 42-43
173-179: LGTM - Man page invocation logic is correct.The --man option is handled as a standalone mode (similar to --bash-complete on line 170-171), which is appropriate given the usage pattern.
cli/man_main.c (1)
1-267: LGTM - Well-structured man page generator.The implementation correctly traverses the EC command tree and generates properly formatted markdown. Memory management is correct throughout:
strdup()allocations are properly freedec_free()used for ecoli library allocationsec_node_free()called for the options treeThe complexity in
extract_option_syntax()is justified by the need to handle different EC node structures.
abhiramnarayana
force-pushed
the
cli-man
branch
from
33f3265 to
bca0a15
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
cli/man_commands.c (4)
200-203: SYNOPSIS should include the executable name.The per-context synopsis omits "grcli", which makes invocation unclear. Align with the main page.
- printf("**%s** ", ctx_name); + printf("**grcli** %s ", ctx_name);
268-271: Standalone command SYNOPSIS formatting inconsistent.Wrap only the binary name in bold and include it explicitly; avoid bolding the entire synopsis.
- printf("# SYNOPSIS\n\n**"); - print_node_synopsis(cmd_node, 0); - printf("**\n\n"); + printf("# SYNOPSIS\n\n**grcli** "); + print_node_synopsis(cmd_node, 0); + printf("\n\n");
45-53: Add help fallback for “re” (and unknown) argument types.Currently “re” (and any unknown types) render nothing. Provide a minimal default.
- } else if (strcmp(type, "dyn") == 0) { + } else if (strcmp(type, "dyn") == 0) { printf("Dynamic value.\n\n"); + } else if (strcmp(type, "re") == 0) { + printf("Pattern.\n\n"); + } else { + printf("Argument.\n\n"); }
10-10: Unused include.isn’t used.
-#include <errno.h>cli/man_main.c (2)
172-187: SYNOPSIS shows only short options; prefer long if present.Users frequently look for long options. Pick long_opt when available, else short_opt.
- if (short_opt != NULL || long_opt != NULL) { + if (short_opt != NULL || long_opt != NULL) { printf("["); - if (short_opt != NULL) - printf("**%s**", short_opt); + const char *opt = long_opt ? long_opt : short_opt; + if (opt != NULL) + printf("**%s**", opt); if (arg_name != NULL) printf(" _%s_", arg_name); - printf("]\n"); + printf("]\n"); }
30-36: Duplicate underline helper across modules.print_title_underline() exists here and in cli/man_commands.c. Consider a shared inline in man.h to avoid drift.
docs/meson.build (1)
66-94: Hardcoded grcli_commands list can drift.When commands change, this list must be maintained. Prefer generating the list at build time (e.g., a “grcli --list-commands” mode) and feeding it to Meson, with a cross-build fallback that disables per-command pages.
I can draft a follow‑up patch adding a hidden “--list-commands” for Meson consumption.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (7)
- debian/grout.install
- meson.build
- cli/man.h
- cli/main.c
- cli/complete.h
- cli/complete.c
- rpm/grout.spec
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man_commands.ccli/man_main.c
🧠 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:
cli/meson.build
🧬 Code graph analysis (2)
cli/man_commands.c (1)
cli/main.c (1)
help(34-53)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (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: build-cross-aarch64
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (2)
cli/meson.build (1)
13-16: Sources wiring looks good.man_commands.c and man_main.c are properly added to cli_src; fits project’s modular pattern.
docs/meson.build (1)
48-64: Build order is correct; no action needed.grcli_exe is defined at line 121 and subdir('docs') is called at line 130—after the executable. The codebase already includes an explicit comment documenting this dependency at line 129. In Meson, subdirectories inherit parent scope variables, so docs/meson.build can safely reference grcli_exe.
cli/man_commands.c
Outdated
| if (id != NULL && strcmp(id, EC_NO_ID) != 0) { | ||
| if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0 | ||
| || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0) { | ||
| if (!find_arg(*args, *count, id)) { | ||
| struct arg_entry *new_args = realloc( | ||
| *args, (*count + 1) * sizeof(**args) | ||
| ); | ||
| if (new_args == NULL) | ||
| return; | ||
| *args = new_args; | ||
| (*args)[*count].id = id; | ||
| (*args)[*count].node = node; | ||
| (*count)++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
String‑typed arguments with IDs aren’t collected.
If a “str” node has an ID (i.e., it’s a real argument, not a literal), it should appear under ARGUMENTS but is currently skipped.
- if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0
- || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0) {
+ if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0
+ || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0
+ || strcmp(type, "str") == 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (id != NULL && strcmp(id, EC_NO_ID) != 0) { | |
| if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0 | |
| || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0) { | |
| if (!find_arg(*args, *count, id)) { | |
| struct arg_entry *new_args = realloc( | |
| *args, (*count + 1) * sizeof(**args) | |
| ); | |
| if (new_args == NULL) | |
| return; | |
| *args = new_args; | |
| (*args)[*count].id = id; | |
| (*args)[*count].node = node; | |
| (*count)++; | |
| } | |
| } | |
| } | |
| if (id != NULL && strcmp(id, EC_NO_ID) != 0) { | |
| if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0 | |
| || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0 | |
| || strcmp(type, "str") == 0) { | |
| if (!find_arg(*args, *count, id)) { | |
| struct arg_entry *new_args = realloc( | |
| *args, (*count + 1) * sizeof(**args) | |
| ); | |
| if (new_args == NULL) | |
| return; | |
| *args = new_args; | |
| (*args)[*count].id = id; | |
| (*args)[*count].node = node; | |
| (*count)++; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/man_commands.c around lines 61 to 76, the condition that decides which
typed nodes with IDs are collected omits string-typed arguments, so "str" nodes
with IDs are skipped; update the type check to include strcmp(type, "str") == 0
alongside "uint","int","dyn","re" (i.e., add || strcmp(type, "str") == 0) so
string arguments are allocated and appended to *args exactly like the other
types, keeping the existing realloc/null-check/assignment logic unchanged.
cli/man_main.c
Outdated
| if (desc != NULL) { | ||
| if (desc[0] == '-' && desc[1] != '-') | ||
| *short_opt = strdup(desc); | ||
| else if (desc[0] == '-' && desc[1] == '-') | ||
| *long_opt = strdup(desc); | ||
| ec_free(desc); | ||
| } |
There was a problem hiding this comment.
Guard against short strings and avoid overwriting allocated opt strings.
desc[1] is accessed without length checks; also, reassigning short_opt/long_opt can leak if multiple candidates exist.
- char *desc = ec_node_desc(str_node);
+ char *desc = ec_node_desc(str_node);
if (desc != NULL) {
- if (desc[0] == '-' && desc[1] != '-')
- *short_opt = strdup(desc);
- else if (desc[0] == '-' && desc[1] == '-')
- *long_opt = strdup(desc);
+ size_t n = strlen(desc);
+ if (n >= 2 && desc[0] == '-' && desc[1] != '-') {
+ if (*short_opt == NULL) *short_opt = strdup(desc);
+ } else if (n >= 2 && desc[0] == '-' && desc[1] == '-') {
+ if (*long_opt == NULL) *long_opt = strdup(desc);
+ }
ec_free(desc);
}Apply the same pattern in the seq/or branch below.
Also applies to: 97-104
🤖 Prompt for AI Agents
In cli/man_main.c around lines 73 to 79, the code accesses desc[1] without
ensuring desc is at least two bytes long and unconditionally overwrites
*short_opt/*long_opt causing leaks; change the logic to first verify desc is
non-NULL and has sufficient length (e.g., strlen(desc) >= 2) before checking
desc[1], and only strdup into *short_opt or *long_opt if the target pointer is
currently NULL (to avoid leaking a previous allocation); still free desc with
ec_free(desc). Apply the identical safe-check-and-only-assign pattern to the
seq/or branch referenced at lines ~97-104 as well.
958008d to
371f889
Compare
abhiramnarayana
force-pushed
the
cli-man
branch
from
371f889 to
76e40ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/man_main.c (1)
67-80: Guard against short strings and avoid overwriting allocated opt strings.This mirrors the previous review comment. The code accesses
desc[1]without verifyingdeschas at least 2 characters, and repeatedly assigns to*short_opt/*long_optwithout checking if they're already allocated, causing potential memory leaks.Apply this pattern to both the "or" and "seq" branches:
char *desc = ec_node_desc(str_node); if (desc != NULL) { - if (desc[0] == '-' && desc[1] != '-') - *short_opt = strdup(desc); - else if (desc[0] == '-' && desc[1] == '-') - *long_opt = strdup(desc); + size_t len = strlen(desc); + if (len >= 2 && desc[0] == '-' && desc[1] != '-' && *short_opt == NULL) + *short_opt = strdup(desc); + else if (len >= 3 && desc[0] == '-' && desc[1] == '-' && *long_opt == NULL) + *long_opt = strdup(desc); ec_free(desc); }cli/man_commands.c (1)
61-76: String-typed arguments with IDs aren't collected.This confirms the previous review comment. String nodes with IDs (like
COMMANDin the--manoption) represent actual arguments but are currently skipped, so they won't appear in the ARGUMENTS section.Include "str" in the type check:
if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0 - || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0) { + || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0 + || strcmp(type, "str") == 0) {
🧹 Nitpick comments (2)
docs/meson.build (1)
66-73: Document the command list maintenance requirement.The hardcoded command list must be manually kept in sync with available commands. Consider adding a comment explaining that when new commands are added to the CLI, this list must be updated, or document this requirement in a developer guide to prevent man pages from falling out of sync.
cli/man_commands.c (1)
87-148: Trailing spaces in synopsis output.The function prints trailing spaces after many node types (lines 96, 100, 102, 115, etc.). When called from line 269 in
print_standalone_command, this creates<synopsis> **with spaces before the closing markdown bold. Consider trimming trailing whitespace or adjusting the print logic for cleaner output.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/man.h
- cli/meson.build
- cli/complete.c
- rpm/grout.spec
- debian/grout.install
- cli/main.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/complete.hcli/man_commands.ccli/man_main.c
🧠 Learnings (1)
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.h
🧬 Code graph analysis (3)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
cli/man_commands.c (1)
cli/main.c (1)
help(34-53)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rpm
- GitHub Check: deb
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
rjarry
left a comment
rjarry
left a comment
There was a problem hiding this comment.
Hey @abhiramnarayana thanks for the respin!
It does the job overall but I find that the code is a bit too complicated. It looks almost as if it was written by AI 
Could you put more of a human element into it and make it less hurtful to the eyes? I am hoping it can be somehow simplified a bit.
docs/meson.build
Outdated
| # Dynamic grcli man pages | ||
| # Skip during cross-compilation since grcli can't run on the build host. | ||
| if not meson.is_cross_build() |
There was a problem hiding this comment.
Could we build a host version of grcli to ensure we can always run this?
cli/man_main.c
Outdated
| #include <stdlib.h> | ||
| #include <string.h> | ||
|
|
||
| static const char *ENV_DPRC_DESCRIPTION = "Set the DPRC - Datapath Resource Container: This value " |
There was a problem hiding this comment.
Underscored capitals are for macros.
There was a problem hiding this comment.
Done. Fixed this. Changed to MACRO and with better format.
cli/man_main.c
Outdated
| "should match the\n" | ||
| "one used by DPDK during the scan of the fslmc bus. It " | ||
| "is recommended to\n" | ||
| "set this on any NXP QorIQ targets. This serves as the " | ||
| "entry point for\n" | ||
| "grcli to enable autocompletion of fslmc devices " | ||
| "manageable by grout.\n" | ||
| "While grcli can configure grout without this " | ||
| "environment setting,\n" | ||
| "autocompletion of the devargs will not be available."; |
There was a problem hiding this comment.
Could we format this differently? Maybe use a macro instead of a static const.
There was a problem hiding this comment.
Done. Fixed this. Changed to MACRO and with better format.
cli/man_main.c
Outdated
| "environment setting,\n" | ||
| "autocompletion of the devargs will not be available."; | ||
|
|
||
| static const char *GRCLI_AUTHORS = "Created and maintained by Robin Jarry."; |
There was a problem hiding this comment.
I would prefer if we found another paragraph here. Maybe the AUTHORS section does not have its place and we should replace this by a REPORTING BUGS section containing a paragraph that mentions the upstream URL of the grout project.
There was a problem hiding this comment.
Done. Updated to print REPORTING BUGS. See the info I have given and see if it is fine.
cli/man_main.c
Outdated
| } | ||
|
|
||
| static void print_cli_options_from_tree(struct ec_node *cmdlist) { | ||
| (void)cmdlist; |
There was a problem hiding this comment.
Why have an argument in this function if you are not using it?
There was a problem hiding this comment.
Sorry. Had missed that. Removed it now.
cli/man_main.c
Outdated
| @@ -0,0 +1,278 @@ | |||
| // SPDX-License-Identifier: BSD-3-Clause | |||
| // Copyright (c) 2025 Robin Jarry | |||
There was a problem hiding this comment.
You did write this code, you should put your name here :)
cli/man_commands.c
Outdated
| if (id != NULL && strcmp(id, EC_NO_ID) != 0) { | ||
| if (strcmp(type, "uint") == 0 || strcmp(type, "int") == 0 | ||
| || strcmp(type, "dyn") == 0 || strcmp(type, "re") == 0) { | ||
| if (!find_arg(*args, *count, id)) { |
There was a problem hiding this comment.
You have three embedded ifs here. This is a bit ugly.
There was a problem hiding this comment.
I have split this now and using the goto . Hoping its a good split. Check and let me know.
cli/man_commands.c
Outdated
| printf("|"); | ||
| print_node_synopsis(child, depth + 1); | ||
| } | ||
| printf(") "); |
There was a problem hiding this comment.
That's a matter of preference, but I prefer to prefix with a space in this kind of code. That way to are sure not to leave any trailing whitespace on the last token.
There was a problem hiding this comment.
Done. keeping space only in prefix now.
cli/man_commands.c
Outdated
| continue; | ||
| print_node_synopsis(child, depth + 1); | ||
| } | ||
| printf("] "); |
cli/man_commands.c
Outdated
| continue; | ||
| printf("["); | ||
| print_node_synopsis(child, depth + 1); | ||
| printf("] "); |
There was a problem hiding this comment.
ditto done :)
abhiramnarayana
force-pushed
the
cli-man
branch
from
76e40ba to
5aa0bb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/main.c (1)
31-32: Do not advertise the internal -m/--man in end-user help.Prior feedback was to keep this internal. Consider hiding it from usage/help (and from grcli_options_node) while keeping the flag functional for build-time docs.
- printf(" %s -m|--man [COMMAND]\n", prog); + // internal: -m|--man (hidden)And in help():
- puts(" -m, --man [COMMAND] Show man page for command or list all"); - puts(" commands."); + // internal: -m/--man (hidden)Also applies to: 42-44
🧹 Nitpick comments (5)
cli/main.c (1)
178-182: Optional: accept -m anywhere, not only as argv[1].Current handling only recognizes -m in position 1. Either document this constraint or route -m through parse_args (FLAGS/long_options) and branch on a parsed flag.
docs/meson.build (2)
77-95: Main grcli man page targets look correct.Feeding md into go-md2man with capture/install is fine. Consider adding
build_by_default: get_option('docs')to avoid surprising local builds when docs are off.
96-123: Hardcoded command list: add a TODO to source this at build-time later.Acceptable for now, but it can drift. Leave a comment or generator note for maintainers.
cli/man_commands.c (2)
56-67: Collect string-typed arguments too.
collect_arguments()skips"str"nodes with IDs, so these never appear under ARGUMENTS.- if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 - && strcmp(type, "dyn") != 0 && strcmp(type, "re") != 0) + if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 + && strcmp(type, "dyn") != 0 && strcmp(type, "re") != 0 + && strcmp(type, "str") != 0) goto recurse;
271-277: SEE ALSO is minimal for standalone commands; OK for now.Links to
grcli(1)only. If needed later, you can mirror the contextual links used inprint_command_details().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cli/complete.c
- cli/complete.h
- rpm/grout.spec
- cli/meson.build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/main.ccli/man_main.ccli/man.hcli/man_commands.c
🧠 Learnings (2)
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
cli/man_main.cdebian/grout.install
📚 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:
docs/meson.build
🧬 Code graph analysis (4)
cli/main.c (2)
cli/man_main.c (1)
print_main_man_page(174-241)cli/man_commands.c (1)
print_man_page(279-380)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(279-380)cli/man_main.c (1)
print_main_man_page(174-241)
cli/man_commands.c (1)
cli/main.c (1)
help(34-53)
⏰ 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). (5)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: rpm
🔇 Additional comments (5)
meson.build (1)
129-131: Good ordering fix; unblocks docs generation.Placing docs after grcli_exe ensures grcli is available for man generation. Looks correct.
cli/man.h (1)
1-9: Header looks fine.Guard via
#pragma once, minimal deps, and return types align with callers.debian/grout.install (1)
5-5: Packaging glob aligns with per-command man pages.LGTM; will pick up grcli.1 and grcli-*.1.
cli/man_main.c (1)
142-172: Graceful error text already in place; minor polish.
fprintf(stderr, ...)path is good. Consider returning earlier ifgrcli_options_node()fails; you already do. Nothing else.docs/meson.build (1)
44-75: Solid cross-build strategy for man generation.Building a native grcli (
native: true) and switching viameson.is_cross_build()is the right approach.
abhiramnarayana
force-pushed
the
cli-man
branch
2 times, most recently
from
4e3f1a9 to
5055c5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/man_commands.c (1)
64-66: Consider including "str" type in argument collection.If string-typed nodes can have meaningful IDs (beyond EC_NO_ID), they should be collected and documented in the ARGUMENTS section.
- if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 - && strcmp(type, "re") != 0) + if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 + && strcmp(type, "re") != 0 && strcmp(type, "str") != 0) goto recurse;cli/man_main.c (1)
12-15: Include <ctype.h> for toupper().Line 125 uses
toupper()which requires this header. Pipeline failure confirms the issue.#include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <ctype.h>
🧹 Nitpick comments (1)
docs/meson.build (1)
98-103: Consider adding a maintenance reminder.The hardcoded command list could drift from actual commands over time. Consider adding a build-time check or script to verify completeness.
Example verification script to run occasionally:
#!/bin/bash # Verify grcli_commands list is complete grcli --man 2>&1 | grep '^**grcli-' | sed 's/\*\*grcli-\(.*\)\*\*(1)/\1/' | sort > /tmp/actual_commands cat <<EOF | sort > /tmp/expected_commands address affinity conntrack dnat44 events graph interface logging nexthop ping quit route router-advert snat44 stats trace traceroute tunsrc EOF diff /tmp/expected_commands /tmp/actual_commands
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (6)
- rpm/grout.spec
- cli/main.c
- debian/grout.install
- cli/meson.build
- cli/complete.c
- meson.build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man.hcli/man_commands.ccli/complete.hcli/man_main.c
🧠 Learnings (3)
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.h
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
cli/man_main.c
📚 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:
docs/meson.build
🧬 Code graph analysis (4)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(279-380)cli/man_main.c (1)
print_main_man_page(174-241)
cli/man_commands.c (1)
cli/main.c (1)
help(34-53)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
🪛 GitHub Actions: Package
cli/man_main.c
[error] 125-125: Implicit declaration of function ‘toupper’. Include <ctype.h> to fix this.
⏰ 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 (clang-16, none, debugoptimized, ubuntu-24.04, false)
🔇 Additional comments (12)
cli/complete.h (1)
9-9: LGTM!Clean addition of the centralized options API.
cli/man.h (1)
1-9: LGTM!Clean header exposing the man-page generation APIs.
cli/man_main.c (4)
17-26: LGTM!Macro definitions are clean and properly formatted.
41-140: LGTM!The option syntax printing logic correctly handles node traversal and properly frees allocated descriptions.
142-172: LGTM!Proper resource management with error handling and cleanup.
174-241: LGTM!Main man page generation logic is well-structured with proper memory management.
docs/meson.build (2)
47-75: LGTM! Addresses cross-build concern.The native grcli build for cross-compilation scenarios properly handles man-page generation during the build phase.
77-94: LGTM!Clean generation pipeline from command tree to installed man page.
cli/man_commands.c (4)
20-54: LGTM!Helper functions are clean and correctly implemented.
90-151: LGTM!Synopsis generation correctly handles the various ecoli node types recursively.
168-257: LGTM!Smart cross-referencing logic that links related commands based on argument types.
279-380: LGTM!Main entry point properly handles different command node types with correct memory management and error reporting.
abhiramnarayana
force-pushed
the
cli-man
branch
from
5055c5d to
9f258ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/man_commands.c (1)
64-66: String-typed arguments with IDs aren't collected.The condition omits
strcmp(type, "str") == 0, so string arguments with IDs won't appear in the ARGUMENTS section even thoughprint_argument_helphandles them.Apply this diff:
- if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 - && strcmp(type, "re") != 0) + if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 + && strcmp(type, "re") != 0 && strcmp(type, "str") != 0) goto recurse;
🧹 Nitpick comments (2)
docs/meson.build (1)
96-123: Hardcoded command list may drift from actual commands.The comment correctly notes that the command list must be hardcoded since grcli can't be executed during Meson configuration. However, when commands are added or removed, this list requires manual updates.
Consider adding a verification step to the CI workflow:
#!/bin/bash # Verify grcli_commands list in docs/meson.build matches actual commands # Extract commands from meson.build MESON_CMDS=$(rg -A 6 "^grcli_commands = \[" docs/meson.build | \ gawk "/'/{ gsub(/[',]/, \"\"); print \$1 }" | sort) # Get actual commands from grcli (requires build) # This would run in CI after build completes echo "Expected commands from meson.build:" echo "$MESON_CMDS" echo "" echo "To verify, compare with: ./build/grcli --man | gawk '/^- \\*\\*/ { print \$2 }' | tr -d '*:' | sort"cli/man_commands.c (1)
203-203: Prefer space prefix over suffix.The trailing space in
printf("**%s** ", ctx_name)combined withprint_node_synopsis's leading spaces produces double spaces in output. Remove the trailing space to follow the prefix pattern used elsewhere (lines 248, 250, 252, 254).Apply this diff:
- printf("**%s** ", ctx_name); + printf("**%s**", ctx_name);Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/check.yml(1 hunks)cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (3)
- rpm/grout.spec
- cli/man_main.c
- cli/main.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/complete.ccli/complete.hcli/man.hcli/man_commands.c
🧠 Learnings (4)
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
debian/grout.install
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.h
📚 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:
docs/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: 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:
cli/meson.build
🧬 Code graph analysis (3)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(279-380)cli/man_main.c (1)
print_main_man_page(175-242)
cli/man_commands.c (1)
cli/main.c (1)
help(34-53)
🔇 Additional comments (11)
debian/grout.install (1)
5-5: LGTM!Wildcard pattern correctly supports installation of dynamically generated per-command man pages alongside the main grcli.1 page.
meson.build (1)
129-131: LGTM!Correct dependency ordering ensures
grcli_exeis available for man page generation in the docs subdirectory. Comment clearly explains the rationale.docs/meson.build (3)
15-21: LGTM!Rename to
static_man_srcclearly distinguishes static man pages from the newly added dynamic grcli pages. Good clarity improvement.
44-75: LGTM!Native grcli build ensures man pages can be generated during cross-compilation. The conditional logic correctly falls back to the target
grcli_exefor native builds. This addresses the previous review concern about cross-build support.
77-94: LGTM!Main grcli man page generation is properly configured with correct install directory and build dependencies.
cli/meson.build (1)
13-14: LGTM!New man page generation sources properly integrated into the modular CLI build system.
cli/complete.h (1)
9-9: LGTM!Declaration correctly exposes centralized option definitions for reuse across completion and man page generation.
cli/complete.c (2)
17-35: LGTM!Centralized option definitions improve maintainability by eliminating duplication. The new
-m|--manoption is properly integrated with the rest of the options.
43-43: LGTM!Refactored to use centralized option definitions from
grcli_options_node(). Clean reuse of shared options.cli/man.h (1)
1-9: LGTM!Clean header file properly declares man page generation functions with appropriate dependencies and license information.
.github/workflows/check.yml (1)
128-128: Both package entries are necessary and intentional.The native dependency declaration at
docs/meson.build:62confirms thatlibsmartcols-dev:arm64(for ARM64 cross-compilation) andlibsmartcols-dev(for the native grcli build generating man pages) are both required.
abhiramnarayana
force-pushed
the
cli-man
branch
from
9f258ba to
4fc1040
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/main.c (2)
31-31: Do not advertise internal -m/--man in usage/help.Per maintainer guidance, keep -m functional but hidden from end users. Remove from usage/help output.
- printf(" %s -m|--man [COMMAND]\n", prog); + /* internal: -m not advertised */- puts(" -m, --man [COMMAND] Show man page for command or list all"); - puts(" commands."); + /* internal: -m not advertised */Also applies to: 42-43
178-181: Bug: Early -c/-m path can call disconnect(NULL) and force failure.When -c/--bash-complete or -m/--man are used, client is never connected; end: unconditionally calls gr_api_client_disconnect(client). Guard it.
end: - if (gr_api_client_disconnect(client) < 0) { + if (client != NULL && gr_api_client_disconnect(client) < 0) { errorf("gr_disconnect: %s", strerror(errno)); ret = EXIT_FAILURE; }Also applies to: 183-189, 222-226
cli/complete.c (1)
17-35: Centralized options: great; but keep -m internal (exclude from options tree).To align with “don’t advertise -m,” omit it from grcli_options_node() so completion and docs don’t expose it. Keep -m handling in cli/main.c.
Based on learnings
struct ec_node *grcli_options_node(void) { return EC_NODE_SUBSET( EC_NO_ID, FLAG("-h|--help", "Show usage help and exit."), OPT("-s|--socket " SOCK_PATH_ID, "Path to the control plane API socket.", ec_node("file", SOCK_PATH_ID)), FLAG("-e|--err-exit", "Abort on first error."), FLAG("-x|--trace-commands", "Print executed commands."), OPT("-f|--file PATH", "Read commands from _PATH_ instead of standard input.", ec_node("file", "PATH")), FLAG("-V|--version", "Print version and exit."), FLAG("-c|--bash-complete", "For use in bash completion."), - OPT("-m|--man COMMAND", - "Show man page for _COMMAND_ or list all commands if no argument.", - ec_node("str", "COMMAND")) ); }
🧹 Nitpick comments (5)
cli/man.h (1)
1-9: Minor header polish: C++ guards and const-correctness.For broader reuse, wrap in extern "C" and mark cmdlist read-only.
#pragma once #include <ecoli.h> -int print_man_page(struct ec_node *cmdlist, int argc, char **argv); -int print_main_man_page(struct ec_node *cmdlist); +#ifdef __cplusplus +extern "C" { +#endif + +int print_man_page(const struct ec_node *cmdlist, int argc, char **argv); +int print_main_man_page(const struct ec_node *cmdlist); + +#ifdef __cplusplus +} /* extern "C" */ +#endifdocs/meson.build (1)
85-93: Hardcoded command list risks drift.Consider generating the list from the command tree at build time, e.g., a small mode like
grcli --man --list-commandsto emit names, then consume it via a custom_target. This keeps docs in lockstep.Happy to draft the tiny CLI mode and Meson glue.
cli/man_main.c (3)
42-141: De-duplicate “or” printing logic.Both branches repeat the same loop to print OR’d flags. Extract a tiny helper to print an OR node once; improves readability.
+static void print_or_list(const struct ec_node *or_node, bool synopsis) { + bool first = true; + unsigned refs; + for (unsigned i = 0; i < ec_node_get_children_count(or_node); i++) { + struct ec_node *str_node; + if (ec_node_get_child(or_node, i, &str_node, &refs) < 0) continue; + char *desc = ec_node_desc(str_node); + if (desc == NULL) continue; + if (synopsis) { + if (first) printf("**%s**", desc); + } else { + if (!first) printf(", "); + printf("**%s**", desc); + } + first = false; + ec_free(desc); + } +} @@ - if (strcmp(child_type, "or") == 0) { - bool first = true; - ... - } + if (strcmp(child_type, "or") == 0) { + print_or_list(child, mode == SYNOPSIS_MODE); + } @@ - if (strcmp(or_type, "or") == 0) { - bool first = true; - ... - } + if (strcmp(or_type, "or") == 0) { + print_or_list(or_node, mode == SYNOPSIS_MODE); + }
143-173: Hide internal -m from SYNOPSIS/OPTIONS output.If -m is removed from grcli_options_node(), this section stops advertising it automatically. If you keep it there for other callers, filter it out here before printing.
- for (unsigned i = 0; i < ec_node_get_children_count(options_tree); i++) { + for (unsigned i = 0; i < ec_node_get_children_count(options_tree); i++) { struct ec_node *opt_node; unsigned refs; if (ec_node_get_child(options_tree, i, &opt_node, &refs) < 0) continue; + const char *id = ec_node_id(opt_node); + if (id && strstr(id, "-m|--man")) continue; /* internal */ print_option_syntax(opt_node, SYNOPSIS_MODE); } @@ - for (unsigned i = 0; i < ec_node_get_children_count(options_tree); i++) { + for (unsigned i = 0; i < ec_node_get_children_count(options_tree); i++) { struct ec_node *opt_node; unsigned refs; if (ec_node_get_child(options_tree, i, &opt_node, &refs) < 0) continue; + const char *id = ec_node_id(opt_node); + if (id && strstr(id, "-m|--man")) continue; /* internal */ print_option_syntax(opt_node, OPTION_MODE); }
195-235: Stabilize SEE ALSO ordering for reproducible builds.Collect names, sort them lexicographically, then print. Avoids order variance from the command tree.
- for (unsigned i = 0; i < ec_node_get_children_count(cmdlist); i++) { - ... - printf("**grcli-%s**(1)\n\n", name_or_copy); - } + struct gr_vec *names = gr_vec_new(sizeof(char *)); + for (unsigned i = 0; i < ec_node_get_children_count(cmdlist); i++) { + ... + /* push strdup(name) into names on success */ + } + qsort(gr_vec_data(names), gr_vec_len(names), sizeof(char *), strcmp_ptr); + for (size_t i = 0; i < gr_vec_len(names); i++) { + printf("**grcli-%s**(1)\n\n", ((char **)gr_vec_data(names))[i]); + free(((char **)gr_vec_data(names))[i]); + } + gr_vec_free(names);Note: gr_vec_*() cannot fail; no need to check returns. As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/check.yml(1 hunks)cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (3)
- meson.build
- cli/man_commands.c
- debian/grout.install
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man.hcli/complete.ccli/complete.hcli/main.ccli/man_main.c
🧠 Learnings (4)
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.h
📚 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:
docs/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: 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:
cli/meson.build
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
cli/man_main.c
🧬 Code graph analysis (4)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(279-380)cli/man_main.c (1)
print_main_man_page(175-242)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
cli/main.c (3)
cli/complete.c (1)
bash_complete(69-148)cli/man_main.c (1)
print_main_man_page(175-242)cli/man_commands.c (1)
print_man_page(279-380)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
⏰ 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 (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: rpm
- GitHub Check: deb
🔇 Additional comments (9)
cli/meson.build (1)
13-14: Sources added to CLI are fine.Wires man generators into build; no issues spotted.
.github/workflows/check.yml (1)
128-129: Native deps for cross-docs: OK, but watch libecoli (native).Having both :arm64 and host variants is intentional for cross + native man-gen. However, docs/meson.build currently requires a native libecoli; if it’s not available in the container, the native grcli link will fail. Either install a native libecoli (if packaged) or switch docs/meson to allow a fallback/subproject for the native dep.
docs/meson.build (3)
15-22: Static manpages refactor: LGTM.Rename to static_man_src + loop is clear and correct.
94-112: Per-command man targets: solid structure.Loop and md2man conversion are fine; will benefit from auto-generated command list when available.
66-83: No action needed — subdir ordering is correct.grcli_exe is defined at line 121 in root meson.build, which falls between subdir('cli') at line 110 and subdir('docs') at line 130. The variable is available to docs/meson.build during the non-cross-build path, and the cross-build path self-contains grcli_native. Build dependency chain is sound.
rpm/grout.spec (1)
108-108: Glob pattern is correct.Verified against meson.build: The pattern
grcli*.1*matches all installed man pages—grcli.1(main) andgrcli-<cmd>.1(command-specific pages: trace, traceroute, tunsrc, etc.), including compressed forms. No duplicates or stray files generated. The rpmbuild verification cannot run in this sandbox (rpm unavailable), but source inspection confirms the change is sound.cli/complete.h (1)
9-9: Public declaration looks good.Matches implementation, correct exposure for reuse.
cli/main.c (1)
8-8: Include looks correct.man.h inclusion is appropriate for -m handlers.
cli/complete.c (1)
43-44: Nice reuse.Replacing inlined subset with grcli_options_node() reduces duplication and keeps completion consistent.
| if meson.is_cross_build() | ||
| # Get native versions of dependencies for the native build | ||
| # Note: Cannot use fallback subproject for native deps in cross-build | ||
| ecoli_native = dependency('libecoli', version: '>= 0.8.0', native: true) | ||
| smartcols_native = dependency('smartcols', native: true) | ||
|
|
||
| grcli_native = executable( | ||
| 'grcli-native', | ||
| cli_src, | ||
| include_directories: cli_inc + api_inc, | ||
| dependencies: [ecoli_native, smartcols_native], | ||
| c_args: cli_cflags, | ||
| native: true, | ||
| ) | ||
| grcli_for_man = grcli_native | ||
| else | ||
| grcli_for_man = grcli_exe | ||
| endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Cross-build native grcli: missing native deps and brittle libecoli resolution.
- Link will likely fail without libedit (interactive CLI object files pull it in). Add a native libedit dep.
- Relying solely on a system-native libecoli can break CI/dev. Prefer allowing a fallback subproject for the native dep, or build a minimal native helper that excludes interact.c.
- ecoli_native = dependency('libecoli', version: '>= 0.8.0', native: true)
- smartcols_native = dependency('smartcols', native: true)
+ # Allow fallback if available; Meson supports native:true + fallback with recent versions.
+ ecoli_native = dependency('libecoli',
+ version: '>= 0.8.0', native: true,
+ fallback: ['libecoli', 'libecoli_dep'], required: true)
+ smartcols_native = dependency('smartcols', native: true)
+ edit_native = dependency('libedit', native: true, required: true)
grcli_native = executable(
'grcli-native',
cli_src,
include_directories: cli_inc + api_inc,
- dependencies: [ecoli_native, smartcols_native],
+ dependencies: [ecoli_native, smartcols_native, edit_native],
c_args: cli_cflags,
native: true,
)Alternatively, define a reduced cli_src_for_man that excludes interact.c to avoid libedit entirely. I can sketch that if you prefer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if meson.is_cross_build() | |
| # Get native versions of dependencies for the native build | |
| # Note: Cannot use fallback subproject for native deps in cross-build | |
| ecoli_native = dependency('libecoli', version: '>= 0.8.0', native: true) | |
| smartcols_native = dependency('smartcols', native: true) | |
| grcli_native = executable( | |
| 'grcli-native', | |
| cli_src, | |
| include_directories: cli_inc + api_inc, | |
| dependencies: [ecoli_native, smartcols_native], | |
| c_args: cli_cflags, | |
| native: true, | |
| ) | |
| grcli_for_man = grcli_native | |
| else | |
| grcli_for_man = grcli_exe | |
| endif | |
| if meson.is_cross_build() | |
| # Get native versions of dependencies for the native build | |
| # Note: Cannot use fallback subproject for native deps in cross-build | |
| # Allow fallback if available; Meson supports native:true + fallback with recent versions. | |
| ecoli_native = dependency('libecoli', | |
| version: '>= 0.8.0', native: true, | |
| fallback: ['libecoli', 'libecoli_dep'], required: true) | |
| smartcols_native = dependency('smartcols', native: true) | |
| edit_native = dependency('libedit', native: true, required: true) | |
| grcli_native = executable( | |
| 'grcli-native', | |
| cli_src, | |
| include_directories: cli_inc + api_inc, | |
| dependencies: [ecoli_native, smartcols_native, edit_native], | |
| c_args: cli_cflags, | |
| native: true, | |
| ) | |
| grcli_for_man = grcli_native | |
| else | |
| grcli_for_man = grcli_exe | |
| endif |
🤖 Prompt for AI Agents
In docs/meson.build around lines 47 to 64, the cross-build branch creating
grcli-native misses a native libedit dependency and relies only on a system
libecoli which is brittle; add dependency('libedit', native: true) to
grcli_native so interactive object files link, and change the libecoli native
resolution to allow a fallback (e.g., try dependency('libecoli', version: '>=
0.8.0', native: true) with fallback to a subproject) or replace grcli_native
with a minimal native helper that uses a reduced cli_src_for_man excluding
interact.c to avoid pulling in libedit — implement one of these fixes and ensure
include_directories/dependencies reflect the chosen approach.
0e99db0 to
59919ca
Compare
abhiramnarayana
force-pushed
the
cli-man
branch
2 times, most recently
from
683f919 to
256b4a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/main.c (1)
178-189: Verify NULL check at disconnect.Early returns for
-cand-moptions correctly set ret and jump to end. Ensure line 223'sgr_api_client_disconnect(client)has a NULL guard (as indicated by the previous review).cli/man_commands.c (1)
48-80: Consider including "str" type in argument collection.Lines 56-58 exclude string-typed nodes from argument collection. If string nodes with IDs represent actual arguments (not literals), they should be collected alongside uint, int, dyn, and re types. The
print_argument_help()function at lines 37-45 already handles "str" type, suggesting they may be valid arguments.Apply this diff if string arguments should be collected:
if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 - && strcmp(type, "re") != 0) + && strcmp(type, "re") != 0 && strcmp(type, "str") != 0) goto recurse;
🧹 Nitpick comments (1)
docs/meson.build (1)
73-78: Consider adding a verification check for the hardcoded command list.The hardcoded command list could become out of sync if commands are added or removed. Consider adding a test or build-time check to verify that this list matches the actual commands available in grcli.
For example, you could add a verification script:
#!/bin/bash # Verify the hardcoded command list matches actual grcli commands # Extract commands from meson.build meson_commands=$(rg -oP "grcli_commands\s*=\s*\[\s*\K[^]]*" docs/meson.build | tr -d "',\n " | tr ' ' '\n' | sort) # Get actual commands from grcli (requires running grcli --help or similar) # This would need to be implemented based on grcli's help output format echo "Manual verification recommended: ensure command list matches grcli's actual commands"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (2)
- debian/grout.install
- rpm/grout.spec
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man.hcli/complete.hcli/man_commands.ccli/man_main.ccli/main.ccli/complete.c
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
📚 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:
meson.buildcli/man_commands.ccli/meson.buildcli/man_main.cdocs/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: 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:
meson.buildcli/man_commands.ccli/meson.builddocs/meson.build
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
PR: DPDK/grout#329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.hcli/complete.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
PR: DPDK/grout#337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
cli/man_main.ccli/main.cdocs/meson.build
🧬 Code graph analysis (5)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(271-372)cli/man_main.c (2)
print_main_man_page(175-242)man_print_title_underline(29-35)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
cli/man_commands.c (2)
cli/main.c (1)
help(34-53)cli/man_main.c (1)
man_print_title_underline(29-35)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
cli/main.c (3)
cli/complete.c (1)
bash_complete(69-148)cli/man_main.c (1)
print_main_man_page(175-242)cli/man_commands.c (1)
print_man_page(271-372)
🔇 Additional comments (14)
cli/complete.h (1)
9-9: LGTM!Clean public API addition for centralizing CLI options.
cli/man.h (1)
1-10: LGTM!Well-structured header with proper licensing and include guards. Function signatures align with implementations.
cli/meson.build (1)
13-14: LGTM!New man page sources correctly added to the modular CLI build.
Based on learnings
meson.build (1)
129-130: LGTM!Correct build ordering ensures grcli executable is available for man page generation.
cli/complete.c (1)
17-35: LGTM!Excellent centralization of option definitions. The reuse at line 43 eliminates duplication.
cli/man_main.c (4)
18-27: LGTM!Well-defined macros for descriptive text keep the code clean and maintainable.
42-141: LGTM!The
print_option_syntax()function correctly handles multiple node types and modes. Thegoto endpattern at line 85 is appropriate for cleanup.
143-173: LGTM!Proper error handling for options tree creation and cleanup at line 171.
175-242: LGTM!Main entry point correctly orchestrates man page generation. Memory management for
name_copy(lines 223-232) is handled properly with NULL check and free.cli/man_commands.c (3)
82-143: LGTM!Complex recursive synopsis rendering handles all relevant node types correctly. Clean structure with appropriate formatting for each type.
160-249: LGTM!Well-structured command detail generation with proper argument collection, cross-reference detection (lines 214-229), and cleanup (line 234).
271-372: LGTM!Entry point correctly iterates through command list, handles both seq and cmd node types, and provides appropriate error handling for unknown commands (lines 365-368). Memory management for
name_copyis sound (lines 327-333, 359).docs/meson.build (2)
47-50: Verify that skipping man page generation during cross-compilation is acceptable.This approach differs from the previous suggestion to build a native grcli executable with native dependencies. While simpler, it means man pages won't be generated during cross-builds. If man pages are needed in cross-compiled packages, consider building a native grcli executable as previously suggested.
Based on learnings
56-56: No action required.
grcli_exeis properly defined in the top-level meson.build at line 121 and will be available when docs/meson.build is processed.
abhiramnarayana
force-pushed
the
cli-man
branch
2 times, most recently
from
f386d8d to
2f4ace3
Compare
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)
cli/main.c (1)
223-226: Guard disconnect when no client was created.
-c/--bash-completeand the new-m/--manpaths never establish a client, yet they now jump toend:where we always callgr_api_client_disconnect(client). Withclient == NULLthis fails and flips a previously successful run intoEXIT_FAILURE, so both commands always report failure. Wrap the disconnect in a null check so these early-exit flows stay successful.end: - if (gr_api_client_disconnect(client) < 0) { + if (client != NULL && gr_api_client_disconnect(client) < 0) { errorf("gr_disconnect: %s", strerror(errno)); ret = EXIT_FAILURE; }
♻️ Duplicate comments (2)
docs/meson.build (1)
44-50: Cross-compilation guard skips man pages.This approach is simple but means man pages won't be generated during cross-builds. Past comments suggested building a native grcli with proper native dependencies instead.
cli/man_commands.c (1)
56-58: String arguments are not collected.The type check excludes
"str", so string-typed arguments with IDs won't appear in ARGUMENTS sections despiteprint_argument_help(line 41-42) handling them.Apply this diff to include string arguments:
- if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 - && strcmp(type, "re") != 0) + if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "str") != 0 + && strcmp(type, "dyn") != 0 && strcmp(type, "re") != 0) goto recurse;
🧹 Nitpick comments (1)
docs/meson.build (1)
73-78: Consider adding a sync check for the hardcoded command list.While the comment explains why this list is hardcoded, there's no protection against drift. Consider adding a test target that runs
grcli --manwithout arguments (if it supports listing commands) to verify the list is complete.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (4)
- meson.build
- cli/meson.build
- cli/complete.c
- cli/complete.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/main.ccli/man_commands.ccli/man_main.ccli/man.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
rpm/grout.specdebian/grout.installcli/man_commands.ccli/man_main.cdocs/meson.build
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
rpm/grout.specdebian/grout.installcli/main.ccli/man_main.cdocs/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: 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:
cli/man_commands.cdocs/meson.build
🧬 Code graph analysis (4)
cli/main.c (2)
cli/man_main.c (1)
print_main_man_page(175-242)cli/man_commands.c (1)
print_man_page(273-374)
cli/man_commands.c (2)
cli/main.c (1)
help(34-53)cli/man_main.c (1)
man_print_title_underline(29-35)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(273-374)cli/man_main.c (2)
print_main_man_page(175-242)man_print_title_underline(29-35)
⏰ 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). (4)
- 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 (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (6)
docs/meson.build (1)
52-69: LGTM!The main man page generation logic correctly uses
grcli_exeand pipes through md2man. Build will fail appropriately if generation fails.cli/man_commands.c (5)
20-26: LGTM!Simple linear search is appropriate for the expected small number of arguments.
82-143: LGTM!Synopsis generation correctly handles all node types with appropriate Markdown formatting. Recursion depth should be bounded by command tree structure in practice.
216-231: Heuristic cross-referencing is pragmatic.The SEE ALSO section uses argument name patterns to detect relationships. While not perfect, this is a reasonable approach without explicit command metadata.
273-374: LGTM!Main entry point correctly handles both seq and cmd node types. Memory management is sound (strdup freed on all paths), and error handling is appropriate.
28-46: LGTM!Function correctly handles all argument types including strings. Will work properly once
collect_argumentsis fixed to include string-typed arguments.
cli/man_main.c
Outdated
| if (mode == SYNOPSIS_MODE) { | ||
| if (first) | ||
| printf("**%s**", desc); | ||
| } else { | ||
| if (!first) | ||
| printf(", "); | ||
| printf("**%s**", desc); | ||
| } | ||
| first = false; | ||
| ec_free(desc); | ||
| } | ||
| } else if (strcmp(child_type, "seq") == 0) { | ||
| if (ec_node_get_children_count(child) < 2) | ||
| goto end; | ||
|
|
||
| struct ec_node *or_node, *arg_node; | ||
| unsigned or_refs, arg_refs; | ||
|
|
||
| if (ec_node_get_child(child, 0, &or_node, &or_refs) >= 0) { | ||
| const char *or_type = ec_node_type(or_node)->name; | ||
| if (strcmp(or_type, "or") == 0) { | ||
| bool first = true; | ||
| for (unsigned i = 0; i < ec_node_get_children_count(or_node); i++) { | ||
| struct ec_node *str_node; | ||
| if (ec_node_get_child(or_node, i, &str_node, &refs) < 0) | ||
| continue; | ||
| char *desc = ec_node_desc(str_node); | ||
| if (desc == NULL) | ||
| continue; | ||
|
|
||
| if (mode == SYNOPSIS_MODE) { | ||
| if (first) | ||
| printf("**%s**", desc); | ||
| } else { | ||
| if (!first) | ||
| printf(", "); | ||
| printf("**%s**", desc); | ||
| } | ||
| first = false; | ||
| ec_free(desc); | ||
| } |
There was a problem hiding this comment.
Render every alias in SYNOPSIS.
In the SYNOPSIS branch we only print the first alternative from the or node, so options like -h|--help lose the long form entirely. That’s a functional doc bug. Print every alias, separated by |, just like the static pages already do.
- if (ec_node_get_child(child, i, &str_node, &refs) < 0)
+ if (ec_node_get_child(child, i, &str_node, &refs) < 0)
continue;
char *desc = ec_node_desc(str_node);
if (desc == NULL)
continue;
- if (mode == SYNOPSIS_MODE) {
- if (first)
- printf("**%s**", desc);
- } else {
+ if (mode == SYNOPSIS_MODE) {
+ if (!first)
+ printf("|");
+ printf("**%s**", desc);
+ } else {
if (!first)
printf(", ");
printf("**%s**", desc);
@@
- if (mode == SYNOPSIS_MODE) {
- if (first)
- printf("**%s**", desc);
- } else {
+ if (mode == SYNOPSIS_MODE) {
+ if (!first)
+ printf("|");
+ printf("**%s**", desc);
+ } else {
if (!first)
printf(", ");
printf("**%s**", desc);🤖 Prompt for AI Agents
In cli/man_main.c around lines 72-112, the SYNOPSIS branch only prints the first
alternative in an "or" node so aliases like "-h|--help" drop the long form;
change the SYNOPSIS handling inside the loop over or_node children to print all
aliases separated by '|' instead of only the first: when mode == SYNOPSIS_MODE,
print the first alias as "**%s**" and for each subsequent alias print a leading
'|' then "**%s**" (i.e. "|**%s**"), otherwise keep the existing comma-separated
behavior; continue to free each desc and maintain refs as currently done.
rjarry
left a comment
rjarry
left a comment
There was a problem hiding this comment.
Could you split your diff in multiple commits?
- expose grcli_options_node for reuse in man page generation
- implement
grcli --man - replace static man page with generated one during build
- implement
grcli --man <command> - generate man pages during build
abhiramnarayana
force-pushed
the
cli-man
branch
from
2f4ace3 to
6c92801
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/man_main.c (2)
72-79: SYNOPSIS drops option aliases after the first.In SYNOPSIS_MODE, only the first alternative is printed (when
firstis true). Options like-h|--helpwill show as just-h. Print all aliases separated by|.Apply this fix:
if (mode == SYNOPSIS_MODE) { - if (first) - printf("**%s**", desc); + if (!first) + printf("|"); + printf("**%s**", desc); } else {
102-109: Same alias-dropping issue in seq branch.The
seqcase also omits aliases after the first in SYNOPSIS_MODE.Apply the same fix:
if (mode == SYNOPSIS_MODE) { - if (first) - printf("**%s**", desc); + if (!first) + printf("|"); + printf("**%s**", desc); } else {cli/man_commands.c (1)
56-58: String arguments are excluded from ARGUMENTS section.
collect_argumentsskips"str"type nodes with IDs, so string arguments won't appear in generated documentation. Includestrcmp(type, "str") == 0in the condition.if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 - && strcmp(type, "re") != 0) + && strcmp(type, "re") != 0 && strcmp(type, "str") != 0) goto recurse;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/complete.c(1 hunks)cli/complete.h(1 hunks)cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
✅ Files skipped from review due to trivial changes (1)
- debian/grout.install
🚧 Files skipped from review as they are similar to previous changes (5)
- rpm/grout.spec
- cli/man.h
- cli/meson.build
- meson.build
- cli/complete.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man_commands.ccli/man_main.ccli/main.ccli/complete.h
🧠 Learnings (5)
📓 Common learnings
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
docs/meson.buildcli/man_commands.ccli/man_main.c
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: 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:
docs/meson.buildcli/man_commands.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
docs/meson.buildcli/man_main.ccli/main.c
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/complete.h
🧬 Code graph analysis (4)
cli/man_commands.c (2)
cli/main.c (1)
help(34-53)cli/man_main.c (1)
man_print_title_underline(29-35)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
cli/main.c (2)
cli/man_main.c (1)
print_main_man_page(175-242)cli/man_commands.c (1)
print_man_page(279-380)
cli/complete.h (1)
cli/complete.c (1)
grcli_options_node(17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: deb
- GitHub Check: rpm
- GitHub Check: build-and-tests (gcc-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-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (3)
cli/complete.h (1)
9-9: LGTM!The new public API for centralizing CLI options is clean and well-integrated.
docs/meson.build (1)
44-50: Cross-build guard is appropriate.Skipping man page generation during cross-compilation is the right approach since the target-architecture
grcli_execannot run on the build machine.cli/main.c (1)
178-189: No issue found —gr_api_client_disconnectsafely handles NULL pointers.The function implementation includes an early NULL check:
if (client == NULL) return 0;. When the-cor-mbranches execute,clientremains NULL (initialized at function start) and is safely passed togr_api_client_disconnectat line 220, returning 0 without side effects.
| # The list is hardcoded since we can't run grcli during meson configuration. | ||
| grcli_commands = [ | ||
| 'address', 'affinity', 'conntrack', 'dnat44', 'events', | ||
| 'graph', 'interface', 'logging', 'nexthop', 'ping', | ||
| 'quit', 'route', 'router-advert', 'snat44', 'stats', | ||
| 'trace', 'traceroute', 'tunsrc', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Hardcoded command list requires manual maintenance.
The comment explains the constraint, but this list will become stale when commands are added or removed. Consider adding a check or comment in the main CLI registration code reminding developers to update this list.
Verify the list matches current commands:
🏁 Script executed:
#!/bin/bash
# Extract command names from CLI source registrations
rg -nP '(?:ec_node_subset|ec_node_or)\s*\([^,]+,\s*"([a-z-]+)"' cli/ -r '$1' | sort -uLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find CLI-related files and directory structure
fd -t f -e 'c|h|go' 'cli|cmd|command' --max-depth 3 | head -20
echo "---"
# Search for any of the hardcoded commands to find where they're defined
rg -l 'address|affinity|conntrack|dnat44' --max-count=5
echo "---"
# Look for ec_node patterns more broadly
rg -n 'ec_node' --type c --max-count=10Length of output: 23838
🏁 Script executed:
#!/bin/bash
# Look at cli/exec.c to see how commands are registered
cat cli/exec.c | head -100
echo "---DIVIDER---"
# Search for files/functions with "logging", "ping", "traceroute", "tunsrc"
fd -t f \( -name '*logging*' -o -name '*ping*' -o -name '*traceroute*' -o -name '*tunsrc*' \)
echo "---DIVIDER---"
# Check meson.build files to understand command structure
grep -r "grcli_commands\|logging\|ping\|traceroute\|tunsrc" docs/ cli/ --include="*.build"Length of output: 2938
🏁 Script executed:
#!/bin/bash
# Search for missing commands with correct fd syntax
find . -type f \( -name '*logging*' -o -name '*ping*' -o -name '*traceroute*' -o -name '*tunsrc*' \) 2>/dev/null
echo "---"
# Also search for where these commands might be registered as context
rg -l 'logging|ping|traceroute|tunsrc' --type c --type h | head -20
echo "---"
# List all .c files in cli directories to see complete list
find modules/ cli/ -path '*/cli/*.c' -type f | sortLength of output: 1178
🏁 Script executed:
#!/bin/bash
# Find all cli_context_register calls to see what's actually registered
rg 'cli_context_register|\.name.*=' cli/ modules/ --type c -B2 -A2 | head -50
echo "---"
# Check if there's a logging CLI context defined anywhere
rg 'logging.*ctx_init|ctx_init.*logging' --type c
echo "---"
# Check quit.c to see command registration pattern
cat cli/quit.cLength of output: 2776
🏁 Script executed:
#!/bin/bash
# Find all cli_context definitions with their .name values
rg 'struct cli_context ctx = \{' cli/ modules/ --type c -A5 | grep -E '\.name = |struct cli_context'
echo "---ALL_ACTUAL_COMMANDS---"
# Extract just the command names from registered contexts
rg 'struct cli_context ctx = \{' cli/ modules/ --type c -A2 | grep "\.name = " | sed 's/.*\.name = "\([^"]*\)".*/\1/' | sortLength of output: 2441
Update meson.build list to match actual registered commands.
The hardcoded list is significantly out of sync. Currently it includes 3 non-existent commands (logging, traceroute, tunsrc), misses 6 registered commands (ping6, infra port, infra vlan, ipip, srv6_localsid, srv6_route), and has incorrect names for 3 others (affinity, interface, router-advert should be "infra affinity", "infra iface", "ipv6 router-advert").
Update docs/meson.build lines 73-78 to:
grcli_commands = [
'address', 'conntrack', 'dnat44', 'events', 'graph',
'infra affinity', 'infra iface', 'infra port', 'infra vlan', 'ipip',
'ipv6 router-advert', 'nexthop', 'ping', 'ping6', 'quit',
'route', 'snat44', 'srv6_localsid', 'srv6_route', 'stats', 'trace',
]
🤖 Prompt for AI Agents
In docs/meson.build around lines 72 to 78, the hardcoded grcli_commands list is
out of sync with registered commands: remove the three non-existent entries
(logging, traceroute, tunsrc), rename three entries to their correct names
("affinity" -> "infra affinity", "interface" -> "infra iface", "router-advert"
-> "ipv6 router-advert"), and add the missing six commands ("ping6", "infra
port", "infra vlan", "ipip", "srv6_localsid", "srv6_route"); update the
grcli_commands list on those lines to contain the corrected set as described in
the review comment.
ae402a8 to
7da7846
Compare
aharivel
left a comment
aharivel
left a comment
There was a problem hiding this comment.
The overall code is really poorly done, no offence. The excessive usage of nested "if" makes it really hard to follow. This code should be simplified and make use of "enum" and smaller functions has much as possible in order to be way more maintainable. Maybe also using of patterns like polymorphism or dictionaries could really help.
cli/man_main.c
Outdated
| if (desc == NULL) | ||
| continue; | ||
|
|
||
| if (mode == SYNOPSIS_MODE) { |
There was a problem hiding this comment.
It looks way too heavy... you could simplify like this :
if (mode != SYNOPSIS_MODE && !first)
printf(", ");
And/or add the ternary operator:
} else {
printf("%s**%s**", (!first ? ", " : ""), desc);
}
There was a problem hiding this comment.
I have changed it as suggested.
| if (find_arg(*args, *count, id)) | ||
| goto recurse; | ||
|
|
||
| struct arg_entry *new_args = realloc(*args, (*count + 1) * sizeof(**args)); |
There was a problem hiding this comment.
Calling realloc for every single argument is inefficient... Either pre-allocate or grow by chunks.
There was a problem hiding this comment.
Thanks for the suggestion — you’re right that reallocating per element can be inefficient for large datasets. But here I would highlight 2 important things i) the idea is to create this man pages during build time once and use it to display. ii) Most commands have short number agruments(typically less than 10) . Except for interface case. (I think it is 15) With the above 2 points and mainly because of i) I feel we can keep it simple and as it is now. Let me know if it is fine.
| const char *id = ec_node_id(node); | ||
|
|
||
| if (id == NULL || strcmp(id, EC_NO_ID) == 0) | ||
| goto recurse; |
There was a problem hiding this comment.
Use return instead of goto recurse.
There was a problem hiding this comment.
Its necessary because even when a node has no ID, its children might have IDs that represent arguments. Even I tried adding ONLY this change and tested now. The arguments section for address man page showed empty
|
|
||
| if (strcmp(type, "uint") != 0 && strcmp(type, "int") != 0 && strcmp(type, "dyn") != 0 | ||
| && strcmp(type, "re") != 0) | ||
| goto recurse; |
There was a problem hiding this comment.
Use lookup table or switch for type checking no? Like:
static const char *argument_types[] = {"uint", "int", "dyn", "re", NULL};
...
if (!is_valid_type(type, argument_types))
return;
There was a problem hiding this comment.
Sure. I have modified as suggested.
cli/man_commands.c
Outdated
|
|
||
| struct arg_entry *new_args = realloc(*args, (*count + 1) * sizeof(**args)); | ||
| if (new_args == NULL) | ||
| return; |
|
|
||
| if (strcmp(name, requested_cmd) != 0) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
All the code above an excessive nesting if statement... Hard to follow :(
should be break into smaller functions and/or use of :
enum ec_node_type_enum {
NODE_TYPE_SEQ,
NODE_TYPE_OR,
NODE_TYPE_CMD,
...
With something like I guess:
static enum ec_node_type_enum get_node_type(const struct ec_node *node) {
const char *type = ec_node_type(node)->name;
There was a problem hiding this comment.
+1 for an enum, this is unreadable
cli/man_commands.c
Outdated
|
|
||
| char *name_copy = strdup(full_id); | ||
| if (name_copy == NULL) | ||
| continue; |
There was a problem hiding this comment.
Just skip?? No error handling ??
There was a problem hiding this comment.
Done. Returning Failure now.
cli/man_main.c
Outdated
| } | ||
| } else if (strcmp(child_type, "seq") == 0) { | ||
| if (ec_node_get_children_count(child) < 2) | ||
| goto end; |
There was a problem hiding this comment.
Why don't you just return ?
There was a problem hiding this comment.
Done. Modified
cli/man_main.c
Outdated
| printf("%s\n\n", help); | ||
| } | ||
|
|
||
| end: |
There was a problem hiding this comment.
Done. Modified.
abhiramnarayana
force-pushed
the
cli-man
branch
from
7da7846 to
a076150
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/meson.build (1)
72-78: Hardcoded command list remains out of sync with registered commands.This list is still significantly outdated per the previous review: it includes 3 non-existent commands (logging, traceroute, tunsrc), misses 6 registered commands (ping6, infra port, infra vlan, ipip, srv6_localsid, srv6_route), and has incorrect names for 3 others (affinity → infra affinity, interface → infra iface, router-advert → ipv6 router-advert).
cli/man_main.c (1)
72-75: SYNOPSIS still omits alternate option forms.The code continues to skip non-first alternatives in SYNOPSIS mode, so
-h|--helpshows only-h. This was flagged in a previous review with a detailed fix to print all aliases separated by|.Also applies to: 100-103
cli/man_commands.c (1)
56-88: String-typed arguments still not collected.Line 58 defines
argument_typesas{"uint", "int", "dyn", "re", NULL}but excludes"str". String arguments with IDs won't appear in the ARGUMENTS section. This duplicates an earlier review finding.Optional: realloc per argument is inefficient.
Growing the args array one entry at a time (line 71) is inefficient for many arguments. Consider pre-allocating or growing in chunks. Given typical CLI argument counts, this is likely acceptable as-is.
🧹 Nitpick comments (1)
cli/man_commands.c (1)
287-390: Main function is complex but functional.The
print_man_pagefunction spans 104 lines with nested conditionals for traversing and matching command nodes. While the logic is correct and the structure is clear, extracting helper functions (e.g.,handle_seq_node_match,handle_cmd_node_match) could improve maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cli/main.c(4 hunks)cli/man.h(1 hunks)cli/man_commands.c(1 hunks)cli/man_main.c(1 hunks)cli/meson.build(1 hunks)debian/grout.install(1 hunks)docs/grcli.1.md(0 hunks)docs/meson.build(2 hunks)meson.build(1 hunks)rpm/grout.spec(1 hunks)
💤 Files with no reviewable changes (1)
- docs/grcli.1.md
🚧 Files skipped from review as they are similar to previous changes (3)
- rpm/grout.spec
- meson.build
- debian/grout.install
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
cli/man_main.ccli/man.hcli/main.ccli/man_commands.c
🧠 Learnings (5)
📓 Common learnings
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: In this project, cli_src is a global variable that gets populated by individual modules (like modules/policy/cli/meson.build) and is ultimately consumed by an executable() call in the top-level meson.build file. This creates a modular CLI build where each module contributes its CLI sources to the main CLI binary.
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
cli/man_main.ccli/meson.builddocs/meson.buildcli/man_commands.c
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
cli/man_main.ccli/main.cdocs/meson.build
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: 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:
cli/meson.builddocs/meson.buildcli/man_commands.c
📚 Learning: 2025-10-03T13:42:52.180Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: cli/complete.c:87-91
Timestamp: 2025-10-03T13:42:52.180Z
Learning: In the ecoli library, node construction functions like ec_node_sh_lex_expand() take ownership of child nodes passed to them. On success, they return a new parent node that owns the children. On failure, they free the children internally and return NULL. This is why bash_complete_node() in cli/complete.c can safely reassign cmdlist and free it on the error path—the library has already freed it if the function fails.
Applied to files:
cli/man_commands.c
🧬 Code graph analysis (4)
cli/man_main.c (2)
cli/main.c (1)
help(34-53)cli/complete.c (1)
grcli_options_node(17-35)
cli/man.h (2)
cli/man_commands.c (1)
print_man_page(287-390)cli/man_main.c (2)
print_main_man_page(170-237)man_print_title_underline(29-35)
cli/main.c (2)
cli/man_main.c (1)
print_main_man_page(170-237)cli/man_commands.c (1)
print_man_page(287-390)
cli/man_commands.c (2)
cli/main.c (1)
help(34-53)cli/man_main.c (1)
man_print_title_underline(29-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: rpm
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
🔇 Additional comments (4)
docs/meson.build (1)
44-50: Cross-build guard is a pragmatic solution.Skipping man page generation during cross-builds avoids the complexity of resolving native dependencies (libecoli, libsmartcols, libedit) for the build machine. The warning message is clear and appropriate.
cli/meson.build (1)
13-14: LGTM!The new man page generation sources are correctly added to the modular CLI build system.
cli/man.h (1)
1-10: LGTM!Clean header with appropriate declarations for the man page generation API.
cli/man_main.c (1)
170-237: LGTM!The main man page generation logic is well-structured, handles both node types correctly, and includes appropriate error handling.
| if (argc >= 2 && (!strcmp(argv[1], "-c") || !strcmp(argv[1], "--bash-complete"))) { | ||
| ret = bash_complete(cmdlist); | ||
| goto end; | ||
| } | ||
|
|
||
| if (argc >= 2 && (!strcmp(argv[1], "-m") || !strcmp(argv[1], "--man"))) { | ||
| if (argc == 2) | ||
| ret = print_main_man_page(cmdlist); | ||
| else | ||
| ret = print_man_page(cmdlist, argc, argv); | ||
| goto end; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify NULL guard for client disconnect.
The early return paths for -c and -m jump to the end label where gr_api_client_disconnect(client) is called (line 223). At these early exit points, client is still NULL. A previous review indicated this was fixed, but the guard isn't visible in the provided code.
Confirm the disconnect call is properly guarded:
Also applies to: 223-226
🏁 Script executed:
#!/bin/bash
# Check if gr_api_client_disconnect is guarded with a NULL check
rg -nC3 'gr_api_client_disconnect' cli/main.cLength of output: 172
Add NULL guard before gr_api_client_disconnect(client) call.
The disconnect call at line 223 has no guard. Early exit paths for -c and -m flags (lines 180, 188) jump to the end: label via goto end while client remains NULL. The fix is straightforward:
if (client != NULL && gr_api_client_disconnect(client) < 0) {🤖 Prompt for AI Agents
In cli/main.c around lines 178 to 189 (early exits for -c and -m) and the end
label where gr_api_client_disconnect(client) is called, add a NULL guard so we
only call gr_api_client_disconnect when client is non-NULL and handle its
negative return; specifically, change the disconnect invocation to first check
client != NULL and then call gr_api_client_disconnect(client) and handle a <0
return (e.g., set ret or log) before proceeding to cleanup and exit.
a076150 to
dcc4ffe
Compare
cli/main.c
Outdated
| if (argc >= 2 && (!strcmp(argv[1], "-c") || !strcmp(argv[1], "--bash-complete"))) | ||
| return bash_complete(cmdlist); | ||
| if (argc >= 2 && (!strcmp(argv[1], "-c") || !strcmp(argv[1], "--bash-complete"))) { | ||
| ret = bash_complete(cmdlist); | ||
| goto end; | ||
| } |
There was a problem hiding this comment.
Move this as a separate commit.
cli/man_main.c
Outdated
| if (cmd_node == NULL) | ||
| return; | ||
|
|
cli/man_main.c
Outdated
| if (child_type == NULL) | ||
| return; | ||
|
|
cli/man_main.c
Outdated
| printf("]\n"); | ||
| else { | ||
| printf("\n\n"); | ||
| const char *help = ec_dict_get(ec_node_attrs(cmd_node), "help"); |
There was a problem hiding this comment.
Reuse HELP_ATTR (defined in cli/ecoli.c, move it to a header)
Line 13 in e5db14a
cli/man_main.c
Outdated
| if (options_tree == NULL) { | ||
| fprintf(stderr, "Error: failed to create options tree\n"); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
replace with
assert(options_tree != NULL)
cli/man_main.c
Outdated
| if (print_cli_options_from_tree() != EXIT_SUCCESS) | ||
| return EXIT_FAILURE; |
There was a problem hiding this comment.
That is a weird return value for a function. Usually, an error is a negative number and 0 on success. The EXIT_SUCCESS and EXIT_FAILURE constants should only be used in main().
| static void usage(const char *prog) { | ||
| printf("Usage: %s [-e] [-f PATH] [-h] [-s PATH] [-V] [-x] ...\n", prog); | ||
| printf(" %s -c|--bash-complete\n", prog); | ||
| printf(" %s -m|--man [COMMAND]\n", prog); |
There was a problem hiding this comment.
In this commit, --man does not take an argument. You should split the synopsis.
|
|
||
| if (strcmp(name, requested_cmd) != 0) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
+1 for an enum, this is unreadable
Separate the CLI options definition into a dedicated function. This will be reused for both bash completion and man page generation (to be added in later commits) Assisted-by: Cursor/Claude-4-Sonnet Signed-off-by: Abhiram R N <[email protected]>
abhiramnarayana
force-pushed
the
cli-man
branch
from
dcc4ffe to
a93783a
Compare
Add dynamic generation of the main grcli man page by traversing the options tree. Need to match the current static grcli.1.md file Assisted-by: Cursor/Claude-4-Sonnet Signed-off-by: Abhiram R N <[email protected]>
Use the goto end pattern for bash_complete() to ensure consistent cleanup and resource management across all CLI exit paths. Signed-off-by: Abhiram R N <[email protected]>
Remove the static docs/grcli.1.md file and replace it with dynamic generation during the build phase. Assisted-by: Cursor/Claude-4-Sonnet Signed-off-by: Abhiram R N <[email protected]>
Add support for generating individual command man pages by passing a command name to `grcli --man`. Dynamically traverse the command tree to find the requested command and generate a complete man page including SYNOPSIS, ARGUMENTS, and SEE ALSO sections. Additionally, update the main grcli man page to list all available commands in its SEE ALSO section which was created in an earlier commit without listing the commands. Assisted-by: Cursor/Claude-4-Sonnet Signed-off-by: Abhiram R N <[email protected]>
Add build rules to dynamically generate man pages for each grcli command by invoking 'grcli --man <command>'. The list of commands is hardcoded in meson.build since meson cannot execute the compiled binary during the configuration phase. Update packaging files (under debian and rpm) to include all generated grcli man pages. Assisted-by: Cursor/Claude-4-Sonnet Signed-off-by: Abhiram R N <[email protected]>
abhiramnarayana
force-pushed
the
cli-man
branch
from
a93783a to
1d468e3
Compare
3 participants
Add dynamic man page generation by traversing the libecoli command tree at runtime. Add new option in grcli (--man and --man [COMMAND]) to walk through the registered command nodes and extract required data to generate properly formatted man pages in Markdown format.
Do build system changes in meson to invoke grcli --man during the build phase. Convert the man pages in Markdown to groff format using go-md2man.
Do changes for packaging files for both Debian and RPM
Assisted-by: Cursor/Claude-4-Sonnet
Summary by CodeRabbit
New Features
Documentation
Chores