bgpd: Support for new "show bgp <vrf> bestpath [json]" show command #20616

soumyar-roy · 2026-01-28T23:16:38Z

bgpd: Support for new "show bgp bestpath [json]" show command.

VTY output -

leaf11# show bgp vrf default bestpath
VRF default
Best Path Selection Criteria:
  AS path ignore is Disabled
  AS path confed is Disabled
  AS path multi-path-relax is Enabled
  Peer type relax is Disabled
  Compare router ID is Disabled
  BGP bestpath MED is disabled
  Link Bandwidth handling set to: ecmp(default)
  Always compare MED is Disabled
  Deterministic MED is Enabled

leaf11# show bgp bestpath
VRF default
Best Path Selection Criteria:
  AS path ignore is Disabled
  AS path confed is Disabled
  AS path multi-path-relax is Enabled
  Peer type relax is Disabled
  Compare router ID is Disabled
  BGP bestpath MED is disabled
  Link Bandwidth handling set to: ecmp(default)
  Always compare MED is Disabled
  Deterministic MED is Enabled
leaf11#

JSON output -

leaf11# show bgp bestpath json
{
  "vrfs":{
    "default":{
      "bestPath":{
        "asPathIgnore":false,
        "asPathConfed":false,
        "asPathMultiPathRelaxEnabled":true,
        "peerTypeRelax":false,
        "compareRouterId":false,
        "medConfed":false,
        "medMissingASWorst":false,
        "linkBandwidth":"ecmp(default)",
        "alwaysCompareMed":false,
        "deterministicMed":true
      }
    }
  }
}

leaf11# show bgp vrf default bestpath json
{
  "default":{
    "bestPath":{
      "asPathIgnore":false,
      "asPathConfed":false,
      "asPathMultiPathRelaxEnabled":true,
      "peerTypeRelax":false,
      "compareRouterId":false,
      "medConfed":false,
      "medMissingASWorst":false,
      "linkBandwidth":"ecmp(default)",
      "alwaysCompareMed":false,
      "deterministicMed":true
    }
  }
}

Ticket: #4445780
Signed-off-by: Aprathi K [email protected]

greptile-apps · 2026-01-28T23:20:01Z

Greptile Overview

Greptile Summary

This PR implements a new BGP show command show bgp [vrf VRFNAME] bestpath [json] that displays best path selection criteria. The implementation adds a helper function bgp_show_bestpath() that outputs BGP best path configuration flags in both text and JSON formats, supporting single VRF, all VRFs, and the default VRF scenarios.

Key Changes:

New command handler show_bgp_vrf_bestpath_cmd with VRF and JSON support
Helper function bgp_show_bestpath() displays AS path, MED, link bandwidth, and other best path settings
Test coverage added to validate JSON output format
Handles multiple output modes: text with VRF name, JSON with vrfs wrapper for all VRFs case, and direct VRF name key for single VRF

Issues Found:

Logic error in MED output handling (lines 12350-12360) where the else clause is incorrectly scoped, causing wrong output when only BGP_FLAG_MED_CONFED is set

Confidence Score: 3/5

This PR has a clear logic bug that will cause incorrect output in specific MED configurations
The implementation is mostly sound with good test coverage, but the MED handling logic error in the text output path needs to be fixed before merging. The bug will cause incorrect display when only BGP_FLAG_MED_CONFED is set, printing both the "confed" message and "MED is Enabled"
bgpd/bgp_vty.c requires attention for the MED logic fix at lines 12350-12360

Important Files Changed

Filename	Overview
bgpd/bgp_vty.c	adds new `show bgp [vrf VRFNAME] bestpath [json]` command with helper function, but contains MED output logic error in text mode

Sequence Diagram

sequenceDiagram
    participant User
    participant VTY as VTY Shell
    participant CMD as show_bgp_vrf_bestpath_cmd
    participant Helper as bgp_show_bestpath()
    participant BGP as BGP Instance

    User->>VTY: show bgp [vrf VRFNAME] bestpath [json]
    VTY->>CMD: Execute command
    
    alt No VRF specified or VRF=all
        CMD->>BGP: Get all BGP instances from bm->bgp
        loop For each BGP instance
            CMD->>BGP: Check inst_type (skip VIEW types)
            CMD->>Helper: bgp_show_bestpath(vty, bgp, json_vrf)
            Helper->>BGP: Read flags (ASPATH_IGNORE, MED_CONFED, etc.)
            Helper->>BGP: Read lb_handling config
            Helper-->>CMD: Format output (text or JSON)
        end
        CMD->>VTY: Output all VRFs (with "vrfs" wrapper in JSON)
    else Single VRF specified
        CMD->>BGP: bgp_lookup_by_name(name) or bgp_get_default()
        alt BGP instance not found
            CMD->>VTY: Error: "Specified BGP instance not found"
        else BGP instance found
            CMD->>Helper: bgp_show_bestpath(vty, bgp, json_vrf)
            Helper->>BGP: Read flags and config
            Helper-->>CMD: Format output
            CMD->>VTY: Output single VRF (direct VRF key in JSON)
        end
    end
    
    VTY-->>User: Display bestpath criteria

