On Thu, Jul 28, 2022 at 03:09:18PM +0200, Theo Buehler wrote:
> On Thu, Jul 28, 2022 at 12:48:05PM +0200, Claudio Jeker wrote:
> > Next step on the epic saga of cleaning up kroute.c
> >
> > Refactor kroute_remove() so that a struct kroute_full can be passed to the
> > function. It updates the struct kroute_full with the route that got removed.
> >
> > I split the code into kroute[46]_remove() to make kroute_remove() less
> > cluttered. The return value of those two functions is a bit overloaded but
> > I hope to make this better further down the road.
>
> That's fine for now. An obvious suggestion would be to use an out parameter
> for
> multipath. Let kroute[46]_remove() return -1, 0 (where -1 and 0 have the same
> meaning as -2 and -1 in your diff), otherwise return 1. Then you can do:
>
> case AID_INET:
> ret = kroute4_remove(kt, kf, any, &multipath);
> if (ret <= 0)
> return ret;
> break;
Yes, since I want to tackle multipath routes (and nexthops) as one of the
next steps I think this will hopefully solve itself.
> Diff looks good, some comments inline.
>
> ok tb
>
> >
> > --
> > :wq Claudio
> >
> > Index: kroute.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> > retrieving revision 1.283
> > diff -u -p -r1.283 kroute.c
> > --- kroute.c 27 Jul 2022 17:23:17 -0000 1.283
> > +++ kroute.c 28 Jul 2022 09:20:50 -0000
> > + if ((kr = kroute_find(kt, &kf->prefix, kf->prefixlen,
> > + kf->priority)) == NULL)
> > + return (-1);
> > +
> > + if ((kr->flags & F_BGPD) != (kf->flags & F_BGPD)) {
> > + log_warnx("%s: wrong type for %s/%u", __func__,
> > + log_addr(&kf->prefix), kf->prefixlen);
> > + if (!(kf->flags & F_BGPD))
> > + kr->flags &= ~F_BGPD_INSERTED;
> > return (-1);
> > }
> >
> > + /* get the correct route to remove */
> > + if (any) {
> > + krm = kr;
> > + } else {
>
> I'd probably have done:
>
> krm = kr
> if (!any) {
>
Good idea. Changed.
> > + if ((krm = kroute_matchgw(kr, &kf->nexthop)) == NULL) {
> > + log_warnx("delete %s/%u: route not found",
> > + log_addr(&kf->prefix), kf->prefixlen);
> > + return (-2);
> > + }
> > + }
> > +
> > if (krm == kr) {
> > /* head element */
> > - if (RB_REMOVE(kroute_tree, &kt->krt, kr) == NULL) {
> > - log_warnx("%s: failed for %s/%u", __func__,
> > - inet_ntoa(kr->prefix), kr->prefixlen);
> > - return (-1);
>
> Would it not be easier to initialize multipath to 1 and set it to 0 here
> if kr->next == NULL?
Probably. Let me fix this.
> > + if (multipath < 0)
> > + return (multipath + 1);
> > +
> > /* check whether a nexthop depends on this kroute */
> > - if (kr->flags & F_NEXTHOP)
> > - RB_FOREACH(s, knexthop_tree, KT2KNT(kt))
> > - if (s->kroute == kr)
> > - knexthop_validate(kt, s);
> > + if (kf->flags & F_NEXTHOP)
> > + RB_FOREACH(n, knexthop_tree, KT2KNT(kt))
>
> One or two pairs of braces would help
>
I added some for the first if and the RB_FOREACH().
> > + if (prefix_compare(&kf->prefix, &n->nexthop,
> > + kf->prefixlen) == 0)
> > + knexthop_validate(kt, n);
> >
> > - if (!(kr->flags & F_BGPD) && kr == krm && kr->next == NULL)
> > + if (!((kf->flags & F_BGPD) || multipath))
>
> maybe use De Morgan:
>
> if (!(kf->flags & F_BGPD) && !multipath)
>
I used De Morgan since I started with that. Guess the !a && !b form is
better here.
> > /* again remove only once */
> > - kr_redistribute(IMSG_NETWORK_REMOVE, kt, kr_tofull(kr));
> > + kr_redistribute(IMSG_NETWORK_REMOVE, kt, kf);
> > +
> > + if (kf->flags & F_BGPD_INSERTED)
> > + send_rtmsg(kr_state.fd, RTM_DELETE, kt, kf);
>
> send_rtmsg() is now called later than before. I suspect that's fine, but I'm
> not 100% sure.
I moved it up before the F_NEXTHOP check. At least then the sequence of
rtmsg generated should be in the expected order.
--
:wq Claudio
