[NaCl SDK} Add EventListener and EventEmitter to support epoll.

Add EventListener and EventEmitter to support epoll.

NOTRY=true
BUG=257723
R=binji@chromium.org

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

[noelallen1, 2013-07-17 16:24:19]

PTAL, you might want to start with the info in event_listener.h and go from
there.

[Sam Clegg, 2013-07-17 17:34:56]

m

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.cc:4: */
C++ comments.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.cc:7: #include
"nacl_io/event_emitter.h"
I think event_emitter.h is supposed to be the very first #include.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:3: * found in the
LICENSE file.
The convention for C++ headers seem to be to use single line C++
commets for the copyright header.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:18: class EventInfo;
Remove since its is declared just below?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:25: volatile uint32_t
events;
Is this a bitfield of KernnelEventTypes?  Perhaps a comment to say so.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:26: volatile uint32_t
filter;
Didn't we come to the conclusion that volatile is not useful, except for people
accessing direct hardware memory (i.e. explicitly not useful in the context of
multi-threading):

http://stackoverflow.com/questions/4557979/when-to-use-volatile-with-multi-th...

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:46: typedef
std::set<ScopedEventInfo> EventInfoSet_t;
Isn't this is duplicate of the typedef 10 lines above?  Is it needed?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:62: // Returns the
current state of the emitter.
The state is a bitfield of KennelEventType?  Perhaps a typedef for this bitfield
would make the code more readable?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:3: * found in the
LICENSE file.
C++ comment

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:4: */
C++ comment

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:14: #include
"nacl_io/event_listener.h"
Put this header first.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:14: #include
"nacl_io/event_listener.h"
put this header first?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:45: // is an event
waiting.
Why always writable?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:116: while ((0 ==
*event_cnt) && ms_timeout) {
be consistent about comparing ms_timeout explicitly with 0 (or not).

Is our convention to explicitly compare with 0?

Also be consistent about putting 0/NULL before or after ==.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:124: int retCode;
return_code?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:182: return EPERM;
Can't you add regular files to an epoll set?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:243: 
Remove trailing newlines.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:4: */
C++ comments

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:25: // specific events
from these objects which are derived from  EventEmitters.
Extra space after "derived from".

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:67: KE_SHUTDOWN = 4
Is our convention for name enums in the plural like this?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:73: uint64_t data;
Maybe user_data?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:82: // Mapping from an
id to an EventInfo
Remove comment?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:101: Error
Wait(EventData* events, int max, int ms_timeout, int* count);
What are max and count?

Is there a way to wait forever?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:108: uint64_t data);
user_data?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:132: volatile uint32_t
waiting_;
Remove volatile?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:140: #endif  /*
LIBRARIES_NACL_IO_EVENT_LISTENER_H_ */
C++ comment

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:4: */
C++ comment

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:4: */
C++ comment

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:133:
EXPECT_EQ(SIG_FILTER, ev[1].events);
There are rather a lot of assertions in this one test.  Can it be split up into
chunks with useful names?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:258:
EXPECT_GT(MAX_MS_TIMEOUT + MAX_MS_TIMEOUT, duration);
ditto.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/sdk_util/scoped_ref.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/sdk_util/scoped_ref.h:4: */
C++ comment

[noelallen1, 2013-07-17 21:00:18]

Leaving out C++ comment style and include order since they are consistent with
the rest of nacl_io.

If we want to change those, it should be a separate CL on the whole library
and/or SDK.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.cc:4: */
On 2013/07/17 17:34:56, Sam Clegg wrote:
> C++ comments.

We use C comments here throughout nacl_io

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.cc:7: #include
"nacl_io/event_emitter.h"
On 2013/07/17 17:34:56, Sam Clegg wrote:
> I think event_emitter.h is supposed to be the very first #include.
Consistent with nacl_io, we could create a separate CL to change this.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:45: // is an event
waiting.
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Why always writable?

You can always modify the EventListener, and write(efd...) will never block.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:124: int retCode;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> return_code?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:182: return EPERM;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Can't you add regular files to an epoll set?

man epoll_ctl: 
  EPERM  The target file fd does not support epoll.

