Extend IRT with nacl_irt_resource_open interface

Extend IRT with nacl_irt_resource_open interface.

We need to solve the problem that we need to provide pointers to PPP_Initialize and PPP_GetInterface in the program before manifest file loading is working and so this program can be loaded.

In this CL, we split PpapiPluginMain in two parts so that ld.so can call the first part, load program, and then call the second part to provide pointers to these functions. This way is preferable since it make code in ld.so simplier.

New IRT interface provides a single function that returns a file descriptor by file name. The name service and manifest service channels are initialized at first use of this function. If an error happens during initialization, the function returns EIO.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2127
TEST= run_irt_manifest_file_chrome_browser_test
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=6453

[halyavin, 2011-08-10 15:20:27]

Roland, please take a look.

This is a preferable way for loading shared libraries. If this works for you,
please disregard another CL with a different way of loading shared libraries.

[khim, 2011-08-10 20:07:34]

Just got an idea: we don't really need ppapi_pre_start.

We can call PpapiPluginPreInit in IRT before we'll call main.nexe (or
runnable-ld.so in case of DSO). Then runnable-ld.so will be able to use
open_file_in_manifest from the beginning and older binaries will not care.

This will minimize changes to IRT, ld.so and glibc at the price of very slight
incompatibility: .nexe will not actually be able to do it's initialization
before SRPC channel opening. I doubt many programs will care (I expect some srpc
tests to be confused, but real programs should know nothing about srpc).

[Roland McGrath, 2011-08-10 20:15:44]

Sorry, I don't understand the rationale for this organization at all.

The intent of Bennet's changes (cf
http://code.google.com/p/nativeclient/issues/detail?id=2127) is that you
should be able to use the manifest-loading interfaces just fine wholly
independent of PPAPI stuff, completely before its initialization.

So what's the problem with just adding the manifest-open stuff alone?
Why do you need to touch the PPAPI initialization sequence at all?


Thanks,
Roland

[khim, 2011-08-10 20:22:50]

Manifest-loading interfaces don't need PPAPI, but they still need SRPC.
Currently SRPC is initialized in IRT when ppapi_start is called from .nexe's
main.

We must initialize it earlier or else Bennet's stuff can not be used by
runnable-ld.so to load main.nexe and libraries.

This CL splits ppapi_start in two: ppapi_pre_start initializes SRPC (you can
load files after that) and ppapi_start initializes PPAPI (it also initializes
SRPC if ppapi_pre_start was never called).

[pasko-google - do not use, 2011-08-10 21:15:31]

On Thu, Aug 11, 2011 at 12:07 AM,  <khim@chromium.org> wrote:
> Just got an idea: we don't really need ppapi_pre_start.
>
> We can call PpapiPluginPreInit in IRT before we'll call main.nexe (or
> runnable-ld.so in case of DSO). Then runnable-ld.so will be able to use
> open_file_in_manifest from the beginning and older binaries will not care.

+1, let's try that first and then go further with review

-- 
Egor Pasko

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

[pasko-google - do not use, 2011-08-10 21:16:33]

there are many aspects that need fixing for readability, but before jumping in
with them I would like to be sure the general direction is agreed on.

hope the naming suggestion below clarifies our intentions a little

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_ppapi.c
File src/untrusted/irt/irt_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_ppapi....
src/untrusted/irt/irt_ppapi.c:14: static void irt_ppapi_pre_start() {
blah_pre_start is not a self descriptive name, I would rather name it like:
* irt_ppapi_manifest_file_open_init
* irt_ppapi_communication_channel_init

[Use chromium.org instead, 2011-08-10 21:38:44]

We should discuss this in more detail with David and/or Bennet in the loop.

The directions you are going that involve changing the public ABI for
PPAPI initialization are not what we expected and I don't think they
make sense.
That said, I don't know the SRPC layers particularly well and I might
be overlooking real issues.  But what we expect is that you can do
everything you need in the IRT code without affecting the public ABI
beyond the addition of a simple interface for opening a named code
resource.

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

[sehr (please use chromium), 2011-08-10 22:24:53]

Adding bsy explicitly.  I haven't had a chance to look at the change yet.

David

On Wed, Aug 10, 2011 at 2:38 PM, Roland McGrath <mcgrathr@google.com> wrote:

> We should discuss this in more detail with David and/or Bennet in the loop.
>
> The directions you are going that involve changing the public ABI for
> PPAPI initialization are not what we expected and I don't think they
> make sense.
> That said, I don't know the SRPC layers particularly well and I might
> be overlooking real issues.  But what we expect is that you can do
> everything you need in the IRT code without affecting the public ABI
> beyond the addition of a simple interface for opening a named code
> resource.
>
> --
> 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.
>
>

-- 
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-08-11 02:32:28]

