code review 8859050: net: add missing DialTCP argument test

net: add missing DialTCP argument test

[mikio, 2013-04-27 15:37:38]

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

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

[dave_cheney.net, 2013-04-27 15:48:08]

On 2013/04/27 15:37:38, mikio wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@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

[mikio, 2013-04-27 16:00:36]

On Sun, Apr 28, 2013 at 12:48 AM,  <dave@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.net, 2013-04-27 16:06:14]

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."

On Sun, Apr 28, 2013 at 2:00 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Sun, Apr 28, 2013 at 12:48 AM,  <dave@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.

[mikio, 2013-04-27 16:16:42]

On Sun, Apr 28, 2013 at 1:06 AM, Dave Cheney <dave@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.

[brainman, 2013-04-27 23:40:09]

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

[brainman, 2013-04-27 23:42:47]

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

[mikio, 2013-05-10 14:51:54]

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.brainman@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.

[mikio, 2013-05-10 14:52:26]

*** Abandoned ***