asomers added a comment.
This review is starting to look pretty good. But in addition to the few
things I mentioned inline, there's one other change that you need to make: you
get to clear the `atf_expect_fail` statements from
tests/sys/netinet/fibs_test.sh.
INLINE COMMENTS
> jhujhiti_adjectivism.org wrote in nd6.c:1295
> I'm rethinking this a little bit. While I think it's true that the most
> correct way to answer the question "Is this address a neighbor?" is to
> consider addresses in any FIB, I'm not sure it's necessary. Since the caller
> is specifying which interface this address should be a neighbor on, it's safe
> to consider only that interface's FIB because interface routes (ie., those
> routes with neighbors) will always be added there.
Remember, the interface fib only matters for forwarding packets. It's totally
valid for an interface to have multiple addresses assigned, each of which is on
a different fib. So, to correctly determine whether `addr` is a neighbor of
`ifp`, we must either
1. Loop over all fibs, and check whether `addr` is a neighbor of `ifp` on any
of them, or
2. Loop over all addresses assigned to `ifp`, and check whether `addr` is a
neighbor of `ifp` on that address's fib. I'm guessing that this will be the
slower option, because an interface can have arbitrarily many addresses
> jhujhiti_adjectivism.org wrote in nd6.c:1353
> Should we pass the ifp->if_fib here? We compare ifps on line 1355, so
> searching all FIBs is usually fine, but if we have two identical addresses in
> two different FIBs, we could find the wrong one here and end up returning
> false.
The original code seems too complicated. I think it should go a little like
this (locks elided):
if (ifp->if_flags & IFF_POINTOPOINT) {
TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
if (ifa->ifa_addr->sa_family != addr->sa_family)
continue;
if (ifa->ifa_dstaddr != NULL &&
sa_equal(addr, ifa->ifa_dstaddr)) {
return (1);
}
}
}
No unnecessary looping over either fibs or interfaces.
> nd6.c:1310
> if ((pr->ndpr_stateflags & NDPRF_ONLINK) == 0) {
> /* Always use the default FIB here. */
> dst6 = (const struct sockaddr *)&pr->ndpr_prefix;
This comment is incorrect now.
REPOSITORY
rS FreeBSD src repository
REVISION DETAIL
https://reviews.freebsd.org/D9451
EMAIL PREFERENCES
https://reviews.freebsd.org/settings/panel/emailpreferences/
To: jhujhiti_adjectivism.org, #network, bz, asomers
Cc: jch, bz, imp, ae, freebsd-net-list
_______________________________________________
freebsd-net@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
