code review 5617051: net: replace implementatin of file/test parseProcNetIGMP

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

[borman, 2012-02-10 01:29:53]

Hello mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[borman, 2012-02-10 01:30:58]

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.

[rsc1, 2012-02-10 03:13:17]

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.

[mikio, 2012-02-10 04:02:54]

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?

[rsc1, 2012-02-10 04:23:36]

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