-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bgpd: fix and improve snmp peer lookups #2207
bgpd: fix and improve snmp peer lookups #2207
Conversation
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3583/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base6 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built and tested this and the results are good. Please remove the unused variables addr and ret in peer_lookup_addr_ipv4() and ret in bgp_peer_lookup_next().
The previous implementation of bgp_peer_lookup_next did not consider the internal ordering of peers when using peer groups, which led to all standalone peers being skipped that had a lower IP address than the highest IP address of a peer belonging to a group. As the ordering of peers can not be arbitrary due to SNMP requiring increasing OIDs when walking an OID tree, this commit fixes the bug by properly looping through all peers and detecting the next highest IP address. Additionally, this commit improved both bgp_peer_lookup_next and peer_lookup_addr_ipv4 by using the socketunion stored within the peer struct (peer->su) instead of calling inet_pton for each peer during comparison. Signed-off-by: Pascal Mathis <mail@pascalmathis.com>
f271114
to
2b8e62f
Compare
@stannous Thank you for your feedback, seems I forgot about cleaning up the variables after trying several variants/solutions. I have updated the PR accordingly. |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3589/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base6 Static Analyzer issues remaining.See details at |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
…ups` (FRRouting/frr#2207) Package-Manager: Portage-2.3.24, Repoman-2.3.6
…ups` (FRRouting/frr#2207) (#10) Package-Manager: Portage-2.3.24, Repoman-2.3.6
The previous implementation of bgp_peer_lookup_next did not consider the
internal ordering of peers when using peer groups, which led to all
standalone peers being skipped that had a lower IP address than the
highest IP address of a peer belonging to a group.
As the ordering of peers can not be arbitrary due to SNMP requiring
increasing OIDs when walking an OID tree, this commit fixes the bug by
properly looping through all peers and detecting the next highest IP
address.
Additionally, this commit improved both bgp_peer_lookup_next and
peer_lookup_addr_ipv4 by using the socketunion stored within the peer
struct (peer->su) instead of calling inet_pton for each peer during
comparison.
Fixes #1190
Signed-off-by: Pascal Mathis mail@pascalmathis.com