[NaCl SDK] nacl_io: Added support for access() syscall.

[NaCl SDK] nacl_io: Added support for access() syscall.

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

[Matt Giuca, 2013-06-19 10:41:42]

Hi again Ben,

This adds support for the access() syscall for all file systems.

I'm interested in getting this into Chrome 29 (by the end of Friday), so if you
think it's possible (just a few nits), then a quick review would be appreciated.
If you have more detailed changes needed, then just take your time, because I
won't be able to deal with it before the branch point.

Regarding HttpFs, it's a bit unclear how access() should interact with the
cache. Currently, it doesn't; it just makes a HEAD request if the file isn't
already cached. There's also an issue testing access on this file system (see
comments). If you'd like, I can just back off on HttpFs for now (make it set
errno to ENOSYS) and we can deal with it in another CL.

Thanks

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:181: void
SetResponseExpectFail(int status_code, const char* headers);
Note: I borrowed this code from my other CL
https://codereview.chromium.org/16959007/.

I'm hoping I'll just submit that one before this.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:355:
TEST_F(MountHttpNodeTest, Access) {
Note: Access and AccessWrite suffer from the same mysterious problem as
OpenAndCloseNoCache and ReadCachedNoContentLength (http://crbug.com/244313), so
they currently fail. But I figure that we already have failing cases, so this
doesn't hurt.

[binji, 2013-06-19 16:46:28]

lgtm.

A few thoughts: what is the correct behavior of access on directories? Also,
what is the value in supporting X_OK for any node? There is nothing the program
can do to execute nacl_io files, so it seems disingenuous.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount.h (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount.h:56: // Returns 0 on success, or
an appropriate errno value on failure.
What is the behavior of access if given a directory?

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_dev.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:274: MountNode* node =
NULL;
probably OK not to, but I would AutoLock the mount here.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:29: // a_mode is
unused, since all files are readable, writable and executable.
probably shouldn't allow executable for any mount files.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:31: return ENODEV;
do you think this is the correct error value to return here? If so, please
change the others to return it is well.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:38: MountNodeHtml5Fs*
node = new MountNodeHtml5Fs(this, fileref);
does this do much more than querying the fileref? If not, may as well do that
here.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_http.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_http.cc:64: std::string url =
url_root_ +
might be nice to refactor this into a MakeURL function to share with Open().
WDYT?

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_http.cc:79: }
release the node here

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_mem.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_mem.cc:91: ((a_mode & X_OK) &&
!(obj_mode & S_IEXEC))) {
no exec

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:181: void
SetResponseExpectFail(int status_code, const char* headers);
On 2013/06/19 10:41:42, Matt Giuca wrote:
> Note: I borrowed this code from my other CL
> https://codereview.chromium.org/16959007/.
> 
> I'm hoping I'll just submit that one before this.

In the future, you can use "git cl upload <branch name>" to upload a patch that
assumes branch is upstream so it won't show up in the diff.

(Though I guess this is nice in a way, because it gives more context to the
CL...)

[Matt Giuca, 2013-06-20 01:43:14]

Thanks for a quick review. Just some contention about the X_OK issue.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount.h (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount.h:56: // Returns 0 on success, or
an appropriate errno value on failure.
On 2013/06/19 16:46:28, binji wrote:
> What is the behavior of access if given a directory?

Should be the same as for normal access. R_OK means you can GetDents on it, W_OK
means you can create files and subdirs in it, X_OK means you can cd to it. Do I
need to say this in comments here or do we assume the reader knows the semantics
of POSIX syscalls?

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_dev.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_dev.cc:274: MountNode* node =
NULL;
On 2013/06/19 16:46:28, binji wrote:
> probably OK not to, but I would AutoLock the mount here.

Done.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:29: // a_mode is
unused, since all files are readable, writable and executable.
On 2013/06/19 16:46:28, binji wrote:
> probably shouldn't allow executable for any mount files.

OK let's discuss this here. You said in the email:
> What is the value in supporting X_OK for any node? There
> is nothing the program can do to execute nacl_io files, so
> it seems disingenuous.

Firstly, X_OK is different for directories and files. If a directory is
non-executable, you can list its files but you cannot stat or open them at all.
So to correctly emulate the semantics of a POSIX system, any directory you want
to be able to use files in needs to have X_OK. Now most programs aren't going to
use access() to check this, so it doesn't really matter, but we still want to
emulate it as closely as possible. So since we don't have any real permissions,
I think we want to report that all directories are executable.

For files, I don't care too much either way, since as you say, there is no real
way to execute the files. But I discussed with some other people here and sammc
pointed out that X_OK doesn't just apply to running binaries. It's an indicator
to the shell that it is okay to try and run this script. I'm picturing a
scenario where someone actually ports bash to NaCl using nacl_io, and it lets
you run bash scripts from the command line. Bash is going to check X_OK to see
whether any given script can be run. Again, since we have no way to store a bit
in the filesystem itself, it is best to assume that all files are executable.
Note that Linux does this for filesystems that have no execute bit (e.g., if you
mount a FAT32 file system in Linux, all files have X_OK). But I'm not too fussed
about this case because it's fairly obscure. I'd say there are two choices (and
we should be consistent across all file systems):

a) All files and directories have X_OK set.
b) No regular files have X_OK set, but all directories have X_OK set.

Do you have a preference? I would go with (a) since there is precedent for it in
the Linux FAT32 driver. This probably means I should update the memfs
implementation to set X_OK on all new files.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:31: return ENODEV;
On 2013/06/19 16:46:28, binji wrote:
> do you think this is the correct error value to return here? If so, please
> change the others to return it is well.

Oh wow, this was copied from Open() but then you changed it in your huge
refactor CL and I didn't update mine! This is totally wrong. Fixed to match
Open().

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:38: MountNodeHtml5Fs*
node = new MountNodeHtml5Fs(this, fileref);
On 2013/06/19 16:46:28, binji wrote:
> does this do much more than querying the fileref? If not, may as well do that
> here.

I don't know what you mean. Calling node->Init is what actually checks whether
the file exists, and gets an appropriate error code if not. This code is almost
the same as Open() except it always closes the node after opening it. In fact,
I'm just going to delete it and call Open().

Note: I just hit http://crbug.com/252064 by changing this.
https://codereview.chromium.org/17450014/ will have to be submitted first.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_http.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_http.cc:64: std::string url =
url_root_ +
On 2013/06/19 16:46:28, binji wrote:
> might be nice to refactor this into a MakeURL function to share with Open().
> WDYT?

Done.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_http.cc:79: }
On 2013/06/19 16:46:28, binji wrote:
> release the node here

