Skip to content

bgpd: refactor bgp_aggregate_{increment,decrement}#22126

Open
enkechen-panw wants to merge 1 commit into
FRRouting:masterfrom
enkechen-panw:bgp-aggregate-refactor
Open

bgpd: refactor bgp_aggregate_{increment,decrement}#22126
enkechen-panw wants to merge 1 commit into
FRRouting:masterfrom
enkechen-panw:bgp-aggregate-refactor

Conversation

@enkechen-panw
Copy link
Copy Markdown
Contributor

The two functions share identical guard logic and loop structure; the only difference is whether they call bgp_add_route_to_aggregate() or bgp_remove_route_from_aggregate(). Factor the common body into a static helper bgp_aggregate_adjust_count() controlled by a bool increment parameter, and reduce bgp_aggregate_increment/decrement to thin wrappers.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 30, 2026

Greptile Summary

This PR refactors bgp_aggregate_increment and bgp_aggregate_decrement by extracting their shared guard/loop body into a new static helper bgp_aggregate_adjust_count, controlled by a bool increment flag. The two public functions become thin wrappers with unchanged signatures.

  • All five early-exit guards, the bgp_node_match traversal loop, and the bgp_dest_unlock_node call are faithfully preserved in the helper; the only structural change is the if (increment) … else … branch that selects bgp_add_route_to_aggregate vs. bgp_remove_route_from_aggregate.
  • The block comment is updated from naming bgp_aggregate_increment() specifically to "This helper", which correctly reflects that both call sites now share the same code path.

Confidence Score: 5/5

Safe to merge — pure refactor with no behavioral changes.

Every guard, the traversal loop, and the unlock call are carried over verbatim into the shared helper; only the inner branch that selects add vs. remove is new. The public function signatures are unchanged, so all callers are unaffected. The previously flagged stale comment has been corrected in this revision.

No files require special attention.

Important Files Changed

Filename Overview
bgpd/bgp_route.c Factored shared guard/loop logic from bgp_aggregate_increment/decrement into static helper bgp_aggregate_adjust_count; all guards, loop structure, and unlock are preserved; thin wrappers maintain original public API.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[bgp_aggregate_increment] -->|increment=true| C[bgp_aggregate_adjust_count]
    B[bgp_aggregate_decrement] -->|increment=false| C
    C --> D{Guards pass?}
    D -- No --> E[return]
    D -- Yes --> F[bgp_node_match]
    F --> G{child found?}
    G -- No --> E
    G -- Yes --> H["Loop: dest → bgp_dest_parent_nolock"]
    H --> I{aggregate != NULL && prefixlen <}
    I -- No --> H
    I -- Yes --> J{increment?}
    J -- true --> K[bgp_add_route_to_aggregate]
    J -- false --> L[bgp_remove_route_from_aggregate]
    K --> H
    L --> H
    H -->|loop done| M[bgp_dest_unlock_node]
Loading

Reviews (2): Last reviewed commit: "bgpd: refactor bgp_aggregate_{increment,..." | Re-trigger Greptile

The two functions share identical guard logic and loop structure; the
only difference is whether they call bgp_add_route_to_aggregate() or
bgp_remove_route_from_aggregate(). Factor the common body into a static
helper bgp_aggregate_adjust_count() controlled by a bool increment
parameter, and reduce bgp_aggregate_increment/decrement to thin wrappers.

Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
@enkechen-panw enkechen-panw force-pushed the bgp-aggregate-refactor branch from c577928 to 113499f Compare May 30, 2026 18:33
@enkechen-panw
Copy link
Copy Markdown
Contributor Author

@greptileai review

@enkechen-panw
Copy link
Copy Markdown
Contributor Author

ci:rerun

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.

1 participant