This is another cleanup round of the route decision process.
This time focusing on prefix_cmp(). Make sure that when using
return (a - b) that the results always fits in an int type.
Also make sure the check of the remote_addr at the end is done
properly. The result is probably the same but this is the same
way it is done in many other places.
Unless I made a mistake the result should still be the same.
--
:wq Claudio
? obj
Index: rde_decide.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
retrieving revision 1.79
diff -u -p -r1.79 rde_decide.c
--- rde_decide.c 13 Jan 2021 11:34:01 -0000 1.79
+++ rde_decide.c 13 Jan 2021 12:08:21 -0000
@@ -113,12 +113,12 @@ prefix_cmp(struct prefix *p1, struct pre
struct rde_peer *peer1, *peer2;
struct attr *a;
u_int32_t p1id, p2id;
- int p1cnt, p2cnt;
+ int p1cnt, p2cnt, i;
if (p1 == NULL)
- return (-1);
+ return -1;
if (p2 == NULL)
- return (1);
+ return 1;
asp1 = prefix_aspath(p1);
asp2 = prefix_aspath(p2);
@@ -127,15 +127,15 @@ prefix_cmp(struct prefix *p1, struct pre
/* pathes with errors are not eligible */
if (asp1 == NULL || asp1->flags & F_ATTR_PARSE_ERR)
- return (-1);
+ return -1;
if (asp2 == NULL || asp2->flags & F_ATTR_PARSE_ERR)
- return (1);
+ return 1;
/* only loop free pathes are eligible */
if (asp1->flags & F_ATTR_LOOP)
- return (-1);
+ return -1;
if (asp2->flags & F_ATTR_LOOP)
- return (1);
+ return 1;
/*
* 1. check if prefix is eligible a.k.a reachable
@@ -144,14 +144,16 @@ prefix_cmp(struct prefix *p1, struct pre
*/
if (prefix_nexthop(p2) != NULL &&
prefix_nexthop(p2)->state != NEXTHOP_REACH)
- return (1);
+ return 1;
if (prefix_nexthop(p1) != NULL &&
prefix_nexthop(p1)->state != NEXTHOP_REACH)
- return (-1);
+ return -1;
/* 2. local preference of prefix, bigger is better */
- if ((asp1->lpref - asp2->lpref) != 0)
- return (asp1->lpref - asp2->lpref);
+ if (asp1->lpref > asp2->lpref)
+ return 1;
+ if (asp1->lpref < asp2->lpref)
+ return -1;
/* 3. aspath count, the shorter the better */
if ((asp2->aspath->ascnt - asp1->aspath->ascnt) != 0)
@@ -161,12 +163,19 @@ prefix_cmp(struct prefix *p1, struct pre
if ((asp2->origin - asp1->origin) != 0)
return (asp2->origin - asp1->origin);
- /* 5. MED decision, only comparable between the same neighboring AS */
- if (rde_decisionflags() & BGPD_FLAG_DECISION_MED_ALWAYS ||
- aspath_neighbor(asp1->aspath) == aspath_neighbor(asp2->aspath))
+ /*
+ * 5. MED decision
+ * Only comparable between the same neighboring AS or if
+ * 'rde med compare always' is set.
+ */
+ if ((rde_decisionflags() & BGPD_FLAG_DECISION_MED_ALWAYS) ||
+ aspath_neighbor(asp1->aspath) == aspath_neighbor(asp2->aspath)) {
/* lowest value wins */
- if ((asp2->med - asp1->med) != 0)
- return (asp2->med - asp1->med);
+ if (asp1->med < asp2->med)
+ return 1;
+ if (asp1->med > asp2->med)
+ return -1;
+ }
/*
* 6. EBGP is cooler than IBGP
@@ -187,8 +196,10 @@ prefix_cmp(struct prefix *p1, struct pre
* a metric that weights a prefix at a very late stage in the
* decision process.
*/
- if ((asp1->weight - asp2->weight) != 0)
- return (asp1->weight - asp2->weight);
+ if (asp1->weight > asp2->weight)
+ return 1;
+ if (asp1->weight < asp2->weight)
+ return -1;
/* 8. nexthop costs. NOT YET -> IGNORE */
@@ -196,9 +207,12 @@ prefix_cmp(struct prefix *p1, struct pre
* 9. older route (more stable) wins but only if route-age
* evaluation is enabled.
*/
- if (rde_decisionflags() & BGPD_FLAG_DECISION_ROUTEAGE)
- if ((p2->lastchange - p1->lastchange) != 0)
- return (p2->lastchange - p1->lastchange);
+ if (rde_decisionflags() & BGPD_FLAG_DECISION_ROUTEAGE) {
+ if (p1->lastchange < p2->lastchange) /* p1 is older */
+ return 1;
+ if (p1->lastchange > p2->lastchange)
+ return -1;
+ }
/* 10. lowest BGP Id wins, use ORIGINATOR_ID if present */
if ((a = attr_optget(asp1, ATTR_ORIGINATOR_ID)) != NULL) {
@@ -211,8 +225,10 @@ prefix_cmp(struct prefix *p1, struct pre
p2id = ntohl(p2id);
} else
p2id = peer2->remote_bgpid;
- if ((p2id - p1id) != 0)
- return (p2id - p1id);
+ if (p1id < p2id)
+ return 1;
+ if (p1id > p2id)
+ return -1;
/* 11. compare CLUSTER_LIST length, shorter is better */
p1cnt = p2cnt = 0;
@@ -224,10 +240,26 @@ prefix_cmp(struct prefix *p1, struct pre
return (p2cnt - p1cnt);
/* 12. lowest peer address wins (IPv4 is better than IPv6) */
- if (memcmp(&peer1->remote_addr, &peer2->remote_addr,
- sizeof(peer1->remote_addr)) != 0)
- return (-memcmp(&peer1->remote_addr, &peer2->remote_addr,
- sizeof(peer1->remote_addr)));
+ if (peer1->remote_addr.aid < peer2->remote_addr.aid)
+ return 1;
+ if (peer1->remote_addr.aid > peer2->remote_addr.aid)
+ return -1;
+ switch (peer1->remote_addr.aid) {
+ case AID_INET:
+ i = memcmp(&peer1->remote_addr.v4, &peer2->remote_addr.v4,
+ sizeof(struct in_addr));
+ break;
+ case AID_INET6:
+ i = memcmp(&peer1->remote_addr.v6, &peer2->remote_addr.v6,
+ sizeof(struct in6_addr));
+ break;
+ default:
+ fatalx("%s: unknown af", __func__);
+ }
+ if (i < 0)
+ return 1;
+ if (i > 0)
+ return -1;
fatalx("Uh, oh a politician in the decision process");
}
