From 63464bd62f5d5a34545a94305b5e8078cc224a90 Mon Sep 17 00:00:00 2001 From: Maxence Younsi Date: Mon, 8 Jan 2024 11:06:48 +0100 Subject: [PATCH] doc, tests, bgpd: address PR#14847 review fixed nits & styling issues fixed bad conditions in bmp_bpi_(un)lock added bgp_peer_get_send_holdtime added bgp_peer_get_local_as Signed-off-by: Maxence Younsi --- bgpd/bgp_bmp.c | 57 +++++---------------- bgpd/bgp_conditional_adv.c | 10 ++-- bgpd/bgp_main.c | 7 --- bgpd/bgp_packet.c | 49 ++++++++++-------- bgpd/bgp_packet.h | 7 +-- bgpd/bgp_route.c | 2 +- bgpd/bgp_updgrp.h | 2 +- bgpd/bgp_updgrp_adv.c | 18 +++---- bgpd/bgpd.h | 2 +- doc/user/bmp.rst | 2 +- tests/topotests/lib/bmp_collector/bmpserver | 2 +- 11 files changed, 64 insertions(+), 94 deletions(-) diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c index e0b67b59c7eb..957cb6d4f823 100644 --- a/bgpd/bgp_bmp.c +++ b/bgpd/bgp_bmp.c @@ -76,7 +76,6 @@ static struct timeval bmp_startup_time = {0}; /* compute the time in millis since the bmp_startup_time recorded */ static uint32_t bmp_time_since_startup(struct timeval *delay) { - if (bmp_startup_time.tv_sec == 0 && bmp_startup_time.tv_usec == 0) { zlog_info("bmp [%s]: Startup time not recorded", __func__); return 0; @@ -91,7 +90,6 @@ static uint32_t bmp_time_since_startup(struct timeval *delay) static const char *bmp_state_str(enum BMP_State state) { switch (state) { - case BMP_StartupIdle: return "Startup-Wait"; case BMP_PeerUp: @@ -209,7 +207,7 @@ struct bmp_lbpi_h_head bmp_lbpi; static struct bmp_bpi_lock *bmp_lock_bpi(struct bgp *bgp, struct bgp_path_info *bpi) { - if (!bpi && !bpi) + if (!bgp || !bpi) return NULL; BMP_LBPI_LOOKUP_BPI(head, prev, hash_lookup, bpi, bgp); @@ -249,8 +247,7 @@ static struct bmp_bpi_lock *bmp_lock_bpi(struct bgp *bgp, static struct bmp_bpi_lock *bmp_unlock_bpi(struct bgp *bgp, struct bgp_path_info *bpi) { - - if (!bpi) + if (!bgp || !bpi) return NULL; BMP_LBPI_LOOKUP_BPI(head, prev, hash_lookup, bpi, bgp); @@ -264,7 +261,6 @@ static struct bmp_bpi_lock *bmp_unlock_bpi(struct bgp *bgp, /* if bpi is not used by bmp anymore */ if (hash_lookup->lock <= 0) { - struct bgp_path_info *tmp_bpi = hash_lookup->locked; struct bgp_dest *tmp_dest = hash_lookup->dest; struct bgp *tmp_bgp = hash_lookup->bgp; @@ -496,20 +492,15 @@ static inline int bmp_get_peer_distinguisher(struct bgp *bgp, afi_t afi, if (afi == AFI_UNSPEC) { { /* scope lock iter variables */ afi_t afi_rd_lookup; - safi_t _; - - FOREACH_AFI_SAFI (afi_rd_lookup, _) { + for (afi_rd_lookup = AFI_IP; afi_rd_lookup < AFI_MAX; afi_rd_lookup++) { if (CHECK_FLAG(bgp->vpn_policy[afi_rd_lookup] .flags, BGP_VPN_POLICY_TOVPN_RD_SET)) { afi = afi_rd_lookup; - /* stop FOREACH_AFI_SAFI macro */ - afi_rd_lookup = AFI_MAX; + /* we found an AFI with a RD set */ + break; } - - /* break safi sub-loop to go over next afi */ - break; } } @@ -733,9 +724,9 @@ static struct stream *bmp_peerstate(struct peer *peer, bool down) /* Local Address (16 bytes) */ if (!peer->su_local || is_locrib) - stream_put(s, 0, 16); + stream_put(s, 0, IPV6_MAX_BYTELEN); else if (peer->su_local->sa.sa_family == AF_INET6) - stream_put(s, &peer->su_local->sin6.sin6_addr, 16); + stream_put(s, &peer->su_local->sin6.sin6_addr, IPV6_MAX_BYTELEN); else if (peer->su_local->sa.sa_family == AF_INET) { stream_putl(s, 0); stream_putl(s, 0); @@ -859,7 +850,6 @@ static int bmp_send_peerup(struct bmp *bmp) static int bmp_send_peerup_vrf(struct bmp *bmp) { - struct bmp_bgp *bmpbgp = bmp->targets->bmpbgp; /* send unconditionally because state may have been set before the @@ -871,7 +861,7 @@ static int bmp_send_peerup_vrf(struct bmp *bmp) struct stream *s = bmp_peerstate(bmpbgp->bgp->peer_self, bmpbgp->vrf_up == vrf_state_down); if (!s) { - zlog_warn("bmp: peer state message error"); + zlog_warn("bmp: vrf peer state failed, skipping message."); return 1; } @@ -899,7 +889,6 @@ static void bmp_send_all(struct bmp_bgp *bmpbgp, struct stream *s) static void bmp_send_all_safe(struct bmp_bgp *bmpbgp, struct stream *s) { - if (!s || !bmpbgp) return; @@ -1470,7 +1459,6 @@ static int bmp_monitor_rib_out_pre_updgrp_walkcb(struct update_group *updgrp, continue; SUBGRP_FOREACH_PEER (subgrp, paf) { - addpath_tx_id = !ctx->bpi ? 0 : bgp_addpath_id_for_peer( @@ -1532,8 +1520,6 @@ struct rib_out_post_updgrp_walkctx { static int bmp_monitor_rib_out_post_updgrp_walkcb(struct update_group *updgrp, void *hidden_ctx) { - - struct rib_out_post_updgrp_walkctx *ctx = (struct rib_out_post_updgrp_walkctx *)hidden_ctx; @@ -1544,7 +1530,6 @@ static int bmp_monitor_rib_out_post_updgrp_walkcb(struct update_group *updgrp, uint32_t addpath_tx_id; UPDGRP_FOREACH_SUBGRP (updgrp, subgrp) { - addpath_tx_id = !ctx->bpi ? 0 : bgp_addpath_id_for_peer( SUBGRP_PEER(subgrp), @@ -1596,7 +1581,6 @@ static inline bool bmp_monitor_rib_out_post_walk( bmp_monitor_rib_out_post_updgrp_walkcb, (void *)&walkctx); - return written; } @@ -2452,8 +2436,9 @@ static int bmp_process_ribinpost(struct bgp *bgp, afi_t afi, safi_t safi, } } - // if bmp_process_one returns NULL - // we don't have anything to do next + /* if bmp_process_one returns NULL + * we don't have anything to do next + */ if (!new_head) continue; @@ -2834,23 +2819,8 @@ static int bmp_bgp_del(struct bgp *bgp) static void bmp_bgp_peer_vrf(struct bmp_bgp_peer *bbpeer, struct bgp *bgp) { - struct peer *peer = bgp->peer_self; - uint16_t send_holdtime; - as_t local_as; - - if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER)) - send_holdtime = peer->holdtime; - else - send_holdtime = peer->bgp->default_holdtime; - - /* local-as Change */ - if (peer->change_local_as) - local_as = peer->change_local_as; - else - local_as = peer->local_as; - - struct stream *s = bgp_open_make(peer, send_holdtime, local_as); + struct stream *s = bgp_open_make(peer); size_t open_len = stream_get_endp(s); bbpeer->open_rx_len = open_len; @@ -2859,6 +2829,8 @@ static void bmp_bgp_peer_vrf(struct bmp_bgp_peer *bbpeer, struct bgp *bgp) bbpeer->open_tx_len = open_len; bbpeer->open_tx = bbpeer->open_rx; + + stream_free(s); } /* update the vrf status of the bmpbgp struct for vrf peer up/down @@ -2869,7 +2841,6 @@ static void bmp_bgp_peer_vrf(struct bmp_bgp_peer *bbpeer, struct bgp *bgp) */ bool bmp_bgp_update_vrf_status(struct bmp_bgp *bmpbgp, enum bmp_vrf_state force) { - if (!bmpbgp || !bmpbgp->bgp) return false; diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c index 97895e506dff..e92baca0848f 100644 --- a/bgpd/bgp_conditional_adv.c +++ b/bgpd/bgp_conditional_adv.c @@ -106,14 +106,14 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi, subgrp, dest, pi, !addpath_capable ? 0 - : bgp_addpath_id_for_peer( - peer, afi, safi, - &pi->tx_addpath), + : bgp_addpath_id_for_peer(peer, + afi, + safi, + &pi->tx_addpath), &attr, false, update_type == UPDATE_TYPE_ADVERTISE ? false - : true, - __func__); + : true); } diff --git a/bgpd/bgp_main.c b/bgpd/bgp_main.c index 805f3ac9e20f..851c4880c37f 100644 --- a/bgpd/bgp_main.c +++ b/bgpd/bgp_main.c @@ -263,8 +263,6 @@ static __attribute__((__noreturn__)) void bgp_exit(int status) static int bgp_vrf_new(struct vrf *vrf) { - zlog_info("BGP VRF CREATE %s", vrf->name); - if (BGP_DEBUG(zebra, ZEBRA)) zlog_debug("VRF Created: %s(%u)", vrf->name, vrf->vrf_id); @@ -273,7 +271,6 @@ static int bgp_vrf_new(struct vrf *vrf) static int bgp_vrf_delete(struct vrf *vrf) { - zlog_info("BGP VRF DELETE %s", vrf->name); if (BGP_DEBUG(zebra, ZEBRA)) zlog_debug("VRF Deletion: %s(%u)", vrf->name, vrf->vrf_id); @@ -285,8 +282,6 @@ static int bgp_vrf_enable(struct vrf *vrf) struct bgp *bgp; vrf_id_t old_vrf_id; - zlog_info("BGP VRF ENABLE %s", vrf->name); - if (BGP_DEBUG(zebra, ZEBRA)) zlog_debug("VRF enable add %s id %u", vrf->name, vrf->vrf_id); @@ -320,8 +315,6 @@ static int bgp_vrf_disable(struct vrf *vrf) { struct bgp *bgp; - zlog_info("BGP VRF DISABLE %s", vrf->name); - if (vrf->vrf_id == VRF_DEFAULT) return 0; diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 591df18c210b..08cae47e6418 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -643,12 +643,29 @@ void bgp_keepalive_send(struct peer *peer) bgp_writes_on(peer->connection); } -struct stream *bgp_open_make(struct peer *peer, uint16_t send_holdtime, - as_t local_as) +uint16_t bgp_peer_get_send_holdtime(struct peer *peer) { + if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER)) + return peer->holdtime; + else + return peer->bgp->default_holdtime; +} + +uint16_t bgp_peer_get_local_as(struct peer *peer) +{ + if (peer->change_local_as) + return peer->change_local_as; + else + return peer->local_as; +} +struct stream *bgp_open_make(struct peer *peer) +{ struct stream *s = stream_new(BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE); + uint16_t send_holdtime = bgp_peer_get_send_holdtime(peer); + as_t local_as = bgp_peer_get_local_as(peer); + /* Make open packet. */ bgp_packet_set_marker(s, BGP_MSG_OPEN); @@ -688,29 +705,17 @@ struct stream *bgp_open_make(struct peer *peer, uint16_t send_holdtime, */ void bgp_open_send(struct peer_connection *connection) { - struct stream *s; - uint16_t send_holdtime; - as_t local_as; struct peer *peer = connection->peer; + struct stream *s = bgp_open_make(peer); - if (CHECK_FLAG(peer->flags, PEER_FLAG_TIMER)) - send_holdtime = peer->holdtime; - else - send_holdtime = peer->bgp->default_holdtime; - - /* local-as Change */ - if (peer->change_local_as) - local_as = peer->change_local_as; - else - local_as = peer->local_as; - - s = bgp_open_make(peer, send_holdtime, local_as); + if (bgp_debug_neighbor_events(peer)) { + uint16_t local_as = bgp_peer_get_local_as(peer); + uint16_t send_holdtime = bgp_peer_get_send_holdtime(peer); - if (bgp_debug_neighbor_events(peer)) - zlog_debug( - "%s sending OPEN, version %d, my as %u, holdtime %d, id %pI4", - peer->host, BGP_VERSION_4, local_as, send_holdtime, - &peer->local_id); + zlog_debug("%s sending OPEN, version %d, my as %u, holdtime %d, id %pI4", + peer->host, BGP_VERSION_4, local_as, send_holdtime, + &peer->local_id); + } /* Dump packet if debug option is set. */ /* bgp_packet_dump (s); */ diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h index 58d6453cc656..488853880d41 100644 --- a/bgpd/bgp_packet.h +++ b/bgpd/bgp_packet.h @@ -43,9 +43,10 @@ DECLARE_HOOK(bgp_packet_send, /* Packet send and receive function prototypes. */ extern void bgp_keepalive_send(struct peer *peer); -extern struct stream *bgp_open_make(struct peer *peer, - uint16_t send_holdtime, - as_t local_as); + +extern uint16_t bgp_peer_get_local_as(struct peer *peer); +extern uint16_t bgp_peer_get_send_holdtime(struct peer *peer); +extern struct stream *bgp_open_make(struct peer *peer); extern void bgp_open_send(struct peer_connection *connection); extern void bgp_notify_send(struct peer_connection *connection, uint8_t code, uint8_t sub_code); diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 3fdef01fe0ba..b8e1520bf2ea 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3037,7 +3037,7 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp, advertise = bgp_check_advertise(bgp, dest, safi); bgp_adj_out_updated(subgrp, dest, selected, addpath_tx_id, &attr, false, - selected && advertise ? false : true, __func__); + selected && advertise ? false : true); if (selected) { if (subgroup_announce_check(dest, selected, subgrp, p, &attr, diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h index 5523fc68503c..f72d2eb2459f 100644 --- a/bgpd/bgp_updgrp.h +++ b/bgpd/bgp_updgrp.h @@ -455,7 +455,7 @@ extern void bgp_adj_out_updated(struct update_subgroup *subgrp, struct bgp_dest *dest, struct bgp_path_info *path, uint32_t addpath_tx, struct attr *attr, bool post_policy, - bool withdraw, const char *caller); + bool withdraw); extern void bgp_adj_out_unset_subgroup(struct bgp_dest *dest, struct update_subgroup *subgrp, char withdraw, uint32_t addpath_tx_id); diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index 97d18852dac3..e7764e1679b1 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -259,10 +259,11 @@ static int group_announce_route_walkcb(struct update_group *updgrp, void *arg) } if (!adj) - bgp_adj_out_updated( - subgrp, ctx->dest, NULL, - 0, NULL, false, true, - __func__); + bgp_adj_out_updated(subgrp, + ctx->dest, + NULL, 0, + NULL, false, + true); } } } @@ -510,8 +511,7 @@ bgp_advertise_clean_subgroup(struct update_subgroup *subgrp, /* call the bgp_adj_out_updated hook for bmp rib-out monitoring */ void bgp_adj_out_updated(struct update_subgroup *subgrp, struct bgp_dest *dest, struct bgp_path_info *path, uint32_t addpath_tx, - struct attr *attr, bool post_policy, bool withdraw, - const char *caller) + struct attr *attr, bool post_policy, bool withdraw) { if (path && !withdraw && CHECK_FLAG(path->flags, BGP_PATH_REMOVED)) { /* path is removed, enforcing withdraw state */ @@ -647,7 +647,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, subgrp->version = MAX(subgrp->version, dest->version); bgp_adj_out_updated(subgrp, dest, path, adj->addpath_tx_id, attr, true, - false, __func__); + false); } /* The only time 'withdraw' will be false is if we are sending @@ -700,7 +700,7 @@ void bgp_adj_out_unset_subgroup(struct bgp_dest *dest, bgp_adj_out_updated(subgrp, dest, NULL, adj->addpath_tx_id, NULL, true, - withdraw, __func__); + withdraw); } else { /* Free allocated information. */ adj_free(adj); @@ -986,7 +986,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, bool withdraw) dest = bgp_safi_node_lookup(bgp->rib[afi][safi_rib], safi_rib, &p, NULL); - // TODO BMP hook call for rib-out pre-policy + // TODO add missing BMP hook call for default-originate in rib-out pre-policy if (withdraw) { /* Withdraw the default route advertised using default diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 5ac43f663d6e..d9c179af55c5 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1650,7 +1650,7 @@ struct peer { uint32_t stat_upd_7606; /* RFC7606: treat-as-withdraw */ uint32_t stat_adj_in_count[AFI_MAX][SAFI_MAX]; uint32_t stat_loc_rib_count[AFI_MAX][SAFI_MAX]; - // no need for stat_adj_out_count here, it is in struct update_subgroup + /* no need for stat_adj_out_count here, it is in struct update_subgroup */ /* BGP state count */ uint32_t established; /* Established */ diff --git a/doc/user/bmp.rst b/doc/user/bmp.rst index a642144096a5..c9890f4c254e 100644 --- a/doc/user/bmp.rst +++ b/doc/user/bmp.rst @@ -88,7 +88,7 @@ There are two options that apply to the BGP instance as a whole: BMP Route Monitoring is not affected by this option. -.. clicmd:: bmp startup-delay delay(0-4294967294) +.. clicmd:: bmp startup-delay delay (0-4294967294) This sets the delay (in milliseconds) after module startup for BMP sessions to become active. diff --git a/tests/topotests/lib/bmp_collector/bmpserver b/tests/topotests/lib/bmp_collector/bmpserver index 89a802f9d719..6ead17fd03da 100755 --- a/tests/topotests/lib/bmp_collector/bmpserver +++ b/tests/topotests/lib/bmp_collector/bmpserver @@ -34,7 +34,7 @@ def main(): data = BMPMsg.dissect(data) except Exception as e: # XXX: do something - print(f"[bmp] Exception occured: {e}") + print(f"[bmp] Exception occured: {e}") pass except KeyboardInterrupt: # XXX: do something