Implement nacl_irt_fdio for non-sfi mode.

Implement nacl_irt_fdio for non-sfi mode.

This CL implements nacl_irt_fdio for non-sfi mode.
Note that getdents is currently not supported because there is no good way to
implement it in a sane manner. Now it always returns ENOSYS.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=Tried to call a newly added function from plugin via AT_SYSINFO.
TEST=Make sure lseek and fstat refer to lseek64 and __fxstat64.

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

[hidehiko, 2014-01-09 10:04:59]

Hi Mark,

Could you take a look at this CL?
This CL is based on crrev.com/100373005. It is still under your review, but I
don't think we need huge design change on it. So, please let me send this out to
the code review to move forward more.
If you think the your new review comment for the base CL will affect hugely to
this CL's design, please feel free to suspend the review.

Thank you for your review in advance,
- hidehiko

[hidehiko, 2014-01-09 10:07:35]

On 2014/01/09 10:04:59, hidehiko wrote:
> Hi Mark,
> 
> Could you take a look at this CL?
> This CL is based on crrev.com/100373005. It is still under your review, but I
> don't think we need huge design change on it. So, please let me send this out
to
> the code review to move forward more.
> If you think the your new review comment for the base CL will affect hugely to
> this CL's design, please feel free to suspend the review.
> 
> Thank you for your review in advance,
> - hidehiko

FYI, I'm not very sure why, but linux_rel_precise32 trybot doesn't start
when I set the base CL is crrev.com/100373005.
I'll run it after the base CL is once landed somehow.
- hidehiko

[hamaji, 2014-01-09 10:47:22]

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_fdio.cc (right):

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:61: off_t result = ::lseek(fd, offset,
whence);
Same as fstat. Probably better to check if this call is referring lseek64.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:70: if (::fstat(fd, &host_st) < 0)
As locally chatted, I think we need to call fstat64 here. Have you checked
irt_fdio.o has a reference to fstat64 (__fxstat64 maybe)? If you've already
checked this, how about adding this in the TEST line?

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:74: st->nacl_abi_st_dev = 0;
You'll need to use this code when you implement IrtStat. How about creating a
function which converts from glibc stat to NaCl stat?

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:86: break;
I'm not sure, but I'd translate all possible values. It should be useful for
unittests based on sel_ldr (or nonsfi_ldr?).

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:88: LOG(ERROR) << "Unusual NaCl
descriptor type.";
LOG(ERROR) might be over kill? And, how about outputting st_mode & S_IFMT?

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:96: m |= NACL_ABI_S_IXUSR;
Is it intentional we are dropping S_I*GRP and S_I*OTH ? If so, it would be
better to have a comment here.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:107: st->nacl_abi_st_ctime =
host_st.st_ctime;
Any reason we don't copy nano seconds? For compatibility maybe? Would be better
to have a comment?

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:117: // be fit to the buf after abi
conversion actually.
TODO maybe?

[hidehiko, 2014-01-09 12:41:18]

Thank you for your quick review. PTAL?

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_fdio.cc (right):

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:61: off_t result = ::lseek(fd, offset,
whence);
On 2014/01/09 10:47:22, hamaji wrote:
> Same as fstat. Probably better to check if this call is referring lseek64.

Yes, I've checked. common.gypi defines _FILE_OFFSET_BITS=64.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:70: if (::fstat(fd, &host_st) < 0)
On 2014/01/09 10:47:22, hamaji wrote:
> As locally chatted, I think we need to call fstat64 here. Have you checked
> irt_fdio.o has a reference to fstat64 (__fxstat64 maybe)? If you've already
> checked this, how about adding this in the TEST line?

Done.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:74: st->nacl_abi_st_dev = 0;
On 2014/01/09 10:47:22, hamaji wrote:
> You'll need to use this code when you implement IrtStat. How about creating a
> function which converts from glibc stat to NaCl stat?

Yes, I was planning to do it in a following CL (w/ irt_filename implementatino),
but doing it in this CL is fine to me. Done.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:86: break;
On 2014/01/09 10:47:22, hamaji wrote:
> I'm not sure, but I'd translate all possible values. It should be useful for
> unittests based on sel_ldr (or nonsfi_ldr?).

I just mimic'ed the NaCl's original implementation, and IMHO keeping it sync'ed
is valuable. Ditto for missing bits/fields below. WDYT?

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:88: LOG(ERROR) << "Unusual NaCl
descriptor type.";
On 2014/01/09 10:47:22, hamaji wrote:
> LOG(ERROR) might be over kill? And, how about outputting st_mode & S_IFMT?