Done.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_mem.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_mem.cc:91: ((a_mode & X_OK) &&
!(obj_mode & S_IEXEC))) {
On 2013/06/19 16:46:28, binji wrote:
> no exec

See comment on mount_html5fs.

Also note that regardless of the outcome of that discussion, I think this code
makes sense. This isn't saying that files are executable, this is saying that if
the EXEC bit is set on a given file, Access with X_OK will succeed. Currently,
that bit is never set, so it will always fail, but given that there *is* an EXEC
bit in the memfs data structure, it is correct to test it here.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_http_test.cc:181: void
SetResponseExpectFail(int status_code, const char* headers);
Yeah I wasn't sure whether to do that in this case.

[binji, 2013-06-20 16:07:21]

slgtm

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount.h (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount.h:56: // Returns 0 on success, or
an appropriate errno value on failure.
On 2013/06/20 01:43:14, Matt Giuca wrote:
> On 2013/06/19 16:46:28, binji wrote:
> > What is the behavior of access if given a directory?
> 
> Should be the same as for normal access. R_OK means you can GetDents on it,
W_OK
> means you can create files and subdirs in it, X_OK means you can cd to it. Do
I
> need to say this in comments here or do we assume the reader knows the
semantics
> of POSIX syscalls?

Hm. I might like to have something similar to your explanation of why X_OK is
handled the way it is here. The semantics of X_OK w.r.t. directories would be
useful information in that discussion.

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:29: // a_mode is
unused, since all files are readable, writable and executable.
On 2013/06/20 01:43:14, Matt Giuca wrote:
> On 2013/06/19 16:46:28, binji wrote:
> > probably shouldn't allow executable for any mount files.
> 
> OK let's discuss this here. You said in the email:
> > What is the value in supporting X_OK for any node? There
> > is nothing the program can do to execute nacl_io files, so
> > it seems disingenuous.
> 
> Firstly, X_OK is different for directories and files. If a directory is
> non-executable, you can list its files but you cannot stat or open them at
all.
> So to correctly emulate the semantics of a POSIX system, any directory you
want
> to be able to use files in needs to have X_OK. Now most programs aren't going
to
> use access() to check this, so it doesn't really matter, but we still want to
> emulate it as closely as possible. So since we don't have any real
permissions,
> I think we want to report that all directories are executable.
> 
> For files, I don't care too much either way, since as you say, there is no
real
> way to execute the files. But I discussed with some other people here and
sammc
> pointed out that X_OK doesn't just apply to running binaries. It's an
indicator
> to the shell that it is okay to try and run this script. I'm picturing a
> scenario where someone actually ports bash to NaCl using nacl_io, and it lets
> you run bash scripts from the command line. Bash is going to check X_OK to see
> whether any given script can be run. Again, since we have no way to store a
bit
> in the filesystem itself, it is best to assume that all files are executable.
> Note that Linux does this for filesystems that have no execute bit (e.g., if
you
> mount a FAT32 file system in Linux, all files have X_OK). But I'm not too
fussed
> about this case because it's fairly obscure. I'd say there are two choices
(and
> we should be consistent across all file systems):
> 
> a) All files and directories have X_OK set.
> b) No regular files have X_OK set, but all directories have X_OK set.
> 
> Do you have a preference? I would go with (a) since there is precedent for it
in
> the Linux FAT32 driver. This probably means I should update the memfs
> implementation to set X_OK on all new files.

