|
|
|
net: replace implementatin of file/test parseProcNetIGMP
Changed the internal file type to implement a new interface
called readStringer that bytes.Buffer also implements.
Fixed issue with parseProcNetIGMP incorrectly parsing
/proc/net/igmp and panicing. Added test case to test the
condition. The old version is still there to demonstrate
the problem.
The simplest fix, but providing no testing, is to just
adopt the parsing logic in interface_linux.c. Alternatively
this CL can go in with the _oldParseProcNetIGMP function
removed and the igmp_test.go file named in such a way it
will only run on linux (to late to research that tonight).
Note that for me the net test still fails. The panic was
masking a different failure (the igmp_tet.go failure is
expected):
--- FAIL: TestOldParseProcNetIGMP (0.00 seconds)
igmp_test.go:35: paniced
--- FAIL: TestListenMulticastUDP (0.00 seconds)
multicast_test.go:126: IPv4 multicast interface: <nil>
multicast_test.go:136: IPv4 multicast TTL: 1
multicast_test.go:146: IPv4 multicast loopback: false
multicast_test.go:82: "224.0.0.254:12345" not found in RIB
fixes issue 2826
Patch Set 1 #Patch Set 2 : diff -r f79343c8a479 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r f79343c8a479 https://go.googlecode.com/hg/ #
Total comments: 4
Total messages: 6
|
borman
|
13 years, 11 months ago (2012年02月06日 21:48:26 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hello mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
On 2012年02月10日 01:29:53, borman wrote: > Hello mailto:mikioh.mikioh@gmail.com (cc: mailto:golang-dev@googlegroups.com), > > I'd like you to review this change to > https://go.googlecode.com/hg/ I remove the demonstration of the panic. igmp_test is linux specific. I am not sure how to make platform specific tests.
Thanks for working on this. Can we get the delta down a bit smaller? http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/dnsconfig.go File src/pkg/net/dnsconfig.go (right): http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/dnsconfig.go#newc... src/pkg/net/dnsconfig.go:29: defer file.Close() I'm sorry, I appreciate the effort to clean things up, but this kind of minor churn needs not to happen this close to Go 1. This new code looks fine but the old code was fine too and more importantly had been running for months without problems. Please revert all changes that are not directly addressing the bug you are trying to fix. http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/parse.go File src/pkg/net/parse.go (right): http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/parse.go#newcode1 src/pkg/net/parse.go:1: // Copyright 2012 The Go Authors. All rights reserved. The convention is to leave copyright years alone. Thanks. http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/parse.go#newcode11 src/pkg/net/parse.go:11: "bytes" Package net is supposed to have as few dependencies as possible. In particular, it should not depend on bytes. If you need a helper, please add one. It looks like you only use IndexByte, which is trivial to write, or maybe you can use splitAtBytes, defined below. http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/parse.go#newcode23 src/pkg/net/parse.go:23: type readStringer interface { Please don't refactor this code. This is fundamental piece of code and we are very close to Go 1. If the change cannot be made in a precise manner that touches few lines of code, we'll postpone it until after Go 1. If this is for testing, it should be easy for the test to write a temporary file that can be read back in.
On Fri, Feb 10, 2012 at 12:13 PM, <rsc@google.com> wrote: > http://codereview.appspot.com/5617051/diff/4011/src/pkg/net/parse.go#newcode11 > src/pkg/net/parse.go:11: "bytes" > Package net is supposed to have as few dependencies as possible. > In particular, it should not depend on bytes. If you need a helper, > please add one. It looks like you only use IndexByte, which is trivial > to write, or maybe you can use splitAtBytes, defined below. Ah... well... I've already embedded bytes dependency in interface.go and sockopt.go. Is it worth to remove all of these dependencies before Go 1 release?
On Thu, Feb 9, 2012 at 23:02, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > Ah... well... I've already embedded bytes dependency in > interface.go and sockopt.go. Is it worth to remove all of > these dependencies before Go 1 release? Perhaps not but let's not strengthen the links. Russ