[NaCl SDK] nacl_io: big refactor to return error value (errno).

native_client_sdk/src/libraries/nacl_io/kernel_proxy.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/kernel_proxy.h"
 
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <pthread.h>
@@ skipping 18 matching lines @@
 #include "utils/ref_object.h"
 
 #ifndef MAXPATHLEN
 #define MAXPATHLEN 256
 #endif
 
 // TODO(noelallen) : Grab/Redefine these in the kernel object once available.
 #define USR_ID 1002
 #define GRP_ID 1003
 
+KernelProxy::KernelProxy() : dev_(0), ppapi_(NULL) {}
 
-
-KernelProxy::KernelProxy()
-   : dev_(0),
-     ppapi_(NULL) {
-}
-
-KernelProxy::~KernelProxy() {
-  delete ppapi_;
-}
+KernelProxy::~KernelProxy() { delete ppapi_; }
 
 void KernelProxy::Init(PepperInterface* ppapi) {
   ppapi_ = ppapi;
   cwd_ = "/";
   dev_ = 1;
 
   factories_["memfs"] = MountMem::Create<MountMem>;
   factories_["dev"] = MountDev::Create<MountDev>;
   factories_["html5fs"] = MountHtml5Fs::Create<MountHtml5Fs>;
   factories_["httpfs"] = MountHttp::Create<MountHttp>;
   factories_["passthroughfs"] = MountPassthrough::Create<MountPassthrough>;
 
-  // Create passthrough mount at root
-  StringMap_t smap;
-  mounts_["/"] = MountPassthrough::Create<MountPassthrough>(
-      dev_++, smap, ppapi_);
-  mounts_["/dev"] = MountDev::Create<MountDev>(dev_++, smap, ppapi_);
+  int result;
+  result = mount("", "/", "passthroughfs", 0, NULL);
+  assert(result == 0);
+
+  result = mount("", "/dev", "dev", 0, NULL);
+  assert(result == 0);
 
   // Open the first three in order to get STDIN, STDOUT, STDERR
   open("/dev/stdin", O_RDONLY);
   open("/dev/stdout", O_WRONLY);
   open("/dev/stderr", O_WRONLY);
 }
 
