Date: Mon, 10 Oct 2011 01:44:30 -0700 From: Qing Li <qingli@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, bz@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r225947 - head/sys/netinet Message-ID: <CAGnGRdL8PraLk6BrMoC7XOxKBab_oNrRzvK7NXjVpgRgfeeknA@mail.gmail.com> In-Reply-To: <20111010061313.GC94905@FreeBSD.org> References: <201110031951.p93JpJLA025249@svn.freebsd.org> <20111009194732.GB94905@glebius.int.ru> <CAGnGRd%2BUfDYb=3y=8DoeArT7Qp2S1rUyTbQMTjD6cgybyoMKhA@mail.gmail.com> <20111010061313.GC94905@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--000e0cd37a74ac83fa04aeedcc3c Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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> =A0 =A0Not 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> =A0 No, the RTF_GATEWAY check cannot be shifted up because if we did > Q> =A0 that, the (indirect host route, with destination matching the gate= way IP) > Q> =A0 would never be executed, if when that set of conditions are true, = which is > Q> =A0 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. > --000e0cd37a74ac83fa04aeedcc3c Content-Type: application/octet-stream; name="in.c.diff" Content-Disposition: attachment; filename="in.c.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gtl7sf890 SW5kZXg6IGluLmMKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gaW4uYwkocmV2aXNpb24gMjI2MTg0KQorKysgaW4u Ywkod29ya2luZyBjb3B5KQpAQCAtMTQxNCw4ICsxNDE0LDYgQEAgc3RhdGljIGludAogaW5fbGx0 YWJsZV9ydGNoZWNrKHN0cnVjdCBpZm5ldCAqaWZwLCB1X2ludCBmbGFncywgY29uc3Qgc3RydWN0 IHNvY2thZGRyICpsM2FkZHIpCiB7CiAJc3RydWN0IHJ0ZW50cnkgKnJ0OwotCXN0cnVjdCBpZm5l dCAqeGlmcDsKLQlpbnQgZXJyb3IgPSAwOwogCiAJS0FTU0VSVChsM2FkZHItPnNhX2ZhbWlseSA9 PSBBRl9JTkVULAogCSAgICAoInNpbl9mYW1pbHkgJWQiLCBsM2FkZHItPnNhX2ZhbWlseSkpOwpA QCAtMTQzMiwyMSArMTQzMCwxNiBAQCBpbl9sbHRhYmxlX3J0Y2hlY2soc3RydWN0IGlmbmV0ICpp ZnAsIHVfaW50IGZsYWdzLAogCSAqIHN1Y2ggYXMgTUFORVQsIGFuZCB0aGUgaW50ZXJmYWNlIGlz IG9mIHRoZSBjb3JyZWN0IHR5cGUsIHRoZW4KIAkgKiBhbGxvdyBmb3IgQVJQIHRvIHByb2NlZWQu CiAJICovCi0JaWYgKHJ0LT5ydF9mbGFncyAmIChSVEZfR0FURVdBWSB8IFJURl9IT1NUKSkgewot CQl4aWZwID0gcnQtPnJ0X2lmcDsKLQkJCi0JCWlmICh4aWZwICYmICh4aWZwLT5pZl90eXBlICE9 IElGVF9FVEhFUiB8fAotCQkgICAgICh4aWZwLT5pZl9mbGFncyAmIChJRkZfTk9BUlAgfCBJRkZf U1RBVElDQVJQKSkgIT0gMCkpCi0JCQllcnJvciA9IEVJTlZBTDsKLQotCQlpZiAobWVtY21wKHJ0 LT5ydF9nYXRld2F5LT5zYV9kYXRhLCBsM2FkZHItPnNhX2RhdGEsCi0JCSAgICBzaXplb2YoaW5f YWRkcl90KSkgIT0gMCkKLQkJCWVycm9yID0gRUlOVkFMOwotCX0KLQogCWlmIChydC0+cnRfZmxh Z3MgJiBSVEZfR0FURVdBWSkgewotCQlSVEZSRUVfTE9DS0VEKHJ0KTsKLQkJcmV0dXJuIChFSU5W QUwpOworCQlpZiAoIShydC0+cnRfZmxhZ3MgJiBSVEZfSE9TVCkgfHwgKCFydC0+cnRfaWZwIHx8 CisJCQlydC0+cnRfaWZwLT5pZl90eXBlICE9IElGVF9FVEhFUiB8fAorCQkJICAocnQtPnJ0X2lm cC0+aWZfZmxhZ3MgJiAKKwkJCSAgIChJRkZfTk9BUlAgfCBJRkZfU1RBVElDQVJQKSkgIT0gMCB8 fAorCQkJICBtZW1jbXAocnQtPnJ0X2dhdGV3YXktPnNhX2RhdGEsIGwzYWRkci0+c2FfZGF0YSwK KwkJCQkgc2l6ZW9mKGluX2FkZHJfdCkpICE9IDApKSB7CisJCQlSVEZSRUVfTE9DS0VEKHJ0KTsK KwkJCXJldHVybiAoRUlOVkFMKTsKKwkJfQogCX0KIAogCS8qCkBAIC0xNDU1LDMyICsxNDQ4LDMx IEBAIGluX2xsdGFibGVfcnRjaGVjayhzdHJ1Y3QgaWZuZXQgKmlmcCwgdV9pbnQgZmxhZ3MsCiAJ ICogaW50ZXJmYWNlcyBoYXZlIHRoZSBzYW1lIHByZWZpeC4gQW4gaW5jb21pbmcgcGFja2V0IGFy cml2ZXMKIAkgKiBvbiBvbmUgaW50ZXJmYWNlIGFuZCB0aGUgY29ycmVzcG9uZGluZyBvdXRnb2lu ZyBwYWNrZXQgbGVhdmVzCiAJICogYW5vdGhlciBpbnRlcmZhY2UuCi0JICogCiAJICovCiAJaWYg KHJ0LT5ydF9pZnAgIT0gaWZwKSB7Ci0JCWNoYXIgKnNhLCAqbWFzaywgKmFkZHIsICpsaW07CisJ CWNvbnN0IGNoYXIgKnNhLCAqbWFzaywgKmFkZHIsICpsaW07CiAJCWludCBsZW47CiAKLQkJc2Eg PSAoY2hhciAqKXJ0X2tleShydCk7Ci0JCW1hc2sgPSAoY2hhciAqKXJ0X21hc2socnQpOwotCQlh ZGRyID0gKGNoYXIgKilfX0RFQ09OU1Qoc3RydWN0IHNvY2thZGRyICosIGwzYWRkcik7Ci0JCWxl biA9ICgoc3RydWN0IHNvY2thZGRyX2luICopX19ERUNPTlNUKHN0cnVjdCBzb2NrYWRkciAqLCBs M2FkZHIpKS0+c2luX2xlbjsKKwkJc2EgPSAoY29uc3QgY2hhciAqKXJ0X2tleShydCk7CisJCW1h c2sgPSAoY29uc3QgY2hhciAqKXJ0X21hc2socnQpOworCQlhZGRyID0gKGNvbnN0IGNoYXIgKils M2FkZHI7CisJCWxlbiA9ICgoY29uc3Qgc3RydWN0IHNvY2thZGRyX2luICopbDNhZGRyKS0+c2lu X2xlbjsKIAkJbGltID0gYWRkciArIGxlbjsKIAogCQlmb3IgKCA7IGFkZHIgPCBsaW07IHNhKyss IG1hc2srKywgYWRkcisrKSB7CiAJCQlpZiAoKCpzYSBeICphZGRyKSAmICptYXNrKSB7Ci0JCQkJ ZXJyb3IgPSBFSU5WQUw7CiAjaWZkZWYgRElBR05PU1RJQwogCQkJCWxvZyhMT0dfSU5GTywgIklQ djQgYWRkcmVzczogXCIlc1wiIGlzIG5vdCBvbiB0aGUgbmV0d29ya1xuIiwKIAkJCQkgICAgaW5l dF9udG9hKCgoY29uc3Qgc3RydWN0IHNvY2thZGRyX2luICopbDNhZGRyKS0+c2luX2FkZHIpKTsK ICNlbmRpZgotCQkJCWJyZWFrOworCQkJCVJURlJFRV9MT0NLRUQocnQpOworCQkJCXJldHVybiAo RUlOVkFMKTsKIAkJCX0KIAkJfQogCX0KIAogCVJURlJFRV9MT0NLRUQocnQpOwotCXJldHVybiAo ZXJyb3IpOworCXJldHVybiAoMCk7CiB9CiAKIC8qCg== --000e0cd37a74ac83fa04aeedcc3c--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGnGRdL8PraLk6BrMoC7XOxKBab_oNrRzvK7NXjVpgRgfeeknA>