Re: bgpd: check max prefix just once

[Claudio Jeker]

On Mon, Jul 09, 2018 at 05:50:07PM +0200, Denis Fondras wrote:
> I am late for a comment because it has already been commited but...
> 
> > @@ -1373,6 +1339,14 @@ rde_update_update(struct rde_peer *peer,
> > if (path_update([RIB_ADJ_IN].rib, peer, asp, prefix, prefixlen, 0))
> > peer->prefix_cnt++;
> >  
> > +   /* max prefix checker */
> > +   if (peer->conf.max_prefix && peer->prefix_cnt > peer->conf.max_prefix) {
> > +   log_peer_warnx(>conf, "prefix limit reached (>%u/%u)",
> 
> Is it useful to display peer->prefix_cnt here ? I think it will always be
> peer->conf.max_prefix+1
> 
> > +   peer->prefix_cnt, peer->conf.max_prefix);
> > +   rde_update_err(peer, ERR_CEASE, ERR_CEASE_MAX_PREFIX, NULL, 0);
> > +   return (-1);
> > +   }
> > +
> > p = prefix_get([RIB_ADJ_IN].rib, peer, prefix, prefixlen, 0);
> > if (p == NULL)
> > fatalx("rde_update_update: no prefix in Adj-RIB-In");
> 

If you change max-prefix and reload then it is possible to be different.
I agree that the message could be better. Lucky us we only need to adjust
it once now :)

-- 
:wq Claudio

Re: bgpd: check max prefix just once

[Denis Fondras]

I am late for a comment because it has already been commited but...

> @@ -1373,6 +1339,14 @@ rde_update_update(struct rde_peer *peer,
>   if (path_update([RIB_ADJ_IN].rib, peer, asp, prefix, prefixlen, 0))
>   peer->prefix_cnt++;
>  
> + /* max prefix checker */
> + if (peer->conf.max_prefix && peer->prefix_cnt > peer->conf.max_prefix) {
> + log_peer_warnx(>conf, "prefix limit reached (>%u/%u)",

Is it useful to display peer->prefix_cnt here ? I think it will always be
peer->conf.max_prefix+1

> + peer->prefix_cnt, peer->conf.max_prefix);
> + rde_update_err(peer, ERR_CEASE, ERR_CEASE_MAX_PREFIX, NULL, 0);
> + return (-1);
> + }
> +
>   p = prefix_get([RIB_ADJ_IN].rib, peer, prefix, prefixlen, 0);
>   if (p == NULL)
>   fatalx("rde_update_update: no prefix in Adj-RIB-In");

bgpd: check max prefix just once

[Claudio Jeker]

No need to duplicate the same block over and over again.
Just check it when we increment the counter and return failure if we hit
the limit.

OK?
-- 
:wq Claudio

Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.384
diff -u -p -r1.384 rde.c
--- rde.c   5 Jul 2018 10:25:26 -   1.384
+++ rde.c   9 Jul 2018 12:47:16 -
@@ -51,7 +51,7 @@ void   rde_sighdlr(int);
 voidrde_dispatch_imsg_session(struct imsgbuf *);
 voidrde_dispatch_imsg_parent(struct imsgbuf *);
 int rde_update_dispatch(struct imsg *);
-voidrde_update_update(struct rde_peer *, struct rde_aspath *,
+int rde_update_update(struct rde_peer *, struct rde_aspath *,
 struct bgpd_addr *, u_int8_t);
 voidrde_update_withdraw(struct rde_peer *, struct bgpd_addr *,
 u_int8_t);
@@ -1208,18 +1208,8 @@ rde_update_dispatch(struct imsg *imsg)
goto done;
}
 
-   rde_update_update(peer, asp, , prefixlen);
-
-   /* max prefix checker */
-   if (peer->conf.max_prefix &&
-   peer->prefix_cnt > peer->conf.max_prefix) {
-   log_peer_warnx(>conf, "prefix limit reached"
-   " (>%u/%u)", peer->prefix_cnt,
-   peer->conf.max_prefix);
-   rde_update_err(peer, ERR_CEASE, ERR_CEASE_MAX_PREFIX,
-   NULL, 0);
+   if (rde_update_update(peer, asp, , prefixlen) == -1)
goto done;
-   }
 
}
 
@@ -1289,21 +1279,9 @@ rde_update_dispatch(struct imsg *imsg)
mpp += pos;
mplen -= pos;
 
-   rde_update_update(peer, asp, ,
-   prefixlen);
-
-   /* max prefix checker */
-   if (peer->conf.max_prefix &&
-   peer->prefix_cnt > peer->conf.max_prefix) {
-   log_peer_warnx(>conf,
-   "prefix limit reached"
-   " (>%u/%u)", peer->prefix_cnt,
-   peer->conf.max_prefix);
-   rde_update_err(peer, ERR_CEASE,
-   ERR_CEASE_MAX_PREFIX, NULL, 0);
+   if (rde_update_update(peer, asp, ,
+   prefixlen) == -1)
goto done;
-   }
-
}
break;
case AID_VPN_IPv4:
@@ -1327,21 +1305,9 @@ rde_update_dispatch(struct imsg *imsg)
mpp += pos;
mplen -= pos;
 
-   rde_update_update(peer, asp, ,
-   prefixlen);
-
-   /* max prefix checker */
-   if (peer->conf.max_prefix &&
-   peer->prefix_cnt > peer->conf.max_prefix) {
-   log_peer_warnx(>conf,
-   "prefix limit reached"
-   " (>%u/%u)", peer->prefix_cnt,
-   peer->conf.max_prefix);
-   rde_update_err(peer, ERR_CEASE,
-   ERR_CEASE_MAX_PREFIX, NULL, 0);
+   if (rde_update_update(peer, asp, ,
+   prefixlen) == -1)
goto done;
-   }
-
}
break;
default:
@@ -1359,7 +1325,7 @@ done:
return (error);
 }
 
-void
+int
 rde_update_update(struct rde_peer *peer, struct rde_aspath *asp,
 struct bgpd_addr *prefix, u_int8_t prefixlen)
 {
@@ -1373,6 +1339,14 @@ rde_update_update(struct rde_peer *peer,
if (path_update([RIB_ADJ_IN].rib, peer, asp, prefix, prefixlen, 0))
peer->prefix_cnt++;
 
+   /* max prefix checker */
+   if (peer->conf.max_prefix && peer->prefix_cnt > peer->conf.max_prefix) {
+   log_peer_warnx(>conf, "prefix limit reached (>%u/%u)",
+   peer->prefix_cnt, peer->conf.max_prefix);
+   rde_update_err(peer, ERR_CEASE, ERR_CEASE_MAX_PREFIX, NULL, 0);
+   return (-1);
+   }
+
p = prefix_get([RIB_ADJ_IN].rib, peer, prefix, prefixlen, 0);
if (p == NULL)
fatalx("rde_update_update: no prefix in Adj-RIB-In");
@@ -1401,6