Skip to content

vtysh, lib: fix 'show ip[v6] prefix-list ... json' JSON display by moving it to vtysh to fix formatting issue #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
suphawitwanl wants to merge 5 commits into
base: aitest-base-pr-15750
Choose a base branch
from
Open

vtysh, lib: fix 'show ip[v6] prefix-list ... json' JSON display by moving it to vtysh to fix formatting issue #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a17289
vtysh, lib: fix 'show ip[v6] prefix-list ... json' formatting by movi…
piotrsuchy Apr 15, 2024
3493e36
chore: inject greptile config into PR code
suphawitwanl Mar 4, 2026
d0075d3
test: remove greptile.json to test baseline AI behavior
suphawitwanl Mar 6, 2026
fe299a9
test: trigger baseline AI review with a dummy C code change
suphawitwanl Mar 6, 2026
c89fd58
test: re-add greptile.json to test custom rules
suphawitwanl Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions greptile.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"strictness": 2,
"commentTypes": ["logic", "syntax", "best_practices"],
"instructions": "You are a senior core maintainer for FreeRangeRouting (FRR). You review the entire PR holistically: C daemon code, Python tools, YANG data models, and Sphinx documentation. Enforce FRR's specific architectural guidelines, memory safety, and formatting standards across all file types. IMPORTANT INSTRUCTION: At the very end of your code review summary, you must exactly output the phrase: 'Hi I can read greptile.json thanks'",
"ignorePatterns": ["**/*.generated.*"],
"customContext": {
"rules": [
{
"rule": "Memory Allocation: NEVER allow standard malloc(), calloc(), realloc(), or free(). Enforce FRR's XCALLOC, XMALLOC, XREALLOC, XSTRDUP, and XFREE macros (requires MTYPE).",
"scope": ["**/*.c", "**/*.h"]
},
{
"rule": "Logging: Reject printf() or stdio logging. Enforce FRR's zlog_* API (e.g., zlog_debug, zlog_err).",
"scope": ["**/*.c"]
},
{
"rule": "String Manipulation: strcpy(), strcat(), and sprintf() are strictly forbidden. Enforce strlcpy(), strlcat(), and snprintf().",
"scope": ["**/*.c"]
},
{
"rule": "Linked Lists: Reject custom linked lists. Enforce FRR's DECLARE_LIST or DECLARE_DLIST macros.",
"scope": ["**/*.c", "**/*.h"]
},
{
"rule": "Documentation: Ensure .rst files use correct Sphinx formatting. CLI commands MUST be documented using the `.. clicmd::` directive.",
"scope": ["**/*.rst"]
},
{
"rule": "YANG Models: Ensure new JSON output keys are camelCased (not kebab-case). YANG modules must follow IETF standard conventions.",
"scope": ["**/*.yang", "**/*.json"]
},
{
"rule": "Python Formatting: Ensure Python scripts follow standard conventions and look like they were formatted with the 'black' formatter.",
"scope": ["**/*.py"]
}
],
"files": [
{
"path": "doc/developer/workflow.rst",
"description": "FRR core coding standards, PR requirements, and architectural guidelines for all languages."
}
]
}
}
30 changes: 15 additions & 15 deletions lib/plist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,17 +1073,13 @@ static int vty_show_prefix_list(struct vty *vty, afi_t afi, const char *name,
struct prefix_master *master;
int64_t seqnum = 0;
json_object *json = NULL;
json_object *json_proto = NULL;

master = prefix_master_get(afi, 0);
if (master == NULL)
return CMD_WARNING;

if (uj) {
if (uj)
json = json_object_new_object();
json_proto = json_object_new_object();
json_object_object_add(json, frr_protoname, json_proto);
}

if (seq)
seqnum = (int64_t)atol(seq);
Expand All @@ -1096,8 +1092,8 @@ static int vty_show_prefix_list(struct vty *vty, afi_t afi, const char *name,
"%% Can't find specified prefix-list\n");
return CMD_WARNING;
}
vty_show_prefix_entry(vty, json_proto, afi, plist, master,
dtype, seqnum);
vty_show_prefix_entry(vty, json, afi, plist, master, dtype,
seqnum);
} else {
if (dtype == detail_display || dtype == summary_display) {
if (master->recent && !uj)
Expand All @@ -1107,8 +1103,8 @@ static int vty_show_prefix_list(struct vty *vty, afi_t afi, const char *name,
}

frr_each (plist, &master->str, plist)
vty_show_prefix_entry(vty, json_proto, afi, plist,
master, dtype, seqnum);
vty_show_prefix_entry(vty, json, afi, plist, master,
dtype, seqnum);
}

return vty_json(vty, json);
Expand Down Expand Up @@ -1227,7 +1223,7 @@ static int vty_clear_prefix_list(struct vty *vty, afi_t afi, const char *name,

#include "lib/plist_clippy.c"

DEFPY (show_ip_prefix_list,
DEFPY_NOSH (show_ip_prefix_list,
show_ip_prefix_list_cmd,
"show ip prefix-list [PREFIXLIST4_NAME$name [seq$dseq (1-4294967295)$arg]] [json$uj]",
SHOW_STR
Expand All @@ -1239,6 +1235,7 @@ DEFPY (show_ip_prefix_list,
JSON_STR)
{
enum display_type dtype = normal_display;

if (dseq)
dtype = sequential_display;

Expand All @@ -1258,6 +1255,7 @@ DEFPY (show_ip_prefix_list_prefix,
"First matched prefix\n")
{
enum display_type dtype = normal_display;

if (dl)
dtype = longer_display;
else if (dfm)
Expand All @@ -1267,7 +1265,7 @@ DEFPY (show_ip_prefix_list_prefix,
dtype);
}

DEFPY (show_ip_prefix_list_summary,
DEFPY_NOSH (show_ip_prefix_list_summary,
show_ip_prefix_list_summary_cmd,
"show ip prefix-list summary [PREFIXLIST4_NAME$name] [json$uj]",
SHOW_STR
Expand All @@ -1281,7 +1279,7 @@ DEFPY (show_ip_prefix_list_summary,
summary_display, !!uj);
}

DEFPY (show_ip_prefix_list_detail,
DEFPY_NOSH (show_ip_prefix_list_detail,
show_ip_prefix_list_detail_cmd,
"show ip prefix-list detail [PREFIXLIST4_NAME$name] [json$uj]",
SHOW_STR
Expand All @@ -1307,7 +1305,7 @@ DEFPY (clear_ip_prefix_list,
return vty_clear_prefix_list(vty, AFI_IP, name, prefix_str);
}

DEFPY (show_ipv6_prefix_list,
DEFPY_NOSH(show_ipv6_prefix_list,
show_ipv6_prefix_list_cmd,
"show ipv6 prefix-list [PREFIXLIST6_NAME$name [seq$dseq (1-4294967295)$arg]] [json$uj]",
SHOW_STR
Expand All @@ -1319,6 +1317,7 @@ DEFPY (show_ipv6_prefix_list,
JSON_STR)
{
enum display_type dtype = normal_display;

if (dseq)
dtype = sequential_display;

Expand All @@ -1338,6 +1337,7 @@ DEFPY (show_ipv6_prefix_list_prefix,
"First matched prefix\n")
{
enum display_type dtype = normal_display;

if (dl)
dtype = longer_display;
else if (dfm)
Expand All @@ -1347,7 +1347,7 @@ DEFPY (show_ipv6_prefix_list_prefix,
prefix_str, dtype);
}

DEFPY (show_ipv6_prefix_list_summary,
DEFPY_NOSH (show_ipv6_prefix_list_summary,
show_ipv6_prefix_list_summary_cmd,
"show ipv6 prefix-list summary [PREFIXLIST6_NAME$name] [json$uj]",
SHOW_STR
Expand All @@ -1361,7 +1361,7 @@ DEFPY (show_ipv6_prefix_list_summary,
summary_display, !!uj);
}

DEFPY (show_ipv6_prefix_list_detail,
DEFPY_NOSH (show_ipv6_prefix_list_detail,
show_ipv6_prefix_list_detail_cmd,
"show ipv6 prefix-list detail [PREFIXLIST6_NAME$name] [json$uj]",
SHOW_STR
Expand Down
164 changes: 163 additions & 1 deletion vtysh/vtysh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3474,6 +3474,161 @@ DEFPY (show_route_map,
return CMD_SUCCESS;
}

static void show_prefix_list_send(afi_t afi, const char *prefix_list,
const char *seq, enum display_type dtype,
bool json)
{
unsigned int i;
bool first = true;
char command_line[128];

if (afi == AFI_IP)
snprintf(command_line, sizeof(command_line),
"do show ip prefix-list ");
else if (afi == AFI_IP6)
snprintf(command_line, sizeof(command_line),
"do show ipv6 prefix-list ");
Comment on lines +3485 to +3490
Copy link

@greptile-apps greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uninitialized buffer if afi is neither AFI_IP nor AFI_IP6

command_line is a local array that is only initialised via snprintf inside the if (afi == AFI_IP) or else if (afi == AFI_IP6) branches. If afi holds any other value, the buffer is left uninitialised and all subsequent strlcat calls operate on garbage memory, invoking undefined behaviour.

Although the current callers always pass AFI_IP or AFI_IP6, the static analysis risk and future-proofing argue for an else branch with an early return, or at minimum an assert/return guard:

	if (afi == AFI_IP)
		snprintf(command_line, sizeof(command_line),
			 "do show ip prefix-list ");
	else if (afi == AFI_IP6)
		snprintf(command_line, sizeof(command_line),
			 "do show ipv6 prefix-list ");
	else
		return;
Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3485-3490

Comment:
**Uninitialized buffer if afi is neither AFI_IP nor AFI_IP6**

`command_line` is a local array that is only initialised via `snprintf` inside the `if (afi == AFI_IP)` or `else if (afi == AFI_IP6)` branches. If `afi` holds any other value, the buffer is left uninitialised and all subsequent `strlcat` calls operate on garbage memory, invoking undefined behaviour.

Although the current callers always pass `AFI_IP` or `AFI_IP6`, the static analysis risk and future-proofing argue for an `else` branch with an early return, or at minimum an `assert`/`return` guard:

```c
	if (afi == AFI_IP)
		snprintf(command_line, sizeof(command_line),
			 "do show ip prefix-list ");
	else if (afi == AFI_IP6)
		snprintf(command_line, sizeof(command_line),
			 "do show ipv6 prefix-list ");
	else
		return;
```

How can I resolve this? If you propose a fix, please make it concise.

if (dtype == detail_display)
strlcat(command_line, "detail ", sizeof(command_line));
else if (dtype == summary_display)
strlcat(command_line, "summary ", sizeof(command_line));
if (prefix_list)
strlcat(command_line, prefix_list, sizeof(command_line));
if (dtype == sequential_display) {
strlcat(command_line, " seq ", sizeof(command_line));
strlcat(command_line, seq, sizeof(command_line));
}
if (json)
strlcat(command_line, " json", sizeof(command_line));

if (json)
vty_out(vty, "{");

for (i = 0; i < array_size(vtysh_client); i++) {
const struct vtysh_client *client = &vtysh_client[i];
bool is_connected = true;

if (!CHECK_FLAG(client->flag, VTYSH_PREFIX_LIST_SHOW))
continue;

for (; client; client = client->next)
if (client->fd < 0)
is_connected = false;

if (!is_connected)
continue;

if (json && !first)
vty_out(vty, ",");
else
first = false;
Comment on lines +3521 to +3524
Copy link

@greptile-apps greptile-apps bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first flag never set when json=false

The else branch executes when !(json && !first) — which includes both "first connected daemon in JSON mode" AND "any connected daemon in non-JSON mode". As a result, when json=false, first is reset to false on every iteration but is never used to suppress a comma (since the if condition is already gated on json). While harmless in non-JSON mode, the problem shows up in JSON mode for edge cases where connected daemons appear after non-connected ones:

Consider the sequence: daemon A (not connected → skipped), daemon B (connected, first=true):

  • json && !first → true && true → false → else: first = false
  • Daemon B outputs without a leading comma ✓

Then daemon C (connected, first=false):

  • json && !first → true && true → true → comma printed ✓

This path is actually correct. However, the non-JSON path means first is unconditionally set to false even on non-connected daemons that were skipped via continue. Since the continue skips both the flag check and the body, first is only mutated for connected daemons, keeping the logic intact.

The concern is that this non-obvious control flow is identical to the show_route_map_send template but could be confusing. A clearer formulation would be:

Suggested change
if (json && !first)
vty_out(vty, ",");
else
first = false;
if (json) {
if (!first)
vty_out(vty, ",");
else
first = false;
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3521-3524

Comment:
**`first` flag never set when `json=false`**

The `else` branch executes when `!(json && !first)` — which includes both "first connected daemon in JSON mode" AND "any connected daemon in non-JSON mode". As a result, when `json=false`, `first` is reset to `false` on every iteration but is never used to suppress a comma (since the `if` condition is already gated on `json`). While harmless in non-JSON mode, the problem shows up in JSON mode for edge cases where connected daemons appear after non-connected ones:

Consider the sequence: daemon A (not connected → skipped), daemon B (connected, `first=true`):
- `json && !first` → `true && true` → `false` → `else: first = false`
- Daemon B outputs without a leading comma ✓

Then daemon C (connected, `first=false`):
- `json && !first` → `true && true` → `true` → comma printed ✓

This path is actually correct. However, the non-JSON path means `first` is unconditionally set to `false` even on non-connected daemons that were skipped via `continue`. Since the `continue` skips both the flag check and the body, `first` is only mutated for **connected** daemons, keeping the logic intact.

The concern is that this non-obvious control flow is identical to the `show_route_map_send` template but could be confusing. A clearer formulation would be:

```suggestion
		if (json) {
			if (!first)
				vty_out(vty, ",");
			else
				first = false;
		}
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


if (json)
vty_out(vty, "\"%s\":", vtysh_client[i].name);

vtysh_client_execute_name(vtysh_client[i].name, command_line);
}

if (json)
vty_out(vty, "}\n");
}

DEFPY (show_ip_prefix_list,
show_ip_prefix_list_cmd,
"show ip prefix-list [PREFIXLIST4_NAME$name [seq$dseq (1-4294967295)$arg]] [json$uj]",
SHOW_STR
IP_STR
PREFIX_LIST_STR
"Name of a prefix list\n"
"sequence number of an entry\n"
"Sequence number\n"
JSON_STR)
{
enum display_type dtype = normal_display;

if (dseq)
dtype = sequential_display;

show_prefix_list_send(AFI_IP, name, arg_str, dtype, !!uj);
return CMD_SUCCESS;
}

DEFPY (show_ip_prefix_list_summary,
show_ip_prefix_list_summary_cmd,
"show ip prefix-list summary [PREFIXLIST4_NAME$name] [json$uj]",
SHOW_STR
IP_STR
PREFIX_LIST_STR
"Summary of prefix lists\n"
"Name of a prefix list\n"
JSON_STR)
{
show_prefix_list_send(AFI_IP, name, NULL, summary_display, !!uj);
return CMD_SUCCESS;
}

DEFPY (show_ip_prefix_list_detail,
show_ip_prefix_list_detail_cmd,
"show ip prefix-list detail [PREFIXLIST4_NAME$name] [json$uj]",
SHOW_STR
IP_STR
PREFIX_LIST_STR
"Detail of prefix lists\n"
"Name of a prefix list\n"
JSON_STR)
{
show_prefix_list_send(AFI_IP, name, NULL, detail_display, !!uj);
return CMD_SUCCESS;
}

DEFPY (show_ipv6_prefix_list,
show_ipv6_prefix_list_cmd,
"show ipv6 prefix-list [PREFIXLIST6_NAME$name [seq$dseq (1-4294967295)$arg]] [json$uj]",
SHOW_STR
IPV6_STR
PREFIX_LIST_STR
"Name of a prefix list\n"
"sequence number of an entry\n"
"Sequence number\n"
JSON_STR)
{
enum display_type dtype = normal_display;

if (dseq)
dtype = sequential_display;

show_prefix_list_send(AFI_IP6, name, arg_str, dtype, !!uj);
return CMD_SUCCESS;
}

DEFPY (show_ipv6_prefix_list_summary,
show_ipv6_prefix_list_summary_cmd,
"show ipv6 prefix-list summary [PREFIXLIST6_NAME$name] [json$uj]",
SHOW_STR
IPV6_STR
PREFIX_LIST_STR
"Summary of prefix lists\n"
"Name of a prefix list\n"
JSON_STR)
{
show_prefix_list_send(AFI_IP6, name, NULL, summary_display, !!uj);
return CMD_SUCCESS;
}

DEFPY (show_ipv6_prefix_list_detail,
show_ipv6_prefix_list_detail_cmd,
"show ipv6 prefix-list detail [PREFIXLIST6_NAME$name] [json$uj]",
SHOW_STR
IPV6_STR
PREFIX_LIST_STR
"Detail of prefix lists\n"
"Name of a prefix list\n"
JSON_STR)
{
show_prefix_list_send(AFI_IP6, name, NULL, detail_display, !!uj);
return CMD_SUCCESS;
}

DEFUN (vtysh_integrated_config,
vtysh_integrated_config_cmd,
"service integrated-vtysh-config",
Expand All @@ -3498,8 +3653,8 @@ DEFUN (no_vtysh_integrated_config,
static void backup_config_file(const char *fbackup)
{
char *integrate_sav = NULL;

size_t integrate_sav_sz = strlen(fbackup) + strlen(CONF_BACKUP_EXT) + 1;

integrate_sav = malloc(integrate_sav_sz);
strlcpy(integrate_sav, fbackup, integrate_sav_sz);
strlcat(integrate_sav, CONF_BACKUP_EXT, integrate_sav_sz);
Expand Down Expand Up @@ -5074,6 +5229,12 @@ void vtysh_init_vty(void)
install_element(ENABLE_NODE, &vtysh_copy_to_running_cmd);

install_element(ENABLE_NODE, &show_route_map_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_detail_cmd);
Comment on lines +5232 to +5237
Copy link

@greptile-apps greptile-apps bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All six show ip[v6] prefix-list commands are registered in each daemon's CLI in VIEW_NODE (see lib/plist.c:1662-1665, 1681-1684). Before this PR, vtysh would forward the command to the daemons when it wasn't registered locally, so it remained accessible from the unprivileged VIEW prompt.

After this PR the vtysh-level handlers are installed only in ENABLE_NODE (lines 5232-5237). Any user who has not entered enable mode in vtysh will now get % Unknown command when running these show commands — a clear behavioural regression.

The reference PR FRRouting#14856 (show route-map) did not introduce a regression because show route-map was already gated behind ENABLE_NODE in each daemon. That is not the case here.

To preserve existing behaviour:

install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_detail_cmd);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 5232-5237

Comment:
All six `show ip[v6] prefix-list` commands are registered in each daemon's CLI in `VIEW_NODE` (see `lib/plist.c:1662-1665, 1681-1684`). Before this PR, vtysh would forward the command to the daemons when it wasn't registered locally, so it remained accessible from the unprivileged VIEW prompt.

After this PR the vtysh-level handlers are installed **only** in `ENABLE_NODE` (lines 5232-5237). Any user who has not entered `enable` mode in vtysh will now get `% Unknown command` when running these show commands — a clear behavioural regression.

The reference PR #14856 (`show route-map`) did not introduce a regression because `show route-map` was already gated behind `ENABLE_NODE` in each daemon. That is not the case here.

To preserve existing behaviour:
```c
install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_cmd);
install_element(ENABLE_NODE, &show_ipv6_prefix_list_detail_cmd);
```

How can I resolve this? If you propose a fix, please make it concise.


/* "write terminal" command. */
install_element(ENABLE_NODE, &vtysh_write_terminal_cmd);
Expand Down Expand Up @@ -5148,3 +5309,4 @@ void vtysh_init_vty(void)
install_element(CONFIG_NODE, &vtysh_enable_password_cmd);
install_element(CONFIG_NODE, &no_vtysh_enable_password_cmd);
}

14 changes: 14 additions & 0 deletions vtysh/vtysh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ extern struct event_loop *master;
VTYSH_ZEBRA | VTYSH_RIPD | VTYSH_RIPNGD | VTYSH_OSPFD | VTYSH_OSPF6D | \
VTYSH_BGPD | VTYSH_ISISD | VTYSH_PIMD | VTYSH_EIGRPD | \
VTYSH_FABRICD
#define VTYSH_PREFIX_LIST_SHOW \
VTYSH_ZEBRA | VTYSH_RIPD | VTYSH_RIPNGD | VTYSH_OSPFD | VTYSH_OSPF6D | \
VTYSH_BGPD | VTYSH_ISISD | VTYSH_PIMD | VTYSH_EIGRPD | \
VTYSH_FABRICD
Comment on lines +71 to +74
Copy link

@greptile-apps greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VTYSH_PREFIX_LIST_SHOW duplicates VTYSH_RMAP_SHOW exactly

Both macros expand to the identical bitmask. Defining them separately creates a latent maintenance hazard: if a new daemon is added to one but not the other, the coverage silently diverges. Consider defining one in terms of the other, or at least adding a comment tying them together:

/* Same set of daemons as VTYSH_RMAP_SHOW — keep in sync */
#define VTYSH_PREFIX_LIST_SHOW VTYSH_RMAP_SHOW
Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 71-74

Comment:
**`VTYSH_PREFIX_LIST_SHOW` duplicates `VTYSH_RMAP_SHOW` exactly**

Both macros expand to the identical bitmask. Defining them separately creates a latent maintenance hazard: if a new daemon is added to one but not the other, the coverage silently diverges. Consider defining one in terms of the other, or at least adding a comment tying them together:

```c
/* Same set of daemons as VTYSH_RMAP_SHOW — keep in sync */
#define VTYSH_PREFIX_LIST_SHOW VTYSH_RMAP_SHOW
```

How can I resolve this? If you propose a fix, please make it concise.

#define VTYSH_INTERFACE_SUBSET \
VTYSH_OSPFD | VTYSH_OSPF6D | \
VTYSH_ISISD | VTYSH_PIMD | VTYSH_PIM6D | VTYSH_NHRPD | \
Expand All @@ -90,7 +94,17 @@ enum vtysh_write_integrated {
WRITE_INTEGRATED_YES
};

enum display_type {
normal_display,
summary_display,
detail_display,
sequential_display,
longer_display,
first_match_display
};
Comment on lines +97 to +104
Copy link

@greptile-apps greptile-apps bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate enum display_type definition

enum display_type is already defined as a file-private type in lib/plist.c at line 912 with identical enumerators. By adding a second definition here in vtysh/vtysh.h, two independent definitions now exist in separate translation units that must be kept manually in sync.

If a new display type is ever added to lib/plist.c but not vtysh/vtysh.h (or vice versa), the mismatch will be silent — the code will compile cleanly but show_prefix_list_send will use the wrong enum values.

The correct fix is to move the definition into lib/plist.h (the public header for plist.c), and include lib/plist.h from vtysh/vtysh.h or vtysh/vtysh.c, removing the duplicate:

/* In lib/plist.h */
enum display_type {
	normal_display,
	summary_display,
	detail_display,
	sequential_display,
	longer_display,
	first_match_display
};

Then remove the definition from both lib/plist.c and vtysh/vtysh.h, and add #include "lib/plist.h" where needed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 97-104

Comment:
**Duplicate `enum display_type` definition**

`enum display_type` is already defined as a file-private type in `lib/plist.c` at line 912 with identical enumerators. By adding a second definition here in `vtysh/vtysh.h`, two independent definitions now exist in separate translation units that must be kept manually in sync.

If a new display type is ever added to `lib/plist.c` but not `vtysh/vtysh.h` (or vice versa), the mismatch will be silent — the code will compile cleanly but `show_prefix_list_send` will use the wrong enum values.

The correct fix is to move the definition into `lib/plist.h` (the public header for `plist.c`), and include `lib/plist.h` from `vtysh/vtysh.h` or `vtysh/vtysh.c`, removing the duplicate:

```c
/* In lib/plist.h */
enum display_type {
	normal_display,
	summary_display,
	detail_display,
	sequential_display,
	longer_display,
	first_match_display
};
```

Then remove the definition from both `lib/plist.c` and `vtysh/vtysh.h`, and add `#include "lib/plist.h"` where needed.

How can I resolve this? If you propose a fix, please make it concise.


extern enum vtysh_write_integrated vtysh_write_integrated;
extern enum display_type display_type;
Copy link

@greptile-apps greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spurious extern variable declaration

extern enum display_type display_type; declares a global variable named display_type of type enum display_type, but no such variable is ever defined anywhere in the codebase. This is a copy-paste error modelled after the vtysh_write_integrated pattern — but display_type is meant to be a type name, not a variable. While this won't cause a compile error because the symbol is never referenced, it is incorrect and will cause a linker error the moment any translation unit actually uses it.

Suggested change
extern enum display_type display_type;
extern enum vtysh_write_integrated vtysh_write_integrated;
Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 107

Comment:
**Spurious extern variable declaration**

`extern enum display_type display_type;` declares a global *variable* named `display_type` of type `enum display_type`, but no such variable is ever defined anywhere in the codebase. This is a copy-paste error modelled after the `vtysh_write_integrated` pattern — but `display_type` is meant to be a *type name*, not a variable. While this won't cause a compile error because the symbol is never referenced, it is incorrect and will cause a linker error the moment any translation unit actually uses it.

```suggestion
extern enum vtysh_write_integrated vtysh_write_integrated;
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link

@greptile-apps greptile-apps bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extern enum display_type display_type; declares a global variable that is never defined anywhere in the codebase. This is a copy-paste error modeled after line 106 (extern enum vtysh_write_integrated vtysh_write_integrated;), but unlike that variable which has a real definition in vtysh.c, display_type is a type, not a variable. This declaration should be removed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 107

Comment:
`extern enum display_type display_type;` declares a global *variable* that is never defined anywhere in the codebase. This is a copy-paste error modeled after line 106 (`extern enum vtysh_write_integrated vtysh_write_integrated;`), but unlike that variable which has a real definition in vtysh.c, `display_type` is a type, not a variable. This declaration should be removed.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Author

@suphawitwanl suphawitwanl Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptileai


extern char frr_config[];
extern char vtydir[];
Expand Down
Loading