> I did this change for most calloc calls in kroute.c. So now that is
> consistent.
Much better.
> I think the name2id code should not set an error for the empty string. I
> think a lot of the code depends on the mapping of "" to 0.
> The ERANGE error should be transformed to a fatalx() so that the API can't
> fail and has only expected results.
>
> Something to tackle once this is in. At least I think most callers do not
> check for errors. So it would be better to "change" the API.
Agreed. Almost nothing checks what is returned and nothing inspects the
errno.
> > > - if ((gw = rti_info[RTAX_GATEWAY]) != NULL)
> > > - switch (gw->sa_family) {
> > > - case AF_INET:
> > > - if (kr == NULL)
> > > - fatalx("v4 gateway for !v4 dst?!");
> > > -
> > > - if (rtm->rtm_flags & RTF_CONNECTED) {
> > > - kr->r.flags |= F_CONNECTED;
> > > - break;
> > > - }
> >
> > I can't spot where this setting of F_CONNECTED is handled after the
> > refactor. The path doing sa2addr(..., &kl->nexthop, NULL) in the new
> > dispatch_rtmsg_addr() explicitly avoids setting that flag.
> >
> > Perhaps this is part of the switch to using RTF_GATEWAY you mentioned...
>
> Yes, in the new code F_CONNECTED is set when RTF_GATEWAY is unset:
>
> kl->ifindex = rtm->rtm_index;
> if (rtm->rtm_flags & RTF_GATEWAY) {
> switch (sa->sa_family) {
> case AF_LINK:
> kl->flags |= F_CONNECTED;
> break;
> case AF_INET:
> case AF_INET6:
> sa2addr(rti_info[RTAX_GATEWAY], &kl->nexthop, NULL);
> break;
> }
> } else {
> kl->flags |= F_CONNECTED;
> }
>
> RTF_CONNECTED is a strange flag which was added to OpenBSD. It is not
> consistent since it is not used for P2P/loopback routes. I think depending
> on RTF_GATEWAY is a much cleaner approach. Since what we want to know is if a
> IP is directly reachable (F_CONNECTED) or reached via a gateway (RTF_GATEWAY).
Ah, now this makes sense, too. Thanks.
> This is the last version with the changes from above plus one addtional
> fix. In kr_tofull() the priority is checked for RTP_MINE and replaced with
> kr_state.fib_prio so that the `bgpctl show fib` output shows the right
> prio.
Still ok, except
> @@ -1595,7 +1595,8 @@ kr_tofull(struct kroute *kr)
> kf.flags = kr->flags;
> kf.ifindex = kr->ifindex;
> kf.prefixlen = kr->prefixlen;
> - kf.priority = kr->priority;
> + kf.priority = kr->priority == RTP_MINE ?
> + kr_state.fib_prio : kr->priority;
Secondary indent with 4 spaces instead of 5.
>
> return (&kf);
> }
> @@ -1615,7 +1616,8 @@ kr6_tofull(struct kroute6 *kr6)
> kf.flags = kr6->flags;
> kf.ifindex = kr6->ifindex;
> kf.prefixlen = kr6->prefixlen;
> - kf.priority = kr6->priority;
> + kf.priority = kr6->priority == RTP_MINE ?
> + kr_state.fib_prio : kr6->priority;
ditto.
