On 10. Oct 2011, at 08:44 , Qing Li wrote:
Hey,
> Okay, now I know what's confusing you ... it's that bug I introduced :-)
>
> The 2nd "if()" check on the RTF_GATEWAY flag should have been
> an "else if()".
>
> In a nutshell, the logic is any indirect route should fail the check,
> except for that special host route.
>
> Attached is the rework of that part of the patch, with some cleaning up
> per your suggestion.
>
> Thank you very much for catching that bug.
>
> --Qing
>
>
>> Q> > Well, after third review it is clear, that
>> Q> > next if() case would definitely be true, and you would proceed
>> Q> > with return. But that is difficult to see from first glance.
>> Q>
>> Q> Not so, only for an indirect prefix route.
>> Q>
>> Q> > I'd suggest to remove error variable, return immediately in
>> Q> > all error cases, and also the RTF_GATEWAY check can be shifted up,
>> Q> > since it is the most simple and the most usual to be true.
>> Q> >
>> Q>
>> Q> No, the RTF_GATEWAY check cannot be shifted up because if we did
>> Q> that, the (indirect host route, with destination matching the gateway
>> IP)
>> Q> would never be executed, if when that set of conditions are true, which
>> is
>> Q> allowed and the reason for the patch in the first place.
>>
>> Can you elaborate on that please? As far as I see, any rtentry that has
>> RTF_GATEWAY would return with EINVAL. The first if() clause doesn't
>> do any actual processing, only checking flags and memcmp()ing. The third
>> clause either. The error is never reset to 0. So, I don't see any
>> difference in returning EINVAL for RTF_GATEWAY immediately or later
>> after other checks.
I am a bit confused by this entire thing; it seems it's only parts of what
I had looked at initially but maybe the commit was split due to follow-ups
on an early change.
I am however happy that some things I had mentioned initially are now being
addressed in the cleanup patch. I have not checked the logic changes on it
however.
Thanks,
Bjoern
--
Bjoern A. Zeeb You have to have visions!
Stop bit received. Insert coin for new address
family._______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
