Date: Mon, 10 Oct 2011 17:05:43 +0000 From: "Bjoern A. Zeeb" <bz@freebsd.org> To: Qing Li <qingli@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Gleb Smirnoff <glebius@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r225947 - head/sys/netinet Message-ID: <D8453582-12A8-4B9E-A7E9-28FD1C9C0B08@freebsd.org> In-Reply-To: <CAGnGRdL8PraLk6BrMoC7XOxKBab_oNrRzvK7NXjVpgRgfeeknA@mail.gmail.com> References: <201110031951.p93JpJLA025249@svn.freebsd.org> <20111009194732.GB94905@glebius.int.ru> <CAGnGRd%2BUfDYb=3y=8DoeArT7Qp2S1rUyTbQMTjD6cgybyoMKhA@mail.gmail.com> <20111010061313.GC94905@FreeBSD.org> <CAGnGRdL8PraLk6BrMoC7XOxKBab_oNrRzvK7NXjVpgRgfeeknA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D8453582-12A8-4B9E-A7E9-28FD1C9C0B08>