bgpd: initialise nh_flag attribute#21498
bgpd: initialise nh_flag attribute#21498fdumontet6WIND wants to merge 2 commits intoFRRouting:masterfrom
Conversation
Greptile SummaryThis PR initializes the Confidence Score: 4/5Not safe to merge until the A single one-character typo ( bgpd/bgp_mplsvpn.c line 1445 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[leak_update called] --> B{Existing bpi found?}
B -- Yes --> C[Replace bpi->attr with new_attr]
C --> D{leak_update_nexthop_valid?}
D -- Valid --> E[SET BGP_PATH_VALID on bpi->flags]
D -- Invalid --> F[UNSET BGP_PATH_VALID on bpi->flags]
E --> G[⚠️ nh_flags NOT updated on bpi->attr in update path]
B -- No --> H[Create new bgp_path_info via info_make]
H --> I{leak_update_nexthop_valid?}
I -- Valid --> J[SET BGP_PATH_VALID on new->flags]
J --> K["SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID) ← this PR (typo: nh_flag)"]
I -- Invalid --> L[UNSET BGP_PATH_VALID on new->flags]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: bgpd/bgp_mplsvpn.c
Line: 1445
Comment:
**Typo: `nh_flag` should be `nh_flags`**
The struct field is `nh_flags` (`uint8_t nh_flags` in `bgp_attr.h:141`), not `nh_flag`. This typo causes a compile-time error — the field `nh_flag` does not exist on `struct attr`, so this line will produce a "no member named 'nh_flag'" error. Every other usage in the codebase (e.g. `bgp_mplsvpn.c:2488`, `bgp_routemap.c:4218`, `bgp_attr.c:1094`) correctly uses `nh_flags`.
```suggestion
SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "bgpd: initialise nh_flag attribute" | Re-trigger Greptile |
bgpd/bgp_mplsvpn.c
Outdated
| source_bpi, new, bgp_orig, p, debug)) | ||
| source_bpi, new, bgp_orig, p, debug)) { | ||
| bgp_path_info_set_flag(bn, new, BGP_PATH_VALID); | ||
| SET_FLAG(new->attr->nh_flag, BGP_ATTR_NH_VALID); |
There was a problem hiding this comment.
Typo:
nh_flag should be nh_flags
The struct field is nh_flags (uint8_t nh_flags in bgp_attr.h:141), not nh_flag. This typo causes a compile-time error — the field nh_flag does not exist on struct attr, so this line will produce a "no member named 'nh_flag'" error. Every other usage in the codebase (e.g. bgp_mplsvpn.c:2488, bgp_routemap.c:4218, bgp_attr.c:1094) correctly uses nh_flags.
| SET_FLAG(new->attr->nh_flag, BGP_ATTR_NH_VALID); | |
| SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_mplsvpn.c
Line: 1445
Comment:
**Typo: `nh_flag` should be `nh_flags`**
The struct field is `nh_flags` (`uint8_t nh_flags` in `bgp_attr.h:141`), not `nh_flag`. This typo causes a compile-time error — the field `nh_flag` does not exist on `struct attr`, so this line will produce a "no member named 'nh_flag'" error. Every other usage in the codebase (e.g. `bgp_mplsvpn.c:2488`, `bgp_routemap.c:4218`, `bgp_attr.c:1094`) correctly uses `nh_flags`.
```suggestion
SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID);
```
How can I resolve this? If you propose a fix, please make it concise.29df5e1 to
c2cf998
Compare
in "leak_update" function nh_flag is not initialized when nexthop is valid at route creation. Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
c2cf998 to
dd5baf1
Compare
| source_bpi, new, bgp_orig, p, debug)) { | ||
| bgp_path_info_set_flag(bn, new, BGP_PATH_VALID); | ||
| else | ||
| SET_FLAG(new->attr->nh_flags, BGP_ATTR_NH_VALID); |
There was a problem hiding this comment.
Is this BGP_ATTR_NH_VALID flag used anywhere at all in the code?
This patch modifies `leak_update()` to accept a non-interned static attribute rather than requiring an already-interned one. Previously, callers such as `vpn_leak_to_vrf_update_onevrf()` and `_vpn_leak_from_vrf_update_leak_attr()` would intern the attribute prior to calling `leak_update()`. If `leak_update()` determined that the route was unchanged, it would simply unintern the attribute and continue, resulting in unnecessary memory allocation and hash table overhead. With this change, the caller passes a pointer to the static attribute, and `leak_update()` now defers calling `bgp_attr_intern()` until it actually needs to update an existing path or allocate a new one. The callers handle flushing the static attribute when done. Additionally, this commit fixes a minor issue during new path creation: if the new leaked path is determined to have a valid nexthop, the `BGP_ATTR_NH_VALID` flag is now correctly set on the newly interned attribute's `nh_flags`. Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
1eea2e3 to
b55ece2
Compare
in "leak_update" function
nh_flag is not initialized when nexthop is valid at route creation.