-int KernelProxy::open(const char *path, int oflags) {
+int KernelProxy::open(const char* path, int oflags) {

[noelallen1, 2013/06/07 21:48:43]

Error KernelProxy::open?

[binji, 2013/06/07 23:23:11]

No, KernelProxy mimicks the actual open call, so it returns 0/-1 and sets errno,
etc.

   Path rel;
 
-  Mount* mnt = AcquireMountAndPath(path, &rel);
-  if (mnt == NULL) return -1;
+  Mount* mnt;
+  Error error = AcquireMountAndPath(path, &mnt, &rel);
+  if (error) {
+    errno = error;
+    return -1;
+  }
 
-  MountNode* node = mnt->Open(rel, oflags);
-  if (node == NULL) {
+  MountNode* node = NULL;
+  error = mnt->Open(rel, oflags, &node);
+  if (error) {
+    errno = error;
     ReleaseMount(mnt);
     return -1;
   }
 
-  KernelHandle* handle = new KernelHandle(mnt, node, oflags);
+  KernelHandle* handle = new KernelHandle(mnt, node);
+  error = handle->Init(oflags);
+  if (error) {
+    errno = error;
+    ReleaseMount(mnt);
+    return -1;
+  }
+
   int fd = AllocateFD(handle);
   mnt->AcquireNode(node);
 
   ReleaseHandle(handle);
   ReleaseMount(mnt);
 
   return fd;
 }
 
 int KernelProxy::close(int fd) {

[noelallen1, 2013/06/07 21:48:43]

Error KernelProxy::close?

-  KernelHandle* handle = AcquireHandle(fd);
-
-  if (NULL == handle) return -1;
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;

[noelallen1, 2013/06/07 21:48:43]

Should a non EOK set errno, so that we don't need to think about this?
Or is the most recent Error not the right thing?

+    return -1;
+  }
 
   Mount* mount = handle->mount_;
   // Acquire the mount to ensure FreeFD doesn't prematurely destroy it.
   mount->Acquire();
 
   // FreeFD will release the handle/mount held by this fd.
   FreeFD(fd);
 
   // If this handle is the last reference to its node, releasing it will close
   // the node.
   ReleaseHandle(handle);
 
   // Finally, release the mount.
   mount->Release();
 
   return 0;
 }
 
 int KernelProxy::dup(int oldfd) {

[noelallen1, 2013/06/07 21:48:43]

Same and below.

-  KernelHandle* handle = AcquireHandle(oldfd);
-  if (NULL == handle) return -1;
+  KernelHandle* handle;
+  Error error = AcquireHandle(oldfd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
 
   int newfd = AllocateFD(handle);
   ReleaseHandle(handle);
 
   return newfd;
 }
 
 int KernelProxy::dup2(int oldfd, int newfd) {
   // If it's the same file handle, just return
-  if (oldfd == newfd) return newfd;
+  if (oldfd == newfd)
+    return newfd;
 
-  KernelHandle* old_handle = AcquireHandle(oldfd);
-  if (NULL == old_handle) return -1;
+  KernelHandle* old_handle;
+  Error error = AcquireHandle(oldfd, &old_handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
 
   FreeAndReassignFD(newfd, old_handle);
   ReleaseHandle(old_handle);
   return newfd;
 }
 
-
 char* KernelProxy::getcwd(char* buf, size_t size) {
   AutoLock lock(&process_lock_);
   if (size <= 0) {
     errno = EINVAL;
     return NULL;
   }
   // If size is 0, allocate as much as we need.
   if (size == 0) {
     size = cwd_.size() + 1;
   }
@@ skipping 14 matching lines @@
 }
 
 char* KernelProxy::getwd(char* buf) {
   if (NULL == buf) {
     errno = EFAULT;
     return NULL;
   }
   return getcwd(buf, MAXPATHLEN);
 }
 
-int KernelProxy::chmod(const char *path, mode_t mode) {
+int KernelProxy::chmod(const char* path, mode_t mode) {
   int fd = KernelProxy::open(path, O_RDWR);
-  if (-1 == fd) return -1;
+  if (-1 == fd)
+    return -1;
 
-  int ret = fchmod(fd, mode);
+  int result = fchmod(fd, mode);
   close(fd);
-  return ret;
+  return result;
 }
 
-int KernelProxy::mkdir(const char *path, mode_t mode) {
+int KernelProxy::mkdir(const char* path, mode_t mode) {
+  Mount* mnt;
   Path rel;
-  Mount* mnt = AcquireMountAndPath(path, &rel);
-  if (mnt == NULL) return -1;
+  Error error = AcquireMountAndPath(path, &mnt, &rel);
+  if (error) {
+    errno = error;
+    return -1;
+  }
 
-  int val = mnt->Mkdir(rel, mode);
+  int result = 0;
+  error = mnt->Mkdir(rel, mode);
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
   ReleaseMount(mnt);
-  return val;
+  return result;
 }
 
-int KernelProxy::rmdir(const char *path) {
+int KernelProxy::rmdir(const char* path) {
+  Mount* mnt;
   Path rel;
-  Mount* mnt = AcquireMountAndPath(path, &rel);
-  if (mnt == NULL) return -1;
+  Error error = AcquireMountAndPath(path, &mnt, &rel);
+  if (error) {
+    errno = error;
+    return -1;
+  }
 
-  int val = mnt->Rmdir(rel);
+  int result = 0;
+  error = mnt->Rmdir(rel);
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
   ReleaseMount(mnt);
-  return val;
+  return result;
 }
 
-int KernelProxy::stat(const char *path, struct stat *buf) {
+int KernelProxy::stat(const char* path, struct stat* buf) {
   int fd = open(path, O_RDONLY);
-  if (-1 == fd) return -1;
+  if (-1 == fd)
+    return -1;
 
-  int ret = fstat(fd, buf);
+  int result = fstat(fd, buf);
   close(fd);
-  return ret;
+  return result;
 }
 
 int KernelProxy::chdir(const char* path) {
   struct stat statbuf;
   if (stat(path, &statbuf) == -1)
     return -1;
 
   bool is_dir = (statbuf.st_mode & S_IFDIR) != 0;
-  if (is_dir) {
-    AutoLock lock(&process_lock_);
-    cwd_ = GetAbsPathLocked(path).Join();
-    return 0;
+  if (!is_dir) {
+    errno = ENOTDIR;
+    return -1;
   }
 
-  errno = ENOTDIR;
-  return -1;
+  AutoLock lock(&process_lock_);
+  cwd_ = GetAbsPathLocked(path).Join();
+  return 0;
 }
 
-int KernelProxy::mount(const char *source, const char *target,
-                       const char *filesystemtype, unsigned long mountflags,
-                       const void *data) {
+int KernelProxy::mount(const char* source,
+                       const char* target,
+                       const char* filesystemtype,
+                       unsigned long mountflags,
+                       const void* data) {
   // See if it's already mounted
   std::string abs_targ;
 
   // Scope this lock to prevent holding both process and kernel locks
   {
     AutoLock lock(&process_lock_);
     abs_targ = GetAbsPathLocked(target).Join();
   }
 
   AutoLock lock(&kernel_lock_);
   if (mounts_.find(abs_targ) != mounts_.end()) {
     errno = EBUSY;

[noelallen1, 2013/06/07 21:48:43]

Error(EBUSY) first?

     return -1;
   }
 
   // Find a factory of that type
   MountFactoryMap_t::iterator factory = factories_.find(filesystemtype);
   if (factory == factories_.end()) {
     errno = ENODEV;
     return -1;
   }
 
   StringMap_t smap;
   smap["SOURCE"] = source;
   smap["TARGET"] = abs_targ;
 
   if (data) {
-    char* str = strdup(static_cast<const char *>(data));
-    char* ptr = strtok(str,",");
+    char* str = strdup(static_cast<const char*>(data));
+    char* ptr = strtok(str, ",");
     char* val;
     while (ptr != NULL) {
       val = strchr(ptr, '=');
       if (val) {
         *val = 0;
         smap[ptr] = val + 1;
       } else {
         smap[ptr] = "TRUE";
       }
       ptr = strtok(NULL, ",");
     }
     free(str);
   }
 
-  Mount* mnt = factory->second(dev_++, smap, ppapi_);
-  if (mnt) {
-    mounts_[abs_targ] = mnt;
-    return 0;
+  Mount* mnt = NULL;
+  Error error = factory->second(dev_++, smap, ppapi_, &mnt);
+  if (error) {
+    errno = error;
+    return -1;
   }

[Sam Clegg, 2013/05/31 18:40:29]

Repetition like this makes me want to use a HANDLE_ERROR macro...

[binji, 2013/06/03 17:16:46]

Agreed, though the if logic depends on what needs to be cleaned up for now. I'd
like to add a scoped kernel handle and scoped mount so cleanup is automatic. But
I think that makes sense for another CL.

[noelallen1, 2013/06/07 21:48:43]

You mean?
#define  CHECK_ERRNO_RETURN(r, expr)
{
  Error error( expr );
  if (error) {
    errno = error;
    return r;
  }
}
 


On 2013/06/03 17:16:46, binji wrote:

[binji, 2013/06/07 23:23:11]

Yes.

-  errno = EINVAL;
-  return -1;
+
+  mounts_[abs_targ] = mnt;
+  return 0;
 }
 
-int KernelProxy::umount(const char *path) {
+int KernelProxy::umount(const char* path) {
   Path abs_path;
 
   // Scope this lock to prevent holding both process and kernel locks
   {
     AutoLock lock(&process_lock_);
     abs_path = GetAbsPathLocked(path);
   }
 
   AutoLock lock(&kernel_lock_);
   MountMap_t::iterator it = mounts_.find(abs_path.Join());
 
   if (mounts_.end() == it) {
     errno = EINVAL;

[noelallen1, 2013/06/07 21:48:43]

Error(EINVAL)?  Same below.

     return -1;
   }
 
   if (it->second->RefCount() != 1) {
     errno = EBUSY;
     return -1;
   }
 
   it->second->Release();
   mounts_.erase(it);
   return 0;
 }
 
-ssize_t KernelProxy::read(int fd, void *buf, size_t nbytes) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-
-  AutoLock lock(&handle->lock_);
-  ssize_t cnt = handle->node_->Read(handle->offs_, buf, nbytes);
-  if (cnt > 0) handle->offs_ += cnt;
+ssize_t KernelProxy::read(int fd, void* buf, size_t nbytes) {
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  AutoLock lock(&handle->lock_);
+  ssize_t cnt = 0;
+  error = handle->node_->Read(handle->offs_, buf, nbytes, &cnt);
+  if (error)
+    errno = error;
+
+  if (cnt > 0)
+    handle->offs_ += cnt;
 
   ReleaseHandle(handle);
   return cnt;
 }
 
-ssize_t KernelProxy::write(int fd, const void *buf, size_t nbytes) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-
-  AutoLock lock(&handle->lock_);
-  ssize_t cnt = handle->node_->Write(handle->offs_, buf, nbytes);
-  if (cnt > 0) handle->offs_ += cnt;
+ssize_t KernelProxy::write(int fd, const void* buf, size_t nbytes) {
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  AutoLock lock(&handle->lock_);
+  ssize_t cnt = 0;
+  error = handle->node_->Write(handle->offs_, buf, nbytes, &cnt);
+  if (error)
+    errno = error;
+
+  if (cnt > 0)
+    handle->offs_ += cnt;
 
   ReleaseHandle(handle);
   return cnt;
 }
 
 int KernelProxy::fstat(int fd, struct stat* buf) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-
-  int ret = handle->node_->GetStat(buf);
-  ReleaseHandle(handle);
-  return ret;
+  KernelHandle* handle;

[noelallen1, 2013/06/07 21:48:43]

   ScopedHandle handle;
   CHECK_ERRNO_RET(-1, AcquireHandle(fd, handle) );
   CHECK_ERRNO_RET(-1, handle->node_->GetStat(buf));
   return 0;
}

[binji, 2013/06/07 23:23:11]

Yep, future CL. :)

+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  int result = 0;
+  error = handle->node_->GetStat(buf);
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
+  ReleaseHandle(handle);
+  return result;
 }
 
 int KernelProxy::getdents(int fd, void* buf, unsigned int count) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-
-  AutoLock lock(&handle->lock_);
-  int cnt = handle->node_->GetDents(handle->offs_,
-      static_cast<dirent *>(buf), count);
-
-  if (cnt > 0) handle->offs_ += cnt;
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  AutoLock lock(&handle->lock_);
+  int cnt = 0;
+  error = handle->node_
+      ->GetDents(handle->offs_, static_cast<dirent*>(buf), count, &cnt);
+  if (error)
+    errno = error;
+
+  if (cnt > 0)
+    handle->offs_ += cnt;
 
   ReleaseHandle(handle);
   return cnt;
 }
 
 int KernelProxy::ftruncate(int fd, off_t length) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-  int ret = handle->node_->FTruncate(length);
-
-  ReleaseHandle(handle);
-  return ret;
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  int result = 0;
+  error = handle->node_->FTruncate(length);

[noelallen1, 2013/06/07 21:48:43]

Should fill with 0 happen here?  Is there a way to make sure every virtual
FTruncate does the correct thing when growing file.   Maybe just add a comment
about this expectation in mountnode.h

[binji, 2013/06/07 23:23:11]

Done.

+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
+  ReleaseHandle(handle);
+  return result;
 }
 
 int KernelProxy::fsync(int fd) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-  int ret = handle->node_->FSync();
-
-  ReleaseHandle(handle);
-  return ret;
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  int result = 0;
+  error = handle->node_->FSync();
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
+  ReleaseHandle(handle);
+  return result;
 }
 
 int KernelProxy::isatty(int fd) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-  int ret = handle->node_->IsaTTY();
-
-  ReleaseHandle(handle);
-  return ret;
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  int result = 0;
+  error = handle->node_->IsaTTY();
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
+  ReleaseHandle(handle);
+  return result;
 }
 
 off_t KernelProxy::lseek(int fd, off_t offset, int whence) {
-  KernelHandle* handle = AcquireHandle(fd);
-
-  // check if fd is valid and handle exists
-  if (NULL == handle) return -1;
-
-  AutoLock lock(&handle->lock_);
-  int ret = handle->Seek(offset, whence);
-
-  ReleaseHandle(handle);
-  return ret;
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  AutoLock lock(&handle->lock_);
+  off_t new_offset;
+  error = handle->Seek(offset, whence, &new_offset);
+  if (error) {
+    errno = error;
+    new_offset = -1;
+  }
+
+  ReleaseHandle(handle);
+  return new_offset;
 }
 
 int KernelProxy::unlink(const char* path) {
+  Mount* mnt;
   Path rel;
-  Mount* mnt = AcquireMountAndPath(path, &rel);
-  if (mnt == NULL) return -1;
-
-  int val = mnt->Unlink(rel);
+  Error error = AcquireMountAndPath(path, &mnt, &rel);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  int result = 0;
+  error = mnt->Unlink(rel);
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
   ReleaseMount(mnt);
-  return val;
+  return result;
 }
 
 int KernelProxy::remove(const char* path) {
+  Mount* mnt;
   Path rel;
-  Mount* mnt = AcquireMountAndPath(path, &rel);
-  if (mnt == NULL) return -1;
-
-  int val = mnt->Remove(rel);
+  Error error = AcquireMountAndPath(path, &mnt, &rel);
+  if (error) {
+    errno = error;
+    return -1;
+  }
+
+  int result = 0;
+  error = mnt->Remove(rel);
+  if (error) {
+    errno = error;
+    result = -1;
+  }
+
   ReleaseMount(mnt);
-  return val;
+  return result;
 }
 
 // TODO(noelallen): Needs implementation.
 int KernelProxy::fchmod(int fd, int mode) {
   errno = EINVAL;
   return -1;
 }
 
 int KernelProxy::access(const char* path, int amode) {
   errno = EINVAL;
   return -1;
 }
 
 int KernelProxy::link(const char* oldpath, const char* newpath) {
   errno = EINVAL;
   return -1;
 }
 
 int KernelProxy::symlink(const char* oldpath, const char* newpath) {
   errno = EINVAL;
   return -1;
 }
 
-void* KernelProxy::mmap(void* addr, size_t length, int prot, int flags, int fd,
+void* KernelProxy::mmap(void* addr,
+                        size_t length,
+                        int prot,
+                        int flags,
+                        int fd,
                         size_t offset) {
   // We shouldn't be getting anonymous mmaps here.
   assert((flags & MAP_ANONYMOUS) == 0);
   assert(fd != -1);
 
-  KernelHandle* handle = AcquireHandle(fd);
-
-  if (NULL == handle)
+  KernelHandle* handle;
+  Error error = AcquireHandle(fd, &handle);
+  if (error) {
+    errno = error;
     return MAP_FAILED;
+  }
 
   void* new_addr;
   {
     AutoLock lock(&handle->lock_);
-    new_addr = handle->node_->MMap(addr, length, prot, flags, offset);
-    if (new_addr == MAP_FAILED) {
+    error = handle->node_->MMap(addr, length, prot, flags, offset, &new_addr);
+    if (error) {
+      errno = error;
       ReleaseHandle(handle);
       return MAP_FAILED;
     }
   }
 
   ReleaseHandle(handle);
   return new_addr;
 }
 
 int KernelProxy::munmap(void* addr, size_t length) {
@@ skipping 25 matching lines @@
   // ReleaseHandle below which takes the process lock. This is safe as long as
   // this is never executed from free() -- we can be reasonably sure this is
   // true, because malloc only makes anonymous mmap() requests, and should only
   // be munmapping those allocations. We never add to mmap_info_list_ for
   // anonymous maps, so the unmap_list should always be empty when called from
   // free().
   return 0;
 }
 
 int KernelProxy::open_resource(const char* path) {
+  Mount* mnt;
   Path rel;
+  Error error = AcquireMountAndPath(path, &mnt, &rel);
+  if (error) {
+    errno = error;
+    return -1;
+  }
 
-  Mount* mnt = AcquireMountAndPath(path, &rel);
-  if (mnt == NULL) return -1;
-
-  MountNode* node = mnt->OpenResource(rel);
-  if (node == NULL) {
-    node = mnt->Open(rel, O_RDONLY);
-    if (node == NULL) {
+  MountNode* node = NULL;
+  error = mnt->OpenResource(rel, &node);
+  if (error) {
+    // OpenResource failed, try Open().
+    error = mnt->Open(rel, O_RDONLY, &node);
+    if (error) {
+      errno = error;
       ReleaseMount(mnt);
       return -1;
     }
   }
 
-  // OpenResource failed, try Open().
+  KernelHandle* handle = new KernelHandle(mnt, node);
+  error = handle->Init(O_RDONLY);
+  if (error) {
+    errno = error;
+    ReleaseMount(mnt);
+    return -1;
+  }
 
-  KernelHandle* handle = new KernelHandle(mnt, node, O_RDONLY);
   int fd = AllocateFD(handle);
   mnt->AcquireNode(node);
 
   ReleaseHandle(handle);
   ReleaseMount(mnt);
 
   return fd;
 }