code review 8859050: net: add missing DialTCP argument test (issue 8859050)

[mikioh...@gmail.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go


Description:
net: add missing DialTCP argument test

Please review this at https://codereview.appspot.com/8859050/

Affected files:
M src/pkg/net/protoconn_test.go


Index: src/pkg/net/protoconn_test.go
===================================================================
--- a/src/pkg/net/protoconn_test.go
+++ b/src/pkg/net/protoconn_test.go
@@ -97,7 +97,7 @@
if err != nil {
t.Fatalf("ResolveTCPAddr failed: %v", err)
}
- c, err := DialTCP("tcp4", nil, ra)
+ c, err := DialTCP("tcp4", la, ra)
if err != nil {
t.Fatalf("DialTCP failed: %v", err)
}

[da...@cheney.net]

On 2013/04/27 15:37:38, mikio wrote:
> Hello mailto:golan...@googlegroups.com (cc:
mailto:golan...@googlegroups.com),

> I'd like you to review this change to
> https://code.google.com/p/go

Hi Mikio,

What is this testing ? From my reading it dials ra using the source
address of la, but la is already bound by a ListenTCP above. Not that I
have ever tried, but does this work ?

Dave

https://codereview.appspot.com/8859050/

[Mikio Hara]

On Sun, Apr 28, 2013 at 12:48 AM, <da...@cheney.net> wrote:

> What is this testing ? From my reading it dials ra using the source
> address of la, but la is already bound by a ListenTCP above.

ResolveTCPAddr just returns a TCPAddr.

[Dave Cheney]

Ok. I think I understand now. Would you please update the description
with something along these lines (assuming it is correct)

"Ensure TestTCPConnSpecificMethods chooses the loopback interface as
both the source and the target."

[Mikio Hara]

On Sun, Apr 28, 2013 at 1:06 AM, Dave Cheney <da...@cheney.net> wrote:

> Ok. I think I understand now. Would you please update the description
> with something along these lines (assuming it is correct)
>
> "Ensure TestTCPConnSpecificMethods chooses the loopback interface as
> both the source and the target."

Thanks but no thanks.

It just tests DialTCP with both destination and local addresses,
nothing more or less. Also we cannot ensure that the test will use
loopback interface because it depends on the platform implementation.

[alex.b...@gmail.com]

I am not an expert in TCP, but I think this is wrong. la represents an
IP/port pair that our listener is listening on. Can it be used as local
IP/port pair for a new TCP connection?

I also suspect that this will break on windows, similar to what is
happening in issue 5355. So this could be a test for issue 5355. But you
should both scenarios: la is nil and la is not nil.

Alex

https://codereview.appspot.com/8859050/

[alex.b...@gmail.com]

But you TEST should both
scenarios: la is nil and la is not nil. (phone fingers).

https://codereview.appspot.com/8859050/

[Mikio Hara]

Thanks for fixing issues related to Dial stuff.
Will abandon this CL and revisit later for refactoring testcases.

On Sun, Apr 28, 2013 at 8:40 AM, <alex.b...@gmail.com> wrote:

> I am not an expert in TCP, but I think this is wrong. la represents an
> IP/port pair that our listener is listening on. Can it be used as local
> IP/port pair for a new TCP connection?

That variable represents "127.0.0.1:0", a pair of specific IP address and
wildcard port. Both Listen and Dial functions never tweak their parameters.
(Also POSIX and Windows socket API requires syscall.Sockaddrs instead
of net.ProtocolAddrs.)

> I also suspect that this will break on windows, similar to what is
> happening in issue 5355. So this could be a test for issue 5355. But you
> should both scenarios: la is nil and la is not nil.

Yup.

[mikioh...@gmail.com]

*** Abandoned ***

https://codereview.appspot.com/8859050/