vtysh, lib: fix 'show ip[v6] prefix-list ... json' JSON display by moving it to vtysh to fix formatting issue "#2"#153
Open
suphawitwanl wants to merge 2 commits intoaitest-base-pr-15750from
Conversation
…ng it to vtysh
Json output is not valid for 'show ip[v6] prefix-list ... json' commands,
as it goes through all the running daemons and for each one it calls
'vty_show_prefix_list' creating a new json object. To aggreagate the output
and create a valid json that can be parsed, the commands were moved to vtysh.
Before:
{
"ZEBRA":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
{
"OSPF":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
{
"BGP":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
After:
{"zebra":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
,"ospfd":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
,"bgpd":{
"DEFAULT":{
"addressFamily":"IPv4",
"entries":[
{
"sequenceNumber":10,
"type":"permit",
"prefix":"0.0.0.0/0"
}
]
}
}
}
Signed-off-by: Piotr Suchy <[email protected]>
Greptile SummaryThis PR fixes broken JSON output for Key changes:
Issues found:
Hi I can read greptile.json thanks Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant vtysh
participant zebra
participant ospfd
participant bgpd
User->>vtysh: show ip prefix-list json
Note over vtysh: show_prefix_list_send()<br/>writes opening "{"
vtysh->>vtysh: vty_out(vty, "{")
vtysh->>vtysh: vty_out(vty, "\"zebra\":")
vtysh->>zebra: do show ip prefix-list json
zebra-->>vtysh: {"DEFAULT":{...}}
vtysh->>vtysh: vty_out(vty, ",")
vtysh->>vtysh: vty_out(vty, "\"ospfd\":")
vtysh->>ospfd: do show ip prefix-list json
ospfd-->>vtysh: {"DEFAULT":{...}}
vtysh->>vtysh: vty_out(vty, ",")
vtysh->>vtysh: vty_out(vty, "\"bgpd\":")
vtysh->>bgpd: do show ip prefix-list json
bgpd-->>vtysh: {"DEFAULT":{...}}
vtysh->>vtysh: vty_out(vty, "}\n")
vtysh-->>User: {"zebra":{...},"ospfd":{...},"bgpd":{...}}
Prompt To Fix All With AIThis is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 5232-5237
Comment:
**VIEW_NODE regression: show commands unavailable to unprivileged users**
The six new commands are only installed in `ENABLE_NODE`, but the original commands in `lib/plist.c` were registered in `VIEW_NODE` (see `prefix_list_init()` and `prefix_list_ipv6_init()` in `lib/plist.c`, lines 1662 and 1681). Since the `DEFPY_NOSH` change prevents vtysh from auto-forwarding the VIEW_NODE commands, any vtysh session that has not yet entered `enable` will no longer be able to run `show ip prefix-list` or `show ipv6 prefix-list`.
To preserve the original privilege level, each command should also be installed in `VIEW_NODE`:
```c
install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_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);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 107
Comment:
**Spurious extern variable declaration with no definition**
`extern enum display_type display_type;` declares a global *variable* named `display_type` of type `enum display_type`. There is no corresponding definition of this variable anywhere in the codebase. It appears this line was added by mistake — the `enum display_type` type is already declared just above on line 97, which is all that is needed.
If any translation unit ever tries to reference `display_type` as a variable (e.g., `display_type = summary_display;`), the linker will emit an undefined-symbol error. This line should be removed.
```suggestion
extern enum vtysh_write_integrated vtysh_write_integrated;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3483
Comment:
**Fixed-size command buffer may silently truncate long prefix-list names**
`command_line` is fixed at 128 bytes. The maximum possible command string is roughly `"do show ipv6 prefix-list detail " (33) + <name> + " seq 4294967295 json" (20)`, leaving only ~75 bytes for the prefix-list name. FRR does not enforce a hard upper bound on prefix-list names short enough to guarantee safety here.
`strlcat` will silently truncate the string rather than crashing, so an overly long name would result in a malformed command being forwarded to the daemon (e.g., `"do show ipv6 prefix-list detail LONGNA"`) with no error reported to the user.
Consider using a dynamic buffer or a larger constant (e.g., `BUFSIZ`) aligned with the actual maximum name length:
```suggestion
char command_line[BUFSIZ];
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: f25506c |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+5232
to
+5237
| 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); |
There was a problem hiding this comment.
VIEW_NODE regression: show commands unavailable to unprivileged users
The six new commands are only installed in ENABLE_NODE, but the original commands in lib/plist.c were registered in VIEW_NODE (see prefix_list_init() and prefix_list_ipv6_init() in lib/plist.c, lines 1662 and 1681). Since the DEFPY_NOSH change prevents vtysh from auto-forwarding the VIEW_NODE commands, any vtysh session that has not yet entered enable will no longer be able to run show ip prefix-list or show ipv6 prefix-list.
To preserve the original privilege level, each command should also be installed in VIEW_NODE:
install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_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);Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 5232-5237
Comment:
**VIEW_NODE regression: show commands unavailable to unprivileged users**
The six new commands are only installed in `ENABLE_NODE`, but the original commands in `lib/plist.c` were registered in `VIEW_NODE` (see `prefix_list_init()` and `prefix_list_ipv6_init()` in `lib/plist.c`, lines 1662 and 1681). Since the `DEFPY_NOSH` change prevents vtysh from auto-forwarding the VIEW_NODE commands, any vtysh session that has not yet entered `enable` will no longer be able to run `show ip prefix-list` or `show ipv6 prefix-list`.
To preserve the original privilege level, each command should also be installed in `VIEW_NODE`:
```c
install_element(VIEW_NODE, &show_ip_prefix_list_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ip_prefix_list_detail_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_summary_cmd);
install_element(VIEW_NODE, &show_ipv6_prefix_list_detail_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);
```
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; |
There was a problem hiding this comment.
Spurious extern variable declaration with no definition
extern enum display_type display_type; declares a global variable named display_type of type enum display_type. There is no corresponding definition of this variable anywhere in the codebase. It appears this line was added by mistake — the enum display_type type is already declared just above on line 97, which is all that is needed.
If any translation unit ever tries to reference display_type as a variable (e.g., display_type = summary_display;), the linker will emit an undefined-symbol error. This line should be removed.
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 with no definition**
`extern enum display_type display_type;` declares a global *variable* named `display_type` of type `enum display_type`. There is no corresponding definition of this variable anywhere in the codebase. It appears this line was added by mistake — the `enum display_type` type is already declared just above on line 97, which is all that is needed.
If any translation unit ever tries to reference `display_type` as a variable (e.g., `display_type = summary_display;`), the linker will emit an undefined-symbol error. This line should be removed.
```suggestion
extern enum vtysh_write_integrated vtysh_write_integrated;
```
How can I resolve this? If you propose a fix, please make it concise.| { | ||
| unsigned int i; | ||
| bool first = true; | ||
| char command_line[128]; |
There was a problem hiding this comment.
Fixed-size command buffer may silently truncate long prefix-list names
command_line is fixed at 128 bytes. The maximum possible command string is roughly "do show ipv6 prefix-list detail " (33) + <name> + " seq 4294967295 json" (20), leaving only ~75 bytes for the prefix-list name. FRR does not enforce a hard upper bound on prefix-list names short enough to guarantee safety here.
strlcat will silently truncate the string rather than crashing, so an overly long name would result in a malformed command being forwarded to the daemon (e.g., "do show ipv6 prefix-list detail LONGNA") with no error reported to the user.
Consider using a dynamic buffer or a larger constant (e.g., BUFSIZ) aligned with the actual maximum name length:
Suggested change
| char command_line[128]; | |
| char command_line[BUFSIZ]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3483
Comment:
**Fixed-size command buffer may silently truncate long prefix-list names**
`command_line` is fixed at 128 bytes. The maximum possible command string is roughly `"do show ipv6 prefix-list detail " (33) + <name> + " seq 4294967295 json" (20)`, leaving only ~75 bytes for the prefix-list name. FRR does not enforce a hard upper bound on prefix-list names short enough to guarantee safety here.
`strlcat` will silently truncate the string rather than crashing, so an overly long name would result in a malformed command being forwarded to the daemon (e.g., `"do show ipv6 prefix-list detail LONGNA"`) with no error reported to the user.
Consider using a dynamic buffer or a larger constant (e.g., `BUFSIZ`) aligned with the actual maximum name length:
```suggestion
char command_line[BUFSIZ];
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Json output is not valid for
show ip prefix-list ... jsoncommands, because it goes through each running daemon and for each one it callsvty_show_prefix_listcreating a new json object. To aggregate the output and create a valid json that can be parsed, the commands were moved to vtysh, similarly to this PR: FRRouting#14856This solves: FRRouting#15277
Before:
{ "ZEBRA":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } } { "OSPF":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } } { "BGP":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } }JSON is not valid.
After:
{"zebra":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } ,"ospfd":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } ,"bgpd":{ "DEFAULT":{ "addressFamily":"IPv4", "entries":[ { "sequenceNumber":10, "type":"permit", "prefix":"0.0.0.0/0" } ] } } }JSON is valid.
Analogous behavior for:
show ip prefix-list summary jsonshow ip prefix-list detail jsonshow ipv6 prefix-list jsonshow ipv6 prefix-list summary jsonshow ipv6 prefix-list detail json