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

native_client_sdk/src/libraries/nacl_io/kernel_object.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_object.h"
 
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <pthread.h>
@@ skipping 13 matching lines @@
   pthread_mutex_init(&process_lock_, NULL);
 }
 
 KernelObject::~KernelObject() {
   pthread_mutex_destroy(&process_lock_);
   pthread_mutex_destroy(&kernel_lock_);
 }
 
 // Uses longest prefix to find the mount for the give path, then
 // acquires the mount and returns it with a relative path.
-Mount* KernelObject::AcquireMountAndPath(const std::string& relpath,
-                                         Path* out_path) {
+int KernelObject::AcquireMountAndPath(const std::string& relpath,
+                                      Mount** out_mount,
+                                      Path* out_path) {
+  *out_mount = NULL;
+  *out_path = Path();
+
   Path abs_path;
   {
     AutoLock lock(&process_lock_);
     abs_path = GetAbsPathLocked(relpath);
   }
 
   AutoLock lock(&kernel_lock_);
   Mount* mount = NULL;
 
-
   // Find longest prefix
   size_t max = abs_path.Size();
   for (size_t len = 0; len < abs_path.Size(); len++) {
     MountMap_t::iterator it = mounts_.find(abs_path.Range(0, max - len));
     if (it != mounts_.end()) {
       out_path->Set("/");
       out_path->Append(abs_path.Range(max - len, max));
       mount = it->second;
       break;
     }
   }
 
-  if (NULL == mount) {
-    errno = ENOTDIR;
-    return NULL;
-  }
+  if (NULL == mount)
+    return ENOTDIR;
 
   // Acquire the mount while we hold the proxy lock

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

if (*out_mount) {

Should we support only computing the path?  Otherwise EINVAL? for and invalid
param?

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

As per our discussion, non-NULL is assumed and documented in the header.

   mount->Acquire();
-  return mount;
+  *out_mount = mount;
+  return 0;

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

Error(EOK)

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

Will fix in next CL.

 }
 
 void KernelObject::ReleaseMount(Mount* mnt) {
   AutoLock lock(&kernel_lock_);
   mnt->Release();
 }
 
-KernelHandle* KernelObject::AcquireHandle(int fd) {
+int KernelObject::AcquireHandle(int fd, KernelHandle** out_handle) {
+  *out_handle = NULL;
+

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

Same.   EINVAL?

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

Done.

   AutoLock lock(&process_lock_);
-  if (fd < 0 || fd >= static_cast<int>(handle_map_.size())) {
-    errno = EBADF;
-    return NULL;
-  }
+  if (fd < 0 || fd >= static_cast<int>(handle_map_.size()))
+    return EBADF;
 
   KernelHandle* handle = handle_map_[fd];
-  if (NULL == handle) {
-    errno = EBADF;
-    return NULL;
-  }
+  if (NULL == handle)
+    return EBADF;
 
   // Ref count while holding parent mutex
   handle->Acquire();
 
   lock.Unlock();
-  if (handle->node_) handle->mount_->AcquireNode(handle->node_);
+  if (handle->node_)
+    handle->mount_->AcquireNode(handle->node_);
 
-  return handle;
+  *out_handle = handle;
+  return 0;

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

Error(EOK)

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

Done.

 }
 
 void KernelObject::ReleaseHandle(KernelHandle* handle) {
   // The handle must already be held before taking the
   // kernel lock.
-  if (handle->node_) handle->mount_->ReleaseNode(handle->node_);
+  if (handle->node_)
+    handle->mount_->ReleaseNode(handle->node_);
 
   AutoLock lock(&process_lock_);
   handle->Release();
 }
 
-// Helper function to properly sort FD order in the heap, forcing
-// lower numbered FD to be available first.
-static bool FdOrder(int i, int j) {
-  return i > j;
-}
-
 int KernelObject::AllocateFD(KernelHandle* handle) {
   AutoLock lock(&process_lock_);
   int id;
 
   // Acquire the handle and its mount since we are about to track it with
   // this FD.
   handle->Acquire();
   handle->mount_->Acquire();
 
   // If we can recycle and FD, use that first
   if (free_fds_.size()) {
     id = free_fds_.front();
-    std::pop_heap(free_fds_.begin(), free_fds_.end(), FdOrder);
+    // Force lower numbered FD to be available first.

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

I don't think this comment or code is correct.
I believe the behavior we are looking for is the last FD freed, should be the
first one available, not the lowest numbered one.

So:
  close(0)
  close(1)
  open("/dev/stdout")
should in theory set STDOUT to /dev/stdout assuming no race.

+    std::pop_heap(free_fds_.begin(), free_fds_.end(), std::greater<int>());
     free_fds_.pop_back();
     handle_map_[id] = handle;
   } else {
     id = handle_map_.size();
     handle_map_.push_back(handle);
   }
   return id;
 }
 
 void KernelObject::FreeAndReassignFD(int fd, KernelHandle* handle) {
@@ skipping 26 matching lines @@
   AutoLock lock(&process_lock_);
 
   // Release the mount and handle since we no longer
   // track them with this FD.
   KernelHandle* handle = handle_map_[fd];
   handle->Release();
   handle->mount_->Release();
 
   handle_map_[fd] = NULL;
   free_fds_.push_back(fd);
-  std::push_heap(free_fds_.begin(), free_fds_.end(), FdOrder);
+  // Force lower numbered FD to be available first.
+  std::push_heap(free_fds_.begin(), free_fds_.end(), std::greater<int>());
 }
 
 Path KernelObject::GetAbsPathLocked(const std::string& path) {
   // Generate absolute path
   Path abs_path(cwd_);
   if (path[0] == '/') {
     abs_path = path;
   } else {
     abs_path = cwd_;
     abs_path.Append(path);
   }
 
   return abs_path;
 }