Done. Redcued to LOG(WARNING).

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:96: m |= NACL_ABI_S_IXUSR;
On 2014/01/09 10:47:22, hamaji wrote:
> Is it intentional we are dropping S_I*GRP and S_I*OTH ? If so, it would be
> better to have a comment here.

Acknowledged.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:107: st->nacl_abi_st_ctime =
host_st.st_ctime;
On 2014/01/09 10:47:22, hamaji wrote:
> Any reason we don't copy nano seconds? For compatibility maybe? Would be
better
> to have a comment?

Acknowledged.

https://codereview.chromium.org/131133002/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:117: // be fit to the buf after abi
conversion actually.
On 2014/01/09 10:47:22, hamaji wrote:
> TODO maybe?

Done.

[Mark Seaborn, 2014-01-10 13:11:20]

LGTM

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/abi_conversion.cc (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/abi_conversion.cc:8: #include <cstring>
Nit: sort #includes within block

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/abi_conversion.h (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/abi_conversion.h:4: 
This file should use the standard #ifndef+#define include guard, so that no-one
gets a surprise if they add non-idempotent declarations to it

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/abi_conversion.h:5: extern "C" {
'extern "C"' doesn't affect struct declarations, so you can remove this

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_fdio.cc (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:22: if (::close(fd) < 0)
Same comment about "::" prefix as previous change...

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:31: 
Nit: be consistent about whether you have an empty line after your "return
errno" lines :-)

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:91: // for seek, fstat and getdents,
their argument types should be nacl_abi_X,
Nit: capitalise as "For"

[Mark Seaborn, 2014-01-10 13:37:23]

One more thing...

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_interfaces.cc (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_interfaces.cc:26:
NACL_INTERFACE_TABLE(NACL_IRT_FDIO_v0_1, kIrtFdIO),
This should be NACL_IRT_DEV_FDIO_v0_1.  The non-"dev" name is only provided for
compatibility in existing ABIs, but new ABIs should not allow querying for the
non-"dev" name.

[hidehiko, 2014-01-15 03:50:44]

Thank you for your review. Submitting...

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/abi_conversion.cc (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/abi_conversion.cc:8: #include <cstring>
On 2014/01/10 13:11:20, Mark Seaborn wrote:
> Nit: sort #includes within block

This is sorted. According to the style guide, C system files are followed by C++
system files.
(and actually our lint warned when I reordered.)

cf)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/abi_conversion.h (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/abi_conversion.h:4: 
On 2014/01/10 13:11:20, Mark Seaborn wrote:
> This file should use the standard #ifndef+#define include guard, so that
no-one
> gets a surprise if they add non-idempotent declarations to it

Oops. Good catch. Fixed.

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/abi_conversion.h:5: extern "C" {
On 2014/01/10 13:11:20, Mark Seaborn wrote:
> 'extern "C"' doesn't affect struct declarations, so you can remove this

Done.

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_fdio.cc (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:22: if (::close(fd) < 0)
On 2014/01/10 13:11:20, Mark Seaborn wrote:
> Same comment about "::" prefix as previous change...

Done. Also aligned to "if (func(...))" style if applicable.

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:31: 
On 2014/01/10 13:11:20, Mark Seaborn wrote:
> Nit: be consistent about whether you have an empty line after your "return
> errno" lines :-)

Done.

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_fdio.cc:91: // for seek, fstat and getdents,
their argument types should be nacl_abi_X,
On 2014/01/10 13:11:20, Mark Seaborn wrote:
> Nit: capitalise as "For"

Done.

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_interfaces.cc (right):

https://codereview.chromium.org/131133002/diff/280001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_interfaces.cc:26:
NACL_INTERFACE_TABLE(NACL_IRT_FDIO_v0_1, kIrtFdIO),
On 2014/01/10 13:37:23, Mark Seaborn wrote:
> This should be NACL_IRT_DEV_FDIO_v0_1.  The non-"dev" name is only provided
for
> compatibility in existing ABIs, but new ABIs should not allow querying for the
> non-"dev" name.

Done.

[commit-bot: I haz the power, 2014-01-15 05:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/131133002/420001

[commit-bot: I haz the power, 2014-01-15 16:23:15]

Change committed as 244893