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

native_client_sdk/src/libraries/nacl_io/kernel_object.cc

Index: native_client_sdk/src/libraries/nacl_io/kernel_object.cc
diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_object.cc b/native_client_sdk/src/libraries/nacl_io/kernel_object.cc
index 5c265309cc1b8a8560f6f2eeabf8ec1ec48ca809..850b39bf2bd64377280134cbd605405078fab5be 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_object.cc
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_object.cc
@@ -31,8 +31,12 @@ KernelObject::~KernelObject() {
 
 // 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_);
@@ -42,7 +46,6 @@ Mount* KernelObject::AcquireMountAndPath(const std::string& 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++) {
@@ -55,14 +58,13 @@ Mount* KernelObject::AcquireMountAndPath(const std::string& relpath,
     }
   }
 
-  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) {
@@ -70,43 +72,38 @@ void KernelObject::ReleaseMount(Mount* mnt) {
   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;
@@ -119,7 +116,8 @@ int KernelObject::AllocateFD(KernelHandle* handle) {
   // 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 {
@@ -166,7 +164,8 @@ void KernelObject::FreeFD(int fd) {
 
   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) {