Skip to content

Commit

Permalink
doc, tests, bgpd: address PR#14847 review
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@mxyns
mxyns committed Feb 15, 2024
1 parent 0502ca3 commit 63464bd
Show file tree
Hide file tree
Showing 11 changed files with 64 additions and 94 deletions.
57 changes: 14 additions & 43 deletions bgpd/bgp_bmp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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:
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;

Expand All @@ -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),
Expand Down Expand Up @@ -1596,7 +1581,6 @@ static inline bool bmp_monitor_rib_out_post_walk(
bmp_monitor_rib_out_post_updgrp_walkcb,
(void *)&walkctx);


return written;
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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;

Expand Down
10 changes: 5 additions & 5 deletions bgpd/bgp_conditional_adv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}


Expand Down
7 changes: 0 additions & 7 deletions bgpd/bgp_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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;

Expand Down
49 changes: 27 additions & 22 deletions bgpd/bgp_packet.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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); */
Expand Down
7 changes: 4 additions & 3 deletions bgpd/bgp_packet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_route.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_updgrp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 63464bd

Please sign in to comment.