this patch adds the manifest proxy server to sel_ldr and the manifest

this patch adds the manifest proxy server to sel_ldr and the manifest
proxy service stubs to reverse_service in the plugin.  the service
stubs are not hooked up, and just logs and sends back bogus replies.
this patch also adds an SRPC-based (irt=0) test to tests/nameservice/
so that the nexe would run in the browser and thereby trigger the RPCs
to the plugin to exercise the name service and manifest sub-name
service functionality.

the plan is that the RPC service stubs in the plugin will be replaced
with Pepper calls that uses the trusted interface to extract the
underlying file descriptor and pass it back to the untrusted IRT code,
initially for read-only access.  the IRT can use the descriptor to
emulate the Pepper file object read/write API using read/write service
runtime syscalls, rather than doing bulk data transfers via RPCs,
providing a short-circuited I/O path.  for read/write access (later),
hooking up quota management callbacks (also via reverse channels) will
be necessary, but quota calls are control plane traffic, and bulk data
plane traffic should continue to go through service runtime syscalls.

TEST= tests/nameservice/srpc_nameservice_test
BUG= http://code.google.com/p/nativeclient/issues/detail?id=1534 http://code.google.com/p/nativeclient/issues/detail?id=566
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=5671

[bsy, 2011-06-10 23:14:10]

polina: plz review the plugin-side changes
noel: plz review the rest

[bsy, 2011-06-13 23:43:52]

ping

[noelallen_use_chromium, 2011-06-14 02:25:45]