Verified by testing on Linux as well.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:243: 
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Remove trailing newlines.

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:25: // specific events
from these objects which are derived from  EventEmitters.
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Extra space after "derived from".

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:67: KE_SHUTDOWN = 4
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Is our convention for name enums in the plural like this?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:73: uint64_t data;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Maybe user_data?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:82: // Mapping from an
id to an EventInfo
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Remove comment?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:101: Error
Wait(EventData* events, int max, int ms_timeout, int* count);
On 2013/07/17 17:34:56, Sam Clegg wrote:
> What are max and count?
> 
> Is there a way to wait forever?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:108: uint64_t data);
On 2013/07/17 17:34:56, Sam Clegg wrote:
> user_data?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:132: volatile uint32_t
waiting_;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Remove volatile?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:133:
EXPECT_EQ(SIG_FILTER, ev[1].events);
On 2013/07/17 17:34:56, Sam Clegg wrote:
> There are rather a lot of assertions in this one test.  Can it be split up
into
> chunks with useful names?

I'm verifying the behavior of only a single call:
   listener->Wait(ev, MAX_EVENTS, 0, &event_cnt);

The event_cnt is expected, and the data in ev[0], and ev[1] is expected.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:258:
EXPECT_GT(MAX_MS_TIMEOUT + MAX_MS_TIMEOUT, duration);
On 2013/07/17 17:34:56, Sam Clegg wrote:
> ditto.

Done.

