Support FD passing for NaCl on posix

Support FD passing for NaCl on posix

We need this to make GetOSFileDescriptor work from NaCl module.

R=bradchen@chromium.org, bradnelson@chromium.org, bbudge@chromium.org
BUG=183015
TEST=locally tested with modified examples/file_io and https://codereview.chromium.org/13032002/. The 13032002 will also test this change as the test modified in 13032002 seems to run on NaCl

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

[hamaji, 2013-03-22 01:40:20]

As I've written in the CL description, I have no idea how to support windows and
how to test this change. Could you give me an advice?

As for Windows support, I didn't see how to convert a NaClHandle (HANDLE) to a
posix FD.

As for the test, it seems nacl_ipc_adapter_unittest.cc doesn't have test cases
for specific types (SHARED_MEMORY, SOCKET, etc.), so I'm guessing there is a
regression test which actually sends a real IPC or there is no test.

Suggestions are really appreciated. Thanks!

[Mark Seaborn, 2013-03-22 03:15:47]

On 2013/03/22 01:40:20, hamaji wrote:
> As I've written in the CL description, I have no idea how to support windows
and
> how to test this change. Could you give me an advice?
> 
> As for Windows support, I didn't see how to convert a NaClHandle (HANDLE) to a
> posix FD.

You can use _open_osfhandle() to create a Windows CRT file descriptor from a
Windows handle.  See NaClDescIoInternalize() in the NaCl tree for an example.

[Mark Seaborn, 2013-03-22 03:19:45]

https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:421:
reinterpret_cast<NaClHostDesc*>(malloc(sizeof(*nhdp)));
Please don't open-code the creation of a NaClHostDesc in the Chromium codebase. 
Can you create a helper function in the NaCl codebase for making a NaClDesc from
a FD/handle and use that here?  See ImportShmHandle() and
ImportSyncSocketHandle() for examples of how to do this and where to put it.

[Brad Chen, 2013-03-22 17:30:25]

https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:417: // TODO(raymes): Handle file handles for
NaCl on Windows.
What will be required to get this working on Windows? In general we avoid
features that can't be supported on all platforms.

[Brad Chen, 2013-03-22 17:31:56]

BBudge ought to have a better idea than me about the Windows question. He or
Mark can also help you with landing changes in the NaCl repo.

On 2013/03/22 17:30:25, Brad Chen wrote:
>
https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
> File chrome/nacl/nacl_ipc_adapter.cc (right):
> 
>
https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter....
> chrome/nacl/nacl_ipc_adapter.cc:417: // TODO(raymes): Handle file handles for
> NaCl on Windows.
> What will be required to get this working on Windows? In general we avoid
> features that can't be supported on all platforms.

[bbudge, 2013-03-22 17:50:05]

https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/13011002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:417: // TODO(raymes): Handle file handles for
NaCl on Windows.
You shouldn't have to do anything special. This switch statement provides a
bunch of examples of how to wrap handles for the NaCl app. We use the
NaClDescWrapper and associated factory classes to wrap native fds and handles.
There is no 'Import' method for file descriptors and you may have to write one.
There is a MakeFileDesc method and it might be all that's needed.

https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...

There is also a MakeFileDescWithQuota. I don't know if that's actually working
yet though.
On 2013/03/22 17:30:25, Brad Chen wrote:
> What will be required to get this working on Windows? In general we avoid
> features that can't be supported on all platforms.

[hamaji, 2013-03-25 06:01:12]

