From owner-freebsd-current@FreeBSD.ORG Mon Apr 12 07:34:08 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id DB77416A4CE; Mon, 12 Apr 2004 07:34:08 -0700 (PDT) Received: from shuttle.wide.toshiba.co.jp (shuttle.wide.toshiba.co.jp [202.249.10.124]) by mx1.FreeBSD.org (Postfix) with ESMTP id D161E43D3F; Mon, 12 Apr 2004 07:34:06 -0700 (PDT) (envelope-from jinmei@isl.rdc.toshiba.co.jp) Received: from ocean.jinmei.org (unknown [2001:200:0:8002:200:39ff:fe5e:cfd7]) by shuttle.wide.toshiba.co.jp (Postfix) with ESMTP id F2DB315214; Mon, 12 Apr 2004 23:34:04 +0900 (JST) Date: Mon, 12 Apr 2004 23:34:04 +0900 Message-ID: From: JINMEI Tatuya / =?ISO-2022-JP?B?GyRCP0BMQEMjOkgbKEI=?= To: Luigi Rizzo In-Reply-To: <20040409042720.A99087@xorpc.icir.org> References: <20040409042720.A99087@xorpc.icir.org> User-Agent: Wanderlust/2.10.1 (Watching The Wheels) Emacs/21.3 Mule/5.0 (SAKAKI) Organization: Research & Development Center, Toshiba Corp., Kawasaki, Japan. MIME-Version: 1.0 (generated by SEMI 1.14.5 - "Awara-Onsen") Content-Type: text/plain; charset=US-ASCII X-Mailman-Approved-At: Tue, 13 Apr 2004 04:50:20 -0700 cc: andre@freebsd.org cc: current@freebsd.org cc: core@kame.net cc: net@freebsd.org cc: ume@freebsd.org Subject: Re: suggested patches for netinet6/ X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Apr 2004 14:34:09 -0000 (cc'ing to core@kame.net) >>>>> On Fri, 9 Apr 2004 04:27:20 -0700, >>>>> Luigi Rizzo 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@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); @@ -1844,11 +1836,8 @@ 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 =================================================================== @@ -653,17 +650,9 @@ 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) ); @@ -744,7 +728,7 @@ 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) { @@ -755,21 +739,25 @@ 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; }