[NaCl SDK] nacl_io: implement set/getsockopt for TCP_NODELAY.

[NaCl SDK] nacl_io: implement set/getsockopt for TCP_NODELAY.

BUG=320450
TEST=nacl_io_socket_test + manual testing
R=binji@chromium.org, noelallen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238281

[binji, 2013-12-02 20:55:32]

lgtm

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:342: Error
MountNodeTCP::SetNoDelay() {
should this be called SetNoDelay_Locked? Or does it work properly without
holding the lock?

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:360: tcp_nodelay_ =
*static_cast<const int*>(optval) ? true : false;
nit: I'd prefer

*static_cast<const int*>(optval) != 0

here instead

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:379: << "expected
EINPROGRESS but got: " << strerror(errno);
nit: 4 space indent

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:436: sock2_ =
socket(AF_INET, SOCK_STREAM, 0);
this leaks the previous sock2_ ... do we care?

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:444: << "Failed
with " << errno << ": " << strerror(errno);
nit: 4 space indent

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:445: ASSERT_EQ(1,
option);
is there an easy way to test that this actually set the option on the PPAPI
socket?

Hm, looks like there's no PPB_TCPSocket.GetOption

[Sam Clegg, 2013-12-02 22:55:44]

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:342: Error
MountNodeTCP::SetNoDelay() {
On 2013/12/02 20:55:32, binji wrote:
> should this be called SetNoDelay_Locked? Or does it work properly without
> holding the lock?

Done.

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:360: tcp_nodelay_ =
*static_cast<const int*>(optval) ? true : false;
On 2013/12/02 20:55:32, binji wrote:
> nit: I'd prefer
> 
> *static_cast<const int*>(optval) != 0
> 
> here instead

Done.

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:379: << "expected
EINPROGRESS but got: " << strerror(errno);
On 2013/12/02 20:55:32, binji wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:436: sock2_ =
socket(AF_INET, SOCK_STREAM, 0);
On 2013/12/02 20:55:32, binji wrote:
> this leaks the previous sock2_ ... do we care?

The previous one is sock1_, right?

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:444: << "Failed
with " << errno << ": " << strerror(errno);
On 2013/12/02 20:55:32, binji wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:445: ASSERT_EQ(1,
option);
On 2013/12/02 20:55:32, binji wrote:
> is there an easy way to test that this actually set the option on the PPAPI
> socket?
> 
> Hm, looks like there's no PPB_TCPSocket.GetOption

Yeah.. ideally we would also actually test this by sending packets,
but I couldn't find a good way to do this, even just on linux.

[binji, 2013-12-02 22:58:43]

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/99813004/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:436: sock2_ =
socket(AF_INET, SOCK_STREAM, 0);
On 2013/12/02 22:55:44, Sam Clegg wrote:
> On 2013/12/02 20:55:32, binji wrote:
> > this leaks the previous sock2_ ... do we care?
> 
> The previous one is sock1_, right?

Sorry, I thought this was one of the tests that automatically created a new
socket. n/m

[noelallen1, 2013-12-02 23:15:47]

lgtm

[Sam Clegg, 2013-12-03 02:40:16]

Committed patchset #2 manually as r238281 (presubmit successful).