[NaCl SDK] Add UDP and TCP Sockets

[NaCl SDK] Add UDP and TCP Sockets

Creates MountSocket, MountNodeSocket, MountNodeUDP, MountNodeTCP
Wires up socket api (socket, connect, bind, send, ...) to MountNodeSocket.

This is a basic blocking only (no select) implementation.

BUG=257723
R=binji@chromium.org, sbc@chromium.org

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

[noelallen1, 2013-08-09 06:16:02]

ptal

[binji, 2013-08-09 19:28:06]

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:913: if
(AcquireSocketHandle(fd, &handle) == -1)
no errno set if the handle cannot be acquired?

(here and the rest of the file)

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:917:
reinterpret_cast<MountNodeSocket*>(handle->node().get());
Why reinterpret_cast? If this is a downcast you can just use static_cast. Though
this is ugly enough I'd rather see it as a function on KernelHandle, i.e.

handle->socket_node()

or something.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:989: Error err =
sock->GetPeerName(addr, len);
GetSockName?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1083: Error err =
sock->RecvFrom(buf, len, flags, addr, addrlen, &out_len);
why not make the RecvFrom out parameter ssize_t here (to avoid the ssize_t cast
below)?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1234: if
(sock->Init(S_IREAD | S_IWRITE) == 0) {
Init part here and above is identical, can you combine?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1238: }
EACCES here too if sock->Init fails?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.h:180:
sdk_util::ScopedRef<MountSocket> socket_mount_;
PROVIDES_SOCKET_API?

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

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:18: const uint32_t
DEFAULT_MAX_BUFFER = 65536 * 2;
make this static or put in an anonymous namespace.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:18: const uint32_t
DEFAULT_MAX_BUFFER = 65536 * 2;
Also, kDefaultMaxBuffer

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:40: // a ready of
TRUE for read, write, or error.
incorrect comment?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:77: Error
MountNodeSocket::PPErrorToErrno(int error) {
can you extend the one in pepper_interface.cc instead?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:123:
mount_->ppapi()->GetInstance(), &addr4);
nit: weird indent. Just indent 4 spaces. (same for ipv6 below too)

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:159: return
sizeof(sockaddr_in);
I was a bit surprised that this returns the socket size, not len -- maybe a
comment saying that the return value mimics getsockname/getpeername?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:181: sockaddr*
saddr1 = (sockaddr*) data1;
reinterpret_cast

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:196: return EPERM;
Why EPERM? maybe just ENOSYS for now, because it's not implemented? (same for
the rest of the functions)

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:241: return
EISCONN;
why EISCONN?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:257: return
EISCONN;
why EISCONN?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:269: if (*len < 1)
0 is invalid? linux man pages say len can't be negative, and opengroup says
nothing: http://pubs.opengroup.org/onlinepubs/7908799/xns/getpeername.html

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

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:85: bool
EquivalentAddress(PP_Resource addr1, PP_Resource addr2);
Maybe IsEquivalentAddress? I prefer functions to start with a verb.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:89: PP_Resource
socket_resource_;
used ScopedResource instead?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:50: /* Lie, we won't
known until we connect. */
s/known/know/

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:73:
mount_->ppapi()->AddRefResource(local_addr_);
did you mean to return early here? Otherwise this falls through and clears the
remote_addr_.