First set of comments, I need to check the nexe to understand exactly what you
are testing.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
File src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp
(right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:44:
'XP_WIN',
Did you copy these, or are you defining them on purpose?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:55:
'configurations': {
This seems inconsistent with how we support x64.    Usually we use:
    'variables': {
            'win_target': 'x64',
    },

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:74:
'type': 'static_library', # 'dynamic_library', ?!?
Did you mean to leave in this comment?  I assume it pertains to the win64
version as well?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
File src/trusted/reverse_service/manifest_rpc.h (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/manifest_rpc.h:8: 
Q: Avoiding use of a *.srpc file?  Reduces the complexity of this widget but
makes it harder later to find our uses of SRPC I would imagine.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/reverse_service.cc:8: 
Why is portability_io.h included before string.h?  Did you mean portability.h
not portability_io.h?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/reverse_service.cc:98: 
No NaClLog?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/reverse_service.cc:121: out_args[2]->u.count = 10;
Either u.count above should be set as sizeof('string') or as the return from
strncpy to prevent drift when changing the string.  In addition this implies
that out_args[2]->arrays.carr was preallocated.  So the strncpy should the a max
size of that preallocated size which I assume to be out_args[2]->u.count?
.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (left):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:275: EXTRA_LIBS=['sel',
Why do so many tests need manifest_proxy?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/name_service/default_name_service.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/default_name_service.c:9: #include
"native_client/src/shared/platform/nacl_log.h"
include order?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/default_name_service.c:20: struct
NaClDescRng  *rng = NULL;
Did you mean to dbl space here?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/default_name_service.c:41: rng = NULL;
You immediately return, this is a NOP.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/name_service/name_service.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/name_service.c:297: nnsep = NULL;
We are clearing this pointer to prevent it's use after the unlock?  Not part of
this CL, but I've seen in a few cases and I'm trying to understand why we do it.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr.c:1532:
NaClThreadDtor(&nap->reverse_setup_thread);
Safe?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr.h (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr.h:544: void                       
*handler_state,
nit: Should these param names be intented to align with '('

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr_standard.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:27: #include
"native_client/src/trusted/manifest_name_service_proxy/manifest_proxy.h"
include order?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:534: NaClXMutexLock(&nap->mu);
Comment?
As is, this code appears racy...  The variable could have been setup between the
unlock and the if.   I suspect the only reason you are locking this at all is to
prevent a thread sanitizer warning?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:572: * StartThread requires lock.
Where is the lock this comment mentions?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:577: NaClRefCountUnref((struct
NaClRefCount *) manifest_proxy);
This is counter-intuitive.  Internally NaClSimpleServiceStartServiceThread does
a refcnt even on failure?  If it failed shouldn't we be freeing this object
instead of unrefing it?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/service_runtime.gyp (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/service_runtime.gyp:339: 'win_target': 'x64',
note: The other way to declare win64 targets.

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons
File tests/nameservice/nacl.scons (right):

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons...
tests/nameservice/nacl.scons:19: srpc_ns_nexe_name =
'srpc_nameservice_test_%s.nexe' % env.get('TARGET_FULLARCH')
Why uniquify the nexe name?  Won't it go into scons-out/nacl-XXX automatically?

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons...
tests/nameservice/nacl.scons:20: srpc_ns_nexe =
env.ComponentProgram(srpc_ns_nexe_name,
Should have Nick dbl check this.

[bsy, 2011-06-14 20:29:32]

ncbray:  noelallen wants you to check line 20 (or thereabouts) of

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons...

-bsy

On Mon, Jun 13, 2011 at 7:25 PM, <noelallen@google.com> wrote:

> First set of comments, I need to check the nexe to understand exactly what
> you
> are testing.
>
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/manifest_name_service_**proxy/manifest_name_service_**proxy.gyp<http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp>
> File
>
> src/trusted/manifest_name_**service_proxy/manifest_name_**
> service_proxy.gyp
> (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
> trusted/manifest_name_service_**proxy/manifest_name_service_**
>
proxy.gyp#newcode44<http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp#newcode44>
> src/trusted/manifest_name_**service_proxy/manifest_name_**
> service_proxy.gyp:44:
> 'XP_WIN',
> Did you copy these, or are you defining them on purpose?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
> trusted/manifest_name_service_**proxy/manifest_name_service_**
>
proxy.gyp#newcode55<http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp#newcode55>
> src/trusted/manifest_name_**service_proxy/manifest_name_**
> service_proxy.gyp:55:
> 'configurations': {
> This seems inconsistent with how we support x64.    Usually we use:
>    'variables': {
>            'win_target': 'x64',
>    },
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
> trusted/manifest_name_service_**proxy/manifest_name_service_**
>
proxy.gyp#newcode74<http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp#newcode74>
> src/trusted/manifest_name_**service_proxy/manifest_name_**
> service_proxy.gyp:74:
> 'type': 'static_library', # 'dynamic_library', ?!?
> Did you mean to leave in this comment?  I assume it pertains to the
> win64 version as well?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/reverse_service/**manifest_rpc.h<http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/manifest_rpc.h>
> File src/trusted/reverse_service/**manifest_rpc.h (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/reverse_service/**manifest_rpc.h#newcode8<http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/manifest_rpc.h#newcode8>
> src/trusted/reverse_service/**manifest_rpc.h:8:
> Q: Avoiding use of a *.srpc file?  Reduces the complexity of this widget
> but makes it harder later to find our uses of SRPC I would imagine.
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/reverse_service/**reverse_service.cc<http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/reverse_service.cc>
> File src/trusted/reverse_service/**reverse_service.cc (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/reverse_service/**reverse_service.cc#newcode8<http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/reverse_service.cc#newcode8>
> src/trusted/reverse_service/**reverse_service.cc:8:
> Why is portability_io.h included before string.h?  Did you mean
> portability.h not portability_io.h?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/reverse_service/**reverse_service.cc#newcode98<http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/reverse_service.cc#newcode98>
> src/trusted/reverse_service/**reverse_service.cc:98:
> No NaClLog?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/reverse_service/**reverse_service.cc#newcode121<http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/reverse_service.cc#newcode121>
> src/trusted/reverse_service/**reverse_service.cc:121: out_args[2]->u.count
> = 10;
> Either u.count above should be set as sizeof('string') or as the return
> from strncpy to prevent drift when changing the string.  In addition
> this implies that out_args[2]->arrays.carr was preallocated.  So the
> strncpy should the a max size of that preallocated size which I assume
> to be out_args[2]->u.count?
> .
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/build.**scons<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/build.scons>
> File src/trusted/service_runtime/**build.scons (left):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/build.**scons#oldcode275<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/build.scons#oldcode275>
> src/trusted/service_runtime/**build.scons:275: EXTRA_LIBS=['sel',
> Why do so many tests need manifest_proxy?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/name_**service/default_name_service.c<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/name_service/default_name_service.c>
> File src/trusted/service_runtime/**name_service/default_name_**service.c
> (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/name_**service/default_name_service.**c#newcode9<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/name_service/default_name_service.c#newcode9>
> src/trusted/service_runtime/**name_service/default_name_**service.c:9:
> #include "native_client/src/shared/**platform/nacl_log.h"
> include order?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/name_**service/default_name_service.**c#newcode20<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/name_service/default_name_service.c#newcode20>
> src/trusted/service_runtime/**name_service/default_name_**service.c:20:
> struct NaClDescRng  *rng = NULL;
> Did you mean to dbl space here?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/name_**service/default_name_service.**c#newcode41<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/name_service/default_name_service.c#newcode41>
> src/trusted/service_runtime/**name_service/default_name_**service.c:41:
> rng
> = NULL;
> You immediately return, this is a NOP.
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/name_**service/name_service.c<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/name_service/name_service.c>
> File src/trusted/service_runtime/**name_service/name_service.c (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/name_**service/name_service.c#**newcode297<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/name_service/name_service.c#newcode297>
> src/trusted/service_runtime/**name_service/name_service.c:**297: nnsep =
> NULL;
> We are clearing this pointer to prevent it's use after the unlock?  Not
> part of this CL, but I've seen in a few cases and I'm trying to
> understand why we do it.
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr.c<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr.c>
> File src/trusted/service_runtime/**sel_ldr.c (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr.c#newcode1532<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr.c#newcode1532>
> src/trusted/service_runtime/**sel_ldr.c:1532:
> NaClThreadDtor(&nap->reverse_**setup_thread);
> Safe?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr.h<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr.h>
> File src/trusted/service_runtime/**sel_ldr.h (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr.h#newcode544<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr.h#newcode544>
> src/trusted/service_runtime/**sel_ldr.h:544: void
> *handler_state,
> nit: Should these param names be intented to align with '('
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr_standard.c<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr_standard.c>
> File src/trusted/service_runtime/**sel_ldr_standard.c (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr_standard.c#newcode27<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr_standard.c#newcode27>
> src/trusted/service_runtime/**sel_ldr_standard.c:27: #include
> "native_client/src/trusted/**manifest_name_service_proxy/**
> manifest_proxy.h"
> include order?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr_standard.c#newcode534<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr_standard.c#newcode534>
> src/trusted/service_runtime/**sel_ldr_standard.c:534:
> NaClXMutexLock(&nap->mu);
> Comment?
> As is, this code appears racy...  The variable could have been setup
> between the unlock and the if.   I suspect the only reason you are
> locking this at all is to prevent a thread sanitizer warning?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr_standard.c#newcode572<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr_standard.c#newcode572>
> src/trusted/service_runtime/**sel_ldr_standard.c:572: * StartThread
> requires lock.
> Where is the lock this comment mentions?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/sel_**ldr_standard.c#newcode577<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/sel_ldr_standard.c#newcode577>
> src/trusted/service_runtime/**sel_ldr_standard.c:577:
> NaClRefCountUnref((struct NaClRefCount *) manifest_proxy);
> This is counter-intuitive.  Internally
> NaClSimpleServiceStartServiceT**hread does a refcnt even on failure?  If
> it failed shouldn't we be freeing this object instead of unrefing it?
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/**service_runtime.gyp<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/service_runtime.gyp>
> File src/trusted/service_runtime/**service_runtime.gyp (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/src/**
>
trusted/service_runtime/**service_runtime.gyp#newcode339<http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/service_runtime.gyp#newcode339>
> src/trusted/service_runtime/**service_runtime.gyp:339: 'win_target':
> 'x64',
> note: The other way to declare win64 targets.
>
> http://codereview.chromium.**org/7108031/diff/4006/tests/**
>
nameservice/nacl.scons<http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons>
> File tests/nameservice/nacl.scons (right):
>
> http://codereview.chromium.**org/7108031/diff/4006/tests/**
>
nameservice/nacl.scons#**newcode19<http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons#newcode19>
> tests/nameservice/nacl.scons:**19: srpc_ns_nexe_name =
> 'srpc_nameservice_test_%s.**nexe' % env.get('TARGET_FULLARCH')
> Why uniquify the nexe name?  Won't it go into scons-out/nacl-XXX
> automatically?
>
> http://codereview.chromium.**org/7108031/diff/4006/tests/**
>
nameservice/nacl.scons#**newcode20<http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons#newcode20>
> tests/nameservice/nacl.scons:**20: srpc_ns_nexe =
> env.ComponentProgram(srpc_ns_**nexe_name,
> Should have Nick dbl check this.
>
>
>
http://codereview.chromium.**org/7108031/<http://codereview.chromium.org/7108...
>



-- 
bennet s yee
i usually don't capitalize due to mild tendonitis

-- 
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.

[bsy, 2011-06-14 20:30:03]

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
File src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp
(right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:44:
'XP_WIN',
cut-n-pasted cargo-cult code.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:55:
'configurations': {
src/trusted/desc/desc.gyp was probably the source of the original cut-n-paste. 
svn blame says ilewis, though possibly the original were from bradnelson and/or
sehr.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:74:
'type': 'static_library', # 'dynamic_library', ?!?
On 2011/06/14 02:25:45, noelallen wrote:
> Did you mean to leave in this comment?  I assume it pertains to the win64
> version as well?

Done.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
File src/trusted/reverse_service/manifest_rpc.h (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/manifest_rpc.h:8: 
Bsy caveman.  Bsy wrote srpc before srpc files.

There are a couple of other non code-gen uses elsewhere.  Should probably be a
separate CL to migrate such code, once better support for the use of code
generators in our build (so we don't have to check in the generated code, etc).

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/reverse_service.cc:8: 
several of our portability* files had to be included before standard includes
for some forgotten reason.  maybe it #defines or turn on flags that needed to be
turned on before the standard includes?  many of those were moved to the cc
command line so this might not be necessary any more.  i'll test to see if this
can be moved.

i did not mean portability.h instead of portability.h -- robertm wanted to split
stuff out of a single portability.h, and i'm only using SNPRINTF here.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/reverse_service.cc:121: out_args[2]->u.count = 10;
this is stub code that will be removed once the manifest lookup actually
consults the manifest and accesses app cache etc, so it's not worth the bother.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (left):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:275: EXTRA_LIBS=['sel',
because scons does not have a way for library A to declare that all uses of
library A must also link against library B.  here A is sel, and B is
manifest_proxy.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/name_service/default_name_service.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/default_name_service.c:9: #include
"native_client/src/shared/platform/nacl_log.h"
On 2011/06/14 02:25:45, noelallen wrote:
> include order?

Done.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/default_name_service.c:20: struct
NaClDescRng  *rng = NULL;
On 2011/06/14 02:25:45, noelallen wrote:
> Did you mean to dbl space here?

i use emacs tab to line up identifiers -- this is the consistent style used in
much of the tcb -- and c++ emacs tab stops are set at every two spaces.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/default_name_service.c:41: rng = NULL;
i know.  the compiler will also know.  i do similar thing in dtors and other
cleanup functions in case there are more code added later, and sometimes in case
there is an object-reuse (use-after-free) problem.  having the explicit
assignment here (should) make obvious the ownership transfer.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/name_service/name_service.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/name_service/name_service.c:297: nnsep = NULL;
defensive programming.  compilers will get rid of it.  new code added later
won't accidentally use it.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr.c:1532:
NaClThreadDtor(&nap->reverse_setup_thread);
yes.  the dtor does bookkeeping cleanup such as closing the thread handle on
windows, but does not o/w have an effect on the actual thread.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr.h (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr.h:544: void                       
*handler_state,
On 2011/06/14 02:25:45, noelallen wrote:
> nit: Should these param names be intented to align with '('

sure, that would make it more readable and less likely to be confused w/ the
arglist formal arguments.  in general i just tab to the tab stop that aligns
with the rightmost identifier.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr_standard.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:27: #include
"native_client/src/trusted/manifest_name_service_proxy/manifest_proxy.h"
On 2011/06/14 02:25:45, noelallen wrote:
> include order?

Done.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:534: NaClXMutexLock(&nap->mu);
reverse_channel_initialized is set in the RPC handler for reverse_setup call --
which occurs in the secure command channel srpc handler thread.  thus access to
it should be locked, or else cache coherency issues might cause us to see it as
not initialized when it really was initialized.

yes, the RPCs needed to launch the app is defined so that the plugin must do
reverse_setup before start_module, so that by the time this fn is invoked,
reverse_channel_initialized will have long be set -- and there would have been
enough other synchronization operations that an unlocked read would still yield
the correct value.  i don't know for sure whether tsan would warn since i
haven't run it, but i suspect it would (it should!).

i'll add a comment.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:572: * StartThread requires lock.
On 2011/06/14 02:25:45, noelallen wrote:
> Where is the lock this comment mentions?

clarified.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr_standard.c:577: NaClRefCountUnref((struct
NaClRefCount *) manifest_proxy);
any object of a NaClRefCount subclass, after it's been fully constructed, is
freed only as a side effect of decrementing its refcount (which also runs the
dtor chain before freeing).  the manifest_proxy is fully constructed at this
point.

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons
File tests/nameservice/nacl.scons (right):

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons...
tests/nameservice/nacl.scons:19: srpc_ns_nexe_name =
'srpc_nameservice_test_%s.nexe' % env.get('TARGET_FULLARCH')
see srpc_hw.  that is where this is cargo-culted from.  i think the intent is
that in real deployments, the nmf file and the various architecture-specific
nexes can be in the same directory.

http://codereview.chromium.org/7108031/diff/4006/tests/nameservice/nacl.scons...
tests/nameservice/nacl.scons:20: srpc_ns_nexe =
env.ComponentProgram(srpc_ns_nexe_name,
On 2011/06/14 02:25:45, noelallen wrote:
> Should have Nick dbl check this.

are you asking me to ask nick?  fewer redirects, please.

[noelallen_use_chromium, 2011-06-14 22:08:13]

A few nits, and a missing error check in the test, otherwise LGTM.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
File src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp
(right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:28:
['OS=="linux"', {
You should be able to ditch all these conditional defines.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:44:
'XP_WIN',
On 2011/06/14 20:30:03, bsy wrote:
> cut-n-pasted cargo-cult code.
_WIN32 is what we should be checking, and it should be automatically defined by
windows compilers.  Although I would prefer to get rid of it, redefining it
should be safe and arguably helps compilers with non-standard macros.   However
WIN32 should be avoided, likewise _WINDOWS because it propagates non-standard
macros.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
File src/trusted/reverse_service/manifest_rpc.h (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/manifest_rpc.h:8: 
agreed.
On 2011/06/14 20:30:03, bsy wrote:
> Bsy caveman.  Bsy wrote srpc before srpc files.
> 
> There are a couple of other non code-gen uses elsewhere.  Should probably be a
> separate CL to migrate such code, once better support for the use of code
> generators in our build (so we don't have to check in the generated code,
etc).

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/reverse_service/...
src/trusted/reverse_service/reverse_service.cc:8: 
I mentioned portability vs portability_io, because portability must be first
(and has a comment to that effect), I do not believe portability_io has the same
requirement (and if it does, it should have the same comment in the header).

On 2011/06/14 20:30:03, bsy wrote:
> several of our portability* files had to be included before standard includes
> for some forgotten reason.  maybe it #defines or turn on flags that needed to
be
> turned on before the standard includes?  many of those were moved to the cc
> command line so this might not be necessary any more.  i'll test to see if
this
> can be moved.
> 
> i did not mean portability.h instead of portability.h -- robertm wanted to
split
> stuff out of a single portability.h, and i'm only using SNPRINTF here.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_ldr.c (right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_ldr.c:1532:
NaClThreadDtor(&nap->reverse_setup_thread);
That was actually a note to me to go check what NaClThreadDtor does, just forgot
to remove it.  :)
On 2011/06/14 20:30:03, bsy wrote:
> yes.  the dtor does bookkeeping cleanup such as closing the thread handle on
> windows, but does not o/w have an effect on the actual thread.

http://codereview.chromium.org/7108031/diff/14001/tests/nameservice/srpc_name...
File tests/nameservice/srpc_nameservice_test.c (right):

http://codereview.chromium.org/7108031/diff/14001/tests/nameservice/srpc_name...
tests/nameservice/srpc_nameservice_test.c:193: }
Missing an error case?  If you are assuming status is enough, they why not
(void) NaClSrpc...?
I believe you could fail to execute the function, such as with a return of
NACL_SRPC_RESULT_APP_ERROR, and status would be unmodified, so the check should
happen.

Since this is a test, and the real interest is to catch a failure, it sufficient
to set status to something other than NACL_NAME_SERVICE_SUCCESS to ensure the
current assert triggers correctly (in case the value on the stack happened to be
...)

http://codereview.chromium.org/7108031/diff/14001/tests/nameservice/srpc_name...
tests/nameservice/srpc_nameservice_test.c:291: }
Why not close NS?

[bsy, 2011-06-15 00:21:03]

polina?

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
File src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp
(right):

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:28:
['OS=="linux"', {
On 2011/06/14 22:08:14, noelallen wrote:
> You should be able to ditch all these conditional defines.

Done.

http://codereview.chromium.org/7108031/diff/4006/src/trusted/manifest_name_se...
src/trusted/manifest_name_service_proxy/manifest_name_service_proxy.gyp:44:
'XP_WIN',
removed defines section

http://codereview.chromium.org/7108031/diff/14001/tests/nameservice/srpc_name...
File tests/nameservice/srpc_nameservice_test.c (right):

http://codereview.chromium.org/7108031/diff/14001/tests/nameservice/srpc_name...
tests/nameservice/srpc_nameservice_test.c:193: }
oops.  missing body to the error check.  i've changed it to an assert.

http://codereview.chromium.org/7108031/diff/14001/tests/nameservice/srpc_name...
tests/nameservice/srpc_nameservice_test.c:291: }
On 2011/06/14 22:08:14, noelallen wrote:
> Why not close NS?

Done.

[polina, 2011-06-15 00:40:29]

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:49: out_args[0]->u.bval = 1;
Start now always succeeds?

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:89: // NACL_NAME_SERVICE_LIST
list::C -- enumerate all names in the manifest
NACL_MANIFEST_LIST?

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:95: char* dest =
out_args[0]->arrays.carr;
It would be more readable if you used out_args directly below: my first reaction
was that you were reading from out args instead of in args by mistake.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:106: // NACL_NAME_SERVICE_LOOKUP
lookup:si:ihC -- look up by string name,
NACL_MANIFEST_LOOKUP?

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:118: out_args[1]->u.hval =
(struct NaClDesc*) NaClDescInvalidMake();
is this a dummy place holder for now?

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:125: // unref:C:i -- dereferences
the file by object proxy handle.  The
NACL_MANIFEST_UNREF?

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:263: { NACL_MANIFEST_LIST,
ManifestListRpc, },
why do these require constants and not others?

[polina, 2011-06-15 00:51:05]

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:118: out_args[1]->u.hval =
(struct NaClDesc*) NaClDescInvalidMake();
On 2011/06/15 00:40:29, polina wrote:
> is this a dummy place holder for now?

It would be helpful to note this here, not just the CL description.

[bsy, 2011-06-15 20:03:35]

ptal

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:49: out_args[0]->u.bval = 1;
weird.  reverted.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:89: // NACL_NAME_SERVICE_LIST
list::C -- enumerate all names in the manifest
On 2011/06/15 00:40:29, polina wrote:
> NACL_MANIFEST_LIST?

Done.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:95: char* dest =
out_args[0]->arrays.carr;
On 2011/06/15 00:40:29, polina wrote:
> It would be more readable if you used out_args directly below: my first
reaction
> was that you were reading from out args instead of in args by mistake.

Done.  Also eliminated nbytes.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:106: // NACL_NAME_SERVICE_LOOKUP
lookup:si:ihC -- look up by string name,
On 2011/06/15 00:40:29, polina wrote:
> NACL_MANIFEST_LOOKUP?

Done.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:118: out_args[1]->u.hval =
(struct NaClDesc*) NaClDescInvalidMake();
On 2011/06/15 00:40:29, polina wrote:
> is this a dummy place holder for now?

yes

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:118: out_args[1]->u.hval =
(struct NaClDesc*) NaClDescInvalidMake();
On 2011/06/15 00:51:05, polina wrote:
> On 2011/06/15 00:40:29, polina wrote:
> > is this a dummy place holder for now?
> 
> It would be helpful to note this here, not just the CL description.

Done.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:125: // unref:C:i -- dereferences
the file by object proxy handle.  The
On 2011/06/15 00:40:29, polina wrote:
> NACL_MANIFEST_UNREF?

Done.

http://codereview.chromium.org/7108031/diff/20001/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:263: { NACL_MANIFEST_LIST,
ManifestListRpc, },
On 2011/06/15 00:40:29, polina wrote:
> why do these require constants and not others?

these are RPCs that are used by the manifest proxy code.  the others are used by
a different sub-module.  i'll make these into preproccessor symbols too and make
the corresponding changes in the relying code.

[polina, 2011-06-15 22:50:12]

LGTM

http://codereview.chromium.org/7108031/diff/26004/src/trusted/reverse_service...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/26004/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:117: // actually do lookups/URL
fetches.
Please capitalize and add a matching comment to the other placeholder(s).

[bsy, 2011-06-15 23:26:57]

thx

r5671

http://codereview.chromium.org/7108031/diff/26004/src/trusted/reverse_service...
File src/trusted/reverse_service/reverse_service.cc (right):

http://codereview.chromium.org/7108031/diff/26004/src/trusted/reverse_service...
src/trusted/reverse_service/reverse_service.cc:117: // actually do lookups/URL
fetches.
On 2011/06/15 22:50:12, polina wrote:
> Please capitalize and add a matching comment to the other placeholder(s).

Done.