[NaCl SDK] nacl_io: Make IRT intercepts (and their test code) more robust.

[NaCl SDK] nacl_io: Make IRT intercepts (and their test code) more robust.

The kernel_wrap_test.cc tests were previously relying on the global
errno value being non-zero before they ran.

This change makes the kernel_wrap_ functions do the right thing even
when the kernel proxy function don't see errno (as is the case for the
mock kernel proxy) and initialises the global errno before each test
run.

BUG=
R=binji@chromium.org

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

[binji, 2014-05-06 23:25:53]

lgtm, though I'd prefer not masking the error. It would be nice to warn at the
very least.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:151: // Most kernal
intercept functions (ki_*) return -1 and set errno accordingly.
nit: kernel

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:158: if (rtn == 0)
rtn = EIO; \
we should probably assert here, right?

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:104: #define
ERRNO_RTN(function_call) \
make this consistent with the other kernel_wrap_* (it doesn't have it)

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:111:
ERRNO_RTN(ki_close(fd));
do this stuff for kernel_wrap_bionic too

[binji, 2014-05-06 23:29:52]

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:292: return errno;
return 0

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:116:
RTN_ERRNO_IF(*newfd < 0);
ERRNO_RTN

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:122:
RTN_ERRNO_IF(newfd < 0);
ERRNO_RTN

[binji, 2014-05-06 23:31:48]

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:133: return
(signed_nread < 0) ? errno : 0;
missed some:
read, write, seek, getdents ...

[Sam Clegg, 2014-05-06 23:34:13]

On 2014/05/06 23:25:53, binji wrote:
> lgtm, though I'd prefer not masking the error. It would be nice to warn at the
> very least.
> 

The probably is that the MockKernelProxy never sets errno, so the wrap tests
would all assert.  Suggestions?


>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> File native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc (right):
> 
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:151: // Most
kernal
> intercept functions (ki_*) return -1 and set errno accordingly.
> nit: kernel
> 
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:158: if (rtn ==
0)
> rtn = EIO; \
> we should probably assert here, right?
> 
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):
> 
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:104: #define
> ERRNO_RTN(function_call) \
> make this consistent with the other kernel_wrap_* (it doesn't have it)
> 
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:111:
> ERRNO_RTN(ki_close(fd));
> do this stuff for kernel_wrap_bionic too

[binji, 2014-05-06 23:47:07]

On 2014/05/06 23:34:13, Sam Clegg wrote:
> On 2014/05/06 23:25:53, binji wrote:
> > lgtm, though I'd prefer not masking the error. It would be nice to warn at
the
> > very least.
> > 
> 
> The probably is that the MockKernelProxy never sets errno, so the wrap tests
> would all assert.  Suggestions?

Oh, I see. Well, the most straightforward thing to do is to add a new gmock
action to set errno when it should.

Take a look at ACTION_P(SetStat...) in kernel_wrap_test.cc, or
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Actions_Quickly

> 
> 
> >
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> > File native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc (right):
> > 
> >
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> > native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:151: // Most
> kernal
> > intercept functions (ki_*) return -1 and set errno accordingly.
> > nit: kernel
> > 
> >
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> > native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:158: if (rtn ==
> 0)
> > rtn = EIO; \
> > we should probably assert here, right?
> > 
> >
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> > File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):
> > 
> >
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> > native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:104: #define
> > ERRNO_RTN(function_call) \
> > make this consistent with the other kernel_wrap_* (it doesn't have it)
> > 
> >
>
https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
> > native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:111:
> > ERRNO_RTN(ki_close(fd));
> > do this stuff for kernel_wrap_bionic too

[Sam Clegg, 2014-05-07 00:29:02]

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:151: // Most kernal
intercept functions (ki_*) return -1 and set errno accordingly.
On 2014/05/06 23:25:54, binji wrote:
> nit: kernel

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:158: if (rtn == 0)
rtn = EIO; \
On 2014/05/06 23:25:54, binji wrote:
> we should probably assert here, right?

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:292: return errno;
On 2014/05/06 23:29:52, binji wrote:
> return 0

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:104: #define
ERRNO_RTN(function_call) \
On 2014/05/06 23:25:54, binji wrote:
> make this consistent with the other kernel_wrap_* (it doesn't have it)

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:111:
ERRNO_RTN(ki_close(fd));
On 2014/05/06 23:25:54, binji wrote:
> do this stuff for kernel_wrap_bionic too

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:116:
RTN_ERRNO_IF(*newfd < 0);
On 2014/05/06 23:29:52, binji wrote:
> ERRNO_RTN

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:122:
RTN_ERRNO_IF(newfd < 0);
On 2014/05/06 23:29:52, binji wrote:
> ERRNO_RTN

Done.

https://codereview.chromium.org/271513002/diff/60001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:133: return
(signed_nread < 0) ? errno : 0;
On 2014/05/06 23:31:49, binji wrote:
> missed some:
> read, write, seek, getdents ...

Done.

[binji, 2014-05-07 14:58:24]

awesome, lgtm

https://codereview.chromium.org/271513002/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc (left):

https://codereview.chromium.org/271513002/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:175: return
(ki_fchdir(fd)) ? errno : 0;
hm, these should have always failed with bionic.

https://codereview.chromium.org/271513002/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:238: if (*result
== 1)
hm, this was broken before too.

[sbc, 2014-05-07 15:58:17]

https://codereview.chromium.org/271513002/diff/100001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc (left):

https://codereview.chromium.org/271513002/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:175: return
(ki_fchdir(fd)) ? errno : 0;
On 2014/05/07 14:58:24, binji wrote:
> hm, these should have always failed with bionic.

Yup..

https://codereview.chromium.org/271513002/diff/100001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:238: if (*result
== 1)
On 2014/05/07 14:58:24, binji wrote:
> hm, this was broken before too.

Yup.. I fixed the bug in the newlib version but didn't realize it was duplicated
here.

[Sam Clegg, 2014-05-15 21:15:31]

Committed patchset #7 manually as r270819 (presubmit successful).