Change the pnacl shim from doing real shimming to just intercepting

Change the pnacl shim from doing real shimming to just intercepting
queries for  "nacl-irt-ppapihook-XXX" and rewriting them to
"nacl-irt-ppapihook-shimmed-XXX" 

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2465 
TEST= trybots

[Robert Muth (chromium), 2012-08-27 19:54:02]

PTAL

This depends on http://codereview.chromium.org/10826171/
It is not fully tested yet but I want to make sure
everybody is ok with the direction this is going

[jvoung (off chromium), 2012-08-27 21:57:39]

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
File src/untrusted/pnacl_irt_shim/nacl.scons (right):

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/nacl.scons:15: env.ClearBits('pnacl_generate_pexe')
Leave the comment about clearing the bit pnacl_generate_pexe, or improve the
comment?

Actually, was it just because it was built by nacl-gcc before?  If so, you can
remove the ClearBits.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/nacl.scons:21: # However, the library is part of
the pnacl sdk (use original env).
This "however" was referring to swapping of toolchains from pnacl -> nacl-gcc. 
Since that is no longer the case, you don't need that "however".

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
File src/untrusted/pnacl_irt_shim/shim_entry.c (right):

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:2: * Copyright 2011 The Native Client
Authors. All rights reserved.
Is the new copyright header not supposed to have a (c), or was that an accident?

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:22: char prefix_search[] =
"nacl-irt-ppapihook";
static const ?

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:25: int my_strcmp(const char* s1,
const char* s2) {
Comments about why we provide a custom strcmp, strcpy.

Make them "static" ?

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:33: void my_strpy(char* s1, const
char* s2) {
my_strcpy

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:34: while( *s2 != '\0') {
"strcpy"

extra space in front of *s2.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:50: /* but  not if it is already 
"nacl-irt-ppapihook-shimmed-XXX" */
extra space "but  not"

[Robert Muth (chromium), 2012-08-27 22:38:18]

+ncbray, +bradnelson

PTAL

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
File src/untrusted/pnacl_irt_shim/nacl.scons (right):

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/nacl.scons:15: env.ClearBits('pnacl_generate_pexe')
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> Leave the comment about clearing the bit pnacl_generate_pexe, or improve the
> comment?
> 
> Actually, was it just because it was built by nacl-gcc before?  If so, you can
> remove the ClearBits.

Done.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/nacl.scons:21: # However, the library is part of
the pnacl sdk (use original env).
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> This "however" was referring to swapping of toolchains from pnacl -> nacl-gcc.

> Since that is no longer the case, you don't need that "however".

Done.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
File src/untrusted/pnacl_irt_shim/shim_entry.c (right):

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:2: * Copyright 2011 The Native Client
Authors. All rights reserved.
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> Is the new copyright header not supposed to have a (c), or was that an
accident?

yes, "(c)" is meaningless I was told

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:22: char prefix_search[] =
"nacl-irt-ppapihook";
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> static const ?

Done.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:25: int my_strcmp(const char* s1,
const char* s2) {
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> Comments about why we provide a custom strcmp, strcpy.
> 
> Make them "static" ?

Done.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:33: void my_strpy(char* s1, const
char* s2) {
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> my_strcpy

Done.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:34: while( *s2 != '\0') {
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> "strcpy"
> 
> extra space in front of *s2.

Done.

http://codereview.chromium.org/10870109/diff/5001/src/untrusted/pnacl_irt_shi...
src/untrusted/pnacl_irt_shim/shim_entry.c:50: /* but  not if it is already 
"nacl-irt-ppapihook-shimmed-XXX" */
On 2012/08/27 21:57:39, jvoung (chromium) wrote:
> extra space "but  not"

Done.

[Mark Seaborn, 2012-08-28 16:04:51]

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
File src/untrusted/pnacl_irt_shim/nacl.scons (right):

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/nacl.scons:2: # Copyright 2012 The Native Client
Authors. All rights reserved.
As in the other change, you don't need to remove the "(c)", so please don't. 
The presubmit scripts accept it either way.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
File src/untrusted/pnacl_irt_shim/shim_entry.c (right):

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:7: #include <stddef.h>
I don't think you actually need stddef.h below, do you?

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:25: /* do not make assumptions about
stcmp being available */
'stcmp' -> 'strcmp'.  Also capitalise the first letter of the sentence, please.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:26: static int my_strcmp(const char*
s1, const char* s2) {
This should use the " *" spacing style (as elsewhere in the file) and not "* ".

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:27: while( *s1 != '\0' && *s2 != '\0'
&& *s1 == *s2) {
"while( " -> "while ("

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:51: if (0 ==
my_strcmp(interface_ident, prefix_search)) {
When is this check going to return true, if interface_ident contains a full
interface name and prefix_search only contains an interface name prefix?  How
have you tested this code?

Anyway, checking for an interface name prefix is unnecessary complexity.  Please
just check for the full interface name, NACL_IRT_PPAPIHOOK_v0_1.  The likelihood
that we will need to change the NACL_IRT_PPAPIHOOK interface in the future is
very small.

[Robert Muth (chromium), 2012-08-28 19:11:23]

PTAL

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
File src/untrusted/pnacl_irt_shim/nacl.scons (right):

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/nacl.scons:2: # Copyright 2012 The Native Client
Authors. All rights reserved.
On 2012/08/28 16:04:51, Mark Seaborn wrote:
> As in the other change, you don't need to remove the "(c)", so please don't. 
> The presubmit scripts accept it either way.

Done.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
File src/untrusted/pnacl_irt_shim/shim_entry.c (right):

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:7: #include <stddef.h>
On 2012/08/28 16:04:51, Mark Seaborn wrote:
> I don't think you actually need stddef.h below, do you?


irt.h depends on it.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:25: /* do not make assumptions about
stcmp being available */
On 2012/08/28 16:04:51, Mark Seaborn wrote:
> 'stcmp' -> 'strcmp'.  Also capitalise the first letter of the sentence,
please.

Done.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:26: static int my_strcmp(const char*
s1, const char* s2) {
On 2012/08/28 16:04:51, Mark Seaborn wrote:
> This should use the " *" spacing style (as elsewhere in the file) and not "*
".

Done.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:27: while( *s1 != '\0' && *s2 != '\0'
&& *s1 == *s2) {
On 2012/08/28 16:04:51, Mark Seaborn wrote:
> "while( " -> "while ("

Done.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:51: if (0 ==
my_strcmp(interface_ident, prefix_search)) {
On 2012/08/28 16:04:51, Mark Seaborn wrote:
> When is this check going to return true, if interface_ident contains a full
> interface name and prefix_search only contains an interface name prefix?  How
> have you tested this code?

Thanks this was a bug.
As I mention in the initial email - this code has not been fully tested yet.


> 
> Anyway, checking for an interface name prefix is unnecessary complexity. 
Please
> just check for the full interface name, NACL_IRT_PPAPIHOOK_v0_1.  The
likelihood
> that we will need to change the NACL_IRT_PPAPIHOOK interface in the future is
> very small.

ok specialized this to the current interface.

[Mark Seaborn, 2012-08-28 19:38:41]

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
File src/untrusted/pnacl_irt_shim/shim_entry.c (right):

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:7: #include <stddef.h>
On 2012/08/28 19:11:24, Robert Muth (chromium) wrote:
> On 2012/08/28 16:04:51, Mark Seaborn wrote:
> > I don't think you actually need stddef.h below, do you?
> 
> irt.h depends on it.

But irt.h contains "#include <stddef.h>".  So I don't think you need this
#include.

https://chromiumcodereview.appspot.com/10870109/diff/8/src/untrusted/pnacl_ir...
src/untrusted/pnacl_irt_shim/shim_entry.c:51: if (0 ==
my_strcmp(interface_ident, prefix_search)) {
On 2012/08/28 19:11:24, Robert Muth (chromium) wrote:
> On 2012/08/28 16:04:51, Mark Seaborn wrote:
> > When is this check going to return true, if interface_ident contains a
> > full interface name and prefix_search only contains an interface name
> > prefix?  How have you tested this code?
> 
> Thanks this was a bug.
> As I mention in the initial email - this code has not been fully tested yet.

Why don't you combine this change into your IRT change,
http://codereview.chromium.org/10826171/?  It would make sense to introduce the
new IRT interface in the same change as you add code that uses it.

https://chromiumcodereview.appspot.com/10870109/diff/15001/src/untrusted/pnac...
File src/untrusted/pnacl_irt_shim/shim_entry.c (right):

https://chromiumcodereview.appspot.com/10870109/diff/15001/src/untrusted/pnac...
src/untrusted/pnacl_irt_shim/shim_entry.c:22: static const char prefix_search[]
= "nacl-irt-ppapihook";
This variable is not used.  Please remove it.  Same for 'prefix_replace'.

https://chromiumcodereview.appspot.com/10870109/diff/15001/src/untrusted/pnac...
src/untrusted/pnacl_irt_shim/shim_entry.c:37: return
real_irt_interface(NACL_IRT_PPAPIHOOK_SHIMMED_v0_1, table, tablesize);
This line is >80 chars

https://chromiumcodereview.appspot.com/10870109/diff/15001/src/untrusted/pnac...
src/untrusted/pnacl_irt_shim/shim_entry.c:39: return real_irt_interface(ident,
table, tablesize);
'ident' is not defined, so I don't think this compiles.  I'd prefer if you
compile-test your code before sending it out.