On Wed, Jun 08, 2022 at 06:10:21PM +0200, Theo Buehler wrote:
> On Wed, Jun 08, 2022 at 01:28:14PM +0200, Claudio Jeker wrote:
> > The idea behind this diff is to use struct kroute_full in more places.
> > Mainly when parsing route messages. This allows to factor out the pure
> > route message parsing into dispatch_rtmsg_addr() and use it in both the
> > gerneral routing socket code but also in fetchtables(). Which removes some
> > duplicated code.
> >
> > As a second step then this kroute_full is applied to the kroute tree. Here
> > I limited my changes to be minimal for now. A follow up will look more
> > into kr_fib_change() and tries to streamline that code and the bits in
> > fetchtable().
> >
> > Also struct kroute_full could be used in more places with the goal to have
> > less mostly similar function for IPv4, IPv6 and the L3VPNs. Again this is
> > for later.
> >
> > This should behave the same as before but dispatch_rtmsg_addr() decides
> > now in a different way if a route is connected or not (using the
> > RTF_GATEWAY flag for this). With this loopback routes show now up as
> > connected. It should have no noticable impact on routing.
> >
> > Please give this a good test.
>
> I spent some time with this diff. This is outside my comfort zone. I'll
> need to read this once more with a fresh head.
>
> I like the direction and less spaghetti is good.
>
> Some small remarks and questions inline.
Thanks for looking into this. Not many people around that actually
understand the route socket so I know this is way out of everyones comfort
zone. The kroute code got hammered into shape and then copied around way
to many times. A lot of the code is pure spahetti. I try to work with
smaller diffs to make review somewhat possible.
> > Index: kroute.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> > retrieving revision 1.251
> > diff -u -p -r1.251 kroute.c
> > --- kroute.c 7 Jun 2022 16:42:07 -0000 1.251
> > +++ kroute.c 7 Jun 2022 17:05:37 -0000
> > @@ -3482,23 +3385,34 @@ dispatch_rtmsg(u_int rdomain)
> > case RTM_ADD:
> > case RTM_CHANGE:
> > case RTM_DELETE:
> > - sa = (struct sockaddr *)(next + rtm->rtm_hdrlen);
> > - get_rtaddrs(rtm->rtm_addrs, sa, rti_info);
> > -
> > if (rtm->rtm_pid == kr_state.pid) /* cause by us */
> > continue;
> >
> > - if (rtm->rtm_errno) /* failed attempts */
> > - continue;
> > + /* failed attempts */
> > + if (rtm->rtm_errno || !(rtm->rtm_flags & RTF_DONE))
> > + return (-1);
> >
> > - if (rtm->rtm_flags & RTF_LLINFO) /* arp cache */
> > + if ((kt = ktable_get(rtm->rtm_tableid)) == NULL)
> > continue;
> >
> > - if ((kt = ktable_get(rtm->rtm_tableid)) == NULL)
> > + if (dispatch_rtmsg_addr(rtm, &kl) == -1)
> > continue;
> >
> > - if (dispatch_rtmsg_addr(rtm, rti_info, kt) == -1)
> > - return (-1);
> > + if (rtm->rtm_flags & RTF_MPATH)
> > + mpath = 1;
>
> I guess the #ifdef RTF_MPATH was dropped since kroute.c is no longer
> built outside of OpenBSD with recent -portable changes?
Yes, also the time when OpenBSD had no RTF_MPATH has long passed. So
I see no reason to clutter this code with #ifdefs.
For portable I would love to split the code further up so that the kroute
code is portable but the routing socket bits are per platform. Right now
changes to kroute.c are a pain since I need to track also the protable
files.
> > @@ -3517,191 +3431,190 @@ dispatch_rtmsg(u_int rdomain)
> > }
> >
> > int
> > -dispatch_rtmsg_addr(struct rt_msghdr *rtm, struct sockaddr
> > *rti_info[RTAX_MAX],
> > - struct ktable *kt)
> > +dispatch_rtmsg_addr(struct rt_msghdr *rtm, struct kroute_full *kr)
>
> This is the only kroute_full called kr in the entire file. I would
> prefer calling it kl (or kf as in the older code you don't touch).
Naming is hard. I agree using kl may be better here. Will adjust the code.
> > case AF_INET:
> > - prefix.aid = AID_INET;
> > - prefix.v4.s_addr = ((struct sockaddr_in *)sa)->sin_addr.s_addr;
> > sa_in = (struct sockaddr_in *)rti_info[RTAX_NETMASK];
> > if (sa_in != NULL) {
> > if (sa_in->sin_len != 0)
> > - prefixlen = mask2prefixlen(
> > + kr->prefixlen = mask2prefixlen(
> > sa_in->sin_addr.s_addr);
>
> I would wrap this the same way as the assignment a few lines down:
>
> kr->prefixlen =
> mask2prefixlen(sa_in->sin_addr.s_addr);
Yes, indeed.
> > case AID_INET:
> > - sa_in = (struct sockaddr_in *)sa;
> > - if ((kr = kroute_find(kt, prefix.v4.s_addr, prefixlen,
> > - prio)) != NULL) {
> > + if ((kr = kroute_find(kt, kl->prefix.v4.s_addr, kl->prefixlen,
> > + kl->priority)) != NULL) {
> > if (kr->r.flags & F_KERNEL) {
> > /* get the correct route */
> > - if (mpath && rtm->rtm_type == RTM_CHANGE &&
> > - (kr = kroute_matchgw(kr, sa_in)) == NULL) {
> > + if (mpath && type == RTM_CHANGE &&
> > + (kr = kroute_matchgw(kr, &kl->nexthop)) ==
> > + NULL) {
> > log_warnx("%s[change]: "
> > "mpath route not found", __func__);
> > goto add4;
> > - } else if (mpath && rtm->rtm_type == RTM_ADD)
> > + } else if (mpath && type == RTM_ADD)
> > goto add4;
> >
> > - if (sa_in != NULL) {
> > + if (kl->nexthop.aid == AID_INET) {
> > if (kr->r.nexthop.s_addr !=
> > - sa_in->sin_addr.s_addr)
> > + kl->nexthop.v4.s_addr)
> > changed = 1;
> > kr->r.nexthop.s_addr =
> > - sa_in->sin_addr.s_addr;
> > + kl->nexthop.v4.s_addr;
>
> why is there no kr->r.ifindex assignment here?
I don't know. The old code was already like this. I remember that ifindex
was unreliable in the old times but again that is like 5+ years ago.
I did not want to introduce code changes which I'm sure wont cause
trouble. I think this is something to tackle at a later stage.
--
:wq Claudio
