Re: suggested patches for netinet6/

>>>>> On Mon, 12 Apr 2004 07:56:38 -0700, 
>>>>> Luigi Rizzo <rizzo_at_icir.org> said:

>> > + 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. 

This is also a protocol compliance issue, since RFC2461 clearly
mentions the use of such a hint (though the requirement level is not
very clear; no RFC2119 keywords are used here).

Also, I don't think the logic of "we can reintroduce it later" is
reasonable.  We originally provided working implementation, which was
then broken by other changes.  In general, what should be fixed
(modified) is the "other changes", not the original code.

So, as a compromise, I'd propose to keep the source code even if it is
not effective.  Perhaps it may make sense to comment out this part
with a note that the part is temporarily broken and will be fixed
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.

Strictly speaking, this reasoning cannot justify the change,
logic-wise.  The original code should read "if sdl->sdl_alen is non 0,
then you have a MAC address stored".  It's a different proposition
than "if you have a MAC address stored for the node then (sdl_alen is
non 0 and) ln_state >= ND6_LLINFO_REACHABLE."

What we should prove to justify the above change is that "if ln_state
>= ND6_LLINFO_REACHABLE then you have a MAC address stored".  I'm
still not sure if this statement holds.

> 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.

So are you now proposing to keep the code for olladdr as it was
(perhaps with some modifications along with the new arp code?) and to
replace the other conditions with boolean expressions?  If so, I don't
oppose to it, though I'd rather personally prefer, e.g.,

-	if (olladdr && lladdr) {
-		if (bcmp(lladdr, LLADDR(sdl), ifp->if_addrlen))
-			llchange = 1;
-		else
-			llchange = 0;
-	} else
-		llchange = 0;

than

+	llchange = olladdr && lladdr &&	bcmp(lladdr, &ln->ll_addr, ifp->if_addrlen);

because the intention is clearer in the former, particularly when we
check the logic comparing to the protocol specification.  (In other
words, IMO it's compiler's business to perform this kind of
"simplification").

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