greptile-apps

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-28T23:20:04Z

bgpd/bgp_vty.c

+		if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED) ||
+		    CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST)) {
+			if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED))
+				vty_out(vty, "  BGP bestpath MED set to: confed\n");
+			if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST))
+				vty_out(vty, "  BGP bestpath MED set to: missing-as-worst\n");
+			else
+				vty_out(vty, "  BGP bestpath MED is Enabled\n");
+		} else {
+			vty_out(vty, "  BGP bestpath MED is Disabled\n");
+		}


the else clause on line 12356 is misplaced - it only applies to the inner if statement checking BGP_FLAG_MED_MISSING_AS_WORST, not both MED conditions. When only BGP_FLAG_MED_CONFED is set (without BGP_FLAG_MED_MISSING_AS_WORST), the output will incorrectly print both "confed" and "MED is Enabled"

Suggested change

if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED) ||

CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST)) {

if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED))

vty_out(vty, " BGP bestpath MED set to: confed\n");

if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST))

vty_out(vty, " BGP bestpath MED set to: missing-as-worst\n");

else

vty_out(vty, " BGP bestpath MED is Enabled\n");

} else {

vty_out(vty, " BGP bestpath MED is Disabled\n");

}

if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED) ||

CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST)) {

if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED))

vty_out(vty, " BGP bestpath MED set to: confed\n");

if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST))

vty_out(vty, " BGP bestpath MED set to: missing-as-worst\n");

} else {

vty_out(vty, " BGP bestpath MED is Disabled\n");

}

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_vty.c Line: 12350:12360 Comment: the `else` clause on line 12356 is misplaced - it only applies to the inner `if` statement checking `BGP_FLAG_MED_MISSING_AS_WORST`, not both MED conditions. When only `BGP_FLAG_MED_CONFED` is set (without `BGP_FLAG_MED_MISSING_AS_WORST`), the output will incorrectly print both "confed" and "MED is Enabled" ```suggestion if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED) || CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST)) { if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED)) vty_out(vty, " BGP bestpath MED set to: confed\n"); if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST)) vty_out(vty, " BGP bestpath MED set to: missing-as-worst\n"); } else { vty_out(vty, " BGP bestpath MED is Disabled\n"); } ``` How can I resolve this? If you propose a fix, please make it concise.

mjstapp · 2026-01-29T13:06:14Z

bgpd/bgp_vty.c

+					CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST));
+
+		if (bgp->lb_handling == BGP_LINK_BW_IGNORE_BW)
+			json_object_string_add(bestpath, "linkBandwidth", "ignore");


couple of things here:
the key sounds like a bandwidth number, but it's really a type of handling, isn't it? Maybe the key should be "linkBwHandling" or something like that?

repeating the string_add calls feels a bit cut-and-paste: try using the logic to set a const char *, and then just have one call to string_add that uses the key?

mjstapp · 2026-01-29T13:11:52Z

bgpd/bgp_vty.c

+
+		if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED) ||
+		    CHECK_FLAG(bgp->flags, BGP_FLAG_MED_MISSING_AS_WORST)) {
+			if (CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED))


maybe the cascade of tests would be clearer if it were:

if (both flags) ; else if (flag_1) ; else if (flag_2) ; else ; /* disabled case */

mjstapp · 2026-01-29T13:14:39Z

bgpd/bgp_vty.c

+					CHECK_FLAG(bgp->flags, BGP_FLAG_COMPARE_ROUTER_ID));
+
+		json_object_boolean_add(bestpath, "medConfed",
+					CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED));


the MED output is different between vty and json - any reason why? that is, the json doesn't have the equivalent of "enabled", it just has the individual flags.

ton31337

Documentation is missing;
Can we use more descriptive "naming" for non-JSON output?

ton31337 · 2026-01-29T13:48:26Z

bgpd/bgp_vty.c

