>From the start bgpd had prefix_link and prefix_unlink to link all the
various data objects together to build an actual prefix. Now prefix_move()
tries to be smart and reimplemented prefix_link and prefix_unlink as
inline versions (with minimal differences). Later the prefix_adjout_*
functions were added and again similar code to prefix_link / prefix_unlink
was added.
This diff removes the code duplication and uses prefix_link and
prefix_unlink in all those extra places. This removes a common error when
changing the object layout because one of the many codepaths was often
missed.
To make this happen prefix_link() gained an extra argument for the
pt_entry (the adjout has no rib_entry and so no way to get the pt from
there). It also moves the pftable handling and prefix_evaluate() call to
the currently only caller (prefix_add). Add an extra check in nexthop_link
to skip prefixes in the adj-rib-out since those are not linked up.
With this prefix_link can be used in all other places.
prefix_unlink can be almost used as is only the pt_unref() needs attention
(or actually setting p->pt to NULL). In the Adj-RIB-Out prefixes are
unlinked but remain on the RB tree to act as withdraw requests. For this
prefix_adjout_update() adds an extra reference to the pt_entry so that
after calling prefix_unlink() the pt_entry is still valid.
prefix_adjout_destroy() takes this last reference before freeing the
prefix again.
The result is a much cleaner handling of prefixes and far fewer places to
introduce new bugs.
--
:wq Claudio
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.227
diff -u -p -r1.227 rde_rib.c
--- rde_rib.c 25 Feb 2022 12:56:12 -0000 1.227
+++ rde_rib.c 28 Feb 2022 09:10:40 -0000
@@ -848,9 +848,9 @@ static int prefix_move(struct prefix *,
struct nexthop *, uint8_t, uint8_t);
static void prefix_link(struct prefix *, struct rib_entry *,
- struct rde_peer *, uint32_t, struct rde_aspath *,
- struct rde_community *, struct nexthop *,
- uint8_t, uint8_t);
+ struct pt_entry *, struct rde_peer *, uint32_t,
+ struct rde_aspath *, struct rde_community *,
+ struct nexthop *, uint8_t, uint8_t);
static void prefix_unlink(struct prefix *);
static struct prefix *prefix_alloc(void);
@@ -941,18 +941,21 @@ prefix_adjout_lookup(struct rde_peer *pe
xp.pt = pt_fill(prefix, prefixlen);
np = RB_NFIND(prefix_index, &peer->adj_rib_out, &xp);
- if (np != NULL && pt_prefix_cmp(np->pt, xp.pt) != 0)
+ if (np == NULL || pt_prefix_cmp(np->pt, xp.pt) != 0)
return NULL;
return np;
}
+/*
+ * Return next prefix after a lookup that is actually an update.
+ */
struct prefix *
prefix_adjout_next(struct rde_peer *peer, struct prefix *p)
{
struct prefix *np;
np = RB_NEXT(prefix_index, &peer->adj_rib_out, p);
- if (np == NULL || p->pt != np->pt)
+ if (np == NULL || np->pt != p->pt)
return NULL;
return np;
}
@@ -1060,7 +1063,14 @@ prefix_add(struct bgpd_addr *prefix, int
re = rib_add(rib, prefix, prefixlen);
p = prefix_alloc();
- prefix_link(p, re, peer, path_id, asp, comm, nexthop, nhflags, vstate);
+ prefix_link(p, re, re->prefix, peer, path_id, asp, comm, nexthop,
+ nhflags, vstate);
+
+ /* add possible pftable reference form aspath */
+ if (asp && asp->pftableid)
+ rde_pftable_add(asp->pftableid, p);
+ /* make route decision */
+ prefix_evaluate(re, p, NULL);
return (1);
}
@@ -1082,18 +1092,8 @@ prefix_move(struct prefix *p, struct rde
/* add new prefix node */
np = prefix_alloc();
- /* add reference to new AS path and communities */
- np->aspath = path_ref(asp);
- np->communities = communities_ref(comm);
- np->peer = peer;
- np->entry.list.re = prefix_re(p);
- np->pt = p->pt; /* skip refcnt update since ref is moved */
- np->path_id = p->path_id;
- np->validation_state = vstate;
- np->nhflags = nhflags;
- np->nexthop = nexthop_ref(nexthop);
- nexthop_link(np);
- np->lastchange = getmonotime();
+ prefix_link(np, prefix_re(p), p->pt, peer, p->path_id, asp, comm,
+ nexthop, nhflags, vstate);
/* add possible pftable reference from new aspath */
if (asp && asp->pftableid)
@@ -1105,21 +1105,12 @@ prefix_move(struct prefix *p, struct rde
*/
prefix_evaluate(prefix_re(np), np, p);
- /* remove possible pftable reference from old path first */
+ /* remove possible pftable reference from old path */
if (p->aspath && p->aspath->pftableid)
rde_pftable_del(p->aspath->pftableid, p);
/* remove old prefix node */
- nexthop_unlink(p);
- nexthop_unref(p->nexthop);
- communities_unref(p->communities);
- path_unref(p->aspath);
- p->communities = NULL;
- p->nexthop = NULL;
- p->aspath = NULL;
- p->peer = NULL;
- p->pt = NULL;
- p->entry.list.re = NULL;
+ prefix_unlink(p);
prefix_free(p);
return (0);
@@ -1213,14 +1204,10 @@ prefix_adjout_update(struct rde_peer *pe
prefix_head = &peer->updates[prefix->aid];
RB_REMOVE(prefix_tree, prefix_head, p);
}
+ /* unlink prefix so it can be relinked below */
+ prefix_unlink(p);
}
- /* unlink from aspath and remove nexthop ref */
- nexthop_unref(p->nexthop);
- communities_unref(p->communities);
- path_unref(p->aspath);
p->flags &= ~PREFIX_FLAG_MASK;
-
- /* peer and pt remain */
} else {
p = prefix_alloc();
p->flags |= PREFIX_FLAG_ADJOUT;
@@ -1248,13 +1235,8 @@ prefix_adjout_update(struct rde_peer *pe
comm = communities_link(&state->communities);
}
- p->aspath = path_ref(asp);
- p->communities = communities_ref(comm);
- p->nexthop = nexthop_ref(state->nexthop);
- p->nhflags = state->nhflags;
-
- p->validation_state = vstate;
- p->lastchange = getmonotime();
+ prefix_link(p, NULL, p->pt, peer, 0, asp, comm, state->nexthop,
+ state->nhflags, vstate);
if (p->flags & PREFIX_FLAG_MASK)
fatalx("%s: bad flags %x", __func__, p->flags);
@@ -1282,35 +1264,18 @@ prefix_adjout_withdraw(struct rde_peer *
if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
- /* already a withdraw, shortcut */
- if (p->flags & PREFIX_FLAG_WITHDRAW) {
- p->lastchange = getmonotime();
- p->flags &= ~PREFIX_FLAG_STALE;
- return (0);
- }
+ /* already a withdraw, error */
+ if (p->flags & PREFIX_FLAG_WITHDRAW)
+ log_warnx("%s: prefix already withdrawed", __func__);
/* pending update just got withdrawn */
if (p->flags & PREFIX_FLAG_UPDATE)
RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
+ /* unlink prefix if it was linked (not a withdraw or dead) */
+ if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0)
+ prefix_unlink(p);
+
/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
p->flags &= ~PREFIX_FLAG_MASK;
-
- /* remove nexthop ref ... */
- nexthop_unref(p->nexthop);
- p->nexthop = NULL;
- p->nhflags = 0;
-
- /* unlink from aspath ...*/
- path_unref(p->aspath);
- p->aspath = NULL;
-
- /* ... communities ... */
- communities_unref(p->communities);
- p->communities = NULL;
- /* and unlink from aspath */
- path_unref(p->aspath);
- p->aspath = NULL;
- /* re already NULL */
-
p->lastchange = getmonotime();
p->flags |= PREFIX_FLAG_WITHDRAW;
@@ -1353,34 +1318,25 @@ prefix_adjout_destroy(struct prefix *p)
if (p->flags & PREFIX_FLAG_WITHDRAW)
RB_REMOVE(prefix_tree, &peer->withdraws[p->pt->aid], p);
- else if (p->flags & PREFIX_FLAG_UPDATE)
+ if (p->flags & PREFIX_FLAG_UPDATE)
RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
+ /* unlink prefix if it was linked (not a withdraw or dead) */
+ if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0)
+ prefix_unlink(p);
+
/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
p->flags &= ~PREFIX_FLAG_MASK;
if (prefix_is_locked(p)) {
- /* remove nexthop ref ... */
- nexthop_unref(p->nexthop);
- p->nexthop = NULL;
- /* ... communities ... */
- communities_unref(p->communities);
- p->communities = NULL;
- /* and unlink from aspath */
- path_unref(p->aspath);
- p->aspath = NULL;
- p->nhflags = 0;
- /* re already NULL */
-
- /* finally mark prefix dead */
+ /* mark prefix dead but leave it for prefix_restart */
p->flags |= PREFIX_FLAG_DEAD;
- return;
+ } else {
+ RB_REMOVE(prefix_index, &peer->adj_rib_out, p);
+ /* remove the last prefix reference before free */
+ pt_unref(p->pt);
+ prefix_free(p);
}
-
- RB_REMOVE(prefix_index, &peer->adj_rib_out, p);
-
- prefix_unlink(p);
- prefix_free(p);
}
static void
@@ -1588,31 +1544,24 @@ prefix_destroy(struct prefix *p)
* Link a prefix into the different parent objects.
*/
static void
-prefix_link(struct prefix *p, struct rib_entry *re, struct rde_peer *peer,
- uint32_t path_id, struct rde_aspath *asp, struct rde_community *comm,
- struct nexthop *nexthop, uint8_t nhflags, uint8_t vstate)
+prefix_link(struct prefix *p, struct rib_entry *re, struct pt_entry *pt,
+ struct rde_peer *peer, uint32_t path_id, struct rde_aspath *asp,
+ struct rde_community *comm, struct nexthop *nexthop, uint8_t nhflags,
+ uint8_t vstate)
{
- if (p->flags & PREFIX_FLAG_ADJOUT)
- fatalx("%s: prefix with PREFIX_FLAG_ADJOUT hit", __func__);
- p->entry.list.re = re;
+ if (re)
+ p->entry.list.re = re;
p->aspath = path_ref(asp);
p->communities = communities_ref(comm);
p->peer = peer;
- p->pt = pt_ref(re->prefix);
+ p->pt = pt_ref(pt);
p->path_id = path_id;
p->validation_state = vstate;
p->nhflags = nhflags;
p->nexthop = nexthop_ref(nexthop);
nexthop_link(p);
p->lastchange = getmonotime();
-
- /* add possible pftable reference from aspath */
- if (asp && asp->pftableid)
- rde_pftable_add(asp->pftableid, p);
-
- /* make route decision */
- prefix_evaluate(re, p, NULL);
}
/*
@@ -1624,24 +1573,22 @@ prefix_unlink(struct prefix *p)
struct rib_entry *re = prefix_re(p);
/* destroy all references to other objects */
+ /* remove nexthop ref ... */
nexthop_unlink(p);
nexthop_unref(p->nexthop);
+ p->nexthop = NULL;
+ p->nhflags = 0;
+ /* ... communities ... */
communities_unref(p->communities);
- path_unref(p->aspath);
- pt_unref(p->pt);
p->communities = NULL;
- p->nexthop = NULL;
+ /* and unlink from aspath */
+ path_unref(p->aspath);
p->aspath = NULL;
- p->peer = NULL;
- p->pt = NULL;
if (re && rib_empty(re))
rib_remove(re);
- /*
- * It's the caller's duty to do accounting and remove empty aspath
- * structures. Also freeing the unlinked prefix is the caller's duty.
- */
+ pt_unref(p->pt);
}
/* alloc and zero new entry. May not fail. */
@@ -1857,6 +1804,8 @@ void
nexthop_link(struct prefix *p)
{
if (p->nexthop == NULL)
+ return;
+ if (p->flags & PREFIX_FLAG_ADJOUT)
return;
/* no need to link prefixes in RIBs that have no decision process */
