From 434c625812acf91c6e3b3f384c7b8ef0a61058c1 Mon Sep 17 00:00:00 2001 From: claudio Date: Thu, 7 Jul 2022 10:46:54 +0000 Subject: [PATCH] Refactor the code that generates updates so that up_generate_updates is only called in one spot. rde_generate_updates() gets a enum eval_mode argument to discern the different cases. peer_generate_update() uses the eval_mode to skip the update if it is not needed. While there also add an extra AID check in IMSG_REFRESH case to make sure the requested AID is actually available for this peer. OK tb@ --- usr.sbin/bgpd/rde.c | 87 +++++++------------------------------- usr.sbin/bgpd/rde.h | 10 ++++- usr.sbin/bgpd/rde_decide.c | 7 +-- usr.sbin/bgpd/rde_peer.c | 72 +++++++++++++++++++++++++------ 4 files changed, 88 insertions(+), 88 deletions(-) diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c index 45b22bfbbc4..f440614919c 100644 --- a/usr.sbin/bgpd/rde.c +++ b/usr.sbin/bgpd/rde.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.c,v 1.547 2022/06/27 13:26:51 claudio Exp $ */ +/* $OpenBSD: rde.c,v 1.548 2022/07/07 10:46:54 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -1123,12 +1123,19 @@ rde_dispatch_imsg_peer(struct rde_peer *peer, void *bula) break; case IMSG_REFRESH: if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(rr)) { - log_warnx("%s: wrong imsg len", __func__); + log_warnx("route refresh: wrong imsg len"); break; } memcpy(&rr, imsg.data, sizeof(rr)); if (rr.aid >= AID_MAX) { - log_warnx("%s: bad AID", __func__); + log_peer_warnx(&peer->conf, + "route refresh: bad AID %d", rr.aid); + break; + } + if (peer->capa.mp[rr.aid]) { + log_peer_warnx(&peer->conf, + "route refresh: AID %s not negotiated", + aid2str(rr.aid)); break; } switch (rr.subtype) { @@ -1156,7 +1163,8 @@ rde_dispatch_imsg_peer(struct rde_peer *peer, void *bula) aid2str(rr.aid)); break; default: - log_warnx("%s: bad subtype %d", __func__, rr.subtype); + log_peer_warnx(&peer->conf, + "route refresh: bad subtype %d", rr.subtype); break; } break; @@ -3037,59 +3045,6 @@ rde_evaluate_all(void) return rde_eval_all; } -static int -rde_skip_peer(struct rde_peer *peer, uint16_t rib_id, uint8_t aid) -{ - /* skip ourself */ - if (peer == peerself) - return 1; - if (peer->state != PEER_UP) - return 1; - /* skip peers using a different rib */ - if (peer->loc_rib_id != rib_id) - return 1; - /* check if peer actually supports the address family */ - if (peer->capa.mp[aid] == 0) - return 1; - /* skip peers with special export types */ - if (peer->export_type == EXPORT_NONE || - peer->export_type == EXPORT_DEFAULT_ROUTE) - return 1; - - return 0; -} - -void -rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old, - int eval_all) -{ - struct rde_peer *peer; - uint8_t aid; - - /* - * If old is != NULL we know it was active and should be removed. - * If new is != NULL we know it is reachable and then we should - * generate an update. - */ - if (old == NULL && new == NULL) - return; - - if (new) - aid = new->pt->aid; - else - aid = old->pt->aid; - - LIST_FOREACH(peer, &peerlist, peer_l) { - if (rde_skip_peer(peer, rib->id, aid)) - continue; - /* skip regular peers if the best path didn't change */ - if ((peer->flags & PEERFLAG_EVALUATE_ALL) == 0 && eval_all) - continue; - - up_generate_updates(out_rules, peer, new, old); - } -} - /* flush Adj-RIB-Out by withdrawing all prefixes */ static void rde_up_flush_upcall(struct prefix *p, void *ptr) @@ -3760,26 +3715,16 @@ rde_softreconfig_in(struct rib_entry *re, void *bula) } static void -rde_softreconfig_out(struct rib_entry *re, void *bula) +rde_softreconfig_out(struct rib_entry *re, void *arg) { - struct prefix *p; - struct rde_peer *peer; - uint8_t aid = re->prefix->aid; + struct rib *rib = arg; + struct prefix *p; if ((p = prefix_best(re)) == NULL) /* no valid path for prefix */ return; - LIST_FOREACH(peer, &peerlist, peer_l) { - if (rde_skip_peer(peer, re->rib_id, aid)) - continue; - /* skip peers which don't need to reconfigure */ - if (peer->reconf_out == 0) - continue; - - /* Regenerate all updates. */ - up_generate_updates(out_rules, peer, p, p); - } + rde_generate_updates(rib, p, NULL, EVAL_RECONF); } static void diff --git a/usr.sbin/bgpd/rde.h b/usr.sbin/bgpd/rde.h index 3f74b06c1c2..1194a82184c 100644 --- a/usr.sbin/bgpd/rde.h +++ b/usr.sbin/bgpd/rde.h @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.h,v 1.254 2022/06/27 13:26:51 claudio Exp $ */ +/* $OpenBSD: rde.h,v 1.255 2022/07/07 10:46:54 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Claudio Jeker and @@ -362,6 +362,12 @@ struct filterstate { uint8_t nhflags; }; +enum eval_mode { + EVAL_DEFAULT, + EVAL_ALL, + EVAL_RECONF, +}; + extern struct rde_memstats rdemem; /* prototypes */ @@ -384,7 +390,7 @@ void rde_pftable_del(uint16_t, struct prefix *); int rde_evaluate_all(void); void rde_generate_updates(struct rib *, struct prefix *, - struct prefix *, int); + struct prefix *, enum eval_mode); uint32_t rde_local_as(void); int rde_decisionflags(void); void rde_peer_send_rrefresh(struct rde_peer *, uint8_t, uint8_t); diff --git a/usr.sbin/bgpd/rde_decide.c b/usr.sbin/bgpd/rde_decide.c index 1dbb4a55734..0f5057c2bca 100644 --- a/usr.sbin/bgpd/rde_decide.c +++ b/usr.sbin/bgpd/rde_decide.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_decide.c,v 1.91 2022/03/22 10:53:08 claudio Exp $ */ +/* $OpenBSD: rde_decide.c,v 1.92 2022/07/07 10:46:54 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Claudio Jeker @@ -493,7 +493,7 @@ prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix *old) * but remember that xp may be NULL aka ineligible. * Additional decision may be made by the called functions. */ - rde_generate_updates(rib, xp, active, 0); + rde_generate_updates(rib, xp, active, EVAL_DEFAULT); if ((rib->flags & F_RIB_NOFIB) == 0) rde_send_kroute(rib, xp, active); return; @@ -506,5 +506,6 @@ prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix *old) */ if (rde_evaluate_all()) if ((new != NULL && prefix_eligible(new)) || old != NULL) - rde_generate_updates(rib, prefix_best(re), NULL, 1); + rde_generate_updates(rib, prefix_best(re), NULL, + EVAL_ALL); } diff --git a/usr.sbin/bgpd/rde_peer.c b/usr.sbin/bgpd/rde_peer.c index 6d22c1925bd..c0bffe2f21b 100644 --- a/usr.sbin/bgpd/rde_peer.c +++ b/usr.sbin/bgpd/rde_peer.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_peer.c,v 1.17 2022/06/27 13:26:51 claudio Exp $ */ +/* $OpenBSD: rde_peer.c,v 1.18 2022/07/07 10:46:54 claudio Exp $ */ /* * Copyright (c) 2019 Claudio Jeker @@ -212,6 +212,63 @@ peer_add(uint32_t id, struct peer_config *p_conf) return (peer); } +static void +peer_generate_update(struct rde_peer *peer, uint16_t rib_id, + struct prefix *new, struct prefix *old, enum eval_mode mode) +{ + uint8_t aid; + + if (new != NULL) + aid = new->pt->aid; + else if (old != NULL) + aid = old->pt->aid; + else + return; + + /* skip ourself */ + if (peer == peerself) + return; + if (peer->state != PEER_UP) + return; + /* skip peers using a different rib */ + if (peer->loc_rib_id != rib_id) + return; + /* check if peer actually supports the address family */ + if (peer->capa.mp[aid] == 0) + return; + /* skip peers with special export types */ + if (peer->export_type == EXPORT_NONE || + peer->export_type == EXPORT_DEFAULT_ROUTE) + return; + + /* if reconf skip peers which don't need to reconfigure */ + if (mode == EVAL_RECONF && peer->reconf_out == 0) + return; + /* skip regular peers if the best path didn't change */ + if (mode == EVAL_ALL && (peer->flags & PEERFLAG_EVALUATE_ALL) == 0) + return; + + up_generate_updates(out_rules, peer, new, old); +} + +void +rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old, + enum eval_mode mode) +{ + struct rde_peer *peer; + + /* + * If old is != NULL we know it was active and should be removed. + * If new is != NULL we know it is reachable and then we should + * generate an update. + */ + if (old == NULL && new == NULL) + return; + + LIST_FOREACH(peer, &peerlist, peer_l) + peer_generate_update(peer, rib->id, new, old, mode); +} + /* * Various RIB walker callbacks. */ @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re, void *ptr) struct rde_peer *peer = ptr; struct prefix *p; - if (peer->state != PEER_UP) - return; - if (re->rib_id != peer->loc_rib_id) - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id, - peer->loc_rib_id); - if (peer->capa.mp[re->prefix->aid] == 0) - fatalx("%s: Unexpected %s prefix", __func__, - aid2str(re->prefix->aid)); - /* no eligible prefix, not even for 'evaluate all' */ if ((p = prefix_best(re)) == NULL) return; - - up_generate_updates(out_rules, peer, p, NULL); + peer_generate_update(peer, re->rib_id, p, NULL, 0); } static void @@ -461,6 +508,7 @@ peer_stale(struct rde_peer *peer, uint8_t aid) peer->staletime[aid] = now = getmonotime(); peer->state = PEER_DOWN; + /* XXX this is not quite correct */ /* mark Adj-RIB-Out stale for this peer */ if (prefix_dump_new(peer, AID_UNSPEC, 0, NULL, peer_adjout_stale_upcall, NULL, NULL) == -1) -- 2.20.1