In bgpd there are a few objects that use reference counts to keep track on
how many things point to them. Those are struct pt_entry, rde_aspath,
rde_communities, and nexthop. The way this reference counting is done and
especially the cleanup is a bit different everywhere. This diff is making
them more similar by calling all functions _ref and _unref also the _unref
function will remove the element once the last reference is dropped.
There are still some internal differences but all in all for the callers
this makes the code look a lot more similar.
OK?
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.474
diff -u -p -r1.474 rde.c
--- rde.c 25 Jun 2019 09:04:42 -0000 1.474
+++ rde.c 25 Jun 2019 09:05:27 -0000
@@ -1353,7 +1353,7 @@ rde_update_dispatch(struct imsg *imsg)
}
/* unlock the previously locked nexthop, it is no longer used */
- (void)nexthop_put(state.nexthop);
+ nexthop_unref(state.nexthop);
state.nexthop = NULL;
if ((pos = rde_get_mp_nexthop(mpp, mplen, aid, &state)) == -1) {
log_peer_warnx(&peer->conf, "bad nlri nexthop");
@@ -1653,7 +1653,7 @@ bad_flags:
op, len);
return (-1);
}
- nexthop_put(state->nexthop); /* just to be sure */
+ nexthop_unref(state->nexthop); /* just to be sure */
state->nexthop = nexthop_get(&nexthop);
break;
case ATTR_MED:
@@ -2015,7 +2015,7 @@ rde_get_mp_nexthop(u_char *data, u_int16
return (-1);
}
- nexthop_put(state->nexthop); /* just to be sure */
+ nexthop_unref(state->nexthop); /* just to be sure */
state->nexthop = nexthop_get(&nexthop);
/* ignore reserved (old SNPA) field as per RFC4760 */
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.218
diff -u -p -r1.218 rde.h
--- rde.h 24 Jun 2019 06:39:49 -0000 1.218
+++ rde.h 25 Jun 2019 09:05:27 -0000
@@ -423,7 +423,7 @@ void communities_copy(struct rde_commun
void communities_clean(struct rde_community *);
static inline struct rde_community *
-communities_get(struct rde_community *comm)
+communities_ref(struct rde_community *comm)
{
if (comm->refcnt == 0)
fatalx("%s: not-referenced community", __func__);
@@ -433,12 +433,12 @@ communities_get(struct rde_community *co
}
static inline void
-communities_put(struct rde_community *comm)
+communities_unref(struct rde_community *comm)
{
if (comm == NULL)
return;
rdemem.comm_refs--;
- if (--comm->refcnt == 1)
+ if (--comm->refcnt == 1) /* last ref is hold internally */
communities_unlink(comm);
}
@@ -460,11 +460,15 @@ int rde_filter_equal(struct filter_hea
void rde_filter_calc_skip_steps(struct filter_head *);
/* rde_prefix.c */
-static inline int
-pt_empty(struct pt_entry *pt)
-{
- return (pt->refcnt == 0);
-}
+void pt_init(void);
+void pt_shutdown(void);
+void pt_getaddr(struct pt_entry *, struct bgpd_addr *);
+struct pt_entry *pt_fill(struct bgpd_addr *, int);
+struct pt_entry *pt_get(struct bgpd_addr *, int);
+struct pt_entry *pt_add(struct bgpd_addr *, int);
+void pt_remove(struct pt_entry *);
+struct pt_entry *pt_lookup(struct bgpd_addr *);
+int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
static inline struct pt_entry *
pt_ref(struct pt_entry *pt)
@@ -475,25 +479,15 @@ pt_ref(struct pt_entry *pt)
return pt;
}
-static inline int
+static inline void
pt_unref(struct pt_entry *pt)
{
if (pt->refcnt == 0)
fatalx("pt_unref: underflow");
- --pt->refcnt;
- return pt_empty(pt);
+ if (--pt->refcnt == 0)
+ pt_remove(pt);
}
-void pt_init(void);
-void pt_shutdown(void);
-void pt_getaddr(struct pt_entry *, struct bgpd_addr *);
-struct pt_entry *pt_fill(struct bgpd_addr *, int);
-struct pt_entry *pt_get(struct bgpd_addr *, int);
-struct pt_entry *pt_add(struct bgpd_addr *, int);
-void pt_remove(struct pt_entry *);
-struct pt_entry *pt_lookup(struct bgpd_addr *);
-int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
-
/* rde_rib.c */
extern u_int16_t rib_size;
extern struct rib_desc *ribs;
@@ -608,7 +602,7 @@ void nexthop_unlink(struct prefix *);
void nexthop_update(struct kroute_nexthop *);
struct nexthop *nexthop_get(struct bgpd_addr *);
struct nexthop *nexthop_ref(struct nexthop *);
-int nexthop_put(struct nexthop *);
+int nexthop_unref(struct nexthop *);
int nexthop_compare(struct nexthop *, struct nexthop *);
/* rde_update.c */
Index: rde_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
retrieving revision 1.120
diff -u -p -r1.120 rde_filter.c
--- rde_filter.c 22 Jun 2019 05:44:05 -0000 1.120
+++ rde_filter.c 25 Jun 2019 09:05:27 -0000
@@ -444,7 +444,7 @@ rde_filterstate_clean(struct filterstate
{
path_clean(&state->aspath);
communities_clean(&state->communities);
- nexthop_put(state->nexthop);
+ nexthop_unref(state->nexthop);
state->nexthop = NULL;
}
@@ -481,7 +481,7 @@ filterset_free(struct filter_set_head *s
pftable_unref(s->action.id);
else if (s->type == ACTION_SET_NEXTHOP &&
bgpd_process == PROC_RDE)
- nexthop_put(s->action.nh);
+ nexthop_unref(s->action.nh);
free(s);
}
}
Index: rde_prefix.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_prefix.c,v
retrieving revision 1.38
diff -u -p -r1.38 rde_prefix.c
--- rde_prefix.c 15 Feb 2019 09:55:21 -0000 1.38
+++ rde_prefix.c 25 Jun 2019 09:05:27 -0000
@@ -188,7 +188,7 @@ pt_add(struct bgpd_addr *prefix, int pre
void
pt_remove(struct pt_entry *pte)
{
- if (!pt_empty(pte))
+ if (pte->refcnt != 0)
fatalx("pt_remove: entry still holds references");
if (RB_REMOVE(pt_tree, &pttable, pte) == NULL)
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.195
diff -u -p -r1.195 rde_rib.c
--- rde_rib.c 22 Jun 2019 05:44:05 -0000 1.195
+++ rde_rib.c 25 Jun 2019 09:05:27 -0000
@@ -62,11 +62,11 @@ struct rib_context {
};
LIST_HEAD(, rib_context) rib_dumps = LIST_HEAD_INITIALIZER(rib_dumps);
-static int prefix_add(struct bgpd_addr *, int, struct rib *,
+static int prefix_add(struct bgpd_addr *, int, struct rib *,
struct rde_peer *, struct rde_aspath *,
struct rde_community *, struct nexthop *,
u_int8_t, u_int8_t);
-static int prefix_move(struct prefix *, struct rde_peer *,
+static int prefix_move(struct prefix *, struct rde_peer *,
struct rde_aspath *, struct rde_community *,
struct nexthop *, u_int8_t, u_int8_t);
@@ -326,8 +326,7 @@ rib_remove(struct rib_entry *re)
/* entry is locked, don't free it. */
return;
- if (pt_unref(re->prefix))
- pt_remove(re->prefix);
+ pt_unref(re->prefix);
if (RB_REMOVE(rib_tree, rib_tree(re_rib(re)), re) == NULL)
log_warnx("rib_remove: remove failed.");
@@ -501,12 +500,6 @@ SIPHASH_KEY pathtablekey;
#define PATH_HASH(x) &pathtable.path_hashtbl[x &
pathtable.path_hashmask]
-static inline int
-path_empty(struct rde_aspath *asp)
-{
- return (asp == NULL || asp->refcnt <= 0);
-}
-
static inline struct rde_aspath *
path_ref(struct rde_aspath *asp)
{
@@ -521,10 +514,14 @@ path_ref(struct rde_aspath *asp)
static inline void
path_unref(struct rde_aspath *asp)
{
+ if (asp == NULL)
+ return;
if ((asp->flags & F_ATTR_LINKED) == 0)
fatalx("%s: unlinked object", __func__);
asp->refcnt--;
rdemem.path_refs--;
+ if (asp->refcnt <= 0)
+ path_unlink(asp);
}
void
@@ -765,8 +762,8 @@ path_unlink(struct rde_aspath *asp)
return;
/* make sure no reference is hold for this rde_aspath */
- if (!path_empty(asp))
- fatalx("%s: still has prefixes", __func__);
+ if (asp->refcnt != 0)
+ fatalx("%s: still holds references", __func__);
LIST_REMOVE(asp, path_l);
asp->flags &= ~F_ATTR_LINKED;
@@ -938,7 +935,7 @@ prefix_move(struct prefix *p, struct rde
np = prefix_alloc();
/* add reference to new AS path and communities */
np->aspath = path_ref(asp);
- np->communities = communities_get(comm);
+ np->communities = communities_ref(comm);
np->peer = peer;
np->pt = p->pt; /* skip refcnt update since ref is moved */
np->re = p->re;
@@ -970,10 +967,9 @@ prefix_move(struct prefix *p, struct rde
/* destroy all references to other objects and free the old prefix */
nexthop_unlink(p);
- nexthop_put(p->nexthop);
- communities_put(p->communities);
- if (oasp)
- path_unref(oasp);
+ nexthop_unref(p->nexthop);
+ communities_unref(p->communities);
+ path_unref(oasp);
p->communities = NULL;
p->nexthop = NULL;
p->aspath = NULL;
@@ -982,10 +978,6 @@ prefix_move(struct prefix *p, struct rde
p->pt = NULL;
prefix_free(p);
- /* destroy old path if empty */
- if (path_empty(oasp))
- path_unlink(oasp);
-
return (0);
}
@@ -1061,28 +1053,22 @@ prefix_withdraw(struct rib *rib, struct
struct bgpd_addr *prefix, int prefixlen)
{
struct prefix *p;
- struct rde_aspath *asp;
p = prefix_get(rib, peer, prefix, prefixlen);
if (p == NULL) /* Got a dummy withdrawn request. */
return (0);
/* unlink from aspath ...*/
- asp = p->aspath;
- if (asp != NULL) {
- path_unref(asp);
- p->aspath = NULL;
- if (path_empty(asp))
- path_unlink(asp);
- }
+ path_unref(p->aspath);
+ p->aspath = NULL;
/* ... communities ... */
- communities_put(p->communities);
+ communities_unref(p->communities);
p->communities = NULL;
/* ... and nexthop but keep the re link */
nexthop_unlink(p);
- nexthop_put(p->nexthop);
+ nexthop_unref(p->nexthop);
p->nexthop = NULL;
p->nhflags = 0;
/* re link still exists */
@@ -1271,14 +1257,8 @@ prefix_updateall(struct prefix *p, enum
void
prefix_destroy(struct prefix *p)
{
- struct rde_aspath *asp;
-
- asp = prefix_aspath(p);
prefix_unlink(p);
prefix_free(p);
-
- if (path_empty(asp))
- path_unlink(asp);
}
/*
@@ -1290,7 +1270,7 @@ prefix_link(struct prefix *p, struct rib
struct nexthop *nexthop, u_int8_t nhflags, u_int8_t vstate)
{
p->aspath = path_ref(asp);
- p->communities = communities_get(comm);
+ p->communities = communities_ref(comm);
p->peer = peer;
p->pt = pt_ref(re->prefix);
p->re = re;
@@ -1321,12 +1301,10 @@ prefix_unlink(struct prefix *p)
/* destroy all references to other objects */
nexthop_unlink(p);
- nexthop_put(p->nexthop);
- communities_put(p->communities);
- if (p->aspath)
- path_unref(p->aspath);
- if (pt_unref(p->pt))
- pt_remove(p->pt);
+ nexthop_unref(p->nexthop);
+ communities_unref(p->communities);
+ path_unref(p->aspath);
+ pt_unref(p->pt);
p->communities = NULL;
p->nexthop = NULL;
p->aspath = NULL;
@@ -1418,7 +1396,7 @@ nexthop_shutdown(void)
nh != NULL; nh = nnh) {
nnh = LIST_NEXT(nh, nexthop_l);
nh->state = NEXTHOP_UNREACH;
- (void)nexthop_put(nh);
+ nexthop_unref(nh);
}
if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i])) {
nh = LIST_FIRST(&nexthoptable.nexthop_hashtbl[i]);
@@ -1481,7 +1459,7 @@ nexthop_update(struct kroute_nexthop *ms
nh->oldstate = nh->state;
if (nh->oldstate == NEXTHOP_LOOKUP)
/* drop reference which was hold during the lookup */
- if (nexthop_put(nh))
+ if (nexthop_unref(nh))
return; /* nh lost last ref, no work left */
if (nh->next_prefix) {
@@ -1546,7 +1524,7 @@ nexthop_modify(struct nexthop *setnh, en
*/
if (aid != setnh->exit_nexthop.aid)
break;
- nexthop_put(*nexthop);
+ nexthop_unref(*nexthop);
*nexthop = nexthop_ref(setnh);
*flags = 0;
break;
@@ -1617,11 +1595,10 @@ nexthop_ref(struct nexthop *nexthop)
}
int
-nexthop_put(struct nexthop *nh)
+nexthop_unref(struct nexthop *nh)
{
if (nh == NULL)
return (0);
-
if (--nh->refcnt > 0)
return (0);