OK, I agree. :)
I like a) better too..

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc:38: MountNodeHtml5Fs*
node = new MountNodeHtml5Fs(this, fileref);
On 2013/06/20 01:43:14, Matt Giuca wrote:
> On 2013/06/19 16:46:28, binji wrote:
> > does this do much more than querying the fileref? If not, may as well do
that
> > here.
> 
> I don't know what you mean. Calling node->Init is what actually checks whether
> the file exists, and gets an appropriate error code if not. This code is
almost
> the same as Open() except it always closes the node after opening it. In fact,
> I'm just going to delete it and call Open().
> 
> Note: I just hit http://crbug.com/252064 by changing this.
> https://codereview.chromium.org/17450014/ will have to be submitted first.

Yes, you're right about Init. It seemed like a potential shortcut -- instead of
creating a new node just to see if it exists, query the FileRef here. But your
solution is probably more robust (in case Init returns other errors, etc).

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io/mount_mem.cc (right):

https://codereview.chromium.org/15800004/diff/11001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io/mount_mem.cc:91: ((a_mode & X_OK) &&
!(obj_mode & S_IEXEC))) {
On 2013/06/20 01:43:14, Matt Giuca wrote:
> On 2013/06/19 16:46:28, binji wrote:
> > no exec
> 
> See comment on mount_html5fs.
> 
> Also note that regardless of the outcome of that discussion, I think this code
> makes sense. This isn't saying that files are executable, this is saying that
if
> the EXEC bit is set on a given file, Access with X_OK will succeed. Currently,
> that bit is never set, so it will always fail, but given that there *is* an
EXEC
> bit in the memfs data structure, it is correct to test it here.

OK

[Matt Giuca, 2013-06-20 22:58:22]

https://codereview.chromium.org/15800004/diff/14002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_html5fs_test.cc (right):

https://codereview.chromium.org/15800004/diff/14002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_html5fs_test.cc:335:
.WillOnce(Return(0))
Ah, I just realised (due to your explanation on the other CL) that this is a bad
mock. In the file missing case, Create should still succeed, but Query should
fail. I will update this test when I get to work.

[Matt Giuca, 2013-06-21 01:57:42]

https://codereview.chromium.org/15800004/diff/14002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_html5fs_test.cc (right):

https://codereview.chromium.org/15800004/diff/14002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_html5fs_test.cc:335:
.WillOnce(Return(0))
On 2013/06/20 22:58:22, Matt Giuca wrote:
> Ah, I just realised (due to your explanation on the other CL) that this is a
bad
> mock. In the file missing case, Create should still succeed, but Query should
> fail. I will update this test when I get to work.

Done.

[commit-bot: I haz the power, 2013-06-21 02:35:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/15800004/29001

[commit-bot: I haz the power, 2013-06-21 08:48:36]

Change committed as 207754

[binji, 2013-06-21 16:13:51]

On 2013/06/21 08:48:36, I haz the power (commit-bot) wrote:
> Change committed as 207754

Strange... the CQ did not send this to the SDK trybots. Anyway, this broke the
win_nacl_sdk bot:
http://build.chromium.org/p/client.nacl.sdk/builders/windows-sdk-multi/builds...

mount_dev.cc(12) : fatal error C1083: Cannot open include file: 'unistd.h': No
such file or directory

I'll look into it.