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

native_client_sdk/src/libraries/nacl_io/mount_html5fs.cc

 /* Copyright (c) 2012 The Chromium Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "nacl_io/mount_html5fs.h"
 
 #include <errno.h>
+#include <fcntl.h>
 #include <ppapi/c/pp_completion_callback.h>
 #include <ppapi/c/pp_errors.h>
 #include <stdlib.h>
 #include <string.h>
 #include <algorithm>
 #include "nacl_io/mount_node_html5fs.h"
 #include "sdk_util/auto_lock.h"
 
 namespace {
 
 #if defined(WIN32)
 int64_t strtoull(const char* nptr, char** endptr, int base) {
   return _strtoui64(nptr, endptr, base);
 }
 #endif
 
 }  // namespace
 
+Error MountHtml5Fs::Access(const Path& path, int a_mode) {
+  // a_mode is unused, since all files are readable, writable and executable.

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

probably shouldn't allow executable for any mount files.

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

OK let's discuss this here. You said in the email:
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.

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

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

+  if (BlockUntilFilesystemOpen() != PP_OK)
+    return ENODEV;

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

do you think this is the correct error value to return here? If so, please
change the others to return it is well.

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

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().

+
+  PP_Resource fileref = ppapi()->GetFileRefInterface()->Create(
+      filesystem_resource_, path.Join().c_str());
+  if (!fileref)
+    return ENOENT;
+
+  MountNodeHtml5Fs* node = new MountNodeHtml5Fs(this, fileref);

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

does this do much more than querying the fileref? If not, may as well do that
here.

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

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.

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

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).

+  Error error = node->Init(O_RDONLY);
+  node->Release();
+  return error;
+}
+
 Error MountHtml5Fs::Open(const Path& path, int mode, MountNode** out_node) {
   *out_node = NULL;
 
   Error error = BlockUntilFilesystemOpen();
   if (error)
     return error;
 
   PP_Resource fileref = ppapi()->GetFileRefInterface()
       ->Create(filesystem_resource_, path.Join().c_str());
   if (!fileref)
@@ skipping 134 matching lines @@
   MountHtml5Fs* self = static_cast<MountHtml5Fs*>(user_data);
   self->FilesystemOpenCallback(result);
 }
 
 void MountHtml5Fs::FilesystemOpenCallback(int32_t result) {
   AutoLock lock(&lock_);
   filesystem_open_has_result_ = true;
   filesystem_open_error_ = PPErrorToErrno(result);
   pthread_cond_signal(&filesystem_open_cond_);
 }