I think it would be clearer if you inverted the if (check for error instead of
success).

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:86: int err = 
TCPSocket()->Read(socket_resource_,
nit: extra space after =

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:88: len & 0xFFFF,  /*
Max 64K at a time. */
this seems arbitrary, why?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:123: if (err >= 0) {
Invert this to match similar check in Recv() above?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h:17: #include
"nacl_io/ossocket.h"
included twice

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:97: int out_len = 0;
this shadows out_len above

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:125: 
need to release resource?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:142: socket_resource_,
nit: weird indent

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:147: if (err >= 0) {
invert check to match Recv

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:165: PP_Resource
out_addr =  SockAddrToResource(dest_addr, addrlen);
nit: extra space after =

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:165: PP_Resource
out_addr =  SockAddrToResource(dest_addr, addrlen);
use ScopedResource instead?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.h:17: #include
"nacl_io/ossocket.h"
included twice

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_socket.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_socket.h:11: #include <list>
unused

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_socket.h:13: #include
<ppapi/c/pp_resource.h>
unused

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_socket.h:18: class
MountNodeSocket;
unused

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/real_pepper_interface.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/real_pepper_interface.h:13: #include
<ppapi/c/ppb_udp_socket.h>
shouldn't be necessary to include here

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc:31:
'SOCKET_PERMISSIONS': ["tcp-listen:*:*", "tcp-connect", "resolve-host",
"udp-bind:*:*", "udp-send-to:*:*"]
nit: expand out list so it is easier to read/doesn't line-wrap?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc:31:
'SOCKET_PERMISSIONS': ["tcp-listen:*:*", "tcp-connect", "resolve-host",
"udp-bind:*:*", "udp-send-to:*:*"]
Maybe just 127.0.0.1 instead of *?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/example.js (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:9:
listeningSocket = createInfo.socketId;
var listeningSocket

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:10:
creatingServer = false;
unused

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:14: 10,
what is 10?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:23:
cconsole.log("Accept failed: " + result);
s/cconsole/console/

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:50:
runTCPEchoServer(4006);
does this work with 0 for ephemeral? Maybe worth doing to reduce possibility of
flake on bots.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/main.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/main.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
A lot of copy-pasta here... we should figure out a better way to share this...

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:33: uint16_t
htons(uint16_t val) {
why not use the functions provided by nacl_io?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:50: void
IP4ToSockAddr(struct sockaddr_in* addr, uint32_t ip, uint16_t port) {
seems backward to have the out parameter first

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:55:
addr->sin_port = rport;
why not just addr->sin_port = htons(port);

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:60: void
Bind(int fd, uint32_t ip, uint16_t port, int expect) {
expect_errno?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:65: int err =
bind(fd, (sockaddr*) &addr, addrlen);
ki_bind?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:67: if
(expect != 0) {
ENONE

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:76:
Bind(sock1, 0x7F000001, 0, ENONE);
use const instead of magic number

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:81: int
sock1;
initialize member variables in constructor

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:113:
EXPECT_LT(ki_socket(AF_UNIX, SOCK_STREAM, 0), 0);
this looks a bit strange, maybe

EXPECT_EQ(-1, ki_socket(...))

instead?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:118: int
sock1 = ki_socket(AF_INET, SOCK_DGRAM, 0);
shadows this->sock1, which has dummy value, and is closed by the destructor. Not
a failure to close a bogus socket?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:156: char
inbuf[512];
why different sizes?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:169:
sendto(sock1, outbuf, sizeof(outbuf), 0,(sockaddr *)  &addr, addrlen);
nit: space after comma, remove extra space after cast

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:169:
sendto(sock1, outbuf, sizeof(outbuf), 0,(sockaddr *)  &addr, addrlen);
ki_sendto?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:179:
EXPECT_EQ(sizeof(sockaddr_in), addrlen);
check addr?

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:197:
EXPECT_EQ(0, errno);
check doesn't make sense -- if err is non-zero, we don't expect errno to be
zero. Remove if and just do EXPECT_EQ(0, errno)

[noelallen1, 2013-08-09 22:53:21]

ptal

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:913: if
(AcquireSocketHandle(fd, &handle) == -1)
On 2013/08/09 19:28:06, binji wrote:
> no errno set if the handle cannot be acquired?
> 
> (here and the rest of the file)

Set by acquire socket handle.   The convention is KernelProxy functions always
return -1 + errno.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:917:
reinterpret_cast<MountNodeSocket*>(handle->node().get());
On 2013/08/09 19:28:06, binji wrote:
> Why reinterpret_cast? If this is a downcast you can just use static_cast.
Though
> this is ugly enough I'd rather see it as a function on KernelHandle, i.e.
> 
> handle->socket_node()
> 
> or something.

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:989: Error err =
sock->GetPeerName(addr, len);
On 2013/08/09 19:28:06, binji wrote:
> GetSockName?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1083: Error err =
sock->RecvFrom(buf, len, flags, addr, addrlen, &out_len);
On 2013/08/09 19:28:06, binji wrote:
> why not make the RecvFrom out parameter ssize_t here (to avoid the ssize_t
cast
> below)?

it should match Read.  
Read(x, y, z) = Recv(x, y, z, 0) = RecvFrom(x, y, z, 0, NULL, NULL);

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1234: if
(sock->Init(S_IREAD | S_IWRITE) == 0) {
On 2013/08/09 19:28:06, binji wrote:
> Init part here and above is identical, can you combine?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1238: }
On 2013/08/09 19:28:06, binji wrote:
> EACCES here too if sock->Init fails?

Done.

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

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:18: const uint32_t
DEFAULT_MAX_BUFFER = 65536 * 2;
On 2013/08/09 19:28:06, binji wrote:
> make this static or put in an anonymous namespace.

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:40: // a ready of
TRUE for read, write, or error.
On 2013/08/09 19:28:06, binji wrote:
> incorrect comment?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:77: Error
MountNodeSocket::PPErrorToErrno(int error) {
On 2013/08/09 19:28:06, binji wrote:
> can you extend the one in pepper_interface.cc instead?

Didn't know there was one.  :)

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:123:
mount_->ppapi()->GetInstance(), &addr4);
On 2013/08/09 19:28:06, binji wrote:
> nit: weird indent. Just indent 4 spaces. (same for ipv6 below too)

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:159: return
sizeof(sockaddr_in);
On 2013/08/09 19:28:06, binji wrote:
> I was a bit surprised that this returns the socket size, not len -- maybe a
> comment saying that the return value mimics getsockname/getpeername?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:181: sockaddr*
saddr1 = (sockaddr*) data1;
On 2013/08/09 19:28:06, binji wrote:
> reinterpret_cast

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:196: return EPERM;
On 2013/08/09 19:28:06, binji wrote:
> Why EPERM? maybe just ENOSYS for now, because it's not implemented? (same for
> the rest of the functions)

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:241: return
EISCONN;
On 2013/08/09 19:28:06, binji wrote:
> why EISCONN?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:257: return
EISCONN;
On 2013/08/09 19:28:06, binji wrote:
> why EISCONN?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:269: if (*len < 1)
On 2013/08/09 19:28:06, binji wrote:
> 0 is invalid? linux man pages say len can't be negative, and opengroup says
> nothing: http://pubs.opengroup.org/onlinepubs/7908799/xns/getpeername.html

We define socklet_t as unsigned int, but it could be signed.  Comparing against
0 with an unsigned causes a waring->error.
I'll just ignore the check.

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

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:85: bool
EquivalentAddress(PP_Resource addr1, PP_Resource addr2);
On 2013/08/09 19:28:06, binji wrote:
> Maybe IsEquivalentAddress? I prefer functions to start with a verb.

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:89: PP_Resource
socket_resource_;
On 2013/08/09 19:28:06, binji wrote:
> used ScopedResource instead?

Can't.  mount_->ppapi_ isn't available until Init.

We should refactor ScopedResource into it's own header, and have it more closely
match ScopedRef.   But that's another CL.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:50: /* Lie, we won't
known until we connect. */
On 2013/08/09 19:28:06, binji wrote:
> s/known/know/

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:73:
mount_->ppapi()->AddRefResource(local_addr_);
On 2013/08/09 19:28:06, binji wrote:
> did you mean to return early here? Otherwise this falls through and clears the
> remote_addr_.
> 
> I think it would be clearer if you inverted the if (check for error instead of
> success).

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:88: len & 0xFFFF,  /*
Max 64K at a time. */
On 2013/08/09 19:28:06, binji wrote:
> this seems arbitrary, why?

Max IP "packet" size is 64K.

Until we support streaming, it seemed a reasonable limit size Pepper doesn't
publish it's limit.
It is always okay to return < len when using read/write although people are lazy
about checking on real files.  On sockets, it's expected to return less based on
buffering behavior.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:123: if (err >= 0) {
On 2013/08/09 19:28:06, binji wrote:
> Invert this to match similar check in Recv() above?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h:17: #include
"nacl_io/ossocket.h"
On 2013/08/09 19:28:06, binji wrote:
> included twice

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:97: int out_len = 0;
On 2013/08/09 19:28:06, binji wrote:
> this shadows out_len above

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:125: 
On 2013/08/09 19:28:06, binji wrote:
> need to release resource?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:142: socket_resource_,
On 2013/08/09 19:28:06, binji wrote:
> nit: weird indent

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:147: if (err >= 0) {
On 2013/08/09 19:28:06, binji wrote:
> invert check to match Recv

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:165: PP_Resource
out_addr =  SockAddrToResource(dest_addr, addrlen);
On 2013/08/09 19:28:06, binji wrote:
> nit: extra space after =

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.h:17: #include
"nacl_io/ossocket.h"
On 2013/08/09 19:28:06, binji wrote:
> included twice

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_socket.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_socket.h:11: #include <list>
On 2013/08/09 19:28:06, binji wrote:
> unused

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_socket.h:13: #include
<ppapi/c/pp_resource.h>
On 2013/08/09 19:28:06, binji wrote:
> unused

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_socket.h:18: class
MountNodeSocket;
On 2013/08/09 19:28:06, binji wrote:
> unused

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/real_pepper_interface.h (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/real_pepper_interface.h:13: #include
<ppapi/c/ppb_udp_socket.h>
On 2013/08/09 19:28:06, binji wrote:
> shouldn't be necessary to include here

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc:31:
'SOCKET_PERMISSIONS': ["tcp-listen:*:*", "tcp-connect", "resolve-host",
"udp-bind:*:*", "udp-send-to:*:*"]
On 2013/08/09 19:28:06, binji wrote:
> Maybe just 127.0.0.1 instead of *?

Matches other DSCs... We should change them all at once.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/example.js (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:9:
listeningSocket = createInfo.socketId;
On 2013/08/09 19:28:06, binji wrote:
> var listeningSocket

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:10:
creatingServer = false;
On 2013/08/09 19:28:06, binji wrote:
> unused

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:14: 10,
On 2013/08/09 19:28:06, binji wrote:
> what is 10?
Q depth.

Cut and paste from socket example.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:23:
cconsole.log("Accept failed: " + result);
On 2013/08/09 19:28:06, binji wrote:
> s/cconsole/console/

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:50:
runTCPEchoServer(4006);
On 2013/08/09 19:28:06, binji wrote:
> does this work with 0 for ephemeral? Maybe worth doing to reduce possibility
of
> flake on bots.

New CL.  We don't really support testing as packaged app ATM.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/main.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/main.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/08/09 19:28:06, binji wrote:
> A lot of copy-pasta here... we should figure out a better way to share this...

Don't have a solution ATM, but we can certainly refactor this.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:33: uint16_t
htons(uint16_t val) {
On 2013/08/09 19:28:06, binji wrote:
> why not use the functions provided by nacl_io?

Has that landed?  I'll merge it in if available.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:50: void
IP4ToSockAddr(struct sockaddr_in* addr, uint32_t ip, uint16_t port) {
On 2013/08/09 19:28:06, binji wrote:
> seems backward to have the out parameter first

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:55:
addr->sin_port = rport;
On 2013/08/09 19:28:06, binji wrote:
> why not just addr->sin_port = htons(port);

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:60: void
Bind(int fd, uint32_t ip, uint16_t port, int expect) {
On 2013/08/09 19:28:06, binji wrote:
> expect_errno?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:65: int err =
bind(fd, (sockaddr*) &addr, addrlen);
On 2013/08/09 19:28:06, binji wrote:
> ki_bind?

Whoops, removing ki_  this should be the end-to-end version of testing.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:67: if
(expect != 0) {
On 2013/08/09 19:28:06, binji wrote:
> ENONE

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:76:
Bind(sock1, 0x7F000001, 0, ENONE);
On 2013/08/09 19:28:06, binji wrote:
> use const instead of magic number

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:81: int
sock1;
On 2013/08/09 19:28:06, binji wrote:
> initialize member variables in constructor

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:113:
EXPECT_LT(ki_socket(AF_UNIX, SOCK_STREAM, 0), 0);
On 2013/08/09 19:28:06, binji wrote:
> this looks a bit strange, maybe
> 
> EXPECT_EQ(-1, ki_socket(...))
> 
> instead?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:118: int
sock1 = ki_socket(AF_INET, SOCK_DGRAM, 0);
On 2013/08/09 19:28:06, binji wrote:
> shadows this->sock1, which has dummy value, and is closed by the destructor.
Not
> a failure to close a bogus socket?

This is not a TEST_F.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:156: char
inbuf[512];
On 2013/08/09 19:28:06, binji wrote:
> why different sizes?

To read > expected

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:169:
sendto(sock1, outbuf, sizeof(outbuf), 0,(sockaddr *)  &addr, addrlen);
On 2013/08/09 19:28:06, binji wrote:
> nit: space after comma, remove extra space after cast

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:169:
sendto(sock1, outbuf, sizeof(outbuf), 0,(sockaddr *)  &addr, addrlen);
On 2013/08/09 19:28:06, binji wrote:
> ki_sendto?

no ki_*, end to end.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:179:
EXPECT_EQ(sizeof(sockaddr_in), addrlen);
On 2013/08/09 19:28:06, binji wrote:
> check addr?

Done.

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:197:
EXPECT_EQ(0, errno);
On 2013/08/09 19:28:06, binji wrote:
> check doesn't make sense -- if err is non-zero, we don't expect errno to be
> zero. Remove if and just do EXPECT_EQ(0, errno)

Done.

[Sam Clegg, 2013-08-09 23:07:59]

lgtm

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:33: uint16_t
htons(uint16_t val) {
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > why not use the functions provided by nacl_io?
> 
> Has that landed?  I'll merge it in if available.

Yup, https://chromiumcodereview.appspot.com/22625004

[Sam Clegg, 2013-08-09 23:08:24]

On 2013/08/09 23:07:59, Sam Clegg wrote:
> lgtm
> 
>
https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
> File native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc
(right):
> 
>
https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
> native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:33:
uint16_t
> htons(uint16_t val) {
> On 2013/08/09 22:53:22, noelallen1 wrote:
> > On 2013/08/09 19:28:06, binji wrote:
> > > why not use the functions provided by nacl_io?
> > 
> > Has that landed?  I'll merge it in if available.
> 
> Yup, https://chromiumcodereview.appspot.com/22625004

Oop, didn't mean to LG that .. I'll leave that to ben.

[binji, 2013-08-09 23:08:27]

lgtm, sorry for the long review... you know me. :)

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:1083: Error err =
sock->RecvFrom(buf, len, flags, addr, addrlen, &out_len);
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > why not make the RecvFrom out parameter ssize_t here (to avoid the ssize_t
> cast
> > below)?
> 
> it should match Read.  
> Read(x, y, z) = Recv(x, y, z, 0) = RecvFrom(x, y, z, 0, NULL, NULL);

OK

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:88: len & 0xFFFF,  /*
Max 64K at a time. */
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > this seems arbitrary, why?
> 
> Max IP "packet" size is 64K.
> 
> Until we support streaming, it seemed a reasonable limit size Pepper doesn't
> publish it's limit.
> It is always okay to return < len when using read/write although people are
lazy
> about checking on real files.  On sockets, it's expected to return less based
on
> buffering behavior.

OK, but you may want to use std::min then, this will behave strangely for len ==
0x10000

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.dsc:31:
'SOCKET_PERMISSIONS': ["tcp-listen:*:*", "tcp-connect", "resolve-host",
"udp-bind:*:*", "udp-send-to:*:*"]
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > Maybe just 127.0.0.1 instead of *?
> 
> Matches other DSCs... We should change them all at once.

OK

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/example.js (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/example.js:50:
runTCPEchoServer(4006);
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > does this work with 0 for ephemeral? Maybe worth doing to reduce possibility
> of
> > flake on bots.
> 
> New CL.  We don't really support testing as packaged app ATM.

OK

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:118: int
sock1 = ki_socket(AF_INET, SOCK_DGRAM, 0);
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > shadows this->sock1, which has dummy value, and is closed by the destructor.
> Not
> > a failure to close a bogus socket?
> 
> This is not a TEST_F.

You're right, whoops!

https://codereview.chromium.org/22587003/diff/6001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_socket_test/socket_test.cc:169:
sendto(sock1, outbuf, sizeof(outbuf), 0,(sockaddr *)  &addr, addrlen);
On 2013/08/09 22:53:22, noelallen1 wrote:
> On 2013/08/09 19:28:06, binji wrote:
> > ki_sendto?
> 
> no ki_*, end to end.

OK, I see.

https://codereview.chromium.org/22587003/diff/23001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/22587003/diff/23001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:35: return
reinterpret_cast<MountNodeSocket*>(node_.get());
static_cast

[noelallen1, 2013-08-15 01:56:05]

Committed patchset #12 manually as r217731 (presubmit successful).