NaCl: Clean up file descriptor setup in nacl_helper on linux

NaCl: Clean up file descriptor setup in nacl_helper on linux

There is no need to be using dup2() to set up an FD with a fixed
number; this risks overwriting an FD.  The point of
base::GlobalDescriptors is that it provides a level of indirection
that allows any FD number to be used.

Remove kNaClBrowserDescriptor.  Remove the browserdesc argument that
is not used for anything other than an assertion and so isn't needed.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=2096
TEST=NaCl tests in browser_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176106

[Roland McGrath, 2013-01-09 19:02:10]

Also remove kNaClBrowserDescriptor.

[Mark Seaborn, 2013-01-09 19:09:59]

On 9 January 2013 11:02, <mcgrathr@chromium.org> wrote:

> Also remove kNaClBrowserDescriptor.
>

It's still used in chrome/app/nacl_fork_delegate_linux.cc.  I will remove
it from there in a separate change; that requires a separate OWNERS review
anyway.

Mark

[Mark Seaborn, 2013-01-09 19:09:59]

On 9 January 2013 11:02, <mcgrathr@chromium.org> wrote:

> Also remove kNaClBrowserDescriptor.
>

It's still used in chrome/app/nacl_fork_delegate_linux.cc.  I will remove
it from there in a separate change; that requires a separate OWNERS review
anyway.

Mark

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

[Roland McGrath, 2013-01-09 19:16:09]

On 2013/01/09 19:09:59, Mark Seaborn wrote:
> On 9 January 2013 11:02, <mailto:mcgrathr@chromium.org> wrote:
> 
> > Also remove kNaClBrowserDescriptor.
> >
> 
> It's still used in chrome/app/nacl_fork_delegate_linux.cc.  I will remove
> it from there in a separate change; that requires a separate OWNERS review
> anyway.

If you want that change to be separate then please do it first.
Without that change having been made and tested, I have no confidence that
letting the fd float doesn't break anything.

[Mark Seaborn, 2013-01-09 22:43:39]

On 9 January 2013 11:16, <mcgrathr@chromium.org> wrote:

> On 2013/01/09 19:09:59, Mark Seaborn wrote:
>
>  On 9 January 2013 11:02, <mailto:mcgrathr@chromium.org> wrote:
>>
>
>  > Also remove kNaClBrowserDescriptor.
>> >
>>
>
>  It's still used in chrome/app/nacl_fork_delegate_**linux.cc.  I will
>> remove
>> it from there in a separate change; that requires a separate OWNERS review
>> anyway.
>>
>
> If you want that change to be separate then please do it first.
> Without that change having been made and tested, I have no confidence that
> letting the fd float doesn't break anything.
>

OK, I've removed kNaClBrowserDescriptor.

Mark

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

[Mark Seaborn, 2013-01-09 22:43:39]

On 9 January 2013 11:16, <mcgrathr@chromium.org> wrote:

> On 2013/01/09 19:09:59, Mark Seaborn wrote:
>
>  On 9 January 2013 11:02, <mailto:mcgrathr@chromium.org> wrote:
>>
>
>  > Also remove kNaClBrowserDescriptor.
>> >
>>
>
>  It's still used in chrome/app/nacl_fork_delegate_**linux.cc.  I will
>> remove
>> it from there in a separate change; that requires a separate OWNERS review
>> anyway.
>>
>
> If you want that change to be separate then please do it first.
> Without that change having been made and tested, I have no confidence that
> letting the fd float doesn't break anything.
>

OK, I've removed kNaClBrowserDescriptor.

Mark

[Roland McGrath, 2013-01-09 22:57:35]

lgtm

[Mark Seaborn, 2013-01-09 23:51:43]

+jam for OWNERS review

[jam, 2013-01-10 17:37:50]

lgtm

https://codereview.chromium.org/11819021/diff/6002/chrome/common/nacl_helper_...
File chrome/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/11819021/diff/6002/chrome/common/nacl_helper_...
chrome/common/nacl_helper_linux.h:18: // For communications between NaCl loader
and browser.
nit: should this be removed now?

[Mark Seaborn, 2013-01-10 17:43:19]

https://codereview.chromium.org/11819021/diff/6002/chrome/common/nacl_helper_...
File chrome/common/nacl_helper_linux.h (right):

https://codereview.chromium.org/11819021/diff/6002/chrome/common/nacl_helper_...
chrome/common/nacl_helper_linux.h:18: // For communications between NaCl loader
and browser.
On 2013/01/10 17:37:50, John Abd-El-Malek wrote:
> nit: should this be removed now?

No, this comment still applies.  kNaClSandboxDescriptor is used for
communicating with the browser process.