Add NaClDescIoDescMakeFromHandle

Add NaClDescIoDescMakeFromHandle

This interface is more similar to other NaClDesc*Make interfaces
such as NaClDescSyncSocketMake than NaClDescIoDescMake.

This change blocks https://codereview.chromium.org/13011002/
I'm planning to use this from chrome/nacl/nacl_ipc_adapter.cc.

R=bradchen@chromium.org, bbudge@chromium.org, mseaborn@chromium.org
BUG=183015
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=11039

[hamaji, 2013-03-22 18:30:15]

Thanks guys for your suggestions in https://codereview.chromium.org/13011002/ !

Mark and Bill suggested I should add DescWrapperFactory::Import*. However, other
case clauses in nacl_ipc_adapter.cc isn't using DescWrapper so I created another
NaClDesc*Make style function. I think this is more consistent with other code
(and we don't need ref count operation), but if you prefer the
DescWrapperFactory solution, please let me know and I'll revise this change.

I'm not so sure what we should do if _open_osfhandle fails. I also considered
LOG_FATAL but as this function is tolerant of invalid unix FDs, I assumed just
allowing -1 would be more consistent.

Suggestions are welcomed. Thanks.

[Mark Seaborn, 2013-03-22 18:40:05]

> Mark and Bill suggested I should add
> DescWrapperFactory::Import*. However, other case clauses in
> nacl_ipc_adapter.cc isn't using DescWrapper so I created another
> NaClDesc*Make style function. I think this is more consistent with
> other code (and we don't need ref count operation), but if you
> prefer the DescWrapperFactory solution, please let me know and I'll
> revise this change.

No, this change is just what I wanted. :-)  I didn't mean you should add to
DescWrapperFactory (which is deprecated and shouldn't be used from new Chromium
code), I just meant that was an example of calling functions like the
NaClDescIoDescMakeFromHandle() you've added.

LGTM with changes below.

https://codereview.chromium.org/12929033/diff/1/src/trusted/desc/nacl_desc_io.c
File src/trusted/desc/nacl_desc_io.c (right):

https://codereview.chromium.org/12929033/diff/1/src/trusted/desc/nacl_desc_io...
src/trusted/desc/nacl_desc_io.c:99: posix_d = _open_osfhandle((intptr_t) handle,
_O_RDWR | _O_BINARY);
You should check for an error here:
if (posix_d == -1)
  return NULL;

https://codereview.chromium.org/12929033/diff/1/src/trusted/desc/nacl_desc_io...
src/trusted/desc/nacl_desc_io.c:103: nhdp = NaClHostDescPosixMake(posix_d,
NACL_ABI_O_RDWR);
You should check for an error here too:
if (nhdp == NULL)
  return NULL;

[hamaji, 2013-03-22 18:45:08]

Thanks for your prompt review!

https://codereview.chromium.org/12929033/diff/1/src/trusted/desc/nacl_desc_io.c
File src/trusted/desc/nacl_desc_io.c (right):

https://codereview.chromium.org/12929033/diff/1/src/trusted/desc/nacl_desc_io...
src/trusted/desc/nacl_desc_io.c:99: posix_d = _open_osfhandle((intptr_t) handle,
_O_RDWR | _O_BINARY);
On 2013/03/22 18:40:05, Mark Seaborn wrote:
> You should check for an error here:
> if (posix_d == -1)
>   return NULL;

Done.

https://codereview.chromium.org/12929033/diff/1/src/trusted/desc/nacl_desc_io...
src/trusted/desc/nacl_desc_io.c:103: nhdp = NaClHostDescPosixMake(posix_d,
NACL_ABI_O_RDWR);
On 2013/03/22 18:40:05, Mark Seaborn wrote:
> You should check for an error here too:
> if (nhdp == NULL)
>   return NULL;

Done.

[commit-bot: I haz the power, 2013-03-22 18:45:31]

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

[commit-bot: I haz the power, 2013-03-22 18:55:01]

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

[commit-bot: I haz the power, 2013-03-22 20:59:24]

Change committed as 11039