Re: bgpd more kroute refactor
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, );
> 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.c27 Jul 2022 17:23:17 - 1.283
> > +++ kroute.c28 Jul 2022 09:20:50 -
> > + if ((kr = kroute_find(kt, >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(>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, >nexthop)) == NULL) {
> > + log_warnx("delete %s/%u: route not found",
> > + log_addr(>prefix), kf->prefixlen);
> > + return (-2);
> > + }
> > + }
> > +
> > if (krm == kr) {
> > /* head element */
> > - if (RB_REMOVE(kroute_tree, >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(>prefix, >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
Re: bgpd more kroute refactor
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, );
if (ret <= 0)
return ret;
break;
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 - 1.283
> +++ kroute.c 28 Jul 2022 09:20:50 -
> @@ -123,10 +123,6 @@ int kr4_change(struct ktable *, struct k
> int kr6_change(struct ktable *, struct kroute_full *);
> int krVPN4_change(struct ktable *, struct kroute_full *);
> int krVPN6_change(struct ktable *, struct kroute_full *);
> -int kr4_delete(struct ktable *, struct kroute_full *);
> -int kr6_delete(struct ktable *, struct kroute_full *);
> -int krVPN4_delete(struct ktable *, struct kroute_full *);
> -int krVPN6_delete(struct ktable *, struct kroute_full *);
> int kr_net_match(struct ktable *, struct network_config *, uint16_t, int);
> struct network *kr_net_find(struct ktable *, struct network *);
> void kr_net_clear(struct ktable *);
> @@ -144,18 +140,17 @@ struct kroute *kroute_find(struct ktable
> uint8_t, uint8_t);
> struct kroute*kroute_matchgw(struct kroute *, struct bgpd_addr *);
> int kroute_insert(struct ktable *, struct kroute_full *);
> -int kroute_remove(struct ktable *, struct kroute *);
> +int kroute_remove(struct ktable *, struct kroute_full *, int);
> void kroute_clear(struct ktable *);
>
> struct kroute6 *kroute6_find(struct ktable *, const struct bgpd_addr *,
> uint8_t, uint8_t);
> struct kroute6 *kroute6_matchgw(struct kroute6 *, struct bgpd_addr *);
> -int kroute6_remove(struct ktable *, struct kroute6 *);
> void kroute6_clear(struct ktable *);
>
> struct knexthop *knexthop_find(struct ktable *, struct bgpd_addr *);
> int knexthop_insert(struct ktable *, struct knexthop *);
> -int knexthop_remove(struct ktable *, struct knexthop *);
> +void knexthop_remove(struct ktable *, struct knexthop *);
> void knexthop_clear(struct ktable *);
>
> struct kif_node *kif_find(int);
> @@ -662,18 +657,7 @@ kr_delete(u_int rtableid, struct kroute_
> return (0);
> kf->flags |= F_BGPD;
> kf->priority = RTP_MINE;
> - switch (kf->prefix.aid) {
> - case AID_INET:
> - return (kr4_delete(kt, kf));
> - case AID_INET6:
> - return (kr6_delete(kt, kf));
> - case AID_VPN_IPv4:
> - return (krVPN4_delete(kt, kf));
> - case AID_VPN_IPv6:
> - return (krVPN6_delete(kt, kf));
> - }
> - log_warnx("%s: not handled AID", __func__);
> - return (-1);
> + return kroute_remove(kt, kf, 1);
> }
>
> int
> @@ -689,18 +673,12 @@ kr_flush(u_int rtableid)
>
> RB_FOREACH_SAFE(kr, kroute_tree, >krt, next)
> if ((kr->flags & F_BGPD_INSERTED)) {
> - if (kt->fib_sync) /* coupled */
> - send_rtmsg(kr_state.fd, RTM_DELETE, kt,
> - kr_tofull(kr));
> - if (kroute_remove(kt, kr) == -1)
> + if (kroute_remove(kt, kr_tofull(kr), 1) == -1)
> return (-1);
> }
> RB_FOREACH_SAFE(kr6, kroute6_tree, >krt6, next6)
> if ((kr6->flags & F_BGPD_INSERTED)) {
> - if (kt->fib_sync) /* coupled */
> - send_rtmsg(kr_state.fd, RTM_DELETE, kt,
> - kr6_tofull(kr6));
> - if (kroute6_remove(kt, kr6) == -1)
> + if (kroute_remove(kt, kr6_tofull(kr6), 1) == -1)
> return (-1);
> }
>
> @@ -708,86 +686,6 @@ kr_flush(u_int rtableid)
> return (0);
> }
>
> -int
> -kr4_delete(struct ktable *kt, struct kroute_full *kf)
> -{
> - struct kroute *kr;
> -
> - if ((kr = kroute_find(kt, >prefix, kf->prefixlen,
> -
bgpd more kroute refactor
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.
--
: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.c27 Jul 2022 17:23:17 - 1.283
+++ kroute.c28 Jul 2022 09:20:50 -
@@ -123,10 +123,6 @@ intkr4_change(struct ktable *, struct k
intkr6_change(struct ktable *, struct kroute_full *);
intkrVPN4_change(struct ktable *, struct kroute_full *);
intkrVPN6_change(struct ktable *, struct kroute_full *);
-intkr4_delete(struct ktable *, struct kroute_full *);
-intkr6_delete(struct ktable *, struct kroute_full *);
-intkrVPN4_delete(struct ktable *, struct kroute_full *);
-intkrVPN6_delete(struct ktable *, struct kroute_full *);
intkr_net_match(struct ktable *, struct network_config *, uint16_t, int);
struct network *kr_net_find(struct ktable *, struct network *);
void kr_net_clear(struct ktable *);
@@ -144,18 +140,17 @@ struct kroute *kroute_find(struct ktable
uint8_t, uint8_t);
struct kroute *kroute_matchgw(struct kroute *, struct bgpd_addr *);
int kroute_insert(struct ktable *, struct kroute_full *);
-int kroute_remove(struct ktable *, struct kroute *);
+int kroute_remove(struct ktable *, struct kroute_full *, int);
voidkroute_clear(struct ktable *);
struct kroute6 *kroute6_find(struct ktable *, const struct bgpd_addr *,
uint8_t, uint8_t);
struct kroute6 *kroute6_matchgw(struct kroute6 *, struct bgpd_addr *);
-int kroute6_remove(struct ktable *, struct kroute6 *);
voidkroute6_clear(struct ktable *);
struct knexthop*knexthop_find(struct ktable *, struct bgpd_addr *);
int knexthop_insert(struct ktable *, struct knexthop *);
-int knexthop_remove(struct ktable *, struct knexthop *);
+voidknexthop_remove(struct ktable *, struct knexthop *);
voidknexthop_clear(struct ktable *);
struct kif_node*kif_find(int);
@@ -662,18 +657,7 @@ kr_delete(u_int rtableid, struct kroute_
return (0);
kf->flags |= F_BGPD;
kf->priority = RTP_MINE;
- switch (kf->prefix.aid) {
- case AID_INET:
- return (kr4_delete(kt, kf));
- case AID_INET6:
- return (kr6_delete(kt, kf));
- case AID_VPN_IPv4:
- return (krVPN4_delete(kt, kf));
- case AID_VPN_IPv6:
- return (krVPN6_delete(kt, kf));
- }
- log_warnx("%s: not handled AID", __func__);
- return (-1);
+ return kroute_remove(kt, kf, 1);
}
int
@@ -689,18 +673,12 @@ kr_flush(u_int rtableid)
RB_FOREACH_SAFE(kr, kroute_tree, >krt, next)
if ((kr->flags & F_BGPD_INSERTED)) {
- if (kt->fib_sync) /* coupled */
- send_rtmsg(kr_state.fd, RTM_DELETE, kt,
- kr_tofull(kr));
- if (kroute_remove(kt, kr) == -1)
+ if (kroute_remove(kt, kr_tofull(kr), 1) == -1)
return (-1);
}
RB_FOREACH_SAFE(kr6, kroute6_tree, >krt6, next6)
if ((kr6->flags & F_BGPD_INSERTED)) {
- if (kt->fib_sync) /* coupled */
- send_rtmsg(kr_state.fd, RTM_DELETE, kt,
- kr6_tofull(kr6));
- if (kroute6_remove(kt, kr6) == -1)
+ if (kroute_remove(kt, kr6_tofull(kr6), 1) == -1)
return (-1);
}
@@ -708,86 +686,6 @@ kr_flush(u_int rtableid)
return (0);
}
-int
-kr4_delete(struct ktable *kt, struct kroute_full *kf)
-{
- struct kroute *kr;
-
- if ((kr = kroute_find(kt, >prefix, kf->prefixlen,
- kf->priority)) == NULL)
- return (0);
-
- if (!(kr->flags & F_BGPD_INSERTED))
- return (0);
-
- send_rtmsg(kr_state.fd, RTM_DELETE, kt, kr_tofull(kr));
-
- if (kroute_remove(kt, kr) == -1)
- return (-1);
-
- return (0);
-}
-
-int
-kr6_delete(struct ktable *kt, struct kroute_full *kf)
-{
- struct kroute6 *kr6;
-
- if ((kr6 = kroute6_find(kt, >prefix, kf->prefixlen,
- kf->priority)) == NULL)
- return (0);
-
- if (!(kr6->flags & F_BGPD_INSERTED))
- return (0);
-
-