+		json_object_boolean_add(bestpath, "asPathConfed",
+					CHECK_FLAG(bgp->flags, BGP_FLAG_ASPATH_CONFED));
+
+		if (CHECK_FLAG(bgp->flags, BGP_FLAG_ASPATH_MULTIPATH_RELAX)) {


This check seems unnecessary..

ton31337 · 2026-01-29T13:49:01Z

bgpd/bgp_vty.c

+		json_object_boolean_add(bestpath, "medConfed",
+					CHECK_FLAG(bgp->flags, BGP_FLAG_MED_CONFED));
+
+		json_object_boolean_add(bestpath, "medMissingASWorst",


medMissingAsWorst

ton31337 · 2026-01-29T13:50:15Z

bgpd/bgp_vty.c

+			json_object_string_add(bestpath, "linkBandwidth",
+					       "default-weight-for-missing");
+		else
+			json_object_string_add(bestpath, "linkBandwidth", "ecmp(default)");


can we drop this (default)?

ton31337 · 2026-01-29T13:53:57Z

bgpd/bgp_vty.c

+		vty_out(vty, "Best Path Selection Criteria:\n");
+		vty_out(vty, "  AS path ignore is %s\n",
+			CHECK_FLAG(bgp->flags, BGP_FLAG_ASPATH_IGNORE) ? "Enabled" : "Disabled");
+		vty_out(vty, "  AS path confed is %s\n",


Can we name it "normally"? Like Include confederation AS numbers or so?

ton31337 · 2026-01-29T13:54:43Z

bgpd/bgp_vty.c

+DEFPY(show_bgp_vrf_bestpath, show_bgp_vrf_bestpath_cmd,
+      "show bgp [vrf VRFNAME$vrf_name] bestpath [json]",
+      SHOW_STR BGP_STR "Show BGP VRFs\n"
+		       "Specific VRF name\n"


Align, please.

Add test to verify that 'show bgp bestpath json' command returns the correct bestpath selection criteria in JSON format. Signed-off-by: Soumya Roy <[email protected]>

VTY output - leaf11# show bgp vrf default bestpath VRF default Best Path Selection Criteria: AS path ignore is Disabled AS path confed is Disabled AS path multi-path-relax is Enabled Peer type relax is Disabled Compare router ID is Disabled BGP bestpath MED is disabled Link Bandwidth handling set to: ecmp(default) Always compare MED is Disabled Deterministic MED is Enabled leaf11# show bgp bestpath VRF default Best Path Selection Criteria: AS path ignore is Disabled AS path confed is Disabled AS path multi-path-relax is Enabled Peer type relax is Disabled Compare router ID is Disabled BGP bestpath MED is disabled Link Bandwidth handling set to: ecmp(default) Always compare MED is Disabled Deterministic MED is Enabled leaf11# JSON output - leaf11# show bgp bestpath json { "vrfs":{ "default":{ "bestPath":{ "asPathIgnore":false, "asPathConfed":false, "asPathMultiPathRelaxEnabled":true, "peerTypeRelax":false, "compareRouterId":false, "medConfed":false, "medMissingASWorst":false, "linkBandwidth":"ecmp(default)", "alwaysCompareMed":false, "deterministicMed":true } } } } leaf11# show bgp vrf default bestpath json { "default":{ "bestPath":{ "asPathIgnore":false, "asPathConfed":false, "asPathMultiPathRelaxEnabled":true, "peerTypeRelax":false, "compareRouterId":false, "medConfed":false, "medMissingASWorst":false, "linkBandwidth":"ecmp(default)", "alwaysCompareMed":false, "deterministicMed":true } } } Ticket: #4445780 Signed-off-by: Aprathi K [email protected]

ton31337 · 2026-01-31T17:12:45Z

bgpd/bgp_vty.c

+		if (bgp->inst_type == BGP_INSTANCE_TYPE_VIEW)
+			continue;
+
+		name = (bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT) ? VRF_DEFAULT_NAME : bgp->name;


bgp->name is always non-NULL meaning that it has "default", right? Why we need this check?

frrbot bot added bgp tests Topotests, make check, etc labels Jan 28, 2026

github-actions bot added size/L master rebase PR needs rebase labels Jan 28, 2026

greptile-apps bot reviewed Jan 28, 2026

View reviewed changes

mjstapp reviewed Jan 29, 2026

View reviewed changes

ton31337 requested changes Jan 29, 2026

View reviewed changes

soumyar-roy and others added 2 commits January 30, 2026 11:04

tests: add test for show bgp bestpath json command

dc9cb5a

Add test to verify that 'show bgp bestpath json' command returns the correct bestpath selection criteria in JSON format. Signed-off-by: Soumya Roy <[email protected]>

soumyar-roy force-pushed the soumya/json2 branch from d3ede32 to 9e3bb1a Compare January 30, 2026 19:12

ton31337 reviewed Jan 31, 2026

View reviewed changes

bgpd: Support for new "show bgp <vrf> bestpath [json]" show command #20616

Are you sure you want to change the base?

bgpd: Support for new "show bgp <vrf> bestpath [json]" show command #20616

Uh oh!

Conversation

soumyar-roy commented Jan 28, 2026

Uh oh!

greptile-apps bot commented Jan 28, 2026

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ton31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants