[NaCl SDK] Support non blocking TCP/UDP

[NaCl SDK] Support non blocking TCP/UDP for NaCl IO library in NaCl SDK.

Does not affect chrome itself.

This change bring in support for non-blocking TCP and UDP send and recv.  This requires rework of EventEmitter, EventListner splitting them out of the MountNode so that the MountNode (socket) could be de-ref'd and closed, while the Emitter (fifo) was still available to accept IO cancellations.

Part of the split out, also simplified EventListener use.  Now listeners are per thread and RefCount emitters (not Nodes) significantly simplifying lifetime issues and reducing emitter/listener code complexity.

In addition, the simplified locking allows for one-to-one Listeners to share locks with the emitter, for cases like  read from UDP, where multiple threads could legally block on RecvFrom, and must only make progress one at a time to ensure correct timeout.

Adds 'pipe', 'packet' and 'signal' Emitters to support pipes, udp sockets, and IO interruption (EINTR) via 'kill'.

* split out EventEmitter and EventListener from MountNode
* Remove ScopedRef from EventListener
* Remove EventInfo
* Create EventEmitterPipe (Emitter + FIFOChar)
* Create EventEmitterTCP (Emitter + FIFOChar * 2)
* Create EventEmitterUDP (Emitter + FIFOPacket * 2)
* Create MountNodeStream base class
* Create MountNodeSocket from MountNodeStream
* Create MountNodePipe
* Rename MountSocket to MountStream
* Add plumbing for pipe
* select uses poll functionality
* move all send/recv on sockets to MountStream thread.

BUG=257723
R=binji@chromium.org

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

[noelallen1, 2013-09-06 22:47:19]

ptal

[binji, 2013-09-12 01:47:56]

sorry this took so long. :)

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h (right):

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:24:
EventEmitterPacket(size_t rsize, size_t wsize)
I'd prefer having a .cc file instead of defining this all in the header

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:30: fifo_ = new
FIFOPacket(rsize);
Why allocate?

If you define these as non-pointer members you can construct them like so:

rfifo_(std::max<size_t>(1, rsize)),
wfifo_(std::max<size_t>(1, wsize)),

And then you can use the default destructor

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:47: Packet*
ReadPacket() {
what's the ownership of the returned packet?

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:55: uint32_t
WritePacket(Packet* packet) {
ownership of passed packet?

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:91: #endif  //
LIBRARIES_NACL_IO_EVENT_EMITTER_PIPE_H_
PACKET_H_

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h (right):

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h:41: size_t
Read(char* data, size_t len) {
Be consistent with:

MountNode::Read(void* buf, size_t count)

?

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_signal.h (right):

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_signal.h:21:
EventEmitterSignal() : events_(0) {};
semicolon is unecessary

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_signal.h:36: protected:
I prefer member variables are private

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:78: // Return the
failure, unlocked
comment is incorrect, still locked here, right?

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:92: uint32_t events) {
nit: line up params

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:100: size_t cnt,
nit: line up params

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:154: 
remove extra line

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:58: protected:
private?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:47: Packet*
ReadTXPacket_Locked() {
why TXPacket? ReadPacket is easier to read...

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:50:
UpdateStatusLocked();
_Locked?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:49: lock_ = new
sdk_util::AutoLock(emitter->GetLock());
move into initializer list above?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:115: explicit
EventListenerLock(EventEmitter* emitter);
nit: two space indent here

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:122: public:
remove

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:133: protected:
private:

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:136: uint32_t events_;
disable copy and assign?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:148: public:
remove

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:157: protected:
private:

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_char.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.cc:20: buffer_ = new
char[size];
use a std::vector instead of an array?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.cc:68: while (len > 0) {
Seems unnecessary: You only need to do one or two iterations, and a memcpy of
zero bytes is a no-op.

How about:

size_t count = /* num available bytes */;
size_t end_count = /* num bytes at the end, before wrapping */
size_t begin_count = /* rest of bytes, maybe 0 */
memcpy(&buf[0], &buffer_[tail_], end_count);
memcpy(&buf[end_count], &buffer_[0], begin_count);

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.cc:93: size_t
FIFOChar::Write(const char* buf, size_t len) {
tests for this class?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_char.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:13: class  FIFOChar {
nit: remove extra space

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:16: virtual ~FIFOChar();
why virtual? here and elsewhere.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:31: size_t Read(char* buf,
size_t len);
match MountNode::Read/Write?

Read(void* buf, size_t count)
Write(const void* buf, size_t count)

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:42: size_t tail_;   // Next
read location
I usually think of the head being the start of the data, and the tail being the
end. So the head is the next read location and the tail is the next write
location.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_packet.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.cc:29: return true;
why always return true here? What if len < cur_bytes_?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_packet.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:26: 
missing copy constructor and assignment operator. (Not needed if you use
std::vector instead)

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:31: char* buffer_;
use std::vector

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:41: virtual bool
IsEmpty();
why virtual?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:47: Packet* PeekPacket();
What is the Packet ownership? (for all these functions)

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:58: private:
nit: indent 1

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:59: std::list<UDPPacket*>
packets_;
UDPPacket? I don't see that defined anywhere.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:186:
handle0->Init(S_IREAD);
check return from Init?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:190: if (pipefds[0] ==
0)
AllocateFD can never return a zero FD.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:202: return -1;
set errno?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:898: if (info->revents &
POLLHUP) {
not POLLERR? You request it above.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:941: fd_info->revents =
fd_info->events & events;
change to "events & fd_info->events" like above?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:953: PollInfo* info =
&event_map[emitter.get()];
this is a lot of extra work to prevent sending the same emitter to WaitOnAny
more than once. I don't see why that should be a problem, though, and allowing
it will simplify this code quite a bit.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/library.dsc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/library.dsc:17: "fifo_char.cc",
fifo_packet.cc?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/library.dsc:127: "fifo_char.h",
fifo_packet.h?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_dev.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:299: // 
INITIALIZE_DEV_NODE("/tty", MountNodeTty);
add TODO?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node.cc:44: state_ |= perm &
(O_NONBLOCK);
where is state_ used?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node.h:112: uint32_t state_;
describe? state_ is pretty generic.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc:16: size_t
kDefaultPipeSize = 65536 * 8;
const size_t

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc:16: size_t
kDefaultPipeSize = 65536 * 8;
Why this size? Also why written this way -- why not 512 * 1024?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc:49: 
nit: remove extra line

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:25: protected:
nit: indent 1 space

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:26: Error
WaitOnEvent(uint32_t events, int ms);
remove, unimplemented

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:29: int read_timeout_;
remove, these are defined in MountNodeStream.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:32: friend class
KernelProxy;
why are these friends needed?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc:19:
read_timeout_(-1),
why use variables if these are never set?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc:23: Error
MountNodeStream::Init(int perm) {
remove, this is identical to MountNode::Init

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_stream.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:22: virtual
EventEmitter* GetEventEmitter() = 0;
Remove, these are identical to MountNode::{GetEventEmitter,Read,Write}

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:27: protected:
nit: indent 1 space

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:28: Error
WaitOnEvent(uint32_t events, int ms);
remove, unimplemented

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:30: int
read_timeout_;
private?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:33: friend class
KernelProxy;
why friends?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:227: //   
RaiseEvent(POLLIN);
Why removed? If necessary to remove, add TODO? Also use #if 0 for commenting
code.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.h (left):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:8: #include
"nacl_io/ossocket.h"
why removed? It is still checked in the .cc file

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:19: private:
why private? MountNode defines these as public

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:51: void
RegisterListener(EventListener* listener, uint32_t events) {
using EventEmitter::RegisterListener;

This makes RegisterListener public, and doesn't require a forwarding function.

See mount_node_test.cc

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:54: void
UnregisterListener(EventListener* listener) {
ditto

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:67: EventListenerTester
listenerA;
nit: listener_a

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:92: EventEmitterPipe*
pipe = new EventEmitterPipe(32);
If you're going to ignore the ref-count, why not just:

EventEmitterPipe pipe;

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:122: public:
nit: 1 space

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:123: TestMountStream()
{};
2 spaces

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:123: TestMountStream()
{};
remove ;

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:137: #define MAX_FDS 32
use const instead of define

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:164: pollfds[index].fd =
fds[0];
fds[index]?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:205: EXPECT_EQ(POLLOUT,
pollfds[1].revents);
doesn't match comment (probably from fds[index] bug above)

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:235: //  EXPECT_EQ(0,
FD_ISSET(fds[0], &wr_set));
why?

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/example.dsc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/example.dsc:34: #       
'mount_node_tty_test.cc',
Remove, the file is #if 0

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc:24: #if 0
add TODO

[noelallen1, 2013-09-12 23:19:02]

Fixed nits.
Add EventEmitterStream as parent for:
  EventEmitterUDP  (EventEmitterPacket)
  EventEmitterTCP  (New)
  EventEmitterPipe

Added FifoInterface as parent for
  FIFOChar
  FIFOPacket

Pushed signalling/block code as much as possible to MountNodeStream for
consolidation between Pipe,UDP,TCP.
(Moved GetEventStatus(), GetEventEmitter()...)

Added multi-threaded test for event emitter/listener.

Still TODO:
 Fix TTY
 Add individual FIFO tests.

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h (right):

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:24:
EventEmitterPacket(size_t rsize, size_t wsize)
On 2013/09/12 01:47:56, binji wrote:
> I'd prefer having a .cc file instead of defining this all in the header
That's in the follow on.

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:30: fifo_ = new
FIFOPacket(rsize);
On 2013/09/12 01:47:56, binji wrote:
> Why allocate?
> 
> If you define these as non-pointer members you can construct them like so:
> 
> rfifo_(std::max<size_t>(1, rsize)),
> wfifo_(std::max<size_t>(1, wsize)),
> 
> And then you can use the default destructor

I was allowing for more sharing in the parent class by letting the child
allocate specializations, but I don't think I need it here.

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:47: Packet*
ReadPacket() {
On 2013/09/12 01:47:56, binji wrote:
> what's the ownership of the returned packet?
I'll comment.

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:55: uint32_t
WritePacket(Packet* packet) {
On 2013/09/12 01:47:56, binji wrote:
> ownership of passed packet?

Done.

https://codereview.chromium.org/23498015/diff/14001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:91: #endif  //
LIBRARIES_NACL_IO_EVENT_EMITTER_PIPE_H_
On 2013/09/12 01:47:56, binji wrote:
> PACKET_H_

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:58: protected:
On 2013/09/12 01:47:57, binji wrote:
> private?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:47: Packet*
ReadTXPacket_Locked() {
On 2013/09/12 01:47:57, binji wrote:
> why TXPacket? ReadPacket is easier to read...

There are two FIFOs, an incoming and and outgoing.
You Write to the TX Fifo to Q for transmit.  You read from it on the MountStream
thread.
You Write to the RX Fifo from the MountStream thread to Q data, then read from
it on the actual thread calling Recv.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_packet.h:50:
UpdateStatusLocked();
On 2013/09/12 01:47:57, binji wrote:
> _Locked?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:49: lock_ = new
sdk_util::AutoLock(emitter->GetLock());
On 2013/09/12 01:47:57, binji wrote:
> move into initializer list above?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:115: explicit
EventListenerLock(EventEmitter* emitter);
On 2013/09/12 01:47:57, binji wrote:
> nit: two space indent here

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:122: public:
On 2013/09/12 01:47:57, binji wrote:
> remove

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:133: protected:
On 2013/09/12 01:47:57, binji wrote:
> private:

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:136: uint32_t events_;
On 2013/09/12 01:47:57, binji wrote:
> disable copy and assign?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:148: public:
On 2013/09/12 01:47:57, binji wrote:
> remove

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.h:157: protected:
On 2013/09/12 01:47:57, binji wrote:
> private:

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_char.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.cc:20: buffer_ = new
char[size];
On 2013/09/12 01:47:57, binji wrote:
> use a std::vector instead of an array.

Since this is a circular buffer, the resize case get's uglier.  I would first
need to move possibly across the edge, then shrink.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_char.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:13: class  FIFOChar {
On 2013/09/12 01:47:57, binji wrote:
> nit: remove extra space

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:16: virtual ~FIFOChar();
On 2013/09/12 01:47:57, binji wrote:
> why virtual? here and elsewhere.

The base 'interface' class has a virtual destructor.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_char.h:31: size_t Read(char* buf,
size_t len);
On 2013/09/12 01:47:57, binji wrote:
> match MountNode::Read/Write?
> 
> Read(void* buf, size_t count)
> Write(const void* buf, size_t count)

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_packet.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.cc:29: return true;
On 2013/09/12 01:47:57, binji wrote:
> why always return true here? What if len < cur_bytes_?

For Packet FIFOs, the requested size is a hint so we can always accept it,
unlike the Char FIFO which is a circular buffer.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_packet.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:26: 
On 2013/09/12 01:47:57, binji wrote:
> missing copy constructor and assignment operator. (Not needed if you use
> std::vector instead)

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:31: char* buffer_;
On 2013/09/12 01:47:57, binji wrote:
> use std::vector

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:41: virtual bool
IsEmpty();
On 2013/09/12 01:47:57, binji wrote:
> why virtual?
Derived from FIFOInterface (was in a different CL)

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:47: Packet* PeekPacket();
On 2013/09/12 01:47:57, binji wrote:
> What is the Packet ownership? (for all these functions)

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:58: private:
On 2013/09/12 01:47:57, binji wrote:
> nit: indent 1

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_packet.h:59: std::list<UDPPacket*>
packets_;
On 2013/09/12 01:47:57, binji wrote:
> UDPPacket? I don't see that defined anywhere.

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:186:
handle0->Init(S_IREAD);
On 2013/09/12 01:47:57, binji wrote:
> check return from Init?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:190: if (pipefds[0] ==
0)
On 2013/09/12 01:47:57, binji wrote:
> AllocateFD can never return a zero FD.

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:202: return -1;
On 2013/09/12 01:47:57, binji wrote:
> set errno?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:898: if (info->revents &
POLLHUP) {
On 2013/09/12 01:47:57, binji wrote:
> not POLLERR? You request it above.

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:941: fd_info->revents =
fd_info->events & events;
On 2013/09/12 01:47:57, binji wrote:
> change to "events & fd_info->events" like above?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc:953: PollInfo* info =
&event_map[emitter.get()];
On 2013/09/12 01:47:57, binji wrote:
> this is a lot of extra work to prevent sending the same emitter to WaitOnAny
> more than once. I don't see why that should be a problem, though, and allowing
> it will simplify this code quite a bit.

Having a one-to-one mapping between Listener and Emitter registration (getting
rid of EventInfo) greatly simplifies the emitter/listener code which is hard to
reason about.   Having it in would must migrate the complexity to a place where
we need to think about locking and signalling.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/library.dsc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/library.dsc:17: "fifo_char.cc",
On 2013/09/12 01:47:57, binji wrote:
> fifo_packet.cc?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/library.dsc:127: "fifo_char.h",
On 2013/09/12 01:47:57, binji wrote:
> fifo_packet.h?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_dev.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:299: // 
INITIALIZE_DEV_NODE("/tty", MountNodeTty);
On 2013/09/12 01:47:57, binji wrote:
> add TODO?

I plan to add this back before committing, so it will be removed in a subsequent
merge.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node.cc:44: state_ |= perm &
(O_NONBLOCK);
On 2013/09/12 01:47:57, binji wrote:
> where is state_ used?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node.h:112: uint32_t state_;
On 2013/09/12 01:47:57, binji wrote:
> describe? state_ is pretty generic.

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc:16: size_t
kDefaultPipeSize = 65536 * 8;
On 2013/09/12 01:47:57, binji wrote:
> const size_t

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc:16: size_t
kDefaultPipeSize = 65536 * 8;
On 2013/09/12 01:47:57, binji wrote:
> const size_t

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.cc:49: 
On 2013/09/12 01:47:57, binji wrote:
> nit: remove extra line

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:25: protected:
On 2013/09/12 01:47:57, binji wrote:
> nit: indent 1 space

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:26: Error
WaitOnEvent(uint32_t events, int ms);
On 2013/09/12 01:47:57, binji wrote:
> remove, unimplemented

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:29: int read_timeout_;
On 2013/09/12 01:47:57, binji wrote:
> remove, these are defined in MountNodeStream.

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_pipe.h:32: friend class
KernelProxy;
On 2013/09/12 01:47:57, binji wrote:
> why are these friends needed?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc:19:
read_timeout_(-1),
On 2013/09/12 01:47:57, binji wrote:
> why use variables if these are never set?

These can be set through IOCTLs, -1 is the default (Block forever)

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc:23: Error
MountNodeStream::Init(int perm) {
On 2013/09/12 01:47:57, binji wrote:
> remove, this is identical to MountNode::Init

Fixed

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_stream.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:22: virtual
EventEmitter* GetEventEmitter() = 0;
On 2013/09/12 01:47:57, binji wrote:
> Remove, these are identical to MountNode::{GetEventEmitter,Read,Write}

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:27: protected:
On 2013/09/12 01:47:57, binji wrote:
> nit: indent 1 space

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:28: Error
WaitOnEvent(uint32_t events, int ms);
On 2013/09/12 01:47:57, binji wrote:
> remove, unimplemented

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:30: int
read_timeout_;
On 2013/09/12 01:47:57, binji wrote:
> private?

Derived classes need them, and might modify them.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.h:33: friend class
KernelProxy;
On 2013/09/12 01:47:57, binji wrote:
> why friends?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tty.cc:227: //   
RaiseEvent(POLLIN);
On 2013/09/12 01:47:57, binji wrote:
> Why removed? If necessary to remove, add TODO? Also use #if 0 for commenting
> code.

Part of TTY CL.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.h (left):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:8: #include
"nacl_io/ossocket.h"
On 2013/09/12 01:47:57, binji wrote:
> why removed? It is still checked in the .cc file

There is no reason to #ifdef it out.    It will compile regardless.
In addition, MountNodePipes are derived from MountNodeStream, so we always need
this.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.h (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:19: private:
On 2013/09/12 01:47:57, binji wrote:
> why private? MountNode defines these as public

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:51: void
RegisterListener(EventListener* listener, uint32_t events) {
On 2013/09/12 01:47:57, binji wrote:
> using EventEmitter::RegisterListener;
> 
> This makes RegisterListener public, and doesn't require a forwarding function.
> 
> See mount_node_test.cc

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:54: void
UnregisterListener(EventListener* listener) {
On 2013/09/12 01:47:57, binji wrote:
> ditto

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:67: EventListenerTester
listenerA;
On 2013/09/12 01:47:57, binji wrote:
> nit: listener_a

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:92: EventEmitterPipe*
pipe = new EventEmitterPipe(32);
On 2013/09/12 01:47:57, binji wrote:
> If you're going to ignore the ref-count, why not just:
> 
> EventEmitterPipe pipe;

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:122: public:
On 2013/09/12 01:47:57, binji wrote:
> nit: 1 space

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:123: TestMountStream()
{};
On 2013/09/12 01:47:57, binji wrote:
> 2 spaces

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:123: TestMountStream()
{};
On 2013/09/12 01:47:57, binji wrote:
> remove ;

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:137: #define MAX_FDS 32
On 2013/09/12 01:47:57, binji wrote:
> use const instead of define

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:164: pollfds[index].fd =
fds[0];
On 2013/09/12 01:47:57, binji wrote:
> fds[index]?

doh!

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:205: EXPECT_EQ(POLLOUT,
pollfds[1].revents);
On 2013/09/12 01:47:57, binji wrote:
> doesn't match comment (probably from fds[index] bug above)

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:235: //  EXPECT_EQ(0,
FD_ISSET(fds[0], &wr_set));
On 2013/09/12 01:47:57, binji wrote:
> why?

Done.

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc (right):

https://codereview.chromium.org/23498015/diff/18001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc:24: #if 0
On 2013/09/12 01:47:57, binji wrote:
> add TODO

This is while I get the patch for TTY.  The actual check in should have TTY
working

[noelallen1, 2013-09-13 22:51:39]

ptal - final merge, contains test, tty fixes, and event/socket changes.

[binji, 2013-09-15 22:18:58]

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:44: // This returns a
snapshot, to ensure the status doens't change from
sp: doesn't

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h:30: virtual
FIFOInterface* rx_fifo() { return &fifo_; }
I don't really like rx/tx. How about read_fifo/write_fifo? or in_fifo/out_fifo?
Or receive/transmit?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h:31: virtual
FIFOInterface* tx_fifo() { return &fifo_; }
You can use covariant return types for all of these:

virtual FIFOChar* tx_fifo() {...}

That way if you have a EventEmitterPipe* you don't have to downcast.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_stream.cc:16:
EventEmitterStream::EventEmitterStream() : EventEmitter(), stream_(NULL) {}
nit: not necessary to specify EventEmitter() here.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_tcp.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tcp.h:22:
~EventEmitterTCP();
this destructor not defined.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tcp.h:24: uint32_t
ReadRXBytes_Locked(char* buffer, uint32_t len);
Bytes is superfluous. Maybe just Read(*)_Locked/Write(*)_Locked.

(where (*) could be rx/tx, but I'd prefer something else).

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_tty.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tty.h:26:
EventEmitterTTY(size_t size);
explicit

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tty.h:37:
DISALLOW_COPY_AND_ASSIGN(EventEmitterPipe);
EventEmitterTTY

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_udp.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_udp.h:23: // Takes or
gives away ownwership of the packet.
sp: ownership

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_interface.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_interface.h:15: // FIFOInterface
provides a common interface for Emitters to update thier
sp: their

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

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:277: 
nit: remove extra lines

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc:50: return
reinterpret_cast<MountStream*>(mount_);
static_cast

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:18: size_t
kMaxPacketSize = 65536;
const

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:26:
TCPWork(EventEmitterTCP* emitter)
explicit

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:41:
TCPSendWork(EventEmitterTCP* emitter) : TCPWork(emitter) {}
explicit

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:45: MountNodeTCP*
stream = reinterpret_cast<MountNodeTCP*>(emitter_->stream());
static_cast (though unnecessary if you use covariant returns on
EventEmitterTCP).

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:62: delete data;
Deleting the data sent to an async call?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:63: if (err ==
PP_OK_COMPLETIONPENDING)
any way to return this error in case it is not PP_OK_COMPLETIONPENDING?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:71: virtual void
Run(int32_t val) {
Run is a strange name for this; it isn't meant to be called directly. Maybe
OnCompleted?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:73: MountNodeTCP*
stream = reinterpret_cast<MountNodeTCP*>(emitter_->stream());
static_cast

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:85:
TCPRecvWork(EventEmitterTCP* emitter) : TCPWork(emitter), data_(NULL) {}
explicit

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:93: MountNodeTCP*
stream = reinterpret_cast<MountNodeTCP*>(emitter_->stream());
static_cast

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:108: data_,
nit: align params

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:121:
reinterpret_cast<MountNodeTCP*>(emitter_->stream());
static_cast

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:127: if (val > 0)
better name for val? (bytes_read or something?)

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:135: char* data_;
std::vector

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:155: return NULL;
Why no emitter?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:157: 
Looks like TCP is not yet implemented?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h:48: friend class
UDPSendWork;
It should be TCP*Work, but if it compiles it isn't needed.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:27:
UDPWork(EventEmitterUDP* emitter)
many of the comments on TCP apply here too (explicit, static_cast, naming, using
std::vector etc.)

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:27:
UDPWork(EventEmitterUDP* emitter)
UDPWork(const ScopedEventEmitterUDP& emitter)

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:61:
mount()->ppapi()->ReleaseResource(packet->addr_);
postpone cleanup until after SendTo completes?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:133: char* data_;
std::vector

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:165: UDPRecvWork* work
= new UDPRecvWork(emitter_.get());
pass scoped emitter directly instead of raw pointer

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:189: QueueInput();
why not queue input and output here?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:242: QueueInput();
The worker is already queuing up more work. Why here too?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:260: int ms =
(GetMode() & O_NONBLOCK) ? 0 : read_timeout_;
write_timeout_?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.cc:47: while(1)
no way to exit the loop?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.cc:61: if (message_loop_ ==
0) {
looks racy. What happens if two threads enqueue work? Both will enter the if,
then one will create a thread, then the other.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:21: class
MountStreamWork;
not needed, you define immediately after

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:24: class MountStreamWork
{
This name confused me at first, because it is named like the other Mounts. Not
sure of a better one, however.

You could make it a nested class of MountStream, then it would be
MountStream::Work.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:26:
MountStreamWork(MountStream* mount) : mount_(mount) {}
explicit

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:29: virtual bool
Start(int32_t val) = 0;
Document? What is return value? What is "val"?

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

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:102: #if 0
why removed?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:83:
pthread_cond_destroy(&multi_cond_);
pthread_cond_init

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:96: void CreateThread() {
It would be clearer if the function order were: CreateThread, ThreadThunk,
ThreadEntry.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:105: waiting_++;
atomic increment?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:126: sleep(1);
why?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:138: sleep(1);
can we do better than sleeping here?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:147: EXPECT_EQ(2,
signaled_);
clean up other threads?

[Sam Clegg, 2013-09-16 12:59:05]

I get two compile errors trying to build
with this CL:

kDefaultFifoSize not used.
~EventEmitterTCP() not defined.

[Sam Clegg, 2013-09-16 16:50:05]

On 2013/09/16 12:59:05, Sam Clegg wrote:
> I get two compile errors trying to build
> with this CL:
> 
> kDefaultFifoSize not used.
> ~EventEmitterTCP() not defined.

I tried patch set 11 with some tty using apps (lua and vim).  lua worked fine
but vim had issues.  I think that select() is not be interrupted correctly.

I wrote a new test to try and demostrate the problem:
https://codereview.chromium.org/23496060

The ResizeDuringSelect test passes on HEAD but fails with patch set 11.  I am
writing a similar test for InputDuringSelect which I suspect will also fail.

[Sam Clegg, 2013-09-16 18:03:32]

On 2013/09/16 16:50:05, Sam Clegg wrote:
> On 2013/09/16 12:59:05, Sam Clegg wrote:
> > I get two compile errors trying to build
> > with this CL:
> > 
> > kDefaultFifoSize not used.
> > ~EventEmitterTCP() not defined.
> 
> I tried patch set 11 with some tty using apps (lua and vim).  lua worked fine
> but vim had issues.  I think that select() is not be interrupted correctly.
> 
> I wrote a new test to try and demostrate the problem:
> https://codereview.chromium.org/23496060
> 
> The ResizeDuringSelect test passes on HEAD but fails with patch set 11.  I am
> writing a similar test for InputDuringSelect which I suspect will also fail.

Actually it looks like this isn't the issue that is causing vim to fail.

Vim seems to be calling select(tty) and getting a positve result even when the
TTY
is not readable.  It seems to get into this state after the initial window
resize
event.  There is some tracing I added (probably not that useful to you).  I will
work on a test case for this tomorrow (so you should have it before you get to
work).

1 ps: Got Message
2 got TIOCSWINSZ
3 sending event
4 event sent
5 done wait.WaitOnAny
6 select error: Interrupted system call
7 select: (nil) 0 0
8 select done: 1
9 tty read

On line 6 the current call to select is interrupted correctly.  Then vim
calls select again with an infinite timeout (line 7).   Line 8 is select
returning that stdin is readable, which is the bug.  stdin is not readable
as nothing has been sent to it.  This results in vim calling read() which
then blocks forever.   So somehow the readability bit of the TTY is turned
on by the interrupted syscall?

[Sam Clegg, 2013-09-17 14:45:12]

On 2013/09/16 18:03:32, Sam Clegg wrote:
> On 2013/09/16 16:50:05, Sam Clegg wrote:
> > On 2013/09/16 12:59:05, Sam Clegg wrote:
> > > I get two compile errors trying to build
> > > with this CL:
> > > 
> > > kDefaultFifoSize not used.
> > > ~EventEmitterTCP() not defined.
> > 
> > I tried patch set 11 with some tty using apps (lua and vim).  lua worked
fine
> > but vim had issues.  I think that select() is not be interrupted correctly.
> > 
> > I wrote a new test to try and demostrate the problem:
> > https://codereview.chromium.org/23496060
> > 
> > The ResizeDuringSelect test passes on HEAD but fails with patch set 11.  I
am
> > writing a similar test for InputDuringSelect which I suspect will also fail.
> 
> Actually it looks like this isn't the issue that is causing vim to fail.
> 
> Vim seems to be calling select(tty) and getting a positve result even when the
> TTY
> is not readable.  It seems to get into this state after the initial window
> resize
> event.  There is some tracing I added (probably not that useful to you).  I
will
> work on a test case for this tomorrow (so you should have it before you get to
> work).
> 
> 1 ps: Got Message
> 2 got TIOCSWINSZ
> 3 sending event
> 4 event sent
> 5 done wait.WaitOnAny
> 6 select error: Interrupted system call
> 7 select: (nil) 0 0
> 8 select done: 1
> 9 tty read
> 
> On line 6 the current call to select is interrupted correctly.  Then vim
> calls select again with an infinite timeout (line 7).   Line 8 is select
> returning that stdin is readable, which is the bug.  stdin is not readable
> as nothing has been sent to it.  This results in vim calling read() which
> then blocks forever.   So somehow the readability bit of the TTY is turned
> on by the interrupted syscall?

I think I fixed the issues with vim by making two minor changes here:
https://codereview.chromium.org/23875034

[noelallen1, 2013-09-17 21:21:53]

Thanks Sam, I came to the same conclusion.  
ptal

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:44: // This returns a
snapshot, to ensure the status doens't change from
On 2013/09/15 22:18:58, binji wrote:
> sp: doesn't

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h:30: virtual
FIFOInterface* rx_fifo() { return &fifo_; }
On 2013/09/15 22:18:58, binji wrote:
> I don't really like rx/tx. How about read_fifo/write_fifo? or
in_fifo/out_fifo?
> Or receive/transmit?

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_pipe.h:31: virtual
FIFOInterface* tx_fifo() { return &fifo_; }
On 2013/09/15 22:18:58, binji wrote:
> You can use covariant return types for all of these:
> 
> virtual FIFOChar* tx_fifo() {...}
> 
> That way if you have a EventEmitterPipe* you don't have to downcast.

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_stream.cc:16:
EventEmitterStream::EventEmitterStream() : EventEmitter(), stream_(NULL) {}
On 2013/09/15 22:18:58, binji wrote:
> nit: not necessary to specify EventEmitter() here.

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_tcp.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tcp.h:22:
~EventEmitterTCP();
On 2013/09/15 22:18:58, binji wrote:
> this destructor not defined.

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_tty.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tty.h:26:
EventEmitterTTY(size_t size);
On 2013/09/15 22:18:58, binji wrote:
> explicit

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_tty.h:37:
DISALLOW_COPY_AND_ASSIGN(EventEmitterPipe);
On 2013/09/15 22:18:58, binji wrote:
> EventEmitterTTY

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/event_emitter_udp.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_emitter_udp.h:23: // Takes or
gives away ownwership of the packet.
On 2013/09/15 22:18:58, binji wrote:
> sp: ownership

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/fifo_interface.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/fifo_interface.h:15: // FIFOInterface
provides a common interface for Emitters to update thier
On 2013/09/15 22:18:58, binji wrote:
> sp: their

Done.

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

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:277: 
On 2013/09/15 22:18:58, binji wrote:
> nit: remove extra lines

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_stream.cc:50: return
reinterpret_cast<MountStream*>(mount_);
On 2013/09/15 22:18:58, binji wrote:
> static_cast

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:18: size_t
kMaxPacketSize = 65536;
On 2013/09/15 22:18:58, binji wrote:
> const

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:26:
TCPWork(EventEmitterTCP* emitter)
On 2013/09/15 22:18:58, binji wrote:
> explicit

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:41:
TCPSendWork(EventEmitterTCP* emitter) : TCPWork(emitter) {}
On 2013/09/15 22:18:58, binji wrote:
> explicit

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:45: MountNodeTCP*
stream = reinterpret_cast<MountNodeTCP*>(emitter_->stream());
On 2013/09/15 22:18:58, binji wrote:
> static_cast (though unnecessary if you use covariant returns on
> EventEmitterTCP).

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:62: delete data;
On 2013/09/15 22:18:58, binji wrote:
> Deleting the data sent to an async call?
Consolidated into base class.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:63: if (err ==
PP_OK_COMPLETIONPENDING)
On 2013/09/15 22:18:58, binji wrote:
> any way to return this error in case it is not PP_OK_COMPLETIONPENDING?

Will log once we officially have logging.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:71: virtual void
Run(int32_t val) {
On 2013/09/15 22:18:58, binji wrote:
> Run is a strange name for this; it isn't meant to be called directly. Maybe
> OnCompleted?

MountStream::GetOnCompletedCompletion?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:73: MountNodeTCP*
stream = reinterpret_cast<MountNodeTCP*>(emitter_->stream());
On 2013/09/15 22:18:58, binji wrote:
> static_cast

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:85:
TCPRecvWork(EventEmitterTCP* emitter) : TCPWork(emitter), data_(NULL) {}
On 2013/09/15 22:18:58, binji wrote:
> explicit

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:93: MountNodeTCP*
stream = reinterpret_cast<MountNodeTCP*>(emitter_->stream());
On 2013/09/15 22:18:58, binji wrote:
> static_cast

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:108: data_,
On 2013/09/15 22:18:58, binji wrote:
> nit: align params

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:121:
reinterpret_cast<MountNodeTCP*>(emitter_->stream());
On 2013/09/15 22:18:58, binji wrote:
> static_cast

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:127: if (val > 0)
On 2013/09/15 22:18:58, binji wrote:
> better name for val? (bytes_read or something?)

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:155: return NULL;
On 2013/09/15 22:18:58, binji wrote:
> Why no emitter?

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:157: 
On 2013/09/15 22:18:58, binji wrote:
> Looks like TCP is not yet implemented?

Bind is not allowed on client sockets.  There's no API for it for TCP.
Added comment.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h:48: friend class
UDPSendWork;
On 2013/09/15 22:18:58, binji wrote:
> It should be TCP*Work, but if it compiles it isn't needed.

Not needed, I've been removing friends all over.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:27:
UDPWork(EventEmitterUDP* emitter)
On 2013/09/15 22:18:58, binji wrote:
> many of the comments on TCP apply here too (explicit, static_cast, naming,
using
> std::vector etc.)

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:61:
mount()->ppapi()->ReleaseResource(packet->addr_);
On 2013/09/15 22:18:58, binji wrote:
> postpone cleanup until after SendTo completes?

Moved referencing to Packet class.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:133: char* data_;
On 2013/09/15 22:18:58, binji wrote:
> std::vector

Using char* allows me to "take" the buffer.  Otherwise I would need to copy at
the vector, or construct a vector from a const char* and swap.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:165: UDPRecvWork* work
= new UDPRecvWork(emitter_.get());
On 2013/09/15 22:18:58, binji wrote:
> pass scoped emitter directly instead of raw pointer

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:189: QueueInput();
On 2013/09/15 22:18:58, binji wrote:
> why not queue input and output here?

Output doesn't make sense.  There's no data yet.  I already must Q on every
write so why Q anywhere else?

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:242: QueueInput();
On 2013/09/15 22:18:58, binji wrote:
> The worker is already queuing up more work. Why here too?

This is safer.  Instead of making the requesting logic complicated, we always
request on the assumption the request will be ignored if there is one waiting.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:260: int ms =
(GetMode() & O_NONBLOCK) ? 0 : read_timeout_;
On 2013/09/15 22:18:58, binji wrote:
> write_timeout_?

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.cc:47: while(1)
On 2013/09/15 22:18:58, binji wrote:
> no way to exit the loop?

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.cc:61: if (message_loop_ ==
0) {
On 2013/09/15 22:18:58, binji wrote:
> looks racy. What happens if two threads enqueue work? Both will enter the if,
> then one will create a thread, then the other.

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.h (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:21: class
MountStreamWork;
On 2013/09/15 22:18:58, binji wrote:
> not needed, you define immediately after

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:24: class MountStreamWork
{
On 2013/09/15 22:18:58, binji wrote:
> This name confused me at first, because it is named like the other Mounts. Not
> sure of a better one, however.
> 
> You could make it a nested class of MountStream, then it would be
> MountStream::Work.

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:26:
MountStreamWork(MountStream* mount) : mount_(mount) {}
On 2013/09/15 22:18:58, binji wrote:
> explicit

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.h:29: virtual bool
Start(int32_t val) = 0;
On 2013/09/15 22:18:58, binji wrote:
> Document? What is return value? What is "val"?

Done.

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

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:102: #if 0
On 2013/09/15 22:18:58, binji wrote:
> why removed?

Stale change version.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:83:
pthread_cond_destroy(&multi_cond_);
On 2013/09/15 22:18:58, binji wrote:
> pthread_cond_init

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:96: void CreateThread() {
On 2013/09/15 22:18:58, binji wrote:
> It would be clearer if the function order were: CreateThread, ThreadThunk,
> ThreadEntry.

Done.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:105: waiting_++;
On 2013/09/15 22:18:58, binji wrote:
> atomic increment?

I'm holding the emitter lock.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:126: sleep(1);
On 2013/09/15 22:18:58, binji wrote:
> why?

Tests are written such that threads that should make progress, must make
progress for the test to move forward, otherwise the tester timeout triggers. 
The sleep give threads that shouldn't move forward a chance to move forward and
trigger a failure.   Removing the sleep would make the test run faster, but
reduces the chance of catching a race.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:138: sleep(1);
On 2013/09/15 22:18:58, binji wrote:
> can we do better than sleeping here?

Not without serious changes to pthread_cond, and/or adding a wrapping class that
supports debugging.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:147: EXPECT_EQ(2,
signaled_);
On 2013/09/15 22:18:58, binji wrote:
> clean up other threads?

The other threads just return after running the test.

[binji, 2013-09-19 00:48:54]

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:157: 
On 2013/09/17 21:21:54, noelallen1 wrote:
> On 2013/09/15 22:18:58, binji wrote:
> > Looks like TCP is not yet implemented?
> 
> Bind is not allowed on client sockets.  There's no API for it for TCP.
> Added comment.

Sorry, I wasn't talking about Bind, I meant non-blocking TCP is not yet
implemented. Send/Recv below still use BlockUntilComplete.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.cc:61: if (message_loop_ ==
0) {
On 2013/09/17 21:21:54, noelallen1 wrote:
> On 2013/09/15 22:18:58, binji wrote:
> > looks racy. What happens if two threads enqueue work? Both will enter the
if,
> > then one will create a thread, then the other.
> 
> Done.

Probably should use pthread_once_t instead of double-checked locking here
instead:
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_once.html

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:147: EXPECT_EQ(2,
signaled_);
On 2013/09/17 21:21:54, noelallen1 wrote:
> On 2013/09/15 22:18:58, binji wrote:
> > clean up other threads?
> 
> The other threads just return after running the test.

Aren't they all blocked on WaitOnEvent?

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

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:114:
stream->ClearStreamFlags(SSF_RECVING);
move inside for failure case

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/packet.cc (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.cc:19:
ppapi_->ReleaseResource(addr_);
delete[] buffer_;

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/packet.h (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.h:11: #include <vector>
remove unneeded headers

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.h:30: void Copy(const void
*buffer, size_t len, PP_Resource addr);
turn this into an alternate constructor?

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.h:33: void Take(const void
*buffer, size_t len, PP_Resource addr);
unused. Remove?

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
File native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc:228: // Read
byte form tty.
sp: from

[noelallen1, 2013-09-19 21:29:26]

Consolidated Send/Recv locking, waiting, and errors into MountNodeSocket.

Updated error handling in UDP & TCP Work classes.
Added TestStreamFlags for cleanup.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:157: 
On 2013/09/19 00:48:54, binji wrote:
> On 2013/09/17 21:21:54, noelallen1 wrote:
> > On 2013/09/15 22:18:58, binji wrote:
> > > Looks like TCP is not yet implemented?
> > 
> > Bind is not allowed on client sockets.  There's no API for it for TCP.
> > Added comment.
> 
> Sorry, I wasn't talking about Bind, I meant non-blocking TCP is not yet
> implemented. Send/Recv below still use BlockUntilComplete.

Ah I see, fixed.

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_stream.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_stream.cc:61: if (message_loop_ ==
0) {
On 2013/09/19 00:48:54, binji wrote:
> On 2013/09/17 21:21:54, noelallen1 wrote:
> > On 2013/09/15 22:18:58, binji wrote:
> > > looks racy. What happens if two threads enqueue work? Both will enter the
> if,
> > > then one will create a thread, then the other.
> > 
> > Done.
> 
> Probably should use pthread_once_t instead of double-checked locking here
> instead:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_once.html

The once init routine doesn't take a parameter, so I don't have a clean way to
call a member init function.   I can't make it static because I may have more
than one mount, for example testing...

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/47001/native_client_sdk/src/tes...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:147: EXPECT_EQ(2,
signaled_);
On 2013/09/19 00:48:54, binji wrote:
> On 2013/09/17 21:21:54, noelallen1 wrote:
> > On 2013/09/15 22:18:58, binji wrote:
> > > clean up other threads?
> > 
> > The other threads just return after running the test.
> 
> Aren't they all blocked on WaitOnEvent?

Done.

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

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:114:
stream->ClearStreamFlags(SSF_RECVING);
On 2013/09/19 00:48:55, binji wrote:
> move inside for failure case

Done.

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/packet.cc (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.cc:19:
ppapi_->ReleaseResource(addr_);
On 2013/09/19 00:48:55, binji wrote:
> delete[] buffer_;

Done.

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/packet.h (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.h:11: #include <vector>
On 2013/09/19 00:48:55, binji wrote:
> remove unneeded headers

Done.

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.h:33: void Take(const void
*buffer, size_t len, PP_Resource addr);
On 2013/09/19 00:48:55, binji wrote:
> unused. Remove?

I was using this... I'll dbl check and remote.  Originally I need to construct
the packet and Copy or Take.

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
File native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
native_client_sdk/src/tests/nacl_io_test/mount_node_tty_test.cc:228: // Read
byte form tty.
On 2013/09/19 00:48:55, binji wrote:
> sp: from

Done.

[binji, 2013-09-19 22:40:25]

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

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:295: bool same =
IsEquivalentAddress(addr, remote_addr_);
This behavior is gone now? Is it necessary?

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:130:
AUTO_LOCK(emitter_.GetLock());
This doesn't seem right. You can't pthread_cond_wait without holding the lock,
right?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:54: // Normal
read/write operations on a Socket are equivelent to
sp: equivalent

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:223: return
RecvFrom(buf, len, flags, NULL, 0, out_len);
forward to RecvHelper directly? Send() forwards to SendHelper, not SendTo

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:226: Error
MountNodeSocket::RecvFrom(void* buffer,
buffer vs. buf? Either is fine, but might as well be consistent.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:291: PP_Resource
out_addr = SockAddrToResource(dest_addr, addrlen);
out_addr makes it look like an output parameter. Maybe just addr (consistent
with RecvFrom above)

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:335:
SetStreamFlags(SSF_ERROR | SSF_CLOSED);
OK for all these changes to be unlocked?

Ah, it seems that this usually (always?) called locked already. SetError_Locked?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.h (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:89: // Wraps common
error checks, timeouts, work pump for send.
Ah, I like this much better. Easier to see the commonalities between socket
types. :)

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:59: bool wth =
stream->TestStreamFlags(SSF_CAN_SEND);
wth?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:98:
stream->QueueOutput();
why does TCPSendWork QueueOutput in the Run callback, but TCPRecvWork doesn't
QueueInput?

Doesn't the MountNodeSocket handle this now?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:120: // If the stream
is valid and we are not currently receiving
bad comment, just "if not currently receiving"

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:126: if (capped_len >
0) {
why did you flip this if? (TCPSendWork uses an if-guard like before)

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:251: PP_Resource*
addr,
out_addr?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h:40: PP_Resource* addr,
out_addr?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:262: return EBADF;
When is this possible? Nothing better than EBADF?

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/te...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/te...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:211:
TEST_F(SocketTestTCP, Connect) {
DISABLE_Connect?

[noelallen1, 2013-09-20 00:51:26]

ptal

I'm going to circle back on Packet constructor thing on another CL when I take a
look at 295177 (udp connect), I'll need to play with packets at that point.

At that point I also plan to add a MountNodeSocketTest, to nacl_io_test, since
the consolidation should allow me to test the logic without needing the TCP or
UDP interface.

TCP tests are back to passing but disabled since they can't run on the bots yet
(as per previous comments).

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

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:295: bool same =
IsEquivalentAddress(addr, remote_addr_);
On 2013/09/19 22:40:25, binji wrote:
> This behavior is gone now? Is it necessary?
Not really needed for first pass.
BUG=295177

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/packet.h (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/packet.h:30: void Copy(const void
*buffer, size_t len, PP_Resource addr);
On 2013/09/19 00:48:55, binji wrote:
> turn this into an alternate constructor?

Going to circle back on this.

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
File native_client_sdk/src/tests/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/23498015/diff/1029/native_client_sdk/src/test...
native_client_sdk/src/tests/nacl_io_test/event_test.cc:130:
AUTO_LOCK(emitter_.GetLock());
On 2013/09/19 22:40:25, binji wrote:
> This doesn't seem right. You can't pthread_cond_wait without holding the lock,
> right?
Good catch, accidental delete.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:54: // Normal
read/write operations on a Socket are equivelent to
On 2013/09/19 22:40:25, binji wrote:
> sp: equivalent

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:223: return
RecvFrom(buf, len, flags, NULL, 0, out_len);
On 2013/09/19 22:40:25, binji wrote:
> forward to RecvHelper directly? Send() forwards to SendHelper, not SendTo

Send forward directly because of a small difference between the two.
For the recvfrom case, the NULL is valid.  For sendto it's not.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:226: Error
MountNodeSocket::RecvFrom(void* buffer,
On 2013/09/19 22:40:25, binji wrote:
> buffer vs. buf? Either is fine, but might as well be consistent.

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:291: PP_Resource
out_addr = SockAddrToResource(dest_addr, addrlen);
On 2013/09/19 22:40:25, binji wrote:
> out_addr makes it look like an output parameter. Maybe just addr (consistent
> with RecvFrom above)

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.cc:335:
SetStreamFlags(SSF_ERROR | SSF_CLOSED);
On 2013/09/19 22:40:25, binji wrote:
> OK for all these changes to be unlocked?
> 
> Ah, it seems that this usually (always?) called locked already.
SetError_Locked?

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_socket.h (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_socket.h:89: // Wraps common
error checks, timeouts, work pump for send.
On 2013/09/19 22:40:25, binji wrote:
> Ah, I like this much better. Easier to see the commonalities between socket
> types. :)

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:59: bool wth =
stream->TestStreamFlags(SSF_CAN_SEND);
On 2013/09/19 22:40:25, binji wrote:
> wth?

Still debugging TCP. I take it you didn't notice the change title.  :)

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:98:
stream->QueueOutput();
On 2013/09/19 22:40:25, binji wrote:
> why does TCPSendWork QueueOutput in the Run callback, but TCPRecvWork doesn't
> QueueInput?
> 
> Doesn't the MountNodeSocket handle this now?

MountNodeSocket handles the foreground thread side (recv more because I just
added something to the Q), Work objs handle the background thread side (recv
again, I'm free to issue another completion request).

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:120: // If the stream
is valid and we are not currently receiving
On 2013/09/19 22:40:25, binji wrote:
> bad comment, just "if not currently receiving"

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:126: if (capped_len >
0) {
On 2013/09/19 22:40:25, binji wrote:
> why did you flip this if? (TCPSendWork uses an if-guard like before)

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.cc:251: PP_Resource*
addr,
On 2013/09/19 22:40:25, binji wrote:
> out_addr?

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_tcp.h:40: PP_Resource* addr,
On 2013/09/19 22:40:25, binji wrote:
> out_addr?

Done.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/mount_node_udp.cc:262: return EBADF;
On 2013/09/19 22:40:25, binji wrote:
> When is this possible? Nothing better than EBADF?

packet should never be NULL.  This should never happen, Recv_Locked should not
be called unless a packet is waiting.

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/te...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/23498015/diff/107001/native_client_sdk/src/te...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:211:
TEST_F(SocketTestTCP, Connect) {
On 2013/09/19 22:40:25, binji wrote:
> DISABLE_Connect?

Done.

[binji, 2013-09-20 00:59:32]

OK, lgtm. :)

[binji, 2013-09-20 01:00:22]

Oh, BTW. Take a look at the CL description -- it seems like it may be
out-of-date.

[commit-bot: I haz the power, 2013-09-20 19:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelallen@chromium.org/23498015/133001

[commit-bot: I haz the power, 2013-09-21 00:25:04]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[noelallen1, 2013-09-21 17:17:40]

Committed patchset #20 manually as r224603 (presubmit successful).