Re: suggested patches for netinet6/

(cc'ing to core_at_kame.net)

>>>>> On Fri, 9 Apr 2004 04:27:20 -0700, 
>>>>> Luigi Rizzo <rizzo_at_icir.org> said:

> While adapting to ipv6 the new arp table code I am developing
> following Andre's ideas, i hit a few places that would deserve a
> fix independently of that:

> + nd6_nud_hint() is only called as nd6_nud_hint(NULL, NULL, 0);
>   in some cases from netinet/tcp_input.c
>   With these args, the routine is a NOP. I propose to remove it
>   (and the associated field, ln_byhint, in struct llinfo_nd6)

Hmm, it seems that the first argument was changed to NULL in
rev. 1.216 of tcp_input.c with the introduction of TCP host cache.  We
have been passing a meaningful argument by then, and I suspect the
change was really made with confidence.  So, I guess the correct
approach would be to provide an appropriate value with nd6_nud_hint
with the logic of TCP host cache, rather than to remove it.

(At the same time, however, I'm personally skeptical about the
effectiveness of such a hint from a higher layer to ND.  In that
sense, it may make sense to purge it...)

> + In a couple of places, the logic to compute 'olladdr' and 'llchange'
>   is rather convoluted, and it could be greatly simplified (see below)

I'm not sure if this is that trivial.  In particular, I don't
understand (at least from the patch) why we can replace

  -	olladdr = (sdl->sdl_alen) ? 1 : 0;

with 

 +	olladdr = ln->ln_state >= ND6_LLINFO_REACHABLE;

Could be a bit more specific about the rationale?

> + nd6_output() contains a tail-recursive call which certainly can be
>   removed by using a 'goto' near the beginning of the function
>   (or maybe in a better way!)

Looks okay, but I'm wondering if we also need to reset 'ifp' to
'rt->rt_ifp' before going back.  (As commented, this logic was derived
from ether_output() in 4.4 BSD, so if my guess is correct, it means
the old ether_output() had a bug.)

> Also:
> + in nd6_na_input() we are responding to an nd6 message on interface 'ifp',
>   so i guess in the routine rt->rt_ifp == ifp, and we can use the latter
>   when needed instead of rt->rt_ifp ?

I think you're correct.  According to nd6_nbr.c in the latest KAME
snap, rt->rt_ifp == ifp is assured by the call to nd6_lookup() in
nd6_na_input().
 
> + is it ok to remove the __P() from the header files, ANSIfy
>   the function declarations and make them static as appropriate ?
>   Of course this ought to be done as a separate step.

I myself do not have a strong opinion on this.   However, these files
would also be shared with other BSDs via KAME snaps, and if this
change is not accepted by other BSDs, I'd like to keep it for future
synchronization between KAME and BSDs.

					JINMEI, Tatuya
					Communication Platform Lab.
					Corporate R&D Center, Toshiba Corp.
					jinmei_at_isl.rdc.toshiba.co.jp

> I am attaching the relevant snippets of the diff... Can anyone comment ?
> Any objection if i commit the trivial fixes to the first 3 issues above ?
> What about the style changes ?

> 	cheers
> 	luigi

Index: nd6.c
===================================================================
-	olladdr = (sdl->sdl_alen) ? 1 : 0;
-	if (olladdr && lladdr) {
-		if (bcmp(lladdr, LLADDR(sdl), ifp->if_addrlen))
-			llchange = 1;
-		else
-			llchange = 0;
-	} else
-		llchange = 0;
+	olladdr = ln->ln_state >= ND6_LLINFO_REACHABLE;
+	llchange = olladdr && lladdr &&	bcmp(lladdr, &ln->ll_addr, ifp->if_addrlen);
 
_at__at_ -1844,11 +1836,8 _at__at_ nd6_output(ifp, origifp, m0, dst, rt0)
+again:
        /*
         * next hop determination.  This routine is derived from ether_outpout.
         */
        if (rt) {
                if ((rt->rt_flags & RTF_UP) == 0) {
                        rt0 = rt = rtalloc1((struct sockaddr *)dst, 1, 0UL);
 			if (rt != NULL) {
 				RT_REMREF(rt);
 				RT_UNLOCK(rt);
-				if (rt->rt_ifp != ifp) {
-					/* XXX: loop care? */
-					return nd6_output(ifp, origifp, m0,
-					    dst, rt);
-				}
+				if (rt->rt_ifp != ifp)
+					goto again;	/* rt has changed. */
 			} else
 				senderr(EHOSTUNREACH);
 		}
 
Index: nd6_nbr.c
===================================================================
_at__at_ -653,17 +650,9 _at__at_ nd6_na_input(m, off, icmp6len)
 		/*
 		 * Check if the link-layer address has changed or not.
 		 */
-		if (!lladdr)
-			llchange = 0;
-		else {
-			if (sdl->sdl_alen) {
-				if (bcmp(lladdr, LLADDR(sdl), ifp->if_addrlen))
-					llchange = 1;
-				else
-					llchange = 0;
-			} else
-				llchange = 1;
-		}
+		llchange = lladdr &&
+			( (ln->ln_state < ND6_LLINFO_REACHABLE) ||
+				bcmp(lladdr, &ln->l3_addr, ifp->if_addrlen) );
 


_at__at_ -744,7 +728,7 _at__at_ nd6_na_input(m, off, icmp6len)
 			 * context.  However, we keep it just for safety.
 			 */
 			s = splnet();
-			dr = defrouter_lookup(in6, rt->rt_ifp);
+			dr = defrouter_lookup(&ln->l3_addr, ifp);
 			if (dr)
 				defrtrlist_del(dr);
 			else if (!ip6_forwarding && ip6_accept_rtadv) {
_at__at_ -755,21 +739,25 _at__at_ nd6_na_input(m, off, icmp6len)
 				 * (e.g. redirect case). So we must
 				 * call rt6_flush explicitly.
 				 */
-				rt6_flush(&ip6->ip6_src, rt->rt_ifp);
+				rt6_flush(&ip6->ip6_src, ifp);
 			}
 			splx(s);
 		}
 		ln->ln_router = is_router;
 	}