Re: suggested patches for netinet6/

On Mon, Apr 12, 2004 at 11:34:04PM +0900, JINMEI Tatuya / ?$B?_at_L_at_C#:H?(B wrote:
> (cc'ing to core_at_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