[NaCl SDK] nacl_io: Add initial support for getsockopt

[NaCl SDK] nacl_io: Add initial support for getsockopt

Currently there is only support for SO_ERROR which is
used to query the result of non-blocking calls to
connect().  Since out connect() is always blocking
right now this always returns 0.

Also fix a bug in kernel_proxy:socket() where EACCESS
was being returned rather than -1 (with errno set to
EACCESS).

BUG=296972
NOTRY=true

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

[noelallen_use_chromium, 2013-09-27 22:04:22]

Do you want to add Set as well, and the setup the RX/TX timeouts?

The code should be there, you just need to modify the variables.

[noelallen_use_chromium, 2013-09-27 22:05:27]

Once you add set, you can then also add the approprate kernel proxy test, and
socket test for getting and setting the timeouts.  :)

[Sam Clegg, 2013-09-28 00:31:03]

On 2013/09/27 22:04:22, noelallen wrote:
> Do you want to add Set as well, and the setup the RX/TX timeouts?
> 
> The code should be there, you just need to modify the variables.

I've added the plumbing for setsockopt too.

I'd rather leave adding more SO_ options for a later CL and leave this one
as mostly plumbing.

I did have a quick look for RX/TX settings but it looks like all the
Read/Write code for sockets in nacl_io right now blocks forever rather
than having a timeout.  Am I missing something?

[Sam Clegg, 2013-09-30 15:17:25]

ping

[noelallen1, 2013-09-30 16:22:50]

MountNodeStream has a read_timeout_ and write_timeout_.

If you check RecvHelper and SendHelper in MountNodeSocket they should use those
timeouts if O_NONBLOCK or MSG_DONTWAIT

[binji, 2013-09-30 16:37:47]

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc (right):

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:212: if (*len <
sizeof(int))
http://pubs.opengroup.org/onlinepubs/009695399/functions/getsockopt.html

It says here that the value should be truncated if the size is too small: "If
the size of the option value is greater than option_len, the value stored in the
object pointed to by the option_value argument shall be silently truncated."

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:214:
*static_cast<int*>(optval) = 0;
always 0? This probably needs to be a virtual function so the TCP/UDP socket can
retrieve error if it has one and clear it. Or their error member variables
should move up here.

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.h (right):

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:50: // Unsuported
Functions
sp: Unsupported

[Sam Clegg, 2013-09-30 17:33:24]

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc (right):

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:212: if (*len <
sizeof(int))
On 2013/09/30 16:37:48, binji wrote:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/getsockopt.html
> 
> It says here that the value should be truncated if the size is too small: "If
> the size of the option value is greater than option_len, the value stored in
the
> object pointed to by the option_value argument shall be silently truncated."

Done.

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:214:
*static_cast<int*>(optval) = 0;
On 2013/09/30 16:37:48, binji wrote:
> always 0? This probably needs to be a virtual function so the TCP/UDP socket
can
> retrieve error if it has one and clear it. Or their error member variables
> should move up here.

Done.

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.h (right):

https://codereview.chromium.org/23456045/diff/9001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:50: // Unsuported
Functions
On 2013/09/30 16:37:48, binji wrote:
> sp: Unsupported

Done.

[Sam Clegg, 2013-09-30 17:34:44]

On 2013/09/30 16:22:50, noelallen1 wrote:
> MountNodeStream has a read_timeout_ and write_timeout_.
> 
> If you check RecvHelper and SendHelper in MountNodeSocket they should use
those
> timeouts if O_NONBLOCK or MSG_DONTWAIT

Great.  I'm happy to add that but can we make it a separate CL so this one is
mostly plumbing?  I'm thinking of trying to drover this CL to 31 (for libcurl)
so trying to keep it small.

[noelallen1, 2013-09-30 19:10:41]

We don't need to land the timeout stuff here.  As long as we add a test for
set/get.  LGTM+

[Sam Clegg, 2013-09-30 22:36:27]

Added some tests for getsockopt/setsockopt.

Also, fixed a bug in kernel_proxy:socket() (see CL desc).

PTAL.

[noelallen1, 2013-09-30 22:56:16]

Good catch on the socket fix.

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc (right):

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:218: 
Clear last_errno_:
The SO_ERROR option obtains and then clears this field,

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:251: }
Read an unbound socket, for a real Errno?

[Sam Clegg, 2013-10-01 00:15:49]

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc (right):

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:218: 
On 2013/09/30 22:56:16, noelallen1 wrote:
> Clear last_errno_:
> The SO_ERROR option obtains and then clears this field, 
> 
> 
> 

Done.

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/23456045/diff/25001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:251: }
On 2013/09/30 22:56:16, noelallen1 wrote:
> Read an unbound socket, for a real Errno?

I'm mimicking the linux behavior and strangely enough, attempting
to read() on an non-bound/non-connected socket returns an error
but doesn't set SO_ERROR.

The behaviors seems to be that SO_ERROR is not set for calls that
return the error synchronously.  As if the stack thinks the error
has already been delivered to the user so it doesn't store it for
retrieval via SO_ERROR.

In summary I think we need to wait for the non-blocking connect()
to be able to test this case.

[binji, 2013-10-02 05:19:02]

On 2013/10/02 05:18:41, binji wrote:

Sorry, lgtm

[commit-bot: I haz the power, 2013-10-02 16:42:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sbc@chromium.org/23456045/38001

[commit-bot: I haz the power, 2013-10-02 17:12:12]

Change committed as 226482

[noelallen1, 2013-10-02 17:14:28]

lgtm