Skip to content

lib: also compare rmap source address when comparing nexthop source a…#21233

Merged
riw777 merged 1 commit intoFRRouting:masterfrom
ak503:rmap-src
Mar 24, 2026
Merged

lib: also compare rmap source address when comparing nexthop source a…#21233
riw777 merged 1 commit intoFRRouting:masterfrom
ak503:rmap-src

Conversation

@ak503
Copy link
Copy Markdown
Contributor

@ak503 ak503 commented Mar 18, 2026

…ddresses

Fix #19611

…ddresses

Fix FRRouting#19611

Signed-off-by: Dmitrii Turlupov <turlupov@bk.ru>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR fixes issue #19611 by including the rmap_src field (a route-map-assigned source address) in nexthop comparison functions, preventing two nexthops that differ only in their rmap_src from being incorrectly treated as equal.

  • Adds a new static helper _nexthop_rmap_source_cmp that wraps nexthop_g_addr_cmp for the rmap_src field, mirroring the existing _nexthop_source_cmp pattern.
  • Calls _nexthop_rmap_source_cmp in _nexthop_cmp_no_labels (used by nexthop_cmp and nexthop_cmp_no_weight) before the existing _nexthop_source_cmp call.
  • Inlines an equivalent nexthop_g_addr_cmp call for rmap_src in nexthop_cmp_basic, consistent with that function's existing style of inlining rather than using helpers.
  • The change is hash-safe: rmap_src is already inside the _hash_begin/_hash_end region of struct nexthop and is already fed into jhash, so the hash/equality invariant is maintained.
  • Minor cosmetic issue: the second parameter of _nexthop_rmap_source_cmp is indented with the same whitespace as in _nexthop_source_cmp, but since the new function name is longer the column alignment is off.

Confidence Score: 4/5

  • Safe to merge — the fix is minimal, logically consistent with existing patterns, and hash-safe.
  • The change correctly extends both comparison sites to include rmap_src, which is already hashed, so there is no hash/equality mismatch risk. The only issue found is a minor indentation misalignment in the new helper's function signature. Logic and data-flow are sound.
  • No files require special attention beyond the minor indentation note in lib/nexthop.c.

Important Files Changed

Filename Overview
lib/nexthop.c Adds rmap_src comparison to both _nexthop_cmp_no_labels and nexthop_cmp_basic; logically correct since rmap_src is already in the hashed region of the struct. Minor indentation misalignment in the new helper function signature.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_nexthop_cmp_no_labels / nexthop_cmp_basic"] --> B{vrf_id equal?}
    B -- no --> Z[return diff]
    B -- yes --> C{type equal?}
    C -- no --> Z
    C -- yes --> D{weight/ifindex/gateway equal?}
    D -- no --> Z
    D -- yes --> E{srte_color equal?}
    E -- no --> Z
    E -- yes --> F["🆕 rmap_src compare\n(_nexthop_rmap_source_cmp)"]
    F -- not equal --> Z
    F -- equal --> G["src compare\n(_nexthop_source_cmp)"]
    G -- not equal --> Z
    G -- equal --> H[compare labels / backup indices]
    H --> I[return result]
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/nexthop.c
Line: 148-149

Comment:
**Parameter alignment is off in new function signature**

The second parameter of `_nexthop_rmap_source_cmp` is indented with the same whitespace as in `_nexthop_source_cmp`, but because the new function name is 5 characters longer, the second parameter no longer aligns with the opening parenthesis of the parameter list. With 8-space tabs, `const struct nexthop *nh1` begins at column 36, but `const struct nexthop *nh2` begins at column 31.

```suggestion
static int _nexthop_rmap_source_cmp(const struct nexthop *nh1,
				    const struct nexthop *nh2)
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "lib: also compare rm..."

Comment thread lib/nexthop.c
Comment on lines +148 to +149
static int _nexthop_rmap_source_cmp(const struct nexthop *nh1,
const struct nexthop *nh2)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Parameter alignment is off in new function signature

The second parameter of _nexthop_rmap_source_cmp is indented with the same whitespace as in _nexthop_source_cmp, but because the new function name is 5 characters longer, the second parameter no longer aligns with the opening parenthesis of the parameter list. With 8-space tabs, const struct nexthop *nh1 begins at column 36, but const struct nexthop *nh2 begins at column 31.

Suggested change
static int _nexthop_rmap_source_cmp(const struct nexthop *nh1,
const struct nexthop *nh2)
static int _nexthop_rmap_source_cmp(const struct nexthop *nh1,
const struct nexthop *nh2)
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/nexthop.c
Line: 148-149

Comment:
**Parameter alignment is off in new function signature**

The second parameter of `_nexthop_rmap_source_cmp` is indented with the same whitespace as in `_nexthop_source_cmp`, but because the new function name is 5 characters longer, the second parameter no longer aligns with the opening parenthesis of the parameter list. With 8-space tabs, `const struct nexthop *nh1` begins at column 36, but `const struct nexthop *nh2` begins at column 31.

```suggestion
static int _nexthop_rmap_source_cmp(const struct nexthop *nh1,
				    const struct nexthop *nh2)
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@riw777 riw777 merged commit 095412f into FRRouting:master Mar 24, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

question: ip protocol <proto> route-map <name> not applied for old routes?

2 participants