Socket subsystem implementation

Socket subsystem implementation
Committed: https://code.google.com/p/naclports/source/detail?r=596

[vissi, 2012-05-15 13:04:20]

This is a mock of sockets implementation (for early review).

[vissi, 2012-05-18 12:36:56]

The problem now is testing, next target is implementing /dev/tty as a Device.

[vissi, 2012-05-18 16:59:23]

On 2012/05/18 12:36:56, vissi wrote:
> The problem now is testing, next target is implementing /dev/tty as a Device.

Got simple HTTP GET test  working, see bitbucket.org/vissi/nacl-fshell for
example (./build.sh, ./serve.py, then run "socktest" command in pseudo command
line).

[Evgeniy Stepanov, 2012-05-19 11:07:46]

On 2012/05/18 16:59:23, vissi wrote:
> On 2012/05/18 12:36:56, vissi wrote:
> > The problem now is testing, next target is implementing /dev/tty as a
Device.
> 
> Got simple HTTP GET test  working, see bitbucket.org/vissi/nacl-fshell for
> example (./build.sh, ./serve.py, then run "socktest" command in pseudo command
> line).

Great.
This CL is missing several files: pthread_helpers.h, etc.

[vissi, 2012-05-21 09:18:41]

On 2012/05/19 11:07:46, Evgeniy Stepanov wrote:
> On 2012/05/18 16:59:23, vissi wrote:
> > On 2012/05/18 12:36:56, vissi wrote:
> > > The problem now is testing, next target is implementing /dev/tty as a
> Device.
> > 
> > Got simple HTTP GET test  working, see bitbucket.org/vissi/nacl-fshell for
> > example (./build.sh, ./serve.py, then run "socktest" command in pseudo
command
> > line).
> 
> Great.
> This CL is missing several files: pthread_helpers.h, etc.

Added now. Going to write select() test.

[vissi, 2012-05-21 13:52:22]

On 2012/05/21 09:18:41, vissi wrote:
> On 2012/05/19 11:07:46, Evgeniy Stepanov wrote:
> > On 2012/05/18 16:59:23, vissi wrote:
> > > On 2012/05/18 12:36:56, vissi wrote:
> > > > The problem now is testing, next target is implementing /dev/tty as a
> > Device.
> > > 
> > > Got simple HTTP GET test  working, see bitbucket.org/vissi/nacl-fshell for
> > > example (./build.sh, ./serve.py, then run "socktest" command in pseudo
> command
> > > line).
> > 
> > Great.
> > This CL is missing several files: pthread_helpers.h, etc.
> 
> Added now. Going to write select() test.

File naming convention changed. Now going to move fd management to KernelProxy.

[Evgeniy Stepanov, 2012-05-21 13:55:24]

BaseSocketSystem <-> SocketSubsystem
pls either remove or add "Sub" somewhere

On 2012/05/21 13:52:22, vissi wrote:
> On 2012/05/21 09:18:41, vissi wrote:
> > On 2012/05/19 11:07:46, Evgeniy Stepanov wrote:
> > > On 2012/05/18 16:59:23, vissi wrote:
> > > > On 2012/05/18 12:36:56, vissi wrote:
> > > > > The problem now is testing, next target is implementing /dev/tty as a
> > > Device.
> > > > 
> > > > Got simple HTTP GET test  working, see bitbucket.org/vissi/nacl-fshell
for
> > > > example (./build.sh, ./serve.py, then run "socktest" command in pseudo
> > command
> > > > line).
> > > 
> > > Great.
> > > This CL is missing several files: pthread_helpers.h, etc.
> > 
> > Added now. Going to write select() test.
> 
> File naming convention changed. Now going to move fd management to
KernelProxy.

[vissi, 2012-05-23 15:17:58]

On 2012/05/21 13:55:24, Evgeniy Stepanov wrote:
> BaseSocketSystem <-> SocketSubsystem
> pls either remove or add "Sub" somewhere
> 
> On 2012/05/21 13:52:22, vissi wrote:
> > On 2012/05/21 09:18:41, vissi wrote:
> > > On 2012/05/19 11:07:46, Evgeniy Stepanov wrote:
> > > > On 2012/05/18 16:59:23, vissi wrote:
> > > > > On 2012/05/18 12:36:56, vissi wrote:
> > > > > > The problem now is testing, next target is implementing /dev/tty as
a
> > > > Device.
> > > > > 
> > > > > Got simple HTTP GET test  working, see bitbucket.org/vissi/nacl-fshell
> for
> > > > > example (./build.sh, ./serve.py, then run "socktest" command in pseudo
> > > command
> > > > > line).
> > > > 
> > > > Great.
> > > > This CL is missing several files: pthread_helpers.h, etc.
> > > 
> > > Added now. Going to write select() test.
> > 
> > File naming convention changed. Now going to move fd management to
> KernelProxy.

Refactored a bit (dup implementation is commented out for now, thttpd uses
dup2).

[Evgeniy Stepanov, 2012-05-23 15:34:16]

No need to expose FileHandle to the socket subsystem. Use FileStream instead.

Also it seems there is quite a bit of dead code, mostly for character device
emulation. It's out of the scope of this CL, and will need to be rewritten
anyway (devices should be files, not sockets).

Unless I'm missing something, dup() and dup2() in SocketSubsystem are dead code.
They are not called from anywhere, and they should not be there -
KernelProxy::dup implementation should be just enough.

Please make sure that SocketSubsystem (and FileStream) does not depend on
KernelProxy.

[Evgeniy Stepanov, 2012-05-23 15:35:13]

https://chromiumcodereview.appspot.com/10392070/diff/22013/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/22013/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:10: #include "console/ConsoleMount.h"
Why this one without ../, and the two below it with ../ ?

