[fuchsia] Replace use of epoll with mx_ primitives in process_fuchsia

runtime/bin/process_fuchsia.cc

Index: runtime/bin/process_fuchsia.cc
diff --git a/runtime/bin/process_fuchsia.cc b/runtime/bin/process_fuchsia.cc
index 14f3faa85e1682c9590941623bb21fc45ec7ce8a..9d55e33dc8120cbb17e71e2d07bcf4aa4ace00f7 100644
--- a/runtime/bin/process_fuchsia.cc
+++ b/runtime/bin/process_fuchsia.cc
@@ -17,6 +17,8 @@
 #include <magenta/status.h>
 #include <magenta/syscalls.h>
 #include <magenta/syscalls/object.h>
+#include <magenta/types.h>
+#include <mxio/private.h>
 #include <mxio/util.h>
 #include <pthread.h>
 #include <stdbool.h>
@@ -77,7 +79,6 @@ class ProcessInfo {
   DISALLOW_COPY_AND_ASSIGN(ProcessInfo);
 };
 
-
 // Singly-linked list of ProcessInfo objects for all active processes
 // started from Dart.
 class ProcessInfoList {
@@ -425,12 +426,10 @@ void Process::TerminateExitCodeHandler() {
   ExitCodeHandler::Terminate();
 }
 
-

[zra, 2017/05/26 16:13:22]

Dart VM style is two newlines between top-level functions in .cc files.

[jamesr1, 2017/05/27 00:14:09]

Turns out I wasn't reading your .clang-format file, fixed!

 intptr_t Process::CurrentProcessId() {
   return static_cast<intptr_t>(getpid());
 }
 
-
 int64_t Process::CurrentRSS() {
   mx_info_task_stats_t task_stats;
   mx_handle_t process = mx_process_self();
@@ -444,27 +443,56 @@ int64_t Process::CurrentRSS() {
   return task_stats.mem_committed_bytes;
 }
 
-
 int64_t Process::MaxRSS() {
   // There is currently no way to get the high watermark value on Fuchsia, so
   // just return the current RSS value.
   return CurrentRSS();
 }
 
-
 static bool ProcessWaitCleanup(intptr_t out,
                                intptr_t err,
-                               intptr_t exit_event,
-                               intptr_t epoll_fd) {
+                               intptr_t exit_event) {
   int e = errno;
   VOID_NO_RETRY_EXPECTED(close(out));
   VOID_NO_RETRY_EXPECTED(close(err));
   VOID_NO_RETRY_EXPECTED(close(exit_event));
-  VOID_NO_RETRY_EXPECTED(close(epoll_fd));
   errno = e;
   return false;
 }
 
+class MxioWaitEntry {
+ public:
+  explicit MxioWaitEntry(int fd) : mxio_(__mxio_fd_to_io(fd)) {}
+  ~MxioWaitEntry() { Cancel(); }
+
+  void WaitBegin(mx_wait_item_t* wait_item) {
+    if (mxio_ == nullptr) {

[zra, 2017/05/26 16:13:22]

To be consistent with the rest of the VM, please use NULL instead of nullptr.

[jamesr1, 2017/05/27 00:14:10]

done

+      *wait_item = {};
+      return;
+    }
+
+    __mxio_wait_begin(mxio_, EPOLLRDHUP | EPOLLIN, &wait_item->handle,
+                      &wait_item->waitfor);
+    wait_item->pending = 0;
+  }
+
+  void WaitEnd(mx_wait_item_t* wait_item, uint32_t* event) {
+    if (mxio_ == nullptr) {
+      *event = 0;
+      return;
+    }
+    __mxio_wait_end(mxio_, wait_item->pending, event);
+  }
+
+  void Cancel() {
+    if (mxio_)

[zra, 2017/05/26 16:13:22]

if (mxio_ != NULL)

Also, please use curly braces around single line if.

[jamesr1, 2017/05/27 00:14:10]

done

+      __mxio_release(mxio_);
+    mxio_ = nullptr;
+  }
+
+ private:
+  mxio_t* mxio_;
+};

[zra, 2017/05/26 16:13:22]

Maybe DISALLOW_COPY_AND_ASSIGN(MxioWaitEntry) or DISALLOW_ALLOCATION().

[jamesr1, 2017/05/27 00:14:10]

done

 
 bool Process::Wait(intptr_t pid,
                    intptr_t in,
@@ -484,83 +512,58 @@ bool Process::Wait(intptr_t pid,
     int32_t ints[2];
   } exit_code_data;
 
-  // The initial size passed to epoll_create is ignore on newer (>=
-  // 2.6.8) Linux versions
-  static const int kEpollInitialSize = 64;
-  int epoll_fd = NO_RETRY_EXPECTED(epoll_create(kEpollInitialSize));
-  if (epoll_fd == -1) {
-    return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-  }
-  if (!FDUtils::SetCloseOnExec(epoll_fd)) {
-    return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-  }
+  uint32_t events[3];
+  mx_wait_item_t wait_items[3];

[zra, 2017/05/26 16:13:22]

const intptr_t kWaitItemsCount = 3;

[jamesr1, 2017/05/27 00:14:10]

done

+  size_t active = 3;
 
-  struct epoll_event event;
-  event.events = EPOLLRDHUP | EPOLLIN;
-  event.data.fd = out;
-  int status = NO_RETRY_EXPECTED(
-      epoll_ctl(epoll_fd, EPOLL_CTL_ADD, out, &event));
-  if (status == -1) {
-    return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-  }
-  event.data.fd = err;
-  status = NO_RETRY_EXPECTED(
-      epoll_ctl(epoll_fd, EPOLL_CTL_ADD, err, &event));
-  if (status == -1) {
-    return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-  }
-  event.data.fd = exit_event;
-  status = NO_RETRY_EXPECTED(
-      epoll_ctl(epoll_fd, EPOLL_CTL_ADD, exit_event, &event));
-  if (status == -1) {
-    return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-  }
-  intptr_t active = 3;
+  MxioWaitEntry entries[3] = {
+      MxioWaitEntry(out), MxioWaitEntry(err), MxioWaitEntry(exit_event),
+  };
 
-  static const intptr_t kMaxEvents = 16;
-  struct epoll_event events[kMaxEvents];
   while (active > 0) {
     // TODO(US-109): When the epoll implementation is properly edge-triggered,
     // remove this sleep, which prevents the message queue from being
     // overwhelmed and leading to memory exhaustion.
     usleep(5000);

[zra, 2017/05/26 16:13:22]

This was copy-pasted from the eventhandler, but probably wasn't needed here in
the first place since the read()s from the ready fds were happening on the same
thread. This is safe to remove.

[jamesr1, 2017/05/27 00:14:10]

done

-    intptr_t result = NO_RETRY_EXPECTED(
-        epoll_wait(epoll_fd, events, kMaxEvents, -1));
-    if ((result < 0) && (errno != EWOULDBLOCK)) {
-      return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-    }
-    for (intptr_t i = 0; i < result; i++) {
-      if ((events[i].events & EPOLLIN) != 0) {
-        const intptr_t avail = FDUtils::AvailableBytes(events[i].data.fd);
-        if (events[i].data.fd == out) {
-          if (!out_data.Read(out, avail)) {
-            return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-          }
-        } else if (events[i].data.fd == err) {
-          if (!err_data.Read(err, avail)) {
-            return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-          }
-        } else if (events[i].data.fd == exit_event) {
-          if (avail == 8) {
-            intptr_t b =
-                NO_RETRY_EXPECTED(read(exit_event, exit_code_data.bytes, 8));
-            if (b != 8) {
-              return ProcessWaitCleanup(out, err, exit_event, epoll_fd);
-            }
-          }
-        } else {
-          UNREACHABLE();
+
+    for (size_t i = 0; i < 3; ++i) {
+      entries[i].WaitBegin(&wait_items[i]);
+    }
+    mx_object_wait_many(wait_items, 3, MX_TIME_INFINITE);
+
+    for (size_t i = 0; i < 3; ++i) {
+      entries[i].WaitEnd(&wait_items[i], &events[i]);
+    }
+
+    if ((events[0] & EPOLLIN) != 0) {
+      const intptr_t avail = FDUtils::AvailableBytes(out);
+      if (!out_data.Read(out, avail)) {
+        return ProcessWaitCleanup(out, err, exit_event);
+      }
+    }
+    if ((events[1] & EPOLLIN) != 0) {
+      const intptr_t avail = FDUtils::AvailableBytes(err);
+      if (!err_data.Read(err, avail)) {
+        return ProcessWaitCleanup(err, err, exit_event);
+      }
+    }
+    if ((events[2] & EPOLLIN) != 0) {
+      const intptr_t avail = FDUtils::AvailableBytes(exit_event);
+      if (avail == 8) {
+        intptr_t b =
+            NO_RETRY_EXPECTED(read(exit_event, exit_code_data.bytes, 8));
+        if (b != 8) {
+          return ProcessWaitCleanup(out, err, exit_event);
         }
       }
-      if ((events[i].events & (EPOLLHUP | EPOLLRDHUP)) != 0) {
-        NO_RETRY_EXPECTED(close(events[i].data.fd));
+    }
+    for (size_t i = 0; i < 3; ++i) {
+      if ((events[i] & EPOLLRDHUP) != 0) {
         active--;
-        VOID_NO_RETRY_EXPECTED(
-            epoll_ctl(epoll_fd, EPOLL_CTL_DEL, events[i].data.fd, NULL));
+        entries[i].Cancel();
       }
     }
   }
-  VOID_NO_RETRY_EXPECTED(close(epoll_fd));
 
   // All handles closed and all data read.
   result->set_stdout_data(out_data.GetData());
@@ -582,7 +585,6 @@ bool Process::Wait(intptr_t pid,
   return true;
 }
 
-
 bool Process::Kill(intptr_t id, int signal) {
   LOG_INFO("Sending signal %d to process with id %ld\n", signal, id);
   // mx_task_kill is definitely going to kill the process.
@@ -609,7 +611,6 @@ bool Process::Kill(intptr_t id, int signal) {
   return true;
 }
 
-
 class ProcessStarter {
  public:
   ProcessStarter(const char* path,
@@ -735,14 +736,14 @@ class ProcessStarter {
   }
 
  private:
-#define CHECK_FOR_ERROR(status, msg)                                           \
-  if (status < 0) {                                                            \
-    const intptr_t kMaxMessageSize = 256;                                      \
-    char* message = DartUtils::ScopedCString(kMaxMessageSize);                 \
-    snprintf(message, kMaxMessageSize, "%s:%d: %s: %s\n", __FILE__, __LINE__,  \
-             msg, mx_status_get_string(status));                               \
-    *os_error_message_ = message;                                              \
-    return status;                                                             \
+#define CHECK_FOR_ERROR(status, msg)                                          \

[zra, 2017/05/26 16:13:22]

It looks like the escaped newlines were already in the right place.

+  if (status < 0) {                                                           \
+    const intptr_t kMaxMessageSize = 256;                                     \
+    char* message = DartUtils::ScopedCString(kMaxMessageSize);                \
+    snprintf(message, kMaxMessageSize, "%s:%d: %s: %s\n", __FILE__, __LINE__, \
+             msg, mx_status_get_string(status));                              \
+    *os_error_message_ = message;                                             \
+    return status;                                                            \
   }
 
   mx_status_t SetupLaunchpad(launchpad_t** launchpad) {
@@ -802,7 +803,6 @@ class ProcessStarter {
   DISALLOW_IMPLICIT_CONSTRUCTORS(ProcessStarter);
 };
 
-
 int Process::Start(const char* path,
                    char* arguments[],
                    intptr_t arguments_length,
@@ -827,15 +827,12 @@ int Process::Start(const char* path,
   return starter.Start();
 }
 
-
 intptr_t Process::SetSignalHandler(intptr_t signal) {
   errno = ENOSYS;
   return -1;
 }
 
-
-void Process::ClearSignalHandler(intptr_t signal) {
-}
+void Process::ClearSignalHandler(intptr_t signal) {}
 
 }  // namespace bin
 }  // namespace dart