[binji, 2013-07-17 22:23:20]

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.cc:52: // Provided to
allow one EventEmitter to register the same EventInfo with
might be more valuable to move this comment to the header.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:23: struct EventInfo :
public RefObject {
might want to mention that this is not POD, because it derives from RefObject
(which is not POD)

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:41: // by registering
EventInfo objects and signalling the EventListener when
sp: whenever their state

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:41: // by registering
EventInfo objects and signalling the EventListener when
signalled or signaled are both correct, but signaled seems to be used more here.

Same with signalling vs. signaling.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:54: // Regisiter or
unregister an EventInfo.  The lock of the EventListener
sp: Register

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:65: // REturns the type
of the emitter (compatible with st_mode in stat)
nit: Returns

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:73: virtual void
ChainRegister(const ScopedEventInfo& event);
I'd prefer a name that made it clear that it was called by RegisterEventInfo:

RegisterEventInfo_Chain, maybe?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:31: // We must always
hold the EventListener lock prior to calling then
s/then/the/

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:33: AutoLock
lock(info_lock_);
Is this necessary? If we are destroying doesn't that mean that this thread holds
the only reference to EventListener? Or can external users call the public
methods of EventListener without ref-counting it?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:61:
pthread_cond_signal(&signal_cond_);
pthread_cond_broadcast?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:65: Error
EventListener::Wait(EventData* events,
This interface could be simplified by passing a std::vector<EventData>* here.
Then max is irrelevant, and event_cnt is the resulting length.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:68: int* event_cnt) {
I've been using out_... for out parameters. Also, this name should match the
name in the header.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:96: if ((*event_cnt >=
max) || (ms_timeout == 0))
The code and comment do not align: Is this supposed to return if anything is
available, or if events is completely full?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:113: }
helper function for this?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:115: // Keep waiting
while we have time left, or no events captured
This comment does not align with the code -- ms_timeout never changes, so it's
not really "time left".

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:221: if
(it->second->emitter)
is this check necessary? It looks like event_info_ should never have an
EventInfo with a NULL emitter (The EventInfo is removed when it is abandoned.)

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:234: event->emitter =
NULL;
why? Who else could be using this EventInfo?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:22: //   Certain file
objects such as pipes or sockets can become signaled when
why indented?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:45: //    RegisterInfo
Object (Track/Update/Free)
I'm having a hard time following this because the names are not explicit. I know
it's more verbose, but maybe:

EventListener::{Track, Update, Free}
  AUTO_LOCK(EventListener::info_lock_)
    EventEmitter::RegisterEventInfo
      AUTO_LOCK(EventEmitter::emitter_lock_)

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:46: //       Ref
Emitter
Looking at the code below, where does EventListener::Track reference the
emitter? It is given a const reference, so it is just sharing the reference with
the caller.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:50: //    AbandonInfo
called AbandonedEventInfo below

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:62: class
EventListener;
remove

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:80: // from
EventEmitter objects.
document why EventListener is also an EventEmitter

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:92: // Declated in
EventEmitter
sp: Declared

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:106: const
ScopedRef<EventEmitter>& emitter,
why not ScopedEventEmitter?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:125: EventInfoMap_t
event_info_;
might be nice to mention "map" in this name somewhere. event_info_map_?
info_map_?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:127: // Protects
waiting_, signaled_ and used with the cond_var.
nit: "used with signal_cond_"

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:30: void
RaiseEvent(uint32_t events) {
just use "using EventEmitter::RaiseEvent;"

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:81: EXPECT_EQ(0,
listener->Update(FD_EMITTER, SIG_FILTER, SIG_DATA));
test that the values change?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:91: EXPECT_EQ(EINVAL,
listener->Wait(ev, 0, 0, &event_cnt));
might be nice to mention that these are all without timeouts. Maybe just as a
new test?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:108:
emitter->SetEventStatus(3);
SIG_FILTER?

Though maybe an explicit test that events are filtered: e.g.

SetEventStatus(1) -> no events signalled
SetEventStatus(3) -> one event signalled

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:184: // NOTE:  These
timing tests are potentially flacky, the real test is
sp: flaky

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:230: EXPECT_GT(25,
duration);
const for 25 too?

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/sdk_util/scoped_ref.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/sdk_util/scoped_ref.h:74: bool operator<(const
ScopedRef& other) const {
Now that I've seen the use case, you could just add this to EventInfo directly
(rather than add it for all RefObjects).

[noelallen1, 2013-07-18 22:18:15]

Fixed Mount/Node release order issue in KernelHandle.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.cc:52: // Provided to
allow one EventEmitter to register the same EventInfo with
On 2013/07/17 22:23:20, binji wrote:
> might be more valuable to move this comment to the header.

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:31: // We must always
hold the EventListener lock prior to calling then
On 2013/07/17 22:23:20, binji wrote:
> s/then/the/

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:33: AutoLock
lock(info_lock_);
On 2013/07/17 22:23:20, binji wrote:
> Is this necessary? If we are destroying doesn't that mean that this thread
holds
> the only reference to EventListener? Or can external users call the public
> methods of EventListener without ref-counting it?

True, there should be no references to the listener at this point.
Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:61:
pthread_cond_signal(&signal_cond_);
On 2013/07/17 22:23:20, binji wrote:
> pthread_cond_broadcast?

Good question...  Do we have multiple threads waiting on the same Listener get
fired off in order, or do they all go...  Probably broadcast work would better
here than chain signalling.

The correct thing here is to broadcast, then one thread at a time will empty the
event set until the last one throws the remainders away.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:65: Error
EventListener::Wait(EventData* events,
This matches the final 'C' interface.  A vector would cause an extra copy, as
well as causing us to "loose" events since they would get dropped later.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:68: int* event_cnt) {
On 2013/07/17 22:23:20, binji wrote:
> I've been using out_... for out parameters. Also, this name should match the
> name in the header.

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:96: if ((*event_cnt >=
max) || (ms_timeout == 0))
On 2013/07/17 22:23:20, binji wrote:
> The code and comment do not align: Is this supposed to return if anything is
> available, or if events is completely full?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:113: }
On 2013/07/17 22:23:20, binji wrote:
> helper function for this?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:115: // Keep waiting
while we have time left, or no events captured
On 2013/07/17 22:23:20, binji wrote:
> This comment does not align with the code -- ms_timeout never changes, so it's
> not really "time left".

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:116: while ((0 ==
*event_cnt) && ms_timeout) {
On 2013/07/17 17:34:56, Sam Clegg wrote:
> be consistent about comparing ms_timeout explicitly with 0 (or not).
> 
> Is our convention to explicitly compare with 0?
> 
> Also be consistent about putting 0/NULL before or after ==.

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:116: while ((0 ==
*event_cnt) && ms_timeout) {
On 2013/07/17 17:34:56, Sam Clegg wrote:
> be consistent about comparing ms_timeout explicitly with 0 (or not).
> 
> Is our convention to explicitly compare with 0?
> 
> Also be consistent about putting 0/NULL before or after ==.

Yes, (foo) (!foo) should be bool only.  Otherwise (0 != foo) (NULL == foo) to
prevent accidental assignment.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:221: if
(it->second->emitter)
On 2013/07/17 22:23:20, binji wrote:
> is this check necessary? It looks like event_info_ should never have an
> EventInfo with a NULL emitter (The EventInfo is removed when it is abandoned.)

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:234: event->emitter =
NULL;
On 2013/07/17 22:23:20, binji wrote:
> why? Who else could be using this EventInfo?

The EventInfo can be in:
   event_info_map_ only - no signal while someone is waiting.
   event_info_map_ and signaled_ - signal while someone is waiting.
   signaled_ only - abandoned, but the "exception/hang up" has yet to be
consumed.

This is why the EventInfo is ref counted.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:22: //   Certain file
objects such as pipes or sockets can become signaled when
On 2013/07/17 22:23:20, binji wrote:
> why indented?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:46: //       Ref
Emitter
On 2013/07/17 22:23:20, binji wrote:
> Looking at the code below, where does EventListener::Track reference the
> emitter? It is given a const reference, so it is just sharing the reference
with
> the caller.

We count on the caller having referenced it, but that's already obvious since we
pass in a const ScopedRef& so I'll just take that comment out

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:50: //    AbandonInfo
On 2013/07/17 22:23:20, binji wrote:
> called AbandonedEventInfo below

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:80: // from
EventEmitter objects.
On 2013/07/17 22:23:20, binji wrote:
> document why EventListener is also an EventEmitter

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:92: // Declated in
EventEmitter
On 2013/07/17 22:23:20, binji wrote:
> sp: Declared

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:106: const
ScopedRef<EventEmitter>& emitter,
On 2013/07/17 22:23:20, binji wrote:
> why not ScopedEventEmitter?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:125: EventInfoMap_t
event_info_;
On 2013/07/17 22:23:20, binji wrote:
> might be nice to mention "map" in this name somewhere. event_info_map_?
> info_map_?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.h:127: // Protects
waiting_, signaled_ and used with the cond_var.
On 2013/07/17 22:23:20, binji wrote:
> nit: "used with signal_cond_"

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:30: void
RaiseEvent(uint32_t events) {
On 2013/07/17 22:23:20, binji wrote:
> just use "using EventEmitter::RaiseEvent;"

I think the style is a bit ambiguous here.   Using namespace is frowned on. 
Using directive is acceptable, but I'm not sure it's preffered to explicitly
chaining the parent here.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:81: EXPECT_EQ(0,
listener->Update(FD_EMITTER, SIG_FILTER, SIG_DATA));
On 2013/07/17 22:23:20, binji wrote:
> test that the values change?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:91: EXPECT_EQ(EINVAL,
listener->Wait(ev, 0, 0, &event_cnt));
On 2013/07/17 22:23:20, binji wrote:
> might be nice to mention that these are all without timeouts. Maybe just as a
> new test?

I've replaced the const values with const variables that are elf documenting.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:184: // NOTE:  These
timing tests are potentially flacky, the real test is
On 2013/07/17 22:23:20, binji wrote:
> sp: flaky

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:230: EXPECT_GT(25,
duration);
On 2013/07/17 22:23:20, binji wrote:
> const for 25 too?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/sdk_util/scoped_ref.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/sdk_util/scoped_ref.h:74: bool operator<(const
ScopedRef& other) const {
On 2013/07/17 22:23:20, binji wrote:
> Now that I've seen the use case, you could just add this to EventInfo directly
> (rather than add it for all RefObjects).

Done.

[binji, 2013-07-19 20:47:17]

nice. lgtm (with nits, of course!)

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:18: class EventInfo;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Remove since its is declared just below?

ping

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:41: // by registering
EventInfo objects and signalling the EventListener when
On 2013/07/17 22:23:20, binji wrote:
> signalled or signaled are both correct, but signaled seems to be used more
here.
> 
> Same with signalling vs. signaling.

ping

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:46: typedef
std::set<ScopedEventInfo> EventInfoSet_t;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Isn't this is duplicate of the typedef 10 lines above?  Is it needed?

ping

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:54: // Regisiter or
unregister an EventInfo.  The lock of the EventListener
On 2013/07/17 22:23:20, binji wrote:
> sp: Register

ping

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:65: // REturns the type
of the emitter (compatible with st_mode in stat)
On 2013/07/17 22:23:20, binji wrote:
> nit: Returns

ping

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:221: if
(it->second->emitter)
On 2013/07/18 22:18:16, noelallen1 wrote:
> On 2013/07/17 22:23:20, binji wrote:
> > is this check necessary? It looks like event_info_ should never have an
> > EventInfo with a NULL emitter (The EventInfo is removed when it is
abandoned.)
> 
> Done.

Still need the UnregisterEventInfo?

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

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:32: // We do not take
the lock since this is the only refence to this object.
sp: reference

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:230: // Notify the
EventEmitter has abandon this event.
comment is unnecessary, it is documented in the header

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:235: // Remove
abandoned events from the event_info_map_ for abandoned
Is this comment necessary? It just describes what the code is doing. Maybe move
this to the comment below:
"Abandoned events are removed from event_info_map_, but must be kept in
signaled_..."

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:247: const int
SCHEDULING_GRANUALRITY = 100;
sp: GRANULARITY

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:295: // Wait
TIMEOUT_LONG in another thread to signal, and verify we waited
This isn't waiting for another thread, that's the test below, right?

[noelallen1, 2013-07-19 21:35:43]

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/event_emitter.h (right):

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:18: class EventInfo;
On 2013/07/19 20:47:17, binji wrote:
> On 2013/07/17 17:34:56, Sam Clegg wrote:
> > Remove since its is declared just below?
> 
> ping

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:23: struct EventInfo :
public RefObject {
On 2013/07/17 22:23:20, binji wrote:
> might want to mention that this is not POD, because it derives from RefObject
> (which is not POD)

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:25: volatile uint32_t
events;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Is this a bitfield of KernnelEventTypes?  Perhaps a comment to say so.

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:26: volatile uint32_t
filter;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Didn't we come to the conclusion that volatile is not useful, except for
people
> accessing direct hardware memory (i.e. explicitly not useful in the context of
> multi-threading):
> 
>
http://stackoverflow.com/questions/4557979/when-to-use-volatile-with-multi-th...

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:41: // by registering
EventInfo objects and signalling the EventListener when
On 2013/07/17 22:23:20, binji wrote:
> sp: whenever their state

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:41: // by registering
EventInfo objects and signalling the EventListener when
On 2013/07/17 22:23:20, binji wrote:
> signalled or signaled are both correct, but signaled seems to be used more
here.
> 
> Same with signalling vs. signaling.

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:41: // by registering
EventInfo objects and signalling the EventListener when
On 2013/07/19 20:47:18, binji wrote:
> On 2013/07/17 22:23:20, binji wrote:
> > signalled or signaled are both correct, but signaled seems to be used more
> here.
> > 
> > Same with signalling vs. signaling.
> 
> ping

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:46: typedef
std::set<ScopedEventInfo> EventInfoSet_t;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Isn't this is duplicate of the typedef 10 lines above?  Is it needed?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:46: typedef
std::set<ScopedEventInfo> EventInfoSet_t;
On 2013/07/17 17:34:56, Sam Clegg wrote:
> Isn't this is duplicate of the typedef 10 lines above?  Is it needed?

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:54: // Regisiter or
unregister an EventInfo.  The lock of the EventListener
On 2013/07/19 20:47:18, binji wrote:
> On 2013/07/17 22:23:20, binji wrote:
> > sp: Register
> 
> ping

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:54: // Regisiter or
unregister an EventInfo.  The lock of the EventListener
On 2013/07/17 22:23:20, binji wrote:
> sp: Register

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:62: // Returns the
current state of the emitter.
On 2013/07/17 17:34:56, Sam Clegg wrote:
> The state is a bitfield of KennelEventType?  Perhaps a typedef for this
bitfield
> would make the code more readable?

I'm going to think about that as I do the select/poll pieces.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:65: // REturns the type
of the emitter (compatible with st_mode in stat)
On 2013/07/17 22:23:20, binji wrote:
> nit: Returns

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:65: // REturns the type
of the emitter (compatible with st_mode in stat)
On 2013/07/19 20:47:18, binji wrote:
> On 2013/07/17 22:23:20, binji wrote:
> > nit: Returns
> 
> ping

Done.

https://codereview.chromium.org/19271009/diff/7001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/event_emitter.h:73: virtual void
ChainRegister(const ScopedEventInfo& event);
On 2013/07/17 22:23:20, binji wrote:
> I'd prefer a name that made it clear that it was called by RegisterEventInfo:
> 
> RegisterEventInfo_Chain, maybe?

Done.

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

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:32: // We do not take
the lock since this is the only refence to this object.
On 2013/07/19 20:47:18, binji wrote:
> sp: reference

Done.

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:230: // Notify the
EventEmitter has abandon this event.
On 2013/07/19 20:47:18, binji wrote:
> comment is unnecessary, it is documented in the header

Done.

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:235: // Remove
abandoned events from the event_info_map_ for abandoned
On 2013/07/19 20:47:18, binji wrote:
> Is this comment necessary? It just describes what the code is doing. Maybe
move
> this to the comment below:
> "Abandoned events are removed from event_info_map_, but must be kept in
> signaled_..."

Done.

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/event_test.cc (right):

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:247: const int
SCHEDULING_GRANUALRITY = 100;
On 2013/07/19 20:47:18, binji wrote:
> sp: GRANULARITY

Done.

https://codereview.chromium.org/19271009/diff/30001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/event_test.cc:295: // Wait
TIMEOUT_LONG in another thread to signal, and verify we waited
On 2013/07/19 20:47:18, binji wrote:
> This isn't waiting for another thread, that's the test below, right?

Done.

[noelallen1, 2013-07-23 02:19:02]

Committed patchset #21 manually as r213030 (presubmit successful).