On 24/05/16(Tue) 00:19, Alexander Bluhm wrote:
> On Mon, May 23, 2016 at 04:55:06PM +0200, Martin Pieuchot wrote:
> > @@ -1532,42 +1527,17 @@ nd6_output(struct ifnet *ifp, struct mbu
> ...
> > - /*
> > - * Since nd6_is_addr_neighbor() internally calls nd6_lookup(),
> > - * the condition below is not very efficient. But we believe
> > - * it is tolerable, because this should be a rare case.
> > - */
> > - if (nd6_is_addr_neighbor(dst, ifp)) {
> > - rt = nd6_lookup(&dst->sin6_addr, 1, ifp,
> > - ifp->if_rdomain);
> > - if (rt != NULL) {
> > - created = 1;
> > - ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> > - }
> > - }
>
> I cannot see where the nd6_lookup() has moved to. Where does it
> happen now?
It is hidden inside the RT_RESOLVE flag of the rtalloc(9) calls that
happen before nd6_output().
> > + if (!ISSET(rt->rt_flags, RTF_LLINFO)) {
> > + char addr[INET6_ADDRSTRLEN];
> > + log(LOG_DEBUG, "%s: %s: route contains no ND information\n",
> > + __func__, inet_ntop(AF_INET6,
> > + &satosin6(rt_key(rt))->sin6_addr, addr, sizeof(addr)));
> > + m_freem(m);
> > + return (EINVAL);
> > }
>
> I think EINVAL is for invalid input from user land. Would EHOSTUNREACH
> be more appropriate?
Well this case should hopefully be turned into a KASSERT() and the log()
removed. I picked EINVAL because it is the value used in arpresolve().
I don't mind changing the error value, but keep in mind that this is a
programing bug, not something that should be reported.
> There should be more real world testing with the ether_output() and
> nd6_output() part. Can you commit pf and send the diff that assumes
> rt != NULL for v4 and v6 output?
Sure.
