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 <[email protected]>
mxyns · Feb 15, 2024 · 6a6d405 · 6a6d405
1 parent 0502ca3
commit 6a6d405
Show file tree

Hide file tree

Showing 11 changed files with 61 additions and 96 deletions.
diff --git 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;
@@ -2869,7 +2839,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
@@ -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
@@ -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
@@ -643,12 +643,27 @@ 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 +703,16 @@ 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))
-		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);
+	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);
+		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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 */