changes in memory mount and socket subsystem to port thttpd

changes in memory mount and socket subsystem to port thttpd
Committed: https://code.google.com/p/naclports/source/detail?r=599

[vissi, 2012-06-15 12:09:13]

Please take a look.

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
File libraries/nacl-mounts/memory/MemNode.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
libraries/nacl-mounts/memory/MemNode.cc:33: | S_IWOTH;
The reason for this is that thttpd treats files as executables.

[Evgeniy Stepanov, 2012-06-15 12:31:47]

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
File libraries/nacl-mounts/memory/MemNode.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
libraries/nacl-mounts/memory/MemNode.cc:10: #include
<nacl-mounts/util/DebugPrint.h>
unused?

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
libraries/nacl-mounts/memory/MemNode.cc:26: if (!buf) return 0;
why do you need this check?

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

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:480: int KernelProxy::IsReady(int
nfds, fd_set* fds,
please add a comment about the meaning of return value, and "apply" argument (at
least).

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:492: Socket* stream = h->stream;
You've lost the check for (!stream).

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:858: iaddr->sin_port = 9000;
Why???

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:976: dbgprintf("select: timedwait
problem %m\n");
missing an argument for %m
s/problem/error/

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:990: dbgprintf("select: EBADF problem
%m\n");
see above

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:1028: return 0;
why 0?

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

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
File libraries/nacl-mounts/memory/MemNode.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
libraries/nacl-mounts/memory/MemNode.cc:10: #include
<nacl-mounts/util/DebugPrint.h>
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> unused?

Yep, rm'd.

https://chromiumcodereview.appspot.com/10556007/diff/11001/libraries/nacl-mou...
libraries/nacl-mounts/memory/MemNode.cc:26: if (!buf) return 0;
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> why do you need this check?

You are right, it's done in the caller.

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

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:492: Socket* stream = h->stream;
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> You've lost the check for (!stream).

Fixed.

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:858: iaddr->sin_port = 9000;
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> Why???

Well, we need to set some address family for the client code to work correctly.

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:976: dbgprintf("select: timedwait
problem %m\n");
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> missing an argument for %m
> s/problem/error/

%m means errno in glibc and does not require argument.

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:990: dbgprintf("select: EBADF problem
%m\n");
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> see above

here too.

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:1028: return 0;
On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> why 0?

Fixed.

[Evgeniy Stepanov, 2012-06-15 13:13:53]

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

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:858: iaddr->sin_port = 9000;
On 2012/06/15 13:03:20, vissi wrote:
> On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> > Why???
> 
> Well, we need to set some address family for the client code to work
correctly.

Move this to SocketSubsytem (or below), and query with
TCPSocketPrivate::GetRemoteAddress.

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:976: dbgprintf("select: timedwait
problem %m\n");
On 2012/06/15 13:03:20, vissi wrote:
> On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> > missing an argument for %m
> > s/problem/error/
> 
> %m means errno in glibc and does not require argument.

please make sure that newlib supports it, too

[vissi, 2012-06-18 08:35:09]

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

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:976: dbgprintf("select: timedwait
problem %m\n");
On 2012/06/15 13:13:53, Evgeniy Stepanov wrote:
> On 2012/06/15 13:03:20, vissi wrote:
> > On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> > > missing an argument for %m
> > > s/problem/error/
> > 
> > %m means errno in glibc and does not require argument.
> 
> please make sure that newlib supports it, too

Didn't find evidence for that in newlib docs, changed to more compatible %d.

[vissi, 2012-06-18 09:54:48]

updated

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

https://chromiumcodereview.appspot.com/10556007/diff/12003/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:858: iaddr->sin_port = 9000;
On 2012/06/15 13:13:53, Evgeniy Stepanov wrote:
> On 2012/06/15 13:03:20, vissi wrote:
> > On 2012/06/15 12:31:47, Evgeniy Stepanov wrote:
> > > Why???
> > 
> > Well, we need to set some address family for the client code to work
> correctly.
> 
> Move this to SocketSubsytem (or below), and query with
> TCPSocketPrivate::GetRemoteAddress.

Thanks to dpolukhin@ for implementation details.

[Evgeniy Stepanov, 2012-06-18 09:56:39]

This change fixes some pretty serious bugs, so it probably makes sense to commit
it without dealing with the issue of remote address in accept(). If that's
easier for you. So, LGTM.

Also, you need an abstract setsockopts() in BaseSocketSubSystem. Please verify
that there are no compilation errors!

https://chromiumcodereview.appspot.com/10556007/diff/3006/libraries/nacl-moun...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/3006/libraries/nacl-moun...
libraries/nacl-mounts/base/KernelProxy.cc:975: dbgprintf("select: timedwait
error %m\n");
still %m

[vissi, 2012-06-18 10:26:59]

included a missing patch now.

https://chromiumcodereview.appspot.com/10556007/diff/3006/libraries/nacl-moun...
File libraries/nacl-mounts/base/KernelProxy.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/3006/libraries/nacl-moun...
libraries/nacl-mounts/base/KernelProxy.cc:975: dbgprintf("select: timedwait
error %m\n");
On 2012/06/18 09:56:39, Evgeniy Stepanov wrote:
> still %m

Done.

[Evgeniy Stepanov, 2012-06-18 10:46:06]

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/pepper/PepperMount.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/pepper/PepperMount.cc:157: buf->st_mode = S_IFDIR |
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH
Why? Directories should be executable.

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/test.nacl/Makefile (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/test.nacl/Makefile:27: $(CXX) $(LDFLAGS) $^ -o $@
-Wl,--start-group -lgtest -lnacl-mounts -lpthread -lppapi -lppapi_cpp
-Wl,--end-group
I'd like to avoid --start-group/--end-group. It looks like simply moving
-lpthread to the end of the line should do the trick.

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/test.nacl/test_main.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/test.nacl/test_main.cc:15: Module* CreateModule() {
Let's avoid linking with ppapi altogether.
Move GetAddress to the abstact Socket class.

[Evgeniy Stepanov, 2012-06-18 10:46:45]

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/net/TcpSocket.cc:175: else if (family ==
PP_NETADDRESSFAMILY_IPV6)
Please make it fail if address family is not one of these two.

[vissi, 2012-06-18 10:56:44]

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/net/TcpSocket.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/net/TcpSocket.cc:175: else if (family ==
PP_NETADDRESSFAMILY_IPV6)
On 2012/06/18 10:46:45, Evgeniy Stepanov wrote:
> Please make it fail if address family is not one of these two.

Done.

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/pepper/PepperMount.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/pepper/PepperMount.cc:157: buf->st_mode = S_IFDIR |
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH
On 2012/06/18 10:46:06, Evgeniy Stepanov wrote:
> Why? Directories should be executable.

Indeed. Fixed for files only below.

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/test.nacl/Makefile (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/test.nacl/Makefile:27: $(CXX) $(LDFLAGS) $^ -o $@
-Wl,--start-group -lgtest -lnacl-mounts -lpthread -lppapi -lppapi_cpp
-Wl,--end-group
On 2012/06/18 10:46:06, Evgeniy Stepanov wrote:
> I'd like to avoid --start-group/--end-group. It looks like simply moving
> -lpthread to the end of the line should do the trick.

Done.

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
File libraries/nacl-mounts/test.nacl/test_main.cc (right):

https://chromiumcodereview.appspot.com/10556007/diff/8009/libraries/nacl-moun...
libraries/nacl-mounts/test.nacl/test_main.cc:15: Module* CreateModule() {
On 2012/06/18 10:46:06, Evgeniy Stepanov wrote:
> Let's avoid linking with ppapi altogether.
> Move GetAddress to the abstact Socket class.

Done.

[Evgeniy Stepanov, 2012-06-18 11:23:21]

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

https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:22: #include "net/TcpSocket.h"
not needed

https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:858: ret->GetAddress(addr);
Did you forget to include Socket.h in the CL?

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

https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:177: else return -1;
This is not checked at the caller side.
Just set the field to AF_UNSPEC and be done with it.
Should not really happen.

[vissi, 2012-06-18 11:29:14]

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

https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:22: #include "net/TcpSocket.h"
On 2012/06/18 11:23:21, Evgeniy Stepanov wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
libraries/nacl-mounts/base/KernelProxy.cc:858: ret->GetAddress(addr);
On 2012/06/18 11:23:21, Evgeniy Stepanov wrote:
> Did you forget to include Socket.h in the CL?

Sure

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

https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
libraries/nacl-mounts/net/TcpSocket.cc:177: else return -1;
On 2012/06/18 11:23:21, Evgeniy Stepanov wrote:
> This is not checked at the caller side.
> Just set the field to AF_UNSPEC and be done with it.
> Should not really happen.

Done.

[Evgeniy Stepanov, 2012-06-18 11:37:11]

LGTM

On 2012/06/18 11:29:14, vissi wrote:
>
https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
> File libraries/nacl-mounts/base/KernelProxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:22: #include "net/TcpSocket.h"
> On 2012/06/18 11:23:21, Evgeniy Stepanov wrote:
> > not needed
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
> libraries/nacl-mounts/base/KernelProxy.cc:858: ret->GetAddress(addr);
> On 2012/06/18 11:23:21, Evgeniy Stepanov wrote:
> > Did you forget to include Socket.h in the CL?
> 
> Sure
> 
>
https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
> File libraries/nacl-mounts/net/TcpSocket.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10556007/diff/12020/libraries/nacl-mou...
> libraries/nacl-mounts/net/TcpSocket.cc:177: else return -1;
> On 2012/06/18 11:23:21, Evgeniy Stepanov wrote:
> > This is not checked at the caller side.
> > Just set the field to AF_UNSPEC and be done with it.
> > Should not really happen.
> 
> Done.