https://chromiumcodereview.appspot.com/10392070/diff/22013/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:50: void
KernelProxy::SetSS(BaseSocketSubSystem* bss) {
A more descriptive setter name pls.

[vissi, 2012-05-23 22:46:22]

On 2012/05/23 15:34:16, Evgeniy Stepanov wrote:
> No need to expose FileHandle to the socket subsystem. Use FileStream instead.
> 
> Also it seems there is quite a bit of dead code, mostly for character device
> emulation. It's out of the scope of this CL, and will need to be rewritten
> anyway (devices should be files, not sockets).
> 
> Unless I'm missing something, dup() and dup2() in SocketSubsystem are dead
code.
> They are not called from anywhere, and they should not be there -
> KernelProxy::dup implementation should be just enough.
> 
> Please make sure that SocketSubsystem (and FileStream) does not depend on
> KernelProxy.

The problem with exposing FileHandles is that there are some methods (bind,
connect) that require setting them. Should I use FileStream** instead?

[Evgeniy Stepanov, 2012-05-24 06:08:17]

On 2012/05/23 22:46:22, vissi wrote:
> On 2012/05/23 15:34:16, Evgeniy Stepanov wrote:
> > No need to expose FileHandle to the socket subsystem. Use FileStream
instead.
> > 
> > Also it seems there is quite a bit of dead code, mostly for character device
> > emulation. It's out of the scope of this CL, and will need to be rewritten
> > anyway (devices should be files, not sockets).
> > 
> > Unless I'm missing something, dup() and dup2() in SocketSubsystem are dead
> code.
> > They are not called from anywhere, and they should not be there -
> > KernelProxy::dup implementation should be just enough.
> > 
> > Please make sure that SocketSubsystem (and FileStream) does not depend on
> > KernelProxy.
> 
> The problem with exposing FileHandles is that there are some methods (bind,
> connect) that require setting them. Should I use FileStream** instead?

Either that, or pass them as return value.

[vissi, 2012-05-24 11:32:13]

On 2012/05/24 06:08:17, Evgeniy Stepanov wrote:
> On 2012/05/23 22:46:22, vissi wrote:
> > On 2012/05/23 15:34:16, Evgeniy Stepanov wrote:
> > > No need to expose FileHandle to the socket subsystem. Use FileStream
> instead.
> > > 
> > > Also it seems there is quite a bit of dead code, mostly for character
device
> > > emulation. It's out of the scope of this CL, and will need to be rewritten
> > > anyway (devices should be files, not sockets).
> > > 
> > > Unless I'm missing something, dup() and dup2() in SocketSubsystem are dead
> > code.
> > > They are not called from anywhere, and they should not be there -
> > > KernelProxy::dup implementation should be just enough.
> > > 
> > > Please make sure that SocketSubsystem (and FileStream) does not depend on
> > > KernelProxy.
> > 
> > The problem with exposing FileHandles is that there are some methods (bind,
> > connect) that require setting them. Should I use FileStream** instead?
> 
> Either that, or pass them as return value.

SlotAllocator implementation now uses set instead of vector as backend (to make
AllocAt operation easier).
I haven't removed all backlinks to KernelProxy from SocketSubSystem yet.

[vissi, 2012-05-25 11:08:50]

Now SocketSubSystem does not reference KernelProxy.

https://chromiumcodereview.appspot.com/10392070/diff/22013/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/22013/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:10: #include "console/ConsoleMount.h"
On 2012/05/23 15:35:13, Evgeniy Stepanov wrote:
> Why this one without ../, and the two below it with ../ ?

Now without ..

https://chromiumcodereview.appspot.com/10392070/diff/22013/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:50: void
KernelProxy::SetSS(BaseSocketSubSystem* bss) {
On 2012/05/23 15:35:13, Evgeniy Stepanov wrote:
> A more descriptive setter name pls.

Done.

[Evgeniy Stepanov, 2012-05-25 12:05:57]

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:253: ino_t KernelProxy::CreateSocket()
{
move into socket()

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:261: handle->stream = NULL;
a comment somewhere that (NULL, NULL) means a generic socket

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:332: FileHandle* handle;
TODO handle sockets in read and write

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:462: new_stream->close();
this will crash

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:468: = stream->dup(newfd);
no need to dup streams, new handle should hold a reference to the same stream

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:516: FileStream* new_stream =
stream->dup(new_fd);
dup does not create new stream or handle, just a new descriptor with a reference
to the old handle; dup2 may destroy a handle and a stream

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (left):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:137: struct FileHandle {
move this back into KernelProxy

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:19: SocketSubSystem*
SocketSubSystem::instance_ = NULL;
unused

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:69: AddressMap::iterator it =
addrs_.find(addr);
factor this piece out

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:110: if (stream && stream !=
kBadFileStream) {
could we get rid of this constant?

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:120: if (stream && stream !=
kBadFileStream)
move this check to KernelProxy, assume that stream is non-null here

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:71: void AddHostAddress(const char*
name, unsigned long addr);
add a comment about this

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:34: FileStream* TCPSocket::dup(int fd) {
this seems unneeded

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:49: bool TCPSocket::accept(PP_Resource
resource) {
call it something like fromAccept to avoid confusion with
TCPServerSocket::accept

[vissi, 2012-05-25 13:07:09]

Fixed

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:253: ino_t KernelProxy::CreateSocket()
{
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> move into socket()

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:261: handle->stream = NULL;
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> a comment somewhere that (NULL, NULL) means a generic socket

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:332: FileHandle* handle;
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> TODO handle sockets in read and write

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:462: new_stream->close();
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> this will crash

Removed

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:468: = stream->dup(newfd);
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> no need to dup streams, new handle should hold a reference to the same stream

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:516: FileStream* new_stream =
stream->dup(new_fd);
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> dup does not create new stream or handle, just a new descriptor with a
reference
> to the old handle; dup2 may destroy a handle and a stream

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (left):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:137: struct FileHandle {
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> move this back into KernelProxy

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:19: SocketSubSystem*
SocketSubSystem::instance_ = NULL;
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> unused

No, it's used by GetSocketSubSystem() referenced in TcpSocket implementation,
for example.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:69: AddressMap::iterator it =
addrs_.find(addr);
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> factor this piece out
Why?

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:110: if (stream && stream !=
kBadFileStream) {
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> could we get rid of this constant?

yup.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:120: if (stream && stream !=
kBadFileStream)
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> move this check to KernelProxy, assume that stream is non-null here

Haven't moved this yet. We check if FileHandle is correct in KernelProxy, I
suppose it's correct to check the pointer there, as it's semantically correct
value (if socket was just initialized).

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:71: void AddHostAddress(const char*
name, unsigned long addr);
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> add a comment about this

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:34: FileStream* TCPSocket::dup(int fd) {
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> this seems unneeded

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:49: bool TCPSocket::accept(PP_Resource
resource) {
On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> call it something like fromAccept to avoid confusion with
> TCPServerSocket::accept

Doen.

[Evgeniy Stepanov, 2012-05-25 13:15:02]

https://chromiumcodereview.appspot.com/10392070/diff/38004/Makefile
File Makefile (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/Makefile#newcode213
Makefile:213: gtest: libraries/gtest-1.5.0
; at the end
please make this a separate CL

[Evgeniy Stepanov, 2012-05-25 13:19:51]

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:19: SocketSubSystem*
SocketSubSystem::instance_ = NULL;
On 2012/05/25 13:07:09, vissi wrote:
> On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> > unused
> 
> No, it's used by GetSocketSubSystem() referenced in TcpSocket implementation,
> for example.

Pass pp::Instance through TCPSocket constructor.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:69: AddressMap::iterator it =
addrs_.find(addr);
On 2012/05/25 13:07:09, vissi wrote:
> On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> > factor this piece out
> Why?

I mean, make it a function. It's repeated twice in bind() and connect().

[vissi, 2012-05-25 14:32:06]

https://chromiumcodereview.appspot.com/10392070/diff/38004/Makefile
File Makefile (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/Makefile#newcode213
Makefile:213: gtest: libraries/gtest-1.5.0
On 2012/05/25 13:15:03, Evgeniy Stepanov wrote:
> ; at the end
> please make this a separate CL

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:19: SocketSubSystem*
SocketSubSystem::instance_ = NULL;
On 2012/05/25 13:19:51, Evgeniy Stepanov wrote:
> On 2012/05/25 13:07:09, vissi wrote:
> > On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> > > unused
> > 
> > No, it's used by GetSocketSubSystem() referenced in TcpSocket
implementation,
> > for example.
> 
> Pass pp::Instance through TCPSocket constructor.

They use the same condition variable. Should they?

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:69: AddressMap::iterator it =
addrs_.find(addr);
On 2012/05/25 13:19:51, Evgeniy Stepanov wrote:
> On 2012/05/25 13:07:09, vissi wrote:
> > On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> > > factor this piece out
> > Why?
> 
> I mean, make it a function. It's repeated twice in bind() and connect().

Done.

[vissi, 2012-05-25 14:54:08]

Done

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38004/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:19: SocketSubSystem*
SocketSubSystem::instance_ = NULL;
On 2012/05/25 14:32:06, vissi wrote:
> On 2012/05/25 13:19:51, Evgeniy Stepanov wrote:
> > On 2012/05/25 13:07:09, vissi wrote:
> > > On 2012/05/25 12:05:57, Evgeniy Stepanov wrote:
> > > > unused
> > > 
> > > No, it's used by GetSocketSubSystem() referenced in TcpSocket
> implementation,
> > > for example.
> > 
> > Pass pp::Instance through TCPSocket constructor.
> 
> They use the same condition variable. Should they?

Per offline discussion, we decided that it would be useful if several socket
backend implementations are in use at the same time.

[Evgeniy Stepanov, 2012-05-25 15:08:07]

https://chromiumcodereview.appspot.com/10392070/diff/40007/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/40007/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:15:
TCPSocket::TCPSocket(BaseSocketSubSystem* sys, int oflag)
Make it SocketSubSustem.

This will need to be refactored when we add character devices, but let's take it
one step at a time.

[vissi, 2012-05-28 13:34:26]

Updated. Now there are separate mutexes for select() notification, socket
subsystem operations (address maps), internal socket operations.

https://chromiumcodereview.appspot.com/10392070/diff/40007/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/40007/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:15:
TCPSocket::TCPSocket(BaseSocketSubSystem* sys, int oflag)
On 2012/05/25 15:08:08, Evgeniy Stepanov wrote:
> Make it SocketSubSustem.
> 
> This will need to be refactored when we add character devices, but let's take
it
> one step at a time.

Done.

[Evgeniy Stepanov, 2012-05-28 14:55:12]

Locking still seems wrong. For instance, TCPSocket::lock does not grab the
socket's lock, and does cond()->wait(), which unlocks the mutex.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:153: Mutex mutex_;
Rename to smth like select_cond_ & select_lock_. Accessors, too.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/net/IOInterfaces.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/net/IOInterfaces.h:85: class InputInterface {
This seems unused atm? Remove from the CL.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/net/ProxyStream.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/net/ProxyStream.h:11: class ProxyStream : public
FileStream {
Also unused.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:27: static const unsigned long
kFirstAddr = 0x00000000;
Hide these constants in .cc, they are not part of the interface.

[vissi, 2012-05-28 15:20:21]

Well, these were originally missing. I can add them.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:153: Mutex mutex_;
On 2012/05/28 14:55:12, Evgeniy Stepanov wrote:
> Rename to smth like select_cond_ & select_lock_. Accessors, too.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/net/IOInterfaces.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/net/IOInterfaces.h:85: class InputInterface {
On 2012/05/28 14:55:12, Evgeniy Stepanov wrote:
> This seems unused atm? Remove from the CL.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/net/ProxyStream.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/net/ProxyStream.h:11: class ProxyStream : public
FileStream {
On 2012/05/28 14:55:12, Evgeniy Stepanov wrote:
> Also unused.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40009/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:27: static const unsigned long
kFirstAddr = 0x00000000;
On 2012/05/28 14:55:12, Evgeniy Stepanov wrote:
> Hide these constants in .cc, they are not part of the interface.

Done.

[vissi, 2012-05-29 11:17:56]

Per offline discussion we decided that using multiple mutexes would be too
complex and returned back to single.

[vissi, 2012-05-29 12:45:36]

On 2012/05/29 11:17:56, vissi wrote:
> Per offline discussion we decided that using multiple mutexes would be too
> complex and returned back to single.

Dup now fixed (does not create a separate FileHandle).

We also agreed that file handle manipulation should be refactored to add a new
layer of abstraction (like kernel does) and provide some interface to hide mount
and socket implementation details (which are now in KernelProxy). This would be
done later and, presumably, discussed in mailing list.

[Evgeniy Stepanov, 2012-05-29 14:28:20]

Add a comment somewhere in SocketSubsystem.h about the current limitations.
I don't see any locking on read() path. cond->wait in TcpSocket::read is being
called without mutex held. Please make sure that works with socket data is
properly locked.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:62: // we store a map of virtual
addresses here, as PPAPA sockes are name-bound
a TODO about getting rid of this with the new resolver interface

[Dmitry Polukhin, 2012-05-29 15:00:51]

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:11: #include <netdb.h>
General comment need to make sure that this code still compiles and work with
newlib toolchain.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:39: void
SetSocketSubSystem(BaseSocketSubSystem* bss);
Please add comment about this function.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:165: BaseSocketSubSystem* ss;
Need better name and underscore at the end.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:30: ~SocketSubSystem();
Please add virtual, because base class d-tor is virtual.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:50: socklen_t* addrlen);
Need 2 idents.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpServerSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpServerSocket.cc:104: // Ignore bind address and
always bind to localhost.
Not sure that in general lib we need bind for localhost only. latest openssh
port has support for binding specified address by using
CreateFromIPv4Address/CreateFromIPv6Address.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpServerSocket.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpServerSocket.h:17: int oflag, const char* host,
uint16_t port);
+ ident

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:157: {
Please put { on the line with 'if'.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:19: explicit TCPSocket(SocketSubSystem*
sys, int oflag);
explicit is useless if c-tor has more more than 1 argument.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:43: void sendtask();
I think this function should be renamed to SendTask. In general my code need to
be cleaned up to follow Chromium style guide in function naming - perhaps we
need to invent some additional conventions to distinguished function that need
to be called on main thread.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:60: SocketSubSystem* sys;
should end with underscore i.e. sys_

[vissi, 2012-05-30 08:10:51]

What limitations should I specify?

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:11: #include <netdb.h>
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> General comment need to make sure that this code still compiles and work with
> newlib toolchain.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:39: void
SetSocketSubSystem(BaseSocketSubSystem* bss);
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> Please add comment about this function.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:165: BaseSocketSubSystem* ss;
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> Need better name and underscore at the end.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:30: ~SocketSubSystem();
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> Please add virtual, because base class d-tor is virtual.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:50: socklen_t* addrlen);
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> Need 2 idents.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:62: // we store a map of virtual
addresses here, as PPAPA sockes are name-bound
On 2012/05/29 14:28:20, Evgeniy Stepanov wrote:
> a TODO about getting rid of this with the new resolver interface

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpServerSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpServerSocket.cc:104: // Ignore bind address and
always bind to localhost.
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> Not sure that in general lib we need bind for localhost only. latest openssh
> port has support for binding specified address by using
> CreateFromIPv4Address/CreateFromIPv6Address.

What else?

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpServerSocket.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpServerSocket.h:17: int oflag, const char* host,
uint16_t port);
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> + ident

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:157: {
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> Please put { on the line with 'if'.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:19: explicit TCPSocket(SocketSubSystem*
sys, int oflag);
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> explicit is useless if c-tor has more more than 1 argument.

I wanted to eliminate lint warning, removed now.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:43: void sendtask();
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> I think this function should be renamed to SendTask. In general my code need
to
> be cleaned up to follow Chromium style guide in function naming - perhaps we
> need to invent some additional conventions to distinguished function that need
> to be called on main thread.

This function is not used anywhere, removed now.

https://chromiumcodereview.appspot.com/10392070/diff/38038/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:60: SocketSubSystem* sys;
On 2012/05/29 15:00:51, Dmitry Polukhin wrote:
> should end with underscore i.e. sys_

Done.

[Dmitry Polukhin, 2012-05-30 15:13:31]

will take a look to the rest tomorrow.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:244: struct hostent *res = (struct
hostent*) malloc(sizeof(struct hostent));
- This implementation leaks memory because caller won't call free on the
strcture.
- malloc doesn't initialize memory so this code also keeps uninitialized
h_aliases and h_name.
- Does't work for IP v6.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:849: if (GetFileHandle(sockfd) == 0)
return EBADF;
It is formally allowed to put simple then statement on the same line as if but
IMHO without any empty lines with following code it is hard to read. Especially
within large block of code.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:852: in_addr->sin_addr.s_addr,
Here and below need 2 idents.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:892: size_t nread;
It is C style to put variable declaration at the beginning of function. Please
keep it closer to use here and in other places.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:157: Cond select_cond_;
It is not good to mix direct pthread work and using wrappers, perhaps in this
particular case it is better to avoid using wrappers because nacl-mounts doesn't
use them.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:127: #define LOG(format, args...) \
I think it is not used.

[vissi, 2012-05-31 13:35:21]

The only thing missing now, imho, is limitations description.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:244: struct hostent *res = (struct
hostent*) malloc(sizeof(struct hostent));
On 2012/05/30 15:13:32, Dmitry Polukhin wrote:
> - This implementation leaks memory because caller won't call free on the
> strcture.
> - malloc doesn't initialize memory so this code also keeps uninitialized
> h_aliases and h_name.
> - Does't work for IP v6.

Now memory is allocated only once. Should I change underlying implementation to
support IPv6?

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:849: if (GetFileHandle(sockfd) == 0)
return EBADF;
On 2012/05/30 15:13:32, Dmitry Polukhin wrote:
> It is formally allowed to put simple then statement on the same line as if but
> IMHO without any empty lines with following code it is hard to read.
Especially
> within large block of code.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:852: in_addr->sin_addr.s_addr,
On 2012/05/30 15:13:32, Dmitry Polukhin wrote:
> Here and below need 2 idents.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:892: size_t nread;
On 2012/05/30 15:13:32, Dmitry Polukhin wrote:
> It is C style to put variable declaration at the beginning of function. Please
> keep it closer to use here and in other places.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:157: Cond select_cond_;
On 2012/05/30 15:13:32, Dmitry Polukhin wrote:
> It is not good to mix direct pthread work and using wrappers, perhaps in this
> particular case it is better to avoid using wrappers because nacl-mounts
doesn't
> use them.

Well, wrapper features (reentrancy) are used in the code, I suppose this can be
factored out later.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:127: #define LOG(format, args...) \
On 2012/05/30 15:13:32, Dmitry Polukhin wrote:
> I think it is not used.

It is used in Tcp[Server]Socket several times. Should I remove it?

[Dmitry Polukhin, 2012-05-31 14:06:44]

Let's discuss it with Evgeny when he gets back.

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:127: #define LOG(format, args...) \
nacl-miunts uses different debug printing so there is no sense to have both.
Please keep only one.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res = NULL;
They should be thread local and static (or in unnamed namespace) to avoid
collision with user variables.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:29: class Mutex {
SimpleAutoLock is analog used in nacl-mounts. I think we don't need to create
more synchronization wrappers. We need to have one set of wrapper and use them
everywhere.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/BaseSocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/BaseSocketSubSystem.h:5: #ifndef BASE_S3_H
Please use guard according coding style.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/IOInterfaces.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/IOInterfaces.h:17: #include "../base/nacl_dirent.h"
I think all path should be from root of nacl-mounts. Here and in all other
places fon't use .. in paths.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:11: //#include <memory.h>
Remove if you don't need it.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:39: dbgprintf("host: %s, port: %d\n",
host, port);
Don't use debug functions directly use macro that will expand different in case
of debug and release.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:13: #include
"../include/ppapi/cpp/completion_callback.h"
I think it should be included as "ppapi/cpp/completion_callback.h" with proper
-I option during build.

[Evgeniy Stepanov, 2012-06-01 07:59:22]

This CL is still missing a lot of locking. Please make sure that all access to
socket guts is done under the sys lock. Make especially sure that the mutex is
held whenever cond->wait(mutex) is called.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:29: class Mutex {
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> SimpleAutoLock is analog used in nacl-mounts. I think we don't need to create
> more synchronization wrappers. We need to have one set of wrapper and use them
> everywhere.

I second that. Let's start with replacing Mutex::Lock with SimpleAutoLock
everywhere.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:39: dbgprintf("host: %s, port: %d\n",
host, port);
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> Don't use debug functions directly use macro that will expand different in
case
> of debug and release.

AFAIK we don't have such macro atm. I've been using unconditional dbgprintf()s
in cases like PPAPI errors, and been mostly silent otherwise.

[vissi, 2012-06-01 09:12:57]

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/40018/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:127: #define LOG(format, args...) \
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> nacl-miunts uses different debug printing so there is no sense to have both.
> Please keep only one.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res = NULL;
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> They should be thread local and static (or in unnamed namespace) to avoid
> collision with user variables.

Now they are static. You mean C++11 thread_local keyword?

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/base/PthreadHelpers.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:29: class Mutex {
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> SimpleAutoLock is analog used in nacl-mounts. I think we don't need to create
> more synchronization wrappers. We need to have one set of wrapper and use them
> everywhere.

Can we leave Cond?

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/PthreadHelpers.h:29: class Mutex {
On 2012/06/01 07:59:23, Evgeniy Stepanov wrote:
> On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > SimpleAutoLock is analog used in nacl-mounts. I think we don't need to
create
> > more synchronization wrappers. We need to have one set of wrapper and use
them
> > everywhere.
> 
> I second that. Let's start with replacing Mutex::Lock with SimpleAutoLock
> everywhere.

Mutex::Lock has been replaced.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/BaseSocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/BaseSocketSubSystem.h:5: #ifndef BASE_S3_H
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> Please use guard according coding style.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/IOInterfaces.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/IOInterfaces.h:17: #include "../base/nacl_dirent.h"
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> I think all path should be from root of nacl-mounts. Here and in all other
> places fon't use .. in paths.

Maybe, however, there are lots of usages like this:

ack-grep "\.\./" | wc -l
126

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:11: //#include <memory.h>
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> Remove if you don't need it.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:39: dbgprintf("host: %s, port: %d\n",
host, port);
On 2012/06/01 07:59:23, Evgeniy Stepanov wrote:
> On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > Don't use debug functions directly use macro that will expand different in
> case
> > of debug and release.
> 
> AFAIK we don't have such macro atm. I've been using unconditional dbgprintf()s
> in cases like PPAPI errors, and been mostly silent otherwise.

Left as is for now.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/TcpSocket.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.h:13: #include
"../include/ppapi/cpp/completion_callback.h"
On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> I think it should be included as "ppapi/cpp/completion_callback.h" with proper
> -I option during build.

Done.

[Dmitry Polukhin, 2012-06-01 09:52:13]

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res = NULL;
On 2012/06/01 09:12:57, vissi wrote:
> On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > They should be thread local and static (or in unnamed namespace) to avoid
> > collision with user variables.
> 
> Now they are static. You mean C++11 thread_local keyword?

I meant __thread for GCC
http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html
But usually it is used indirectly via macro that expands to compiler specific
keyword. But it won't remove memory leak completely because when thread exists
the objects will be lost.

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
File libraries/nacl-mounts/net/IOInterfaces.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
libraries/nacl-mounts/net/IOInterfaces.h:17: #include "../base/nacl_dirent.h"
On 2012/06/01 09:12:57, vissi wrote:
> On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > I think all path should be from root of nacl-mounts. Here and in all other
> > places fon't use .. in paths.
> 
> Maybe, however, there are lots of usages like this:
> 
> ack-grep "\.\./" | wc -l
> 126

It is really ugly and there is no reason for doing it. It is much easier add one
-I nacl-mounts and remove all these '..'.

[Evgeniy Stepanov, 2012-06-01 10:08:30]

On 2012/06/01 09:52:13, Dmitry Polukhin wrote:
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> File libraries/nacl-mounts/base/KernelProxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res = NULL;
> On 2012/06/01 09:12:57, vissi wrote:
> > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > They should be thread local and static (or in unnamed namespace) to avoid
> > > collision with user variables.
> > 
> > Now they are static. You mean C++11 thread_local keyword?
> 
> I meant __thread for GCC
> http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html
> But usually it is used indirectly via macro that expands to compiler specific
> keyword. But it won't remove memory leak completely because when thread exists
> the objects will be lost.
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> File libraries/nacl-mounts/net/IOInterfaces.h (right):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> libraries/nacl-mounts/net/IOInterfaces.h:17: #include "../base/nacl_dirent.h"
> On 2012/06/01 09:12:57, vissi wrote:
> > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > I think all path should be from root of nacl-mounts. Here and in all other
> > > places fon't use .. in paths.
> > 
> > Maybe, however, there are lots of usages like this:
> > 
> > ack-grep "\.\./" | wc -l
> > 126
> 
> It is really ugly and there is no reason for doing it. It is much easier add
one
> -I nacl-mounts and remove all these '..'.

If you want to do it everywhere in nacl-mounts, it should be a separate commit.
Let's do it in the files we touch for now, as part of this change, to avoid
merge troubles.

[dpolukhin, 2012-06-01 10:26:36]

>
> https://chromiumcodereview.**appspot.com/10392070/diff/**
>
49006/libraries/nacl-mounts/**net/IOInterfaces.h#newcode17<https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mounts/net/IOInterfaces.h#newcode17>
>
>> libraries/nacl-mounts/net/**IOInterfaces.h:17: #include
>> "../base/nacl_dirent.h"
>> On 2012/06/01 09:12:57, vissi wrote:
>> > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
>> > > I think all path should be from root of nacl-mounts. Here and in all
>> other
>> > > places fon't use .. in paths.
>> >
>> > Maybe, however, there are lots of usages like this:
>> >
>> > ack-grep "\.\./" | wc -l
>> > 126
>>
>
>  It is really ugly and there is no reason for doing it. It is much easier
>> add
>>
> one
>
>> -I nacl-mounts and remove all these '..'.
>>
>
> If you want to do it everywhere in nacl-mounts, it should be a separate
> commit.
> Let's do it in the files we touch for now, as part of this change, to avoid
> merge troubles.
>

I agree. I just want to don't introduce new '..'  - we really don't need
them.

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[vissi, 2012-06-01 14:32:03]

On 2012/06/01 10:08:30, Evgeniy Stepanov wrote:
> On 2012/06/01 09:52:13, Dmitry Polukhin wrote:
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > File libraries/nacl-mounts/base/KernelProxy.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res =
NULL;
> > On 2012/06/01 09:12:57, vissi wrote:
> > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > They should be thread local and static (or in unnamed namespace) to
avoid
> > > > collision with user variables.
> > > 
> > > Now they are static. You mean C++11 thread_local keyword?
> > 
> > I meant __thread for GCC
> > http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html
> > But usually it is used indirectly via macro that expands to compiler
specific
> > keyword. But it won't remove memory leak completely because when thread
exists
> > the objects will be lost.
> > 
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > File libraries/nacl-mounts/net/IOInterfaces.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > libraries/nacl-mounts/net/IOInterfaces.h:17: #include
"../base/nacl_dirent.h"
> > On 2012/06/01 09:12:57, vissi wrote:
> > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > I think all path should be from root of nacl-mounts. Here and in all
other
> > > > places fon't use .. in paths.
> > > 
> > > Maybe, however, there are lots of usages like this:
> > > 
> > > ack-grep "\.\./" | wc -l
> > > 126
> > 
> > It is really ugly and there is no reason for doing it. It is much easier add
> one
> > -I nacl-mounts and remove all these '..'.
> 
> If you want to do it everywhere in nacl-mounts, it should be a separate
commit.
> Let's do it in the files we touch for now, as part of this change, to avoid
> merge troubles.

Concerning the paths: relative paths matter when building apps, we can configure
naclports anyway, but applications should receive consistent set of headers,
that's why there are "../" in headers installed.

[Evgeniy Stepanov, 2012-06-01 14:39:05]

On 2012/06/01 14:32:03, vissi wrote:
> On 2012/06/01 10:08:30, Evgeniy Stepanov wrote:
> > On 2012/06/01 09:52:13, Dmitry Polukhin wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > File libraries/nacl-mounts/base/KernelProxy.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res =
> NULL;
> > > On 2012/06/01 09:12:57, vissi wrote:
> > > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > > They should be thread local and static (or in unnamed namespace) to
> avoid
> > > > > collision with user variables.
> > > > 
> > > > Now they are static. You mean C++11 thread_local keyword?
> > > 
> > > I meant __thread for GCC
> > > http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html
> > > But usually it is used indirectly via macro that expands to compiler
> specific
> > > keyword. But it won't remove memory leak completely because when thread
> exists
> > > the objects will be lost.
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > File libraries/nacl-mounts/net/IOInterfaces.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > libraries/nacl-mounts/net/IOInterfaces.h:17: #include
> "../base/nacl_dirent.h"
> > > On 2012/06/01 09:12:57, vissi wrote:
> > > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > > I think all path should be from root of nacl-mounts. Here and in all
> other
> > > > > places fon't use .. in paths.
> > > > 
> > > > Maybe, however, there are lots of usages like this:
> > > > 
> > > > ack-grep "\.\./" | wc -l
> > > > 126
> > > 
> > > It is really ugly and there is no reason for doing it. It is much easier
add
> > one
> > > -I nacl-mounts and remove all these '..'.
> > 
> > If you want to do it everywhere in nacl-mounts, it should be a separate
> commit.
> > Let's do it in the files we touch for now, as part of this change, to avoid
> > merge troubles.
> 
> Concerning the paths: relative paths matter when building apps, we can
configure
> naclports anyway, but applications should receive consistent set of headers,
> that's why there are "../" in headers installed.

Good point. That's why they were added in the first place.

Now, is it (or is it not) the common approach to specify paths relative to a
common include path? Like if we install to /usr/include/nacl-mounts/..., then do
#include <nacl-mounts/base/KernelProxy.h>.

[Evgeniy Stepanov, 2012-06-01 14:42:53]

On 2012/06/01 14:39:05, Evgeniy Stepanov wrote:
> On 2012/06/01 14:32:03, vissi wrote:
> > On 2012/06/01 10:08:30, Evgeniy Stepanov wrote:
> > > On 2012/06/01 09:52:13, Dmitry Polukhin wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > File libraries/nacl-mounts/base/KernelProxy.cc (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res =
> > NULL;
> > > > On 2012/06/01 09:12:57, vissi wrote:
> > > > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > > > They should be thread local and static (or in unnamed namespace) to
> > avoid
> > > > > > collision with user variables.
> > > > > 
> > > > > Now they are static. You mean C++11 thread_local keyword?
> > > > 
> > > > I meant __thread for GCC
> > > > http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html
> > > > But usually it is used indirectly via macro that expands to compiler
> > specific
> > > > keyword. But it won't remove memory leak completely because when thread
> > exists
> > > > the objects will be lost.
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > File libraries/nacl-mounts/net/IOInterfaces.h (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > libraries/nacl-mounts/net/IOInterfaces.h:17: #include
> > "../base/nacl_dirent.h"
> > > > On 2012/06/01 09:12:57, vissi wrote:
> > > > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > > > I think all path should be from root of nacl-mounts. Here and in all
> > other
> > > > > > places fon't use .. in paths.
> > > > > 
> > > > > Maybe, however, there are lots of usages like this:
> > > > > 
> > > > > ack-grep "\.\./" | wc -l
> > > > > 126
> > > > 
> > > > It is really ugly and there is no reason for doing it. It is much easier
> add
> > > one
> > > > -I nacl-mounts and remove all these '..'.
> > > 
> > > If you want to do it everywhere in nacl-mounts, it should be a separate
> > commit.
> > > Let's do it in the files we touch for now, as part of this change, to
avoid
> > > merge troubles.
> > 
> > Concerning the paths: relative paths matter when building apps, we can
> configure
> > naclports anyway, but applications should receive consistent set of headers,
> > that's why there are "../" in headers installed.
> 
> Good point. That's why they were added in the first place.
> 
> Now, is it (or is it not) the common approach to specify paths relative to a
> common include path? Like if we install to /usr/include/nacl-mounts/..., then
do
> #include <nacl-mounts/base/KernelProxy.h>.

Yes, it seems so. See files in /usr/include/unicode, for instance.
Another approach is to require -I when building dependent apps: see
/usr/include/gtk-2.0.

The former is probably easier for us.

[vissi, 2012-06-01 15:39:46]

On 2012/06/01 14:42:53, Evgeniy Stepanov wrote:
> On 2012/06/01 14:39:05, Evgeniy Stepanov wrote:
> > On 2012/06/01 14:32:03, vissi wrote:
> > > On 2012/06/01 10:08:30, Evgeniy Stepanov wrote:
> > > > On 2012/06/01 09:52:13, Dmitry Polukhin wrote:
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > > File libraries/nacl-mounts/base/KernelProxy.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > > libraries/nacl-mounts/base/KernelProxy.cc:242: struct hostent* ghn_res
=
> > > NULL;
> > > > > On 2012/06/01 09:12:57, vissi wrote:
> > > > > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > > > > They should be thread local and static (or in unnamed namespace)
to
> > > avoid
> > > > > > > collision with user variables.
> > > > > > 
> > > > > > Now they are static. You mean C++11 thread_local keyword?
> > > > > 
> > > > > I meant __thread for GCC
> > > > > http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html
> > > > > But usually it is used indirectly via macro that expands to compiler
> > > specific
> > > > > keyword. But it won't remove memory leak completely because when
thread
> > > exists
> > > > > the objects will be lost.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > > File libraries/nacl-mounts/net/IOInterfaces.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10392070/diff/49006/libraries/nacl-mou...
> > > > > libraries/nacl-mounts/net/IOInterfaces.h:17: #include
> > > "../base/nacl_dirent.h"
> > > > > On 2012/06/01 09:12:57, vissi wrote:
> > > > > > On 2012/05/31 14:06:45, Dmitry Polukhin wrote:
> > > > > > > I think all path should be from root of nacl-mounts. Here and in
all
> > > other
> > > > > > > places fon't use .. in paths.
> > > > > > 
> > > > > > Maybe, however, there are lots of usages like this:
> > > > > > 
> > > > > > ack-grep "\.\./" | wc -l
> > > > > > 126
> > > > > 
> > > > > It is really ugly and there is no reason for doing it. It is much
easier
> > add
> > > > one
> > > > > -I nacl-mounts and remove all these '..'.
> > > > 
> > > > If you want to do it everywhere in nacl-mounts, it should be a separate
> > > commit.
> > > > Let's do it in the files we touch for now, as part of this change, to
> avoid
> > > > merge troubles.
> > > 
> > > Concerning the paths: relative paths matter when building apps, we can
> > configure
> > > naclports anyway, but applications should receive consistent set of
headers,
> > > that's why there are "../" in headers installed.
> > 
> > Good point. That's why they were added in the first place.
> > 
> > Now, is it (or is it not) the common approach to specify paths relative to a
> > common include path? Like if we install to /usr/include/nacl-mounts/...,
then
> do
> > #include <nacl-mounts/base/KernelProxy.h>.
> 
> Yes, it seems so. See files in /usr/include/unicode, for instance.
> Another approach is to require -I when building dependent apps: see
> /usr/include/gtk-2.0.
> 
> The former is probably easier for us.

So, I've performed a couple of refactorings that you and @dpolukhin discussed.
These include:
* FileStream → Socket
* merging SimpleAutoLock into PthreadHelperes
* moving to new NetAddress API
On the way:
* additional headers for newlib compatibility
* moving moar socket stream calls to KernelProxy
  (thus making SocketSubSystem just a factory).
Please take a look.

[vissi, 2012-06-01 15:42:29]

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
File libraries/nacl-mounts/util/SimpleAutoLock.h (left):

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
libraries/nacl-mounts/util/SimpleAutoLock.h:13: explicit
SimpleAutoLock(pthread_mutex_t *lock);
This file would be removed completely, I just missed it before uploading).

[vissi, 2012-06-01 15:52:08]

On 2012/06/01 15:42:29, vissi wrote:
>
https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
> File libraries/nacl-mounts/util/SimpleAutoLock.h (left):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
> libraries/nacl-mounts/util/SimpleAutoLock.h:13: explicit
> SimpleAutoLock(pthread_mutex_t *lock);
> This file would be removed completely, I just missed it before uploading).

Btw, don't you know, is there some more optimal way of working with such 'high
impact' changes? As we cannot use version control for tracking changes performed
while discussing the CL, we may lose some points, the table of deltas is bloated
an is not as useful as it could be.

[Evgeniy Stepanov, 2012-06-04 10:27:19]

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:168: Socket* stream;
add a comment about types of handles (file/socket/pre-socket)

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.cc:361: unsigned long
SocketSubSystem::gethostbyname(const char* name) {
Do we still need this fake implementation?
This should go through Resolv().

[vissi, 2012-06-04 12:43:37]

Now we need inet_pton, inet_ntop, ntohs, htons implementations to build for
newlib.

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:168: Socket* stream;
On 2012/06/04 10:27:20, Evgeniy Stepanov wrote:
> add a comment about types of handles (file/socket/pre-socket)

Done.

[Evgeniy Stepanov, 2012-06-04 13:10:18]

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:168: Socket* stream;
On 2012/06/04 12:43:37, vissi wrote:
> On 2012/06/04 10:27:20, Evgeniy Stepanov wrote:
> > add a comment about types of handles (file/socket/pre-socket)
> 
> Done.

where?

https://chromiumcodereview.appspot.com/10392070/diff/52001/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/52001/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:13: #include "../base/newlib_hacks.h"
net/newlib_compat.h

Please do not add new includes starting with ../
#include <nacl-mounts/...>

[vissi, 2012-06-04 13:26:35]

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:168: Socket* stream;
On 2012/06/04 13:10:18, Evgeniy Stepanov wrote:
> On 2012/06/04 12:43:37, vissi wrote:
> > On 2012/06/04 10:27:20, Evgeniy Stepanov wrote:
> > > add a comment about types of handles (file/socket/pre-socket)
> > 
> > Done.
> 
> where?

Forgot to include it, please, check now.

https://chromiumcodereview.appspot.com/10392070/diff/52001/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/52001/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.h:13: #include "../base/newlib_hacks.h"
On 2012/06/04 13:10:18, Evgeniy Stepanov wrote:
> net/newlib_compat.h
> 
> Please do not add new includes starting with ../
> #include <nacl-mounts/...>

Done.

[vissi, 2012-06-05 13:15:20]

On 2012/06/04 13:26:35, vissi wrote:
>
https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
> File libraries/nacl-mounts/base/KernelProxy.h (right):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/49012/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.h:168: Socket* stream;
> On 2012/06/04 13:10:18, Evgeniy Stepanov wrote:
> > On 2012/06/04 12:43:37, vissi wrote:
> > > On 2012/06/04 10:27:20, Evgeniy Stepanov wrote:
> > > > add a comment about types of handles (file/socket/pre-socket)
> > > 
> > > Done.
> > 
> > where?
> 
> Forgot to include it, please, check now.
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/52001/libraries/nacl-mou...
> File libraries/nacl-mounts/base/KernelProxy.h (right):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/52001/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.h:13: #include "../base/newlib_hacks.h"
> On 2012/06/04 13:10:18, Evgeniy Stepanov wrote:
> > net/newlib_compat.h
> > 
> > Please do not add new includes starting with ../
> > #include <nacl-mounts/...>
> 
> Done.

Removed FakeAddress usages.

[vissi, 2012-06-05 14:19:29]

Fixed problem with gethostbyname.

[Dmitry Polukhin, 2012-06-05 14:44:55]

Please fix lint warnings, make sure that old nacl-mounts clients still works
with new nacl-mounts and I think it is close to commit.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:265: dbgprintf("getaddrinfo: %s %s\n",
Please keep logging only in case of errors.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:307: SimpleAutoLock* lock = new
SimpleAutoLock(&kp_lock_);
SimpleAutoLock shouldn't be allocated with new. It should be used on stack only.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:312: return -1;
For example here you don't delete lock.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:378: if (!(handle =
GetFileHandle(fd))) {
Not for this CL but FileHandle could be destroyed between call to GetFileHandle
and lock.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:935: (ts)->tv_sec = (tv)->tv_sec;     
                              \
Please align \

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:1030: return
socket_subsystem_->shutdown(GetFileHandle(sockfd)->stream, how);
GetFileHandle is expensive operation don't call it twice for the same thing.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:83: AddressMap addrs_;
It looks like we don't need them anymore.

[vissi, 2012-06-07 10:04:06]

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:265: dbgprintf("getaddrinfo: %s %s\n",
On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> Please keep logging only in case of errors.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:307: SimpleAutoLock* lock = new
SimpleAutoLock(&kp_lock_);
On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> SimpleAutoLock shouldn't be allocated with new. It should be used on stack
only.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:935: (ts)->tv_sec = (tv)->tv_sec;     
                              \
On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> Please align \

Done.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:1030: return
socket_subsystem_->shutdown(GetFileHandle(sockfd)->stream, how);
On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> GetFileHandle is expensive operation don't call it twice for the same thing.

Done.

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
File libraries/nacl-mounts/net/SocketSubSystem.h (right):

https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
libraries/nacl-mounts/net/SocketSubSystem.h:83: AddressMap addrs_;
On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> It looks like we don't need them anymore.

Done.

[Evgeniy Stepanov, 2012-06-07 11:04:42]

LGTM, but wait for comments from Dmitry.

On 2012/06/07 10:04:06, vissi wrote:
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> File libraries/nacl-mounts/base/KernelProxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:265: dbgprintf("getaddrinfo: %s
%s\n",
> On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> > Please keep logging only in case of errors.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:307: SimpleAutoLock* lock = new
> SimpleAutoLock(&kp_lock_);
> On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> > SimpleAutoLock shouldn't be allocated with new. It should be used on stack
> only.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:935: (ts)->tv_sec = (tv)->tv_sec;   
 
>                               \
> On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> > Please align \
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:1030: return
> socket_subsystem_->shutdown(GetFileHandle(sockfd)->stream, how);
> On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> > GetFileHandle is expensive operation don't call it twice for the same thing.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> File libraries/nacl-mounts/net/SocketSubSystem.h (right):
> 
>
https://chromiumcodereview.appspot.com/10392070/diff/75003/libraries/nacl-mou...
> libraries/nacl-mounts/net/SocketSubSystem.h:83: AddressMap addrs_;
> On 2012/06/05 14:44:55, Dmitry Polukhin wrote:
> > It looks like we don't need them anymore.
> 
> Done.

[Dmitry Polukhin, 2012-06-07 12:55:38]

I'm fine with committing this CL but please look carefully what will happen on
bots after your CL.