am confused.  why can't +khim's suggestion of initializing srpc and opening
the srpc channels before calling main work?  -bsy

On Wed, Aug 10, 2011 at 3:24 PM, David Sehr <sehr@google.com> wrote:

> Adding bsy explicitly.  I haven't had a chance to look at the change yet.
>
> David
>
>
> On Wed, Aug 10, 2011 at 2:38 PM, Roland McGrath <mcgrathr@google.com>wrote:
>
>> We should discuss this in more detail with David and/or Bennet in the
>> loop.
>>
>> The directions you are going that involve changing the public ABI for
>> PPAPI initialization are not what we expected and I don't think they
>> make sense.
>> That said, I don't know the SRPC layers particularly well and I might
>> be overlooking real issues.  But what we expect is that you can do
>> everything you need in the IRT code without affecting the public ABI
>> beyond the addition of a simple interface for opening a named code
>> resource.
>>
>> --
>> 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.
>>
>>
>


-- 
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-08-11 02:32:43]

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_manife...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_manife...
src/untrusted/irt/irt_manifest.c:72: NaClSrpcDtor(&manifest_channel);
the irt should be able to cache these channels for its own use.  there's no
sense in doing the setup / tear down over and over on a per-file basis.

http://codereview.chromium.org/7605029/diff/4010/tests/manifest_file/irt_mani...
File tests/manifest_file/irt_manifest_file_test.cc (right):

http://codereview.chromium.org/7605029/diff/4010/tests/manifest_file/irt_mani...
tests/manifest_file/irt_manifest_file_test.cc:140: if (strncmp(message, kHello,
strlen(kHello)) == 0) {
why use strncmp instead of strcmp?  did you intend to permit "hello world" as
well as "hello" by comparing only the first 5 characters?

[halyavin, 2011-08-11 15:13:06]

I implemented solution with initialization in irt_entry. It passes the bots now.

[halyavin, 2011-08-12 08:52:07]

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_manife...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_manife...
src/untrusted/irt/irt_manifest.c:72: NaClSrpcDtor(&manifest_channel);
On 2011/08/11 02:32:43, bsy wrote:
> the irt should be able to cache these channels for its own use.  there's no
> sense in doing the setup / tear down over and over on a per-file basis.

Done.

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_ppapi.c
File src/untrusted/irt/irt_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/4010/src/untrusted/irt/irt_ppapi....
src/untrusted/irt/irt_ppapi.c:14: static void irt_ppapi_pre_start() {
On 2011/08/10 21:16:33, pasko wrote:
> blah_pre_start is not a self descriptive name, I would rather name it like:
> * irt_ppapi_manifest_file_open_init
> * irt_ppapi_communication_channel_init

This code is rewritten.

http://codereview.chromium.org/7605029/diff/4010/tests/manifest_file/irt_mani...
File tests/manifest_file/irt_manifest_file_test.cc (right):

http://codereview.chromium.org/7605029/diff/4010/tests/manifest_file/irt_mani...
tests/manifest_file/irt_manifest_file_test.cc:140: if (strncmp(message, kHello,
strlen(kHello)) == 0) {
On 2011/08/11 02:32:43, bsy wrote:
> why use strncmp instead of strcmp?  did you intend to permit "hello world" as
> well as "hello" by comparing only the first 5 characters?

I rewrote test to use C++ API.

[Roland McGrath, 2011-08-12 16:44:56]

http://codereview.chromium.org/7605029/diff/30004/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_globals.cc (right):

http://codereview.chromium.org/7605029/diff/30004/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_globals.cc:167:
PpapiPluginRegisterDefaultThreadCreator();
This and MarkMainThread are only needed for PPAPI.
They are not needed to let manifest access work.
They don't belong here.

This function is misnamed.  It has nothing to do with PPAPI, it's just
getting the srpc library initialized.

IMHO the simplest thing to do would be to modify NaClSrpcModuleInit so that
it can be called repeatedly and does its work only once.  Then simply call
this in the places that rely on the library.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:146: #define NACL_IRT_MANIFEST_OPEN_v0_0
"nacl-irt-manifest-0.0"
This is not the best name for this interface.  It should be something more like
"open code resource".  In the long run, this interface will not always be
implemented in a way that relates to a manifest file.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:148: int (*open_file_in_manifest)(const char* file);
This is C, not C++, and uses normal C style: 'const char *file'.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_entry.c
File src/untrusted/irt/irt_entry.c (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry.c:73: if (PpapiPluginPreInit()) {
Don't do this here.  It could be in a constructor (i.e. a function someplace
like irt_ppapi.c with __attribute__((constructor))).  But better would be to
have this done only as needed, i.e. on the first call to the manifest loading
stuff.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:29: struct NaClSrpcChannel
*get_nameservice_channel() {
C functions with no arguments have a prototype of (void).

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:34: do {
Using do {...} while (0) this way is no better than goto.
If you want it to be cleaner than goto, then put the work inside a subfunction
that's called with the lock held.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:63: * Returns file descriptor or -1 if some
error happened.
The norm in IRT interfaces is to return an errno code or 0.
The result goes in a result parameter.  (See e.g. nacl_irt_filename.open.)

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:76: if (NACL_SRPC_RESULT_OK !=
NaClSrpcInvokeBySignature(
Why repeat this work in every call?  Shouldn't we cache the manifest connection
descriptor?

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:92: if (!NaClSrpcClientCtor(&manifest_channel,
manifest_conn)) {
We can also cache the manifest channel struct, no?

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:100: print_error("Manifest lookup RPC
failed\n");
This error case should not print.  The others are internal setup so printing
makes some amount of sense.  Here the failure is likely to be just that the user
supplied a bad file name.  Just return an appropriate error code.

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
File tests/manifest_file/irt_manifest_file_test.cc (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
tests/manifest_file/irt_manifest_file_test.cc:152: TYPE_nacl_irt_query
query_func = grok_auxv(find_auxv());
Don't do all the auxv grovelling here.  You can just use __nacl_irt_query. 
Better yet, we should add a public interface for doing the query, that both
newlib/libnacl and glibc implement.

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/nacl.scons
File tests/manifest_file/nacl.scons (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/nacl.sc...
tests/manifest_file/nacl.scons:120: 'irt_manifest_file_test.cc')
No need for this.  Just list the source file name in ComponentProgram.

[pasko-google - do not use, 2011-08-15 10:35:08]

please, also fix the commit description.  It should say that we enable a new
interface in IRT.  Just like that:
"Extend IRT with nacl_irt_resource_open() interface".

Also mention in the description about lazy initialization of the channel and
other big items about the implementation.

People will read this description many times while looking through history.

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
File tests/manifest_file/irt_manifest_file_test.cc (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
tests/manifest_file/irt_manifest_file_test.cc:87: // Note that CR LF is the
default end-of-line style for Windows.
This is too complex and tied to our build-try-commit quirks on windows.  It
should be simpler to just build a special nexe for loading (but not for running)
with a magic symbol exported, the test would just verify that this magic symbol
exists by reading through the file.

or just add a comment in the manifest, put a magic there compare only the first
line, that would be enough of a test

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
File tests/manifest_file/irt_manifest_file_test.html (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
tests/manifest_file/irt_manifest_file_test.html:17: <h1>Native Client Post
Message Manifest File Test</h1>
there is already a test with the same title, your test should have a different
title.  Please, describe what you are testing.

[halyavin, 2011-08-15 10:35:52]

http://codereview.chromium.org/7605029/diff/30004/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_globals.cc (right):

http://codereview.chromium.org/7605029/diff/30004/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_globals.cc:167:
PpapiPluginRegisterDefaultThreadCreator();
On 2011/08/12 16:44:56, Roland McGrath wrote:
> This and MarkMainThread are only needed for PPAPI.
> They are not needed to let manifest access work.
> They don't belong here.
> 
> This function is misnamed.  It has nothing to do with PPAPI, it's just
> getting the srpc library initialized.
> 
> IMHO the simplest thing to do would be to modify NaClSrpcModuleInit so that
> it can be called repeatedly and does its work only once.  Then simply call
> this in the places that rely on the library.

I think that we need logs working in ld.so and main. And so we need separate
function.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:146: #define NACL_IRT_MANIFEST_OPEN_v0_0
"nacl-irt-manifest-0.0"
On 2011/08/12 16:44:56, Roland McGrath wrote:
> This is not the best name for this interface.  It should be something more
like
> "open code resource".  In the long run, this interface will not always be
> implemented in a way that relates to a manifest file.

Done.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:148: int (*open_file_in_manifest)(const char* file);
On 2011/08/12 16:44:56, Roland McGrath wrote:
> This is C, not C++, and uses normal C style: 'const char *file'.

Done.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_entry.c
File src/untrusted/irt/irt_entry.c (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry.c:73: if (PpapiPluginPreInit()) {
On 2011/08/12 16:44:56, Roland McGrath wrote:
> Don't do this here.  It could be in a constructor (i.e. a function someplace
> like irt_ppapi.c with __attribute__((constructor))).  But better would be to
> have this done only as needed, i.e. on the first call to the manifest loading
> stuff.

I implemented this as a constructor.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:29: struct NaClSrpcChannel
*get_nameservice_channel() {
On 2011/08/12 16:44:56, Roland McGrath wrote:
> C functions with no arguments have a prototype of (void).

Done.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:34: do {
On 2011/08/12 16:44:56, Roland McGrath wrote:
> Using do {...} while (0) this way is no better than goto.
> If you want it to be cleaner than goto, then put the work inside a subfunction
> that's called with the lock held.

I extracted subfunction here.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:76: if (NACL_SRPC_RESULT_OK !=
NaClSrpcInvokeBySignature(
On 2011/08/12 16:44:56, Roland McGrath wrote:
> Why repeat this work in every call?  Shouldn't we cache the manifest
connection
> descriptor?

Done.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:92: if (!NaClSrpcClientCtor(&manifest_channel,
manifest_conn)) {
On 2011/08/12 16:44:56, Roland McGrath wrote:
> We can also cache the manifest channel struct, no?

Done.

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:100: print_error("Manifest lookup RPC
failed\n");
On 2011/08/12 16:44:56, Roland McGrath wrote:
> This error case should not print.  The others are internal setup so printing
> makes some amount of sense.  Here the failure is likely to be just that the
user
> supplied a bad file name.  Just return an appropriate error code.

Done.

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
File tests/manifest_file/irt_manifest_file_test.cc (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
tests/manifest_file/irt_manifest_file_test.cc:152: TYPE_nacl_irt_query
query_func = grok_auxv(find_auxv());
On 2011/08/12 16:44:56, Roland McGrath wrote:
> Don't do all the auxv grovelling here.  You can just use __nacl_irt_query. 
> Better yet, we should add a public interface for doing the query, that both
> newlib/libnacl and glibc implement.

Done.

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/nacl.scons
File tests/manifest_file/nacl.scons (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/nacl.sc...
tests/manifest_file/nacl.scons:120: 'irt_manifest_file_test.cc')
On 2011/08/12 16:44:56, Roland McGrath wrote:
> No need for this.  Just list the source file name in ComponentProgram.

Should I simplify all 3 tests above too?

[halyavin, 2011-08-15 10:46:03]

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/30004/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:63: * Returns file descriptor or -1 if some
error happened.
On 2011/08/12 16:44:56, Roland McGrath wrote:
> The norm in IRT interfaces is to return an errno code or 0.
> The result goes in a result parameter.  (See e.g. nacl_irt_filename.open.)

Done.

[halyavin, 2011-08-15 14:04:22]

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
File tests/manifest_file/irt_manifest_file_test.html (right):

http://codereview.chromium.org/7605029/diff/30004/tests/manifest_file/irt_man...
tests/manifest_file/irt_manifest_file_test.html:17: <h1>Native Client Post
Message Manifest File Test</h1>
On 2011/08/15 10:35:09, pasko wrote:
> there is already a test with the same title, your test should have a different
> title.  Please, describe what you are testing.

Done.

[pasko-google - do not use, 2011-08-15 16:23:03]

please, remove the link to the "1st way" from the description

http://codereview.chromium.org/7605029/diff/34006/src/shared/ppapi_proxy/ppru...
File src/shared/ppapi_proxy/ppruntime.h (right):

http://codereview.chromium.org/7605029/diff/34006/src/shared/ppapi_proxy/ppru...
src/shared/ppapi_proxy/ppruntime.h:16: int IRTInit();
google style guide suggests a different naming convention:
IrtInit().

The name is not quite self-descriptive, but I cannot suggest anything better. 
Roland?

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:146: #define NACL_IRT_RESOURCE_OPEN_v0_0
"nacl-irt-resource-open-0.0"
v0_1 seems more logical than v0_0: the initial version of all other interfaces
are v0_1

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_entry...
File src/untrusted/irt/irt_entry_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry_ppapi.c:9: __attribute__((constructor)) void
CallIRTInit() {
you could write a clarifying comment saying that it is possible to initialize
the manifest channel lazily, but it would require constructing a mutex

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:1: #include <errno.h>
system headers first, then nacl system headers, after an empty line, then nacl
headers

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:18: * usefull for other APIs.
s/usefull/useful/

frankly, I do not see the reason for TODO here, it's so obvious

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:30: int get_nameservice_channel_locked(struct
NaClSrpcChannel **result) {
should be static

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:42: return -EIO;
the convention is to return the *same* value as errno, so here and in other
places it should be return EIO

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:64: int get_nameservice_channel(struct
NaClSrpcChannel **result) {
should be static

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:85: int get_manifest_channel_locked(struct
NaClSrpcChannel **result) {
should be static

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:133: * Returns file descriptor or negative
error code.
again, according to the common IRT convention, error code should return a
positive value that equals errno

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:135: int irt_open_resource(const char *file,
int *fd) {
should be static

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:142: *fd = 0;
touching a value when an error occurred is awkward, please, leave it unchanged.

[Roland McGrath, 2011-08-15 16:30:49]

On 2011/08/15 10:35:52, halyavin wrote:
> > IMHO the simplest thing to do would be to modify NaClSrpcModuleInit so that
> > it can be called repeatedly and does its work only once.  Then simply call
> > this in the places that rely on the library.
> 
> I think that we need logs working in ld.so and main. And so we need separate
> function.

Ok, a function that calls NaClLogModuleInit and NaClSrpcModuleInit, and
that can safely be called multiple times, is fine.

> I implemented this as a constructor.

Doing it lazily on demand seems preferable.  Why not do that?

> On 2011/08/12 16:44:56, Roland McGrath wrote:
> > No need for this.  Just list the source file name in ComponentProgram.
> 
> Should I simplify all 3 tests above too?

It really doesn't hurt, but there is no reason to conflate unrelated
cleanup into the same commit.

[pasko-google - do not use, 2011-08-15 16:59:42]

On 2011/08/15 16:30:49, Roland McGrath wrote:
> > I implemented this as a constructor.
> 
> Doing it lazily on demand seems preferable.  Why not do that?

the argument that I heard sounded as: for lazy initialization we need a mutex,
which would have to be initialized in a constructor, so we have a constructor
anyway, so just do more work in it directly

[Use chromium.org instead, 2011-08-15 19:42:56]

On Mon, Aug 15, 2011 at 9:59 AM,  <pasko@google.com> wrote:
> On 2011/08/15 16:30:49, Roland McGrath wrote:
>>
>> > I implemented this as a constructor.
>
>> Doing it lazily on demand seems preferable.  Why not do that?
>
> the argument that I heard sounded as: for lazy initialization we need a
> mutex, which would have to be initialized in a constructor, so we have a
> constructor anyway, so just do more work in it directly

That doesn't really wash.  You can just use a pthread_mutex_t or
pthread_once_t with a simple static initializer and have no constructor,
i.e. no startup cost when not used.  Moreover, the work like setting up a
nameservice connection is dramatically more costly than simple mutex
initialization.

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

[halyavin, 2011-08-16 08:50:52]

> Doing it lazily on demand seems preferable.  Why not do that?
Lazy initialization is used if
1. It may not be needed during whole application lifecycle.
2. It allows to show something to the user faster.
3. Eager initialization is not possible.
None of this is applied to IrtInit. IrtInit is called in every NaCl application
in the browser. And you can't show anything to the user before IrtInit is
called. At last, eager initialization is possible and its code is simplier than
code for lazy initialization.

So why make code more complex without any benefits?

[halyavin, 2011-08-16 09:05:36]

http://codereview.chromium.org/7605029/diff/34006/src/shared/ppapi_proxy/ppru...
File src/shared/ppapi_proxy/ppruntime.h (right):

http://codereview.chromium.org/7605029/diff/34006/src/shared/ppapi_proxy/ppru...
src/shared/ppapi_proxy/ppruntime.h:16: int IRTInit();
On 2011/08/15 16:23:04, pasko wrote:
> google style guide suggests a different naming convention:
> IrtInit().
> 
> The name is not quite self-descriptive, but I cannot suggest anything better. 
> Roland?

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:146: #define NACL_IRT_RESOURCE_OPEN_v0_0
"nacl-irt-resource-open-0.0"
On 2011/08/15 16:23:04, pasko wrote:
> v0_1 seems more logical than v0_0: the initial version of all other interfaces
> are v0_1

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_entry...
File src/untrusted/irt/irt_entry_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry_ppapi.c:9: __attribute__((constructor)) void
CallIRTInit() {
On 2011/08/15 16:23:04, pasko wrote:
> you could write a clarifying comment saying that it is possible to initialize
> the manifest channel lazily, but it would require constructing a mutex

I don't think that we need to comment all roads not taken in the comments. I
added more reasons for eager initialization in the answer to Roland.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:1: #include <errno.h>
On 2011/08/15 16:23:04, pasko wrote:
> system headers first, then nacl system headers, after an empty line, then nacl
> headers

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:18: * usefull for other APIs.
On 2011/08/15 16:23:04, pasko wrote:
> s/usefull/useful/
> 
> frankly, I do not see the reason for TODO here, it's so obvious

I want to remember to do this.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:30: int get_nameservice_channel_locked(struct
NaClSrpcChannel **result) {
On 2011/08/15 16:23:04, pasko wrote:
> should be static

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:42: return -EIO;
On 2011/08/15 16:23:04, pasko wrote:
> the convention is to return the *same* value as errno, so here and in other
> places it should be return EIO

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:64: int get_nameservice_channel(struct
NaClSrpcChannel **result) {
On 2011/08/15 16:23:04, pasko wrote:
> should be static
This function may be extracted to the separate file in the future and so I think
it shouldn't be static.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:85: int get_manifest_channel_locked(struct
NaClSrpcChannel **result) {
On 2011/08/15 16:23:04, pasko wrote:
> should be static

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:133: * Returns file descriptor or negative
error code.
On 2011/08/15 16:23:04, pasko wrote:
> again, according to the common IRT convention, error code should return a
> positive value that equals errno

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:135: int irt_open_resource(const char *file,
int *fd) {
On 2011/08/15 16:23:04, pasko wrote:
> should be static

Done.

http://codereview.chromium.org/7605029/diff/34006/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:142: *fd = 0;
On 2011/08/15 16:23:04, pasko wrote:
> touching a value when an error occurred is awkward, please, leave it
unchanged.

Done.

[Roland McGrath, 2011-08-16 16:25:38]

On 2011/08/16 08:50:52, halyavin wrote:
> > Doing it lazily on demand seems preferable.  Why not do that?
> Lazy initialization is used if
> 1. It may not be needed during whole application lifecycle.
> 2. It allows to show something to the user faster.
> 3. Eager initialization is not possible.
> None of this is applied to IrtInit. IrtInit is called in every NaCl
application
> in the browser. And you can't show anything to the user before IrtInit is
> called. At last, eager initialization is possible and its code is simplier
than
> code for lazy initialization.
> 
> So why make code more complex without any benefits?

Sorry, I don't buy this logic at all.  It's frankly less complex, not more.

[Roland McGrath, 2011-08-16 17:32:50]

Most are nits, but the locking issue needs to be resolved before commit.

http://codereview.chromium.org/7605029/diff/41005/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_globals.cc (right):

http://codereview.chromium.org/7605029/diff/41005/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_globals.cc:159: // in irt_entry.c where
PpapiPluginPreInit is called.
This comment refers to a function that does not exist.

Anyway, it's cleaner to move this inside IrtInit itself.

http://codereview.chromium.org/7605029/diff/41005/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_globals.cc:172: if (!initialized) {
This is not thread-safe, which could be a problem in some potential usage
patterns.  It's fine to leave that for the moment, but it needs a TODO comment.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:148: int (*open_resource)(const char* file, int *fd);
'const char *file'.  We don't use C++ style in C.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_entry...
File src/untrusted/irt/irt_entry_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry_ppapi.c:9: static __attribute__((constructor)) void
CallIrtInit() {
Use (void) for a C function of no arguments.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:27: static __attribute__((constructor)) void
init_ns_mutex(void) {
Just use a pthread_mutex_t here, and a simple C static initializer suffices.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:82: static __attribute__((constructor)) void
init_manifest_mutex(void) {
Use pthread_mutex_t here too.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:90: struct NaClSrpcChannel* ns_channel;
Use C style: 'struct NaClSrpcChannel *ns_channel;'

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:149: if (NACL_SRPC_RESULT_OK !=
NaClSrpcInvokeBySignature(
This call cannot be used on the same NaClSrpcChannel object without locking.  If
we're going to cache and share the channel object, access to it must be locked. 
Alternatively, we could use a per-thread cache in a __thread variable, which
would impose less serialization in the general case.

http://codereview.chromium.org/7605029/diff/44017/src/untrusted/irt/irt_entry...
File src/untrusted/irt/irt_entry_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/44017/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry_ppapi.c:9: static __attribute__((constructor)) void
CallIrtInit() {
Just move this into irt_manifest.c and remove this file.

[halyavin, 2011-08-16 18:06:25]

http://codereview.chromium.org/7605029/diff/41005/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_globals.cc (right):

http://codereview.chromium.org/7605029/diff/41005/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_globals.cc:159: // in irt_entry.c where
PpapiPluginPreInit is called.
On 2011/08/16 17:32:50, Roland McGrath wrote:
> This comment refers to a function that does not exist.
> 
> Anyway, it's cleaner to move this inside IrtInit itself.

Done.

http://codereview.chromium.org/7605029/diff/41005/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_globals.cc:172: if (!initialized) {
This will be changed, if we implement lazy initialization.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt.h#new...
src/untrusted/irt/irt.h:148: int (*open_resource)(const char* file, int *fd);
On 2011/08/16 17:32:50, Roland McGrath wrote:
> 'const char *file'.  We don't use C++ style in C.

Done.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_entry...
File src/untrusted/irt/irt_entry_ppapi.c (right):

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_entry...
src/untrusted/irt/irt_entry_ppapi.c:9: static __attribute__((constructor)) void
CallIrtInit() {
On 2011/08/16 17:32:50, Roland McGrath wrote:
> Use (void) for a C function of no arguments.

Done.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
File src/untrusted/irt/irt_manifest.c (right):

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:27: static __attribute__((constructor)) void
init_ns_mutex(void) {
On 2011/08/16 17:32:50, Roland McGrath wrote:
> Just use a pthread_mutex_t here, and a simple C static initializer suffices.

Done.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:82: static __attribute__((constructor)) void
init_manifest_mutex(void) {
On 2011/08/16 17:32:50, Roland McGrath wrote:
> Use pthread_mutex_t here too.

Done.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:90: struct NaClSrpcChannel* ns_channel;
On 2011/08/16 17:32:50, Roland McGrath wrote:
> Use C style: 'struct NaClSrpcChannel *ns_channel;'

Done.

http://codereview.chromium.org/7605029/diff/41005/src/untrusted/irt/irt_manif...
src/untrusted/irt/irt_manifest.c:149: if (NACL_SRPC_RESULT_OK !=
NaClSrpcInvokeBySignature(
On 2011/08/16 17:32:50, Roland McGrath wrote:
> This call cannot be used on the same NaClSrpcChannel object without locking. 
If
> we're going to cache and share the channel object, access to it must be
locked. 
> Alternatively, we could use a per-thread cache in a __thread variable, which
> would impose less serialization in the general case.

Done.

[Roland McGrath, 2011-08-16 18:12:53]

We definitely want more cleanup of this later.
But for the moment, LGTM if all tests pass.

I think this breaks the browser_dynamic_library test.  That test should go
away now that we have cleaner ways of doing this stuff.  But if it's still
enabled now, it should be disabled or removed in this commit to avoid
breaking the bots.