Added Tor

Added Tor

This pull request is in continuation to this issue
https://codereview.chromium.org/1226403004/

For more details please read README.nacl in libevent and
tor

[deepankar-tyagi, 2015-08-20 00:00:39]

deepankar7tyagi@gmail.com changed reviewers:
+ binji@chromium.org, sbc@chromium.org

[agaurav77, 2015-08-24 19:00:04]

On 2015/08/20 00:00:39, deepankar-tyagi wrote:
> mailto:deepankar7tyagi@gmail.com changed reviewers:
> + mailto:binji@chromium.org, mailto:sbc@chromium.org

Probably dt forgot to publish+mail?

[binji, 2015-08-24 22:28:02]

mostly nits, but please see if you can make this work without turning on the
NaCl socket API flag.

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/readv.c
File ports/glibc-compat/src/readv.c (right):

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:9: ssize_t readv(int fd, const struct iovec *iov,
int iovcnt){
nit: add space between ) and {

here and elsewhere

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:10: if(iovcnt < 0){
nit: space after if

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:15: ssize_t bytes_read = 0 ;
nit: remove space before ;

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:19: bytes_read +=  read(fd, iov[i].iov_base,
iov[i].iov_len);
nit: remove double space

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:28: // #TODO(dt) (mark for)update st_atime if
required.
shouldn't be necessary -- call to read(...) above should have done it, right?

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/writ...
File ports/glibc-compat/src/writev.c (right):

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/writ...
ports/glibc-compat/src/writev.c:9: ssize_t writev(int fd,const struct iovec
*iov,int iovcnt){
similar formatting nits to readv.c

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl
File ports/libevent/README.nacl (right):

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl#...
ports/libevent/README.nacl:11: **Reason:** nacl.io do not support POSIX signals.
nacl_io

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl#...
ports/libevent/README.nacl:37: 3. **Build against glibc is not functional, ie
either static linking fails or
If non-functional, please disable it in pkg_info using
DISABLED_TOOLCHAIN=(glibc)

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl#...
ports/libevent/README.nacl:46: - Libevnt uses socketpair(...) extensively,
however the same is not
sp: Libevent

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/nacl.patch
File ports/libevent/nacl.patch (right):

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/nacl.patch#n...
ports/libevent/nacl.patch:40: +/*
use #ifndef __native_client__ instead of comment

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/nacl.patch#n...
ports/libevent/nacl.patch:56: +/*
same here

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh
File ports/tor/build.sh (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode7
ports/tor/build.sh:7: -lppapi_simple -lnacl_io -lppapi -l${NACL_CXX_LIB}"
nit: indent continued line by 4 spaces

here and elsewhere

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode13
ports/tor/build.sh:13: 
nit: remove extra line

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode36
ports/tor/build.sh:36: # LogExecute "${NACLSTRIP}" "${exe}"
remove

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode42
ports/tor/build.sh:42: # LogExecute python ${TOOLS_DIR}/create_term.py tor.nmf
remove

https://codereview.chromium.org/1289343005/diff/1/ports/tor/index.html
File ports/tor/index.html (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/index.html#newcode37
ports/tor/index.html:37: <li>Since Chrome Socket API is required,"NaCl Socket
API" flag[chrome://flags] must be enabled for testing.
Shouldn't be needed -- let's try to figure out why this isn't working with the
normal app permissions. We don't want to suggest to people to enable Chrome
flags.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json
File ports/tor/manifest.json (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json#new...
ports/tor/manifest.json:8: "sockets": {
not needed, see the other ports

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json#new...
ports/tor/manifest.json:18: "*://0.0.0.0/*",
probably unneeded

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json#new...
ports/tor/manifest.json:19: "*://localhost/*",
probably unneeded

https://codereview.chromium.org/1289343005/diff/1/ports/tor/nacl.patch
File ports/tor/nacl.patch (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/nacl.patch#newcode32
ports/tor/nacl.patch:32: +    //tor_free(process_ownername);
why comment out the tor_free?

https://codereview.chromium.org/1289343005/diff/1/ports/tor/nacl.patch#newcode70
ports/tor/nacl.patch:70: +/*  if (running_tor && !have_lockfile()) {
use #ifndef __native_client__ instead of commenting code

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js
File ports/tor/tor.js (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode8
ports/tor/tor.js:8: //NaClTerm.cwd = '/home/user';
remove

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode24
ports/tor/tor.js:24: //
Log an error message here?

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode33
ports/tor/tor.js:33: // NaClTerm.argv = ['--SOCKSPort', '9999'];
Remove or clean up these comments -- as it is, it is not clear what they do and
why you would use them.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode39
ports/tor/tor.js:39: //console.log(items.torArgs);
nit: indent 2 spaces everywhere

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode43
ports/tor/tor.js:43: if(!anotherTorRunning()){
nit: indent 2 spaces

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode47
ports/tor/tor.js:47: console.log("another tor instance running,exiting...");
remove else clause for now, as it is untested.

[deepankar-tyagi, 2015-08-25 18:14:45]

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/readv.c
File ports/glibc-compat/src/readv.c (right):

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:9: ssize_t readv(int fd, const struct iovec *iov,
int iovcnt){
On 2015/08/24 22:28:00, binji wrote:
> nit: add space between ) and {
> 
> here and elsewhere

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:10: if(iovcnt < 0){
On 2015/08/24 22:28:01, binji wrote:
> nit: space after if

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:15: ssize_t bytes_read = 0 ;
On 2015/08/24 22:28:01, binji wrote:
> nit: remove space before ;

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:19: bytes_read +=  read(fd, iov[i].iov_base,
iov[i].iov_len);
On 2015/08/24 22:28:00, binji wrote:
> nit: remove double space

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/read...
ports/glibc-compat/src/readv.c:28: // #TODO(dt) (mark for)update st_atime if
required.
On 2015/08/24 22:28:00, binji wrote:
> shouldn't be necessary -- call to read(...) above should have done it, right?

haven't looked at read(...) implementation yet, for now removed the comment.But
I think it shouldn't be a concern

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/writ...
File ports/glibc-compat/src/writev.c (right):

https://codereview.chromium.org/1289343005/diff/1/ports/glibc-compat/src/writ...
ports/glibc-compat/src/writev.c:9: ssize_t writev(int fd,const struct iovec
*iov,int iovcnt){
On 2015/08/24 22:28:01, binji wrote:
> similar formatting nits to readv.c

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl
File ports/libevent/README.nacl (right):

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl#...
ports/libevent/README.nacl:11: **Reason:** nacl.io do not support POSIX signals.
On 2015/08/24 22:28:01, binji wrote:
> nacl_io

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl#...
ports/libevent/README.nacl:37: 3. **Build against glibc is not functional, ie
either static linking fails or
On 2015/08/24 22:28:01, binji wrote:
> If non-functional, please disable it in pkg_info using
> DISABLED_TOOLCHAIN=(glibc)

disabled

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/README.nacl#...
ports/libevent/README.nacl:46: - Libevnt uses socketpair(...) extensively,
however the same is not
On 2015/08/24 22:28:01, binji wrote:
> sp: Libevent

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/nacl.patch
File ports/libevent/nacl.patch (right):

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/nacl.patch#n...
ports/libevent/nacl.patch:40: +/*
On 2015/08/24 22:28:01, binji wrote:
> use #ifndef __native_client__ instead of comment

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/libevent/nacl.patch#n...
ports/libevent/nacl.patch:56: +/*
On 2015/08/24 22:28:01, binji wrote:
> same here

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh
File ports/tor/build.sh (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode7
ports/tor/build.sh:7: -lppapi_simple -lnacl_io -lppapi -l${NACL_CXX_LIB}"
On 2015/08/24 22:28:01, binji wrote:
> nit: indent continued line by 4 spaces
> 
> here and elsewhere

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode13
ports/tor/build.sh:13: 
On 2015/08/24 22:28:01, binji wrote:
> nit: remove extra line

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode36
ports/tor/build.sh:36: # LogExecute "${NACLSTRIP}" "${exe}"
On 2015/08/24 22:28:01, binji wrote:
> remove

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/build.sh#newcode42
ports/tor/build.sh:42: # LogExecute python ${TOOLS_DIR}/create_term.py tor.nmf
On 2015/08/24 22:28:01, binji wrote:
> remove

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/index.html
File ports/tor/index.html (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/index.html#newcode37
ports/tor/index.html:37: <li>Since Chrome Socket API is required,"NaCl Socket
API" flag[chrome://flags] must be enabled for testing.
On 2015/08/24 22:28:01, binji wrote:
> Shouldn't be needed -- let's try to figure out why this isn't working with the
> normal app permissions. We don't want to suggest to people to enable Chrome
> flags.

Issue resolved,turns out I messed up in permissions
while testing, the app crashed, enabled the flag, it worked and I thought the
flag was the issue.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json
File ports/tor/manifest.json (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json#new...
ports/tor/manifest.json:8: "sockets": {
On 2015/08/24 22:28:01, binji wrote:
> not needed, see the other ports

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json#new...
ports/tor/manifest.json:18: "*://0.0.0.0/*",
On 2015/08/24 22:28:01, binji wrote:
> probably unneeded

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/manifest.json#new...
ports/tor/manifest.json:19: "*://localhost/*",
On 2015/08/24 22:28:01, binji wrote:
> probably unneeded

fixed...

https://codereview.chromium.org/1289343005/diff/1/ports/tor/nacl.patch
File ports/tor/nacl.patch (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/nacl.patch#newcode32
ports/tor/nacl.patch:32: +    //tor_free(process_ownername);
On 2015/08/24 22:28:01, binji wrote:
> why comment out the tor_free?

I was of opinion that a structure might be keeping track, however still unsure
what effect it might have, for now I have un-commented it and there is no change
as far as functionality is concerned.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/nacl.patch#newcode70
ports/tor/nacl.patch:70: +/*  if (running_tor && !have_lockfile()) {
On 2015/08/24 22:28:01, binji wrote:
> use #ifndef __native_client__ instead of commenting code

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js
File ports/tor/tor.js (right):

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode8
ports/tor/tor.js:8: //NaClTerm.cwd = '/home/user';
On 2015/08/24 22:28:02, binji wrote:
> remove

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode24
ports/tor/tor.js:24: //
On 2015/08/24 22:28:01, binji wrote:
> Log an error message here?

remove the function for now(till GUI is developed)

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode33
ports/tor/tor.js:33: // NaClTerm.argv = ['--SOCKSPort', '9999'];
On 2015/08/24 22:28:01, binji wrote:
> Remove or clean up these comments -- as it is, it is not clear what they do
and
> why you would use them.

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode39
ports/tor/tor.js:39: //console.log(items.torArgs);
On 2015/08/24 22:28:01, binji wrote:
> nit: indent 2 spaces everywhere

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode43
ports/tor/tor.js:43: if(!anotherTorRunning()){
On 2015/08/24 22:28:01, binji wrote:
> nit: indent 2 spaces

Done.

https://codereview.chromium.org/1289343005/diff/1/ports/tor/tor.js#newcode47
ports/tor/tor.js:47: console.log("another tor instance running,exiting...");
On 2015/08/24 22:28:02, binji wrote:
> remove else clause for now, as it is untested.

Done.

[deepankar-tyagi, 2015-08-25 18:18:51]

The app can be installed from here
https://chrome.google.com/webstore/detail/krotorbeta-access-interne/dfdhngcah...

[Sam Clegg, 2015-08-25 18:25:30]

lgtm

https://codereview.chromium.org/1289343005/diff/20001/ports/glibc-compat/src/...
File ports/glibc-compat/src/writev.c (right):

https://codereview.chromium.org/1289343005/diff/20001/ports/glibc-compat/src/...
ports/glibc-compat/src/writev.c:4: 
Have you don't a sync recently?  I think these functions were already added a
few weeks ago in this change:
https://codereview.chromium.org/1260083004

https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/build.sh
File ports/libevent/build.sh (right):

https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/build.sh...
ports/libevent/build.sh:8: -lppapi_simple -lnacl_io -lppapi -l${NACL_CXX_LIB}"
You only need NACL_CLI_MAIN_LIB.. that other are all included in that.

https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.patch
File ports/libevent/nacl.patch (right):

https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.pat...
ports/libevent/nacl.patch:37: +        }
Is this useful?   If so, match indentation style of surrounding code.

https://codereview.chromium.org/1289343005/diff/20001/ports/tor/build.sh
File ports/tor/build.sh (right):

https://codereview.chromium.org/1289343005/diff/20001/ports/tor/build.sh#newc...
ports/tor/build.sh:7: -lppapi_simple -lnacl_io -lppapi -l${NACL_CXX_LIB}"
Just NACL_CLI_MAIN_LIB

https://codereview.chromium.org/1289343005/diff/20001/ports/tor/build.sh#newc...
ports/tor/build.sh:37: _platform_specific/*/tor${NACL_EXEEXT} \
Indent the continuation 4 spaces

https://codereview.chromium.org/1289343005/diff/20001/ports/tor/build.sh#newc...
ports/tor/build.sh:39: -o tor.nmf
Looks like these two lines will fit on the preceding line.

[deepankar-tyagi, 2015-08-25 18:34:40]

On 2015/08/25 18:25:30, Sam Clegg wrote:
> lgtm
> 
>
https://codereview.chromium.org/1289343005/diff/20001/ports/glibc-compat/src/...
> ports/glibc-compat/src/writev.c:4: 
> Have you don't a sync recently?  I think these functions were already added a
> few weeks ago in this change:
> https://codereview.chromium.org/1260083004
Its been around 8 days since I synced, will update and send the patch(if needed)
> 
> 
>
https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/build.sh...
> ports/libevent/build.sh:8: -lppapi_simple -lnacl_io -lppapi -l${NACL_CXX_LIB}"
> You only need NACL_CLI_MAIN_LIB.. that other are all included in that.
After I updated, the build started failing, looked at the changes/commits in
other ports,
copied the same.
> 

making rest of the modifications, will send the patch shortly

[deepankar-tyagi, 2015-08-25 19:50:55]

On 2015/08/25 18:34:40, deepankar-tyagi wrote:
> On 2015/08/25 18:25:30, Sam Clegg wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1289343005/diff/20001/ports/glibc-compat/src/...
> > ports/glibc-compat/src/writev.c:4: 
> > Have you don't a sync recently?  I think these functions were already added
a
> > few weeks ago in this change:
> > https://codereview.chromium.org/1260083004
> Its been around 8 days since I synced, will update and send the patch(if
needed)
> > 
> > 
> >
>
https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/build.sh...
> > ports/libevent/build.sh:8: -lppapi_simple -lnacl_io -lppapi
-l${NACL_CXX_LIB}"
> > You only need NACL_CLI_MAIN_LIB.. that other are all included in that.
> After I updated, the build started failing, looked at the changes/commits in
> other ports,
> copied the same.
> > 
> 
> making rest of the modifications, will send the patch shortly

I was on pepper 44 branch, shifted to master, sending the patch shortly
 (as expected git-cl created a separate issue)

[deepankar-tyagi, 2015-08-25 19:53:36]

https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.patch
File ports/libevent/nacl.patch (right):

https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.pat...
ports/libevent/nacl.patch:37: +        }
On 2015/08/25 18:25:30, Sam Clegg wrote:
> Is this useful?   If so, match indentation style of surrounding code.

when executed, the binary doesn't provide any info, only way to know about port
is to see the code.
Indented.

[Sam Clegg, 2015-12-03 20:13:29]

On 2015/08/25 19:53:36, deepankar-tyagi wrote:
>
https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.patch
> File ports/libevent/nacl.patch (right):
> 
>
https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.pat...
> ports/libevent/nacl.patch:37: +        }
> On 2015/08/25 18:25:30, Sam Clegg wrote:
> > Is this useful?   If so, match indentation style of surrounding code.
> 
> when executed, the binary doesn't provide any info, only way to know about
port
> is to see the code.
> Indented.

Id there an update on this?  We have others who would be interested in the
libevent point.

[deepankar-tyagi, 2015-12-03 21:20:30]

On 2015/12/03 20:13:29, Sam Clegg wrote:
> On 2015/08/25 19:53:36, deepankar-tyagi wrote:
> >
>
https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.patch
> > File ports/libevent/nacl.patch (right):
> > 
> >
>
https://codereview.chromium.org/1289343005/diff/20001/ports/libevent/nacl.pat...
> > ports/libevent/nacl.patch:37: +        }
> > On 2015/08/25 18:25:30, Sam Clegg wrote:
> > > Is this useful?   If so, match indentation style of surrounding code.
> > 
> > when executed, the binary doesn't provide any info, only way to know about
> port
> > is to see the code.
> > Indented.
> 
> Id there an update on this?  We have others who would be interested in the
> libevent point.

Yes the patch at https://codereview.chromium.org/1311703003/ incorporates all
the suggestions and is functional.