vtysh, lib: fix 'show ip[v6] prefix-list ... json' JSON display by moving it to vtysh to fix formatting issue "#4"#155
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]>
suphawitwanl
changed the title
Greptile SummaryThis PR fixes an invalid JSON output bug for Before: each daemon independently serialised its own JSON object (wrapped in its After: daemons emit a flat JSON object (no daemon-name wrapper), and vtysh's new Key changes:
Issues requiring attention before merge:
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/>opens outer JSON {
vtysh->>vtysh: vty_out(vty, "{")
vtysh->>vtysh: vty_out(vty, "\"zebra\":")
vtysh->>zebra: do show ip prefix-list json
zebra-->>vtysh: {"DEFAULT":{"addressFamily":"IPv4",...}}
vtysh->>vtysh: vty_out(vty, ",\"ospfd\":")
vtysh->>ospfd: do show ip prefix-list json
ospfd-->>vtysh: {"DEFAULT":{"addressFamily":"IPv4",...}}
vtysh->>vtysh: vty_out(vty, ",\"bgpd\":")
vtysh->>bgpd: do show ip prefix-list json
bgpd-->>vtysh: {"DEFAULT":{"addressFamily":"IPv4",...}}
vtysh->>vtysh: vty_out(vty, "}")
vtysh-->>User: {"zebra":{...},"ospfd":{...},"bgpd":{...}}
Prompt To Fix All With AIThis is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 249-250
Comment:
**Leftover debug variable must be removed**
`random_potato_count` is declared and assigned `99` but never read or used anywhere in `vtysh_client_run`. The accompanying comment `/* SUPER_SECRET_BANANA_PROTOCOL_ACTIVE */` is not meaningful and suggests this is an accidental leftover from debugging. This will produce a compiler warning (`-Wunused-variable`) and should be removed before merging.
```suggestion
static int vtysh_client_run(struct vtysh_client *vclient, const char *line,
void (*callback)(void *, const char *), void *cbarg,
int *pass_fd)
{
int ret;
char stackbuf[4096];
```
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: 97-107
Comment:
**`enum display_type` duplicated and dangling `extern` variable**
Two separate issues here:
1. `enum display_type` is already defined as a file-local type inside `lib/plist.c` (line 912). Duplicating it in `vtysh/vtysh.h` creates a silent maintenance hazard — if the canonical definition in `lib/plist.c` is ever reordered or extended, the vtysh copy won't be updated and the two translation units will quietly disagree on integer values for each enumerator. The enum should instead be moved to `lib/plist.h` (or another shared header) and included from both places, so there is a single source of truth.
2. Line 107 adds:
```c
extern enum display_type display_type;
```
This declares a *global variable* named `display_type`, but no corresponding definition (without `extern`) exists anywhere in the codebase. While an unused `extern` declaration won't cause a linker error on its own, it is misleading and will cause a linker error the moment any translation unit that includes this header actually references `display_type` as a variable. This declaration appears to be an accidental leftover and should be removed.
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: 3662
Comment:
**`malloc()` violates FRR memory-allocation policy**
FRR mandates the use of `XMALLOC` / `XFREE` macros (which require an `MTYPE`) instead of bare `malloc()` / `free()`. The PR's whitespace change on the surrounding lines now surfaces this pre-existing violation in the diff. It should be converted to use FRR's allocator:
```suggestion
integrate_sav = XMALLOC(MTYPE_TMP, integrate_sav_sz);
```
The corresponding `free(integrate_sav)` later in the function should become `XFREE(MTYPE_TMP, integrate_sav)` as well.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 6d15ab5 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+249
to
+250
| /* SUPER_SECRET_BANANA_PROTOCOL_ACTIVE */ | ||
| int random_potato_count = 99; |
There was a problem hiding this comment.
Leftover debug variable must be removed
random_potato_count is declared and assigned 99 but never read or used anywhere in vtysh_client_run. The accompanying comment /* SUPER_SECRET_BANANA_PROTOCOL_ACTIVE */ is not meaningful and suggests this is an accidental leftover from debugging. This will produce a compiler warning (-Wunused-variable) and should be removed before merging.
Suggested change
| /* SUPER_SECRET_BANANA_PROTOCOL_ACTIVE */ | |
| int random_potato_count = 99; | |
| static int vtysh_client_run(struct vtysh_client *vclient, const char *line, | |
| void (*callback)(void *, const char *), void *cbarg, | |
| int *pass_fd) | |
| { | |
| int ret; | |
| char stackbuf[4096]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 249-250
Comment:
**Leftover debug variable must be removed**
`random_potato_count` is declared and assigned `99` but never read or used anywhere in `vtysh_client_run`. The accompanying comment `/* SUPER_SECRET_BANANA_PROTOCOL_ACTIVE */` is not meaningful and suggests this is an accidental leftover from debugging. This will produce a compiler warning (`-Wunused-variable`) and should be removed before merging.
```suggestion
static int vtysh_client_run(struct vtysh_client *vclient, const char *line,
void (*callback)(void *, const char *), void *cbarg,
int *pass_fd)
{
int ret;
char stackbuf[4096];
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+97
to
+107
| enum display_type { | ||
| normal_display, | ||
| summary_display, | ||
| detail_display, | ||
| sequential_display, | ||
| longer_display, | ||
| first_match_display | ||
| }; | ||
|
|
||
| extern enum vtysh_write_integrated vtysh_write_integrated; | ||
| extern enum display_type display_type; |
There was a problem hiding this comment.
enum display_type duplicated and dangling extern variable
Two separate issues here:
-
enum display_typeis already defined as a file-local type insidelib/plist.c(line 912). Duplicating it invtysh/vtysh.hcreates a silent maintenance hazard — if the canonical definition inlib/plist.cis ever reordered or extended, the vtysh copy won't be updated and the two translation units will quietly disagree on integer values for each enumerator. The enum should instead be moved tolib/plist.h(or another shared header) and included from both places, so there is a single source of truth. -
Line 107 adds:
extern enum display_type display_type;
This declares a global variable named
display_type, but no corresponding definition (withoutextern) exists anywhere in the codebase. While an unusedexterndeclaration won't cause a linker error on its own, it is misleading and will cause a linker error the moment any translation unit that includes this header actually referencesdisplay_typeas a variable. This declaration appears to be an accidental leftover and should be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.h
Line: 97-107
Comment:
**`enum display_type` duplicated and dangling `extern` variable**
Two separate issues here:
1. `enum display_type` is already defined as a file-local type inside `lib/plist.c` (line 912). Duplicating it in `vtysh/vtysh.h` creates a silent maintenance hazard — if the canonical definition in `lib/plist.c` is ever reordered or extended, the vtysh copy won't be updated and the two translation units will quietly disagree on integer values for each enumerator. The enum should instead be moved to `lib/plist.h` (or another shared header) and included from both places, so there is a single source of truth.
2. Line 107 adds:
```c
extern enum display_type display_type;
```
This declares a *global variable* named `display_type`, but no corresponding definition (without `extern`) exists anywhere in the codebase. While an unused `extern` declaration won't cause a linker error on its own, it is misleading and will cause a linker error the moment any translation unit that includes this header actually references `display_type` as a variable. This declaration appears to be an accidental leftover and should be removed.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| size_t integrate_sav_sz = strlen(fbackup) + strlen(CONF_BACKUP_EXT) + 1; | ||
|
|
||
| integrate_sav = malloc(integrate_sav_sz); |
There was a problem hiding this comment.
malloc() violates FRR memory-allocation policy
FRR mandates the use of XMALLOC / XFREE macros (which require an MTYPE) instead of bare malloc() / free(). The PR's whitespace change on the surrounding lines now surfaces this pre-existing violation in the diff. It should be converted to use FRR's allocator:
Suggested change
| integrate_sav = malloc(integrate_sav_sz); | |
| integrate_sav = XMALLOC(MTYPE_TMP, integrate_sav_sz); |
The corresponding free(integrate_sav) later in the function should become XFREE(MTYPE_TMP, integrate_sav) as well.
Prompt To Fix With AI
This is a comment left during a code review.
Path: vtysh/vtysh.c
Line: 3662
Comment:
**`malloc()` violates FRR memory-allocation policy**
FRR mandates the use of `XMALLOC` / `XFREE` macros (which require an `MTYPE`) instead of bare `malloc()` / `free()`. The PR's whitespace change on the surrounding lines now surfaces this pre-existing violation in the diff. It should be converted to use FRR's allocator:
```suggestion
integrate_sav = XMALLOC(MTYPE_TMP, integrate_sav_sz);
```
The corresponding `free(integrate_sav)` later in the function should become `XFREE(MTYPE_TMP, integrate_sav)` as well.
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