As we have https://codereview.chromium.org/12929033/ in native client code and
Mark did NaCl roll
(http://src.chromium.org/viewvc/chrome?view=rev&revision=190105) now, this CL
becomes very simple (almost identical to ppapi::proxy::SerializedHandle::SOCKET
case).

Could you take another look?

[bbudge, 2013-03-25 14:07:30]

lgtm

[Mark Seaborn, 2013-03-25 14:19:16]

LGTM.  Can you add a TEST= field to say how you tested this or say explicitly
that you didn't test this code path if that's the case?

https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adap...
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adap...
chrome/nacl/nacl_ipc_adapter.cc:414: reinterpret_cast<const
NaClHandle>(iter->descriptor())
No cast should be necessary here.  iter->descriptor() is already a HANDLE and
adding 'const' is unnecessary.

[hamaji, 2013-03-25 18:11:35]

On 2013/03/25 14:19:16, Mark Seaborn wrote:
> LGTM.  Can you add a TEST= field to say how you tested this or say explicitly
> that you didn't test this code path if that's the case?

I added a TEST= line. I tested this change with another change for PPAPI which
depends on FD passing. Please let me know if there is a nice way to write
automated unittest only for this change.

> 
>
https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adap...
> File chrome/nacl/nacl_ipc_adapter.cc (right):
> 
>
https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adap...
> chrome/nacl/nacl_ipc_adapter.cc:414: reinterpret_cast<const
> NaClHandle>(iter->descriptor())
> No cast should be necessary here.  iter->descriptor() is already a HANDLE and
> adding 'const' is unnecessary.

Done. I also modified case for SOCKET for consistency.

[Mark Seaborn, 2013-03-25 18:46:38]

LGTM

On 25 March 2013 11:11, <hamaji@chromium.org> wrote:

> On 2013/03/25 14:19:16, Mark Seaborn wrote:
>
>> LGTM.  Can you add a TEST= field to say how you tested this or say
>> explicitly
>> that you didn't test this code path if that's the case?
>>
>
> I added a TEST= line. I tested this change with another change for PPAPI
> which
> depends on FD passing.


Thanks, good to know. :-)


> Please let me know if there is a nice way to write
> automated unittest only for this change.
>

I don't think there is, unfortunately.


>
>
https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adap...
>
>> File chrome/nacl/nacl_ipc_adapter.**cc (right):
>>
>
>
> https://codereview.chromium.**org/13011002/diff/11001/**
>
chrome/nacl/nacl_ipc_adapter.**cc#newcode414<https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adapter.cc#newcode414>
>
>> chrome/nacl/nacl_ipc_adapter.**cc:414: reinterpret_cast<const
>> NaClHandle>(iter->descriptor()**)
>> No cast should be necessary here.  iter->descriptor() is already a HANDLE
>> and
>> adding 'const' is unnecessary.
>>
>
> Done. I also modified case for SOCKET for consistency.
>

You could also modify the SHARED_MEMORY case.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Mark Seaborn, 2013-03-25 18:46:39]

LGTM

On 25 March 2013 11:11, <hamaji@chromium.org> wrote:

> On 2013/03/25 14:19:16, Mark Seaborn wrote:
>
>> LGTM.  Can you add a TEST= field to say how you tested this or say
>> explicitly
>> that you didn't test this code path if that's the case?
>>
>
> I added a TEST= line. I tested this change with another change for PPAPI
> which
> depends on FD passing.


Thanks, good to know. :-)


> Please let me know if there is a nice way to write
> automated unittest only for this change.
>

I don't think there is, unfortunately.


>
>
https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adap...
>
>> File chrome/nacl/nacl_ipc_adapter.**cc (right):
>>
>
>
> https://codereview.chromium.**org/13011002/diff/11001/**
>
chrome/nacl/nacl_ipc_adapter.**cc#newcode414<https://codereview.chromium.org/13011002/diff/11001/chrome/nacl/nacl_ipc_adapter.cc#newcode414>
>
>> chrome/nacl/nacl_ipc_adapter.**cc:414: reinterpret_cast<const
>> NaClHandle>(iter->descriptor()**)
>> No cast should be necessary here.  iter->descriptor() is already a HANDLE
>> and
>> adding 'const' is unnecessary.
>>
>
> Done. I also modified case for SOCKET for consistency.
>

You could also modify the SHARED_MEMORY case.

Mark

[commit-bot: I haz the power, 2013-03-25 21:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/13011002/26001

[commit-bot: I haz the power, 2013-03-25 21:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/13011002/26001

[hamaji, 2013-03-26 04:11:43]

Committed patchset #5 manually as r190552 (presubmit successful).