On Thu, Oct 25, 2018 at 09:04:18PM +0200, Denis Fondras wrote:
> On Thu, Oct 25, 2018 at 08:51:30AM +0200, Claudio Jeker wrote:
> > Next step on my quest to make the RIB code better.
> > This changes the following things:
> > - network_flush is now using rib_dump_new to walk the Adj-RIB-In and
> > remove all dynamically added announcements
> > - peer_flush got generalized and is now used also in peer_down.
> > It also uses a rib_dump_new call to walk the Adj-RIB-In and remove
> > all prefixes from a peer but this is done synchronous for now.
> > - peer_flush is now working correctly for AID_UNSPEC.
> > - Change the rib_valid check to continue instead of break out of the loop.
> > The rib table can have holes so the loop needs to continue.
> >
> > This removes the last three places that use the peer -> path -> prefix
> > tailqs so the next step is to remove these and make rde_aspath just an
> > object that is referenced by prefixes. After that adding a proper
> > Adj-RIB-Out should be possible.
> >
>
> Question below.
> Running on prod too :)
>
> > Index: rde_rib.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> > retrieving revision 1.180
> > diff -u -p -r1.180 rde_rib.c
> > --- rde_rib.c 24 Oct 2018 08:26:37 -0000 1.180
> > +++ rde_rib.c 24 Oct 2018 08:44:54 -0000
> > @@ -339,8 +339,8 @@ rib_restart(struct rib_context *ctx)
> > static void
> > rib_dump_r(struct rib_context *ctx)
> > {
> > + struct rib_entry *re, *next;
> > struct rib *rib;
> > - struct rib_entry *re;
> > unsigned int i;
> >
> > rib = rib_byid(ctx->ctx_rib_id);
> > @@ -352,7 +352,8 @@ rib_dump_r(struct rib_context *ctx)
> > else
> > re = rib_restart(ctx);
> >
> > - for (i = 0; re != NULL; re = RB_NEXT(rib_tree, unused, re)) {
> > + for (i = 0; re != NULL; re = next) {
> > + next = RB_NEXT(rib_tree, unused, re);
>
> Why not RB_FOREACH_SAFE ?
Because we do not start with RB_MIN. The inital element is set above and
so we can't use a regular RB_FOREACH loop either.
> > if (re->rib_id != ctx->ctx_rib_id)
> > fatalx("%s: Unexpected RIB %u != %u.", __func__,
> > re->rib_id, ctx->ctx_rib_id);
> > @@ -435,6 +436,10 @@ rib_dump_new(u_int16_t id, u_int8_t aid,
> >
> > LIST_INSERT_HEAD(&rib_dumps, ctx, entry);
> >
> > + /* requested a sync traversal */
> > + if (count == 0)
> > + rib_dump_r(ctx);
> > +
> > return 0;
> > }
> >
> > @@ -664,65 +669,6 @@ path_lookup(struct rde_aspath *aspath, s
> > return (NULL);
> > }
> >
> > -void
> > -path_remove(struct rde_aspath *asp)
> > -{
> > - struct prefix *p, *np;
> > -
> > - for (p = TAILQ_FIRST(&asp->prefixes); p != NULL; p = np) {
> > - np = TAILQ_NEXT(p, path_l);
> > - if (asp->pftableid) {
> > - struct bgpd_addr addr;
> > -
> > - pt_getaddr(p->re->prefix, &addr);
> > - /* Commit is done in peer_down() */
> > - rde_send_pftable(prefix_aspath(p)->pftableid, &addr,
> > - p->re->prefix->prefixlen, 1);
> > - }
> > - prefix_destroy(p);
> > - }
> > -}
> > -
> > -/* remove all stale routes or if staletime is 0 remove all routes for
> > - a specified AID. */
> > -u_int32_t
> > -path_remove_stale(struct rde_aspath *asp, u_int8_t aid, time_t staletime)
> > -{
> > - struct prefix *p, *np;
> > - u_int32_t rprefixes;
> > -
> > - rprefixes=0;
> > - /*
> > - * This is called when a session flapped and during that time
> > - * the pending updates for that peer are getting reset.
> > - */
> > - for (p = TAILQ_FIRST(&asp->prefixes); p != NULL; p = np) {
> > - np = TAILQ_NEXT(p, path_l);
> > - if (p->re->prefix->aid != aid)
> > - continue;
> > -
> > - if (staletime && p->lastchange > staletime)
> > - continue;
> > -
> > - if (asp->pftableid) {
> > - struct bgpd_addr addr;
> > -
> > - pt_getaddr(p->re->prefix, &addr);
> > - /* Commit is done in peer_flush() */
> > - rde_send_pftable(prefix_aspath(p)->pftableid, &addr,
> > - p->re->prefix->prefixlen, 1);
> > - }
> > -
> > - /* only count Adj-RIB-In */
> > - if (re_rib(p->re) == &ribs[RIB_ADJ_IN].rib)
> > - rprefixes++;
> > -
> > - prefix_destroy(p);
> > - }
> > - return (rprefixes);
> > -}
> > -
> > -
> > /*
> > * This function can only called when all prefix have been removed first.
> > * Normally this happens directly out of the prefix removal functions.
> > @@ -1144,29 +1090,6 @@ prefix_destroy(struct prefix *p)
> >
> > if (path_empty(asp))
> > path_destroy(asp);
> > -}
> > -
> > -/*
> > - * helper function to clean up the dynamically added networks
> > - */
> > -void
> > -prefix_network_clean(struct rde_peer *peer)
> > -{
> > - struct rde_aspath *asp, *xasp;
> > - struct prefix *p, *xp;
> > -
> > - for (asp = TAILQ_FIRST(&peer->path_h); asp != NULL; asp = xasp) {
> > - xasp = TAILQ_NEXT(asp, peer_l);
> > - if ((asp->flags & F_ANN_DYNAMIC) != F_ANN_DYNAMIC)
> > - continue;
> > - for (p = TAILQ_FIRST(&asp->prefixes); p != NULL; p = xp) {
> > - xp = TAILQ_NEXT(p, path_l);
> > - prefix_unlink(p);
> > - prefix_free(p);
> > - }
> > - if (path_empty(asp))
> > - path_destroy(asp);
> > - }
> > }
> >
> > /*
> >
>
--
:wq Claudio
