[NaCl SDK] nacl_io: Change default root filesystem type from passthroughfs to memfs

[NaCl SDK] nacl_io: Change default root filesystem type from passthroughfs to memfs

Mounting a passthroughfs as root doesn't make much
sense since its not generally possible to create
new files and directories in the actual system root.
This change is needed before we can add symlink
support since symlink support requires implementing more
thorough pathname resolution.  For example, today its
possible to open /dev/stdin without /dev existing in
the root filesystem.

This change improves that passthroughfs on linux by
adding to kernel_wrap_dummy and adds some basic
testing of passthroughfs.

Although this change will effect all users of nacl_io
it seems unlikely that anyone was relying on the
passthroughfs being mounted at root (especially since
passthroughfs only works under sel_ldr). For the more
common sel_ldr case the user would either not use
nacl_io or not initialize it in order to get direct
access to the host file system.

Also, add tests for mkdir/rmdir.

CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk
BUG=

[Sam Clegg, 2015-01-06 17:28:26]

sbc@chromium.org changed reviewers:
+ binji@chromium.org

[binji, 2015-01-07 19:14:59]

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:45: // is enough
Will this do the right thing if you request one dirent, then? If I understand
the logic here correctly, it will make a syscall to read less than one
linux_dirent's size in bytes.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:56: struct dirent*
nacl_d = (struct dirent*)((char*)in_buf + in_offset);
consistency between in_ and nacl_ prefixes?

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:96: static void
check_dir_contents(const char* dirname, const char** expected,
nit: use CamelCase for functions

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:113: for (i = 0; i
< count; i++) {
this code is untested. Best to use it or remove it

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:115: expected[i] =
NULL;
This is a bit unexpected, probably should be documented

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:349: ASSERT_EQ(-1,
ki_mkdir("/does_not_exist/foo", S_IREAD | S_IWRITE));
is this implemented yet?

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:908:
check_dir_contents("/passthrough", NULL, -1);
This is a manual test only?

[Sam Clegg, 2015-01-08 12:59:08]

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:45: // is enough
On 2015/01/07 19:14:59, binji wrote:
> Will this do the right thing if you request one dirent, then? If I understand
> the logic here correctly, it will make a syscall to read less than one
> linux_dirent's size in bytes.

in_count is normally much bigger than the size of the struct itself to account
for the variable size of the d_name field.   The idea is to be conservative and
always read fewer elements than we can pack into in_buf (otherwise we'd have to
through away the extra onces).  Its true that if in_count is too small we might
not even be able to read a single one but this is true of the native syscall
too.   This code is essentially borrowed from kernel_wrap_glibc.cc were it seems
to work fine.

I did find a few issues though so I've largely re-written it.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:56: struct dirent*
nacl_d = (struct dirent*)((char*)in_buf + in_offset);
On 2015/01/07 19:14:59, binji wrote:
> consistency between in_ and nacl_ prefixes?

Done.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
File native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc (right):

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:96: static void
check_dir_contents(const char* dirname, const char** expected,
On 2015/01/07 19:14:59, binji wrote:
> nit: use CamelCase for functions

Done.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:113: for (i = 0; i
< count; i++) {
On 2015/01/07 19:14:59, binji wrote:
> this code is untested. Best to use it or remove it

Sorry, I meant to add a getdents tests.  Done that now.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:115: expected[i] =
NULL;
On 2015/01/07 19:14:59, binji wrote:
> This is a bit unexpected, probably should be documented

Done.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:349: ASSERT_EQ(-1,
ki_mkdir("/does_not_exist/foo", S_IREAD | S_IWRITE));
On 2015/01/07 19:14:59, binji wrote:
> is this implemented yet?

At least the parent must exist yes.

https://codereview.chromium.org/832413004/diff/1/native_client_sdk/src/tests/...
native_client_sdk/src/tests/nacl_io_test/kernel_proxy_test.cc:908:
check_dir_contents("/passthrough", NULL, -1);
On 2015/01/07 19:14:59, binji wrote:
> This is a manual test only?

It verified that the directory can be read, nothing more.  I guess its mostly
useful for manual testing yes since it prints the contents.

[binji, 2015-01-09 21:11:45]

code looks OK, but what's going on with the trybots? I see crashes on
KernelWrapTest.access

https://codereview.chromium.org/832413004/diff/20001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/832413004/diff/20001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:43: #define
ROUND_UP(X, SIZE) ( (X + SIZE-1) & (~(SIZE-1)) )
parens around X and SIZE

[Sam Clegg, 2015-05-20 00:18:34]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2015-05-20 00:21:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832413004/80001

[commit-bot: I haz the power, 2015-05-20 00:21:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 00:21:35]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Sam Clegg, 2015-05-20 00:31:21]

ptal

[binji, 2015-05-20 09:08:51]

On 2015/05/20 00:31:21, Sam Clegg wrote:
> ptal

This still seems to be failing many tests.

[Sam Clegg, 2015-06-02 22:13:41]

The CQ bit was checked by sbc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-02 22:14:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832413004/100001

[commit-bot: I haz the power, 2015-06-03 00:19:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-03 00:19:17]

Dry run: Try jobs failed on following builders:
  win_nacl_sdk on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[Sam Clegg, 2015-09-08 17:13:46]

The CQ bit was checked by sbc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-08 17:14:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832413004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/832413004/120001

[commit-bot: I haz the power, 2015-09-08 17:43:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 17:43:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam Clegg, 2015-09-08 17:44:49]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2015-09-08 17:45:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832413004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/832413004/120001

[commit-bot: I haz the power, 2015-09-08 17:45:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 17:45:51]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Sam Clegg, 2015-09-08 17:48:51]

ptal.  trybots should be green now.

Found a nasty bug in the glibc _getdents_real

[binji, 2015-09-08 19:18:57]

lgtm

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:49: // Since the
linux_dirent is larger (by a single pad byte) we know that we
where is the single pad byte?

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:67:
memcpy(out_d->d_name, d->d_name, name_len);
shouldn't this clamp to the max size of out_d->d_name? Or is this always
variable size?

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:71: sizeof(ino_t));
why round to sizeof(ino_t)? Is this to align the subsequent record?

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc (right):

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_glibc.cc:559: nacl_offset +=
nacl_d->nacl_abi_d_reclen;
ooh, nasty.

[Sam Clegg, 2015-09-09 10:11:54]

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:49: // Since the
linux_dirent is larger (by a single pad byte) we know that we
On 2015/09/08 19:18:57, binji wrote:
> where is the single pad byte?

At the end.  See 'man getdents'.

https://codereview.chromium.org/832413004/diff/120001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:67:
memcpy(out_d->d_name, d->d_name, name_len);
On 2015/09/08 19:18:57, binji wrote:
> shouldn't this clamp to the max size of out_d->d_name? Or is this always
> variable size?

Its variable size.  The max size should be 256 in both cases.

[Sam Clegg, 2015-09-09 10:23:40]

The CQ bit was checked by sbc@chromium.org

[Sam Clegg, 2015-09-09 10:23:41]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org
Link to the patchset: https://codereview.chromium.org/832413004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-09-09 10:23:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832413004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/832413004/140001

[commit-bot: I haz the power, 2015-09-09 13:42:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 13:42:36]

Try jobs failed on following builders:
  win_nacl_sdk on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_nacl_sdk/buil...)