Add support to specify the source address for socket connect

Add support to specify the source address for socket connect

BUG=http://dartbug.com/22253
R=kustermann@google.com, lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43638

[Søren Gjesse, 2015-02-09 16:32:33]

sgjesse@google.com changed reviewers:
+ lrn@chromium.org

[Søren Gjesse, 2015-02-09 16:41:38]

sgjesse@google.com changed reviewers:
+ lrn@google.com
- lrn@chromium.org

[kustermann, 2015-02-10 10:06:55]

lgtm

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
File tests/standalone/io/socket_source_address_test.dart (right):

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:91: }
Since there is no difference at all between these two methods, you could pass a
tearoff method as an argument, i.e.

Future testArgumentSocket(Function connectFunction) {
  ...
}

testArgumentSocket(Socket.connect)
testArgumentSocket(RawSocket.connect)

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:98: s.destroy();
Maybe have a counter here, to make sure it actually got called?
Also further down.

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:157: }
This is loop is an exact duplicate of the one in testConnectRawSocket() - it
makes it relaly hard to see what is actually tested / what is the difference to
the other tests.

If possible, I'd collapse these tests into less, more concise code.

https://codereview.chromium.org/908873002/diff/60001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/908873002/diff/60001/runtime/bin/socket.h#new...
runtime/bin/socket.h:234: RawAddr source_addr);
You could pass these RawAddr's as const Reference since they are bigger structs
(i.e. bigger than pointer size).

https://codereview.chromium.org/908873002/diff/60001/tests/standalone/io/sock...
File tests/standalone/io/socket_source_address_test.dart (right):

https://codereview.chromium.org/908873002/diff/60001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:8: //
VMOptions=--short_socket_read --short_socket_write
You're not transfering any data between sockets, so no need to make this test
expand to 4 tests.

[Søren Gjesse, 2015-02-10 11:12:40]

New patchsets have been uploaded after l-g-t-m from kustermann@google.com

[Søren Gjesse, 2015-02-10 11:13:33]

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
File tests/standalone/io/socket_source_address_test.dart (right):

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:91: }
On 2015/02/10 10:06:54, kustermann wrote:
> Since there is no difference at all between these two methods, you could pass
a
> tearoff method as an argument, i.e.
> 
> Future testArgumentSocket(Function connectFunction) {
>   ...
> }
> 
> testArgumentSocket(Socket.connect)
> testArgumentSocket(RawSocket.connect)

Good point. Done.

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:98: s.destroy();
On 2015/02/10 10:06:54, kustermann wrote:
> Maybe have a counter here, to make sure it actually got called?
> Also further down.

Added more asyncStart() and asyncEnd() to do the counting.

https://codereview.chromium.org/908873002/diff/20001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:157: }
On 2015/02/10 10:06:54, kustermann wrote:
> This is loop is an exact duplicate of the one in testConnectRawSocket() - it
> makes it relaly hard to see what is actually tested / what is the difference
to
> the other tests.
> 
> If possible, I'd collapse these tests into less, more concise code.

Created a parameterized test to cover the last 4 tests.

https://codereview.chromium.org/908873002/diff/60001/runtime/bin/socket.h
File runtime/bin/socket.h (right):

https://codereview.chromium.org/908873002/diff/60001/runtime/bin/socket.h#new...
runtime/bin/socket.h:234: RawAddr source_addr);
On 2015/02/10 10:06:54, kustermann wrote:
> You could pass these RawAddr's as const Reference since they are bigger
structs
> (i.e. bigger than pointer size).

Done. I will do another change to change this in more places.

https://codereview.chromium.org/908873002/diff/60001/tests/standalone/io/sock...
File tests/standalone/io/socket_source_address_test.dart (right):

https://codereview.chromium.org/908873002/diff/60001/tests/standalone/io/sock...
tests/standalone/io/socket_source_address_test.dart:8: //
VMOptions=--short_socket_read --short_socket_write
On 2015/02/10 10:06:54, kustermann wrote:
> You're not transfering any data between sockets, so no need to make this test
> expand to 4 tests.

Done.

[Søren Gjesse, 2015-02-10 11:57:25]

Committed patchset #6 (id:100001) manually as 43638.

[kevmoo, 2015-02-10 15:07:07]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2015-02-10 15:07:08]

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart
File sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart#...
sdk/lib/io/socket.dart:462: external static Future<RawSocket> connect(host, int
port, {sourceAddress});
What's the expected format of `sourceAddress` if not `InternetAddress?

Just want to understand what we need to support String.

[kustermann, 2015-02-10 15:15:37]

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart
File sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart#...
sdk/lib/io/socket.dart:125: external factory InternetAddress(String address);
See here.

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart#...
sdk/lib/io/socket.dart:462: external static Future<RawSocket> connect(host, int
port, {sourceAddress});
On 2015/02/10 15:07:08, kevmoo wrote:
> What's the expected format of `sourceAddress` if not `InternetAddress?
> 
> Just want to understand what we need to support String.

Because it's much more convenient to write. It's a shorthand for `new
InternetAddress(sourceAddress)`. You can check the documentation above.

"must hold a numeric IP address" means  e.g. `1.2.3.4` or `ff02::1`.

[Søren Gjesse, 2015-02-11 10:12:22]

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart
File sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/908873002/diff/100001/sdk/lib/io/socket.dart#...
sdk/lib/io/socket.dart:462: external static Future<RawSocket> connect(host, int
port, {sourceAddress});
On 2015/02/10 15:15:37, kustermann wrote:
> On 2015/02/10 15:07:08, kevmoo wrote:
> > What's the expected format of `sourceAddress` if not `InternetAddress?
> > 
> > Just want to understand what we need to support String.
> 
> Because it's much more convenient to write. It's a shorthand for `new
> InternetAddress(sourceAddress)`. You can check the documentation above.
> 
> "must hold a numeric IP address" means  e.g. `1.2.3.4` or `ff02::1`.

I thought about this, and with the first argument 'host' we already went with
type dynamic. Of course when 'host' is a string is can be any DNS address. So
for consistency sourceAddress is dynamic.