On Fri, Dec 28, 2018 at 06:08:16PM +0100, Klemens Nanni wrote:
> On Fri, Dec 28, 2018 at 05:21:02PM +0100, Denis Fondras wrote:
> > int
> > +krVPN6_change(struct ktable *kt, struct kroute_full *kl, u_int8_t fib_prio)
> > +{
> > + struct kroute6_node *kr6;
> > + struct in6_addr lo6 = IN6ADDR_LOOPBACK_INIT;
> > + int action = RTM_ADD;
> > + u_int32_t mplslabel = 0;
> > + u_int16_t labelid;
> > +
> > + if ((kr6 = kroute6_find(kt, &kl->prefix.vpn6.addr, kl->prefixlen,
> > + fib_prio)) != NULL)
> > + action = RTM_CHANGE;
> Can this be moved below the conditional returns or does kroute6_find()
> have side effects? `actions' is not used until much later.
>
> > + /* nexthop to loopback -> ignore silently */
> > + if (IN6_IS_ADDR_LOOPBACK(&kl->nexthop.v6))
> > + return (0);
> > +
> > + /* only single MPLS label are supported for now */
> > + if (kl->prefix.vpn6.labellen != 3) {
> > + log_warnx("%s: %s/%u has not a single label", __func__,
> > + log_addr(&kl->prefix), kl->prefixlen);
> > + return (0);
> > + }
> Here.
>
> > + mplslabel = (kl->prefix.vpn6.labelstack[0] << 24) |
> > + (kl->prefix.vpn6.labelstack[1] << 16) |
> > + (kl->prefix.vpn6.labelstack[2] << 8);
> > + mplslabel = htonl(mplslabel);
> > +
> > + /* for blackhole and reject routes nexthop needs to be ::1 */
> > + if (kl->flags & (F_BLACKHOLE|F_REJECT))
> > + bcopy(&lo6, &kl->nexthop.v6, sizeof(kl->nexthop.v6));
> > +
> > + labelid = rtlabel_name2id(kl->label);
> Or even here right before it's used. `kr6' is not used above.
>
> > + if (action == RTM_ADD) {
Yes, we can, it may save a few cycles. Here is a diff to update the current
revision (I updated my MPLS diff with similar change).
Index: kroute.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.226
diff -u -p -r1.226 kroute.c
--- kroute.c 6 Dec 2018 13:04:40 -0000 1.226
+++ kroute.c 28 Dec 2018 18:46:07 -0000
@@ -487,10 +487,6 @@ kr4_change(struct ktable *kt, struct kro
int action = RTM_ADD;
u_int16_t labelid;
- if ((kr = kroute_find(kt, kl->prefix.v4.s_addr, kl->prefixlen,
- fib_prio)) != NULL)
- action = RTM_CHANGE;
-
/* for blackhole and reject routes nexthop needs to be 127.0.0.1 */
if (kl->flags & (F_BLACKHOLE|F_REJECT))
kl->nexthop.v4.s_addr = htonl(INADDR_LOOPBACK);
@@ -501,6 +497,10 @@ kr4_change(struct ktable *kt, struct kro
labelid = rtlabel_name2id(kl->label);
+ if ((kr = kroute_find(kt, kl->prefix.v4.s_addr, kl->prefixlen,
+ fib_prio)) != NULL)
+ action = RTM_CHANGE;
+
if (action == RTM_ADD) {
if ((kr = calloc(1, sizeof(struct kroute_node))) == NULL) {
log_warn("%s", __func__);
@@ -545,10 +545,6 @@ kr6_change(struct ktable *kt, struct kro
int action = RTM_ADD;
u_int16_t labelid;
- if ((kr6 = kroute6_find(kt, &kl->prefix.v6, kl->prefixlen, fib_prio)) !=
- NULL)
- action = RTM_CHANGE;
-
/* for blackhole and reject routes nexthop needs to be ::1 */
if (kl->flags & (F_BLACKHOLE|F_REJECT))
bcopy(&lo6, &kl->nexthop.v6, sizeof(kl->nexthop.v6));
@@ -558,6 +554,10 @@ kr6_change(struct ktable *kt, struct kro
labelid = rtlabel_name2id(kl->label);
+ if ((kr6 = kroute6_find(kt, &kl->prefix.v6, kl->prefixlen, fib_prio)) !=
+ NULL)
+ action = RTM_CHANGE;
+
if (action == RTM_ADD) {
if ((kr6 = calloc(1, sizeof(struct kroute6_node))) == NULL) {
log_warn("%s", __func__);
@@ -604,10 +604,6 @@ krVPN4_change(struct ktable *kt, struct
u_int32_t mplslabel = 0;
u_int16_t labelid;
- if ((kr = kroute_find(kt, kl->prefix.vpn4.addr.s_addr, kl->prefixlen,
- fib_prio)) != NULL)
- action = RTM_CHANGE;
-
/* nexthop within 127/8 -> ignore silently */
if ((kl->nexthop.v4.s_addr & htonl(IN_CLASSA_NET)) ==
htonl(INADDR_LOOPBACK & IN_CLASSA_NET))
@@ -629,6 +625,10 @@ krVPN4_change(struct ktable *kt, struct
/* for blackhole and reject routes nexthop needs to be 127.0.0.1 */
if (kl->flags & (F_BLACKHOLE|F_REJECT))
kl->nexthop.v4.s_addr = htonl(INADDR_LOOPBACK);
+
+ if ((kr = kroute_find(kt, kl->prefix.vpn4.addr.s_addr, kl->prefixlen,
+ fib_prio)) != NULL)
+ action = RTM_CHANGE;
if (action == RTM_ADD) {
if ((kr = calloc(1, sizeof(struct kroute_node))) == NULL) {
