Date: Mon, 12 Apr 2004 07:56:38 -0700 From: Luigi Rizzo <rizzo@icir.org> To: "JINMEI Tatuya / ?$B?@L@C#:H?(B" <jinmei@isl.rdc.toshiba.co.jp> Cc: net@freebsd.org Subject: Re: suggested patches for netinet6/ Message-ID: <20040412075638.B67293@xorpc.icir.org> In-Reply-To: <y7v7jwlw9ib.wl@ocean.jinmei.org>; from jinmei@isl.rdc.toshiba.co.jp on Mon, Apr 12, 2004 at 11:34:04PM %2B0900 References: <20040409042720.A99087@xorpc.icir.org> <y7v7jwlw9ib.wl@ocean.jinmei.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Apr 12, 2004 at 11:34:04PM +0900, JINMEI Tatuya / ?$B?@L@C#:H?(B wrote: > (cc'ing to core@kame.net) [thanks for the cc] > > + 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) ... > (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...) not knowing much about ipv6 i cannot comment, however if this is just a performance optimization it can be [re]introduced later. > > + 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; sorry, that included a bit from my new arp code (basically, if you have a MAC address stored for the node then both sdl->sdl_alen !=0 and ln->ln_state >= ND6_LLINFO_REACHABLE. The second form is more appealing to me because with the new arp code there are no more host route entries generated by cloning, and the MAC address resides elsewhere...) Leave olladdr as it was, the rest of the patch just tried to replace the conditional statements with boolean expressions. > > > + 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(). ok thanks... once again using ifp eases my task because the new arp code does not have route entries associated with MAC addresses so we need to grab the relevant info elsewhere. > > + 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. ok, I am just unclear if we periodically import KAME sources in the tree and then reapply freebsd changes (trying to keep the latter as small as possible) or someone from time to time looks at relevant changes in the KAME tree and patches the freebsd version accordingly. In the latter case, ANSIfying the code would have little impact on the people porting back the patches, yet would help a lot in using stricter compiler checks. cheers luigi
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040412075638.B67293>