vtysh, lib: fix 'show ip[v6] prefix-list ... json' JSON display by moving it to vtysh to fix formatting issue "#3"#154
Open
suphawitwanl wants to merge 3 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 moves the
Hi I can read greptile.json thanks #3 Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant vtysh
participant zebra
participant ospfd
participant bgpd
Note over User,bgpd: show ip prefix-list json (ENABLE_NODE, after this PR)
User->>vtysh: show ip prefix-list json
vtysh->>vtysh: show_prefix_list_send(AFI_IP, ...)
vtysh->>vtysh: vty_out("{")
vtysh->>zebra: do show ip prefix-list json
zebra-->>vtysh: {"DEFAULT":{...}}
vtysh->>vtysh: vty_out(",\"ospfd\":")
vtysh->>ospfd: do show ip prefix-list json
ospfd-->>vtysh: {"DEFAULT":{...}}
vtysh->>vtysh: vty_out(",\"bgpd\":")
vtysh->>bgpd: do show ip prefix-list json
bgpd-->>vtysh: {"DEFAULT":{...}}
vtysh->>vtysh: vty_out("}")
vtysh-->>User: {"zebra":{...},"ospfd":{...},"bgpd":{...}}
Note over User,bgpd: show ip prefix-list json (VIEW_NODE, after this PR — still broken)
User->>vtysh: show ip prefix-list json
vtysh->>zebra: forward (DEFPY_NOSH passthrough)
zebra-->>vtysh: {"zebra":{"DEFAULT":{...}}}
vtysh->>ospfd: forward (DEFPY_NOSH passthrough)
ospfd-->>vtysh: {"ospf":{"DEFAULT":{...}}}
vtysh-->>User: two separate JSON objects (invalid)
Prompt To Fix All With AIThis is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 5229-5234
Comment:
**Show commands missing VIEW_NODE registration**
The new aggregating show commands are only installed in `ENABLE_NODE`, but the original daemon-side commands (now `DEFPY_NOSH` in `lib/plist.c`) are registered in `VIEW_NODE` via `install_element(VIEW_NODE, &show_ip_prefix_list_cmd)`. Because `DEFPY_NOSH` prevents vtysh from automatically creating a local passthrough, users in `VIEW_NODE` will still receive the old broken per-daemon JSON output instead of the new aggregated result.
The six new commands need to also be installed in `VIEW_NODE` to maintain the original node coverage and ensure the fix applies to all users:
```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);
// ... etc.
```
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 (`enum display_type display_type;`) anywhere in the codebase — this is a dangling `extern`. If any translation unit that includes `vtysh.h` attempts to read or write `display_type` as a variable (which is easy to do accidentally given the name collision with the type itself), the linker will emit an undefined reference error.
The type declaration `enum display_type { ... };` on line 97 is sufficient — this extern variable 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-3490
Comment:
**`command_line` is uninitialized when `afi` is neither `AFI_IP` nor `AFI_IP6`**
The `if/else if` block initializes `command_line` only for `AFI_IP` and `AFI_IP6`. If any other AFI value is passed, `command_line` remains uninitialized. The subsequent `strlcat` calls scan for a null terminator in an uninitialized buffer — undefined behavior that can produce a corrupted command string sent to every connected daemon.
An `else` fallback or an upfront initialization with `snprintf`/`memset` would prevent this. For example:
```c
char command_line[128] = {0};
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; /* unsupported AFI */
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 094399d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
5229
to
+5234
| 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); |
There was a problem hiding this comment.
Show commands missing VIEW_NODE registration
The new aggregating show commands are only installed in ENABLE_NODE, but the original daemon-side commands (now DEFPY_NOSH in lib/plist.c) are registered in VIEW_NODE via install_element(VIEW_NODE, &show_ip_prefix_list_cmd). Because DEFPY_NOSH prevents vtysh from automatically creating a local passthrough, users in VIEW_NODE will still receive the old broken per-daemon JSON output instead of the new aggregated result.
The six new commands need to also be installed in VIEW_NODE to maintain the original node coverage and ensure the fix applies to all users:
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);
// ... etc.Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 5229-5234
Comment:
**Show commands missing VIEW_NODE registration**
The new aggregating show commands are only installed in `ENABLE_NODE`, but the original daemon-side commands (now `DEFPY_NOSH` in `lib/plist.c`) are registered in `VIEW_NODE` via `install_element(VIEW_NODE, &show_ip_prefix_list_cmd)`. Because `DEFPY_NOSH` prevents vtysh from automatically creating a local passthrough, users in `VIEW_NODE` will still receive the old broken per-daemon JSON output instead of the new aggregated result.
The six new commands need to also be installed in `VIEW_NODE` to maintain the original node coverage and ensure the fix applies to all users:
```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);
// ... etc.
```
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 (enum display_type display_type;) anywhere in the codebase — this is a dangling extern. If any translation unit that includes vtysh.h attempts to read or write display_type as a variable (which is easy to do accidentally given the name collision with the type itself), the linker will emit an undefined reference error.
The type declaration enum display_type { ... }; on line 97 is sufficient — this extern variable 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 (`enum display_type display_type;`) anywhere in the codebase — this is a dangling `extern`. If any translation unit that includes `vtysh.h` attempts to read or write `display_type` as a variable (which is easy to do accidentally given the name collision with the type itself), the linker will emit an undefined reference error.
The type declaration `enum display_type { ... };` on line 97 is sufficient — this extern variable 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.
Comment on lines
+3483
to
+3490
| 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 "); |
There was a problem hiding this comment.
command_line is uninitialized when afi is neither AFI_IP nor AFI_IP6
The if/else if block initializes command_line only for AFI_IP and AFI_IP6. If any other AFI value is passed, command_line remains uninitialized. The subsequent strlcat calls scan for a null terminator in an uninitialized buffer — undefined behavior that can produce a corrupted command string sent to every connected daemon.
An else fallback or an upfront initialization with snprintf/memset would prevent this. For example:
char command_line[128] = {0};
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; /* unsupported AFI */Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3483-3490
Comment:
**`command_line` is uninitialized when `afi` is neither `AFI_IP` nor `AFI_IP6`**
The `if/else if` block initializes `command_line` only for `AFI_IP` and `AFI_IP6`. If any other AFI value is passed, `command_line` remains uninitialized. The subsequent `strlcat` calls scan for a null terminator in an uninitialized buffer — undefined behavior that can produce a corrupted command string sent to every connected daemon.
An `else` fallback or an upfront initialization with `snprintf`/`memset` would prevent this. For example:
```c
char command_line[128] = {0};
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; /* unsupported AFI */
```
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