On Wed, Jul 06, 2022 at 06:15:45PM +0200, Theo Buehler wrote:
> On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> > This diff changes various loops which call into up_generate_update() so
> > that all these loops call the same function peer_generate_update() which
> > then calls up_generate_update(). This is a step to add an alternative path
> > to generate updates for add-path send support without altering many
> > code-paths.
> >
> > If I did not fool myself this should not alter current behaviour.
> > rde_up_dump_upcall() gets a few more checks but all those checks should be
> > false in this case (e.g. peer_dump checks the export_type already).
>
> This looks good and I agree that the behavior is mostly preserved. I
> noticed are two changes of behavior and I have a small question for
> rde_up_dump_upcall():
>
> > @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> > 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);
>
> Failure of this check will now be silent.
This is fine, that code path was acting as an assert().
rde_up_dump_upcall() is called by a RIB table walk for the
peer->loc_rib_id RIB (see peer_dump()) so there should be no way that
if (re->rib_id != peer->loc_rib_id) is true.
> > - if (peer->capa.mp[re->prefix->aid] == 0)
> > - fatalx("%s: Unexpected %s prefix", __func__,
> > - aid2str(re->prefix->aid));
>
> Same here.
Similar reason. The rib walks each address family independent:
for (i = 0; i < AID_MAX; i++) {
if (peer->capa.mp[i])
peer_dump(peer, i);
}
Now this check is missing in the IMSG_REFRESH case in rde.c but other than
that there should be no way to reach this code with a wrong aid.
Skipping routes with non-negotiated address families should not matter
here but it will make the code more robust.
> I got lost when checking that re->prefix->aid == p->pt->aid. I assume
> that follows from the definition of the rib_entry's prefix queue?
If the prefix p is on the list of prefixes in re (part of re->prefix_h)
then re->prefix == p->pt and so the aid check also needs to match.
We cache the struct pt_entry * in a few places to simplify access.
Since the code uses prefix_best(re) to get the p we know that p is part of
the re list of prefixes.
> > -
> > /* 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
Updated diff below that adds the extra aid check in the IMSG_REFRESH case.
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.547
diff -u -p -r1.547 rde.c
--- rde.c 27 Jun 2022 13:26:51 -0000 1.547
+++ rde.c 6 Jul 2022 16:43:22 -0000
@@ -1123,12 +1123,19 @@ rde_dispatch_imsg_peer(struct rde_peer *
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 *
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
}
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
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.254
diff -u -p -r1.254 rde.h
--- rde.h 27 Jun 2022 13:26:51 -0000 1.254
+++ rde.h 6 Jul 2022 16:54:39 -0000
@@ -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 p
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);
Index: rde_decide.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
retrieving revision 1.91
diff -u -p -r1.91 rde_decide.c
--- rde_decide.c 22 Mar 2022 10:53:08 -0000 1.91
+++ rde_decide.c 6 Jul 2022 14:54:58 -0000
@@ -493,7 +493,7 @@ prefix_evaluate(struct rib_entry *re, st
* 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, st
*/
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);
}
Index: rde_peer.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
retrieving revision 1.17
diff -u -p -r1.17 rde_peer.c
--- rde_peer.c 27 Jun 2022 13:26:51 -0000 1.17
+++ rde_peer.c 6 Jul 2022 16:46:56 -0000
@@ -212,6 +212,63 @@ peer_add(uint32_t id, struct peer_config
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,
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_
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)
