Add support for capturing stdout/err in Application

This also makes it so that the Application object can poll instead of
just relying on SIGCHLD to watch for application stops, to make it a bit
cleaner for simple use-cases.

Change-Id: I8af71e1dd89e0cfa1b189ba1e5264df0df9b9560
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
diff --git a/aos/starter/BUILD b/aos/starter/BUILD
index f998a93..e6dad81 100644
--- a/aos/starter/BUILD
+++ b/aos/starter/BUILD
@@ -72,6 +72,24 @@
 )
 
 cc_test(
+    name = "subprocess_test",
+    srcs = ["subprocess_test.cc"],
+    data = [
+        "//aos/events:pingpong_config",
+    ],
+    # The roborio compiler doesn't support <filesystem>.
+    target_compatible_with =
+        ["@platforms//os:linux"],
+    deps = [
+        ":subprocess",
+        "//aos/events:shm_event_loop",
+        "//aos/testing:googletest",
+        "//aos/testing:path",
+        "//aos/testing:tmpdir",
+    ],
+)
+
+cc_test(
     name = "starter_test",
     srcs = ["starter_test.cc"],
     data = [
diff --git a/aos/starter/starterd_lib.cc b/aos/starter/starterd_lib.cc
index 03b40c8..008c46f 100644
--- a/aos/starter/starterd_lib.cc
+++ b/aos/starter/starterd_lib.cc
@@ -169,9 +169,11 @@
       applications_.try_emplace(application->name()->str(), application,
                                 &event_loop_, [this]() { MaybeSendStatus(); });
   if (success) {
-    if (application->has_args()) {
-      iter->second.set_args(*application->args());
-    }
+    // We should be catching and handling SIGCHLD correctly in the starter, so
+    // don't leave in the crutch for polling for the child process status (this
+    // is less about efficiency, and more about making sure bit rot doesn't
+    // result in the signal handling breaking).
+    iter->second.DisableChildDeathPolling();
     return &(iter->second);
   }
   return nullptr;
diff --git a/aos/starter/subprocess.cc b/aos/starter/subprocess.cc
index e68f604..c1eb618 100644
--- a/aos/starter/subprocess.cc
+++ b/aos/starter/subprocess.cc
@@ -34,21 +34,12 @@
 
 SignalListener::~SignalListener() { loop_->epoll()->DeleteFd(signalfd_.fd()); }
 
-Application::Application(const aos::Application *application,
+Application::Application(std::string_view name,
+                         std::string_view executable_name,
                          aos::EventLoop *event_loop,
                          std::function<void()> on_change)
-    : name_(application->name()->string_view()),
-      path_(application->has_executable_name()
-                ? application->executable_name()->string_view()
-                : application->name()->string_view()),
-      args_(1),
-      user_name_(application->has_user() ? application->user()->str() : ""),
-      user_(application->has_user() ? FindUid(user_name_.c_str())
-                                    : std::nullopt),
-      group_(application->has_user() ? FindPrimaryGidForUser(user_name_.c_str())
-                                     : std::nullopt),
-      autostart_(application->autostart()),
-      autorestart_(application->autorestart()),
+    : name_(name),
+      path_(executable_name),
       event_loop_(event_loop),
       start_timer_(event_loop_->AddTimer([this] {
         status_ = aos::starter::State::RUNNING;
@@ -61,7 +52,37 @@
                        << "' pid: " << pid_;
         }
       })),
-      on_change_(on_change) {}
+      pipe_timer_(event_loop_->AddTimer([this]() { FetchOutputs(); })),
+      child_status_handler_(
+          event_loop_->AddTimer([this]() { MaybeHandleSignal(); })),
+      on_change_(on_change) {
+  event_loop_->OnRun([this]() {
+    // Every second poll to check if the child is dead. This is used as a
+    // default for the case where the user is not directly catching SIGCHLD and
+    // calling MaybeHandleSignal for us.
+    child_status_handler_->Setup(event_loop_->monotonic_now(),
+                                 std::chrono::seconds(1));
+  });
+}
+
+Application::Application(const aos::Application *application,
+                         aos::EventLoop *event_loop,
+                         std::function<void()> on_change)
+    : Application(application->name()->string_view(),
+                  application->has_executable_name()
+                      ? application->executable_name()->string_view()
+                      : application->name()->string_view(),
+                  event_loop, on_change) {
+  user_name_ = application->has_user() ? application->user()->str() : "";
+  user_ = application->has_user() ? FindUid(user_name_.c_str()) : std::nullopt;
+  group_ = application->has_user() ? FindPrimaryGidForUser(user_name_.c_str())
+                                   : std::nullopt;
+  autostart_ = application->autostart();
+  autorestart_ = application->autorestart();
+  if (application->has_args()) {
+    set_args(*application->args());
+  }
+}
 
 void Application::DoStart() {
   if (status_ != aos::starter::State::WAITING) {
@@ -71,7 +92,19 @@
   start_timer_->Disable();
   restart_timer_->Disable();
 
-  std::tie(read_pipe_, write_pipe_) = util::ScopedPipe::MakePipe();
+  status_pipes_ = util::ScopedPipe::MakePipe();
+
+  if (capture_stdout_) {
+    stdout_pipes_ = util::ScopedPipe::MakePipe();
+    stdout_.clear();
+  }
+  if (capture_stderr_) {
+    stderr_pipes_ = util::ScopedPipe::MakePipe();
+    stderr_.clear();
+  }
+
+  pipe_timer_->Setup(event_loop_->monotonic_now(),
+                     std::chrono::milliseconds(100));
 
   const pid_t pid = fork();
 
@@ -91,11 +124,23 @@
       // alive in 1 second.
       start_timer_->Setup(event_loop_->monotonic_now() +
                           std::chrono::seconds(1));
+      // Since we are the parent process, clear our write-side of all the pipes.
+      status_pipes_.write.reset();
+      stdout_pipes_.write.reset();
+      stderr_pipes_.write.reset();
     }
     on_change_();
     return;
   }
 
+  // Since we are the child process, clear our read-side of all the pipes.
+  status_pipes_.read.reset();
+  stdout_pipes_.read.reset();
+  stderr_pipes_.read.reset();
+
+  // The status pipe will not be needed if the execve succeeds.
+  status_pipes_.write->SetCloexec();
+
   // Clear out signal mask of parent so forked process receives all signals
   // normally.
   sigset_t empty_mask;
@@ -104,7 +149,7 @@
 
   // Cleanup children if starter dies in a way that is not handled gracefully.
   if (prctl(PR_SET_PDEATHSIG, SIGKILL) == -1) {
-    write_pipe_.Write(
+    status_pipes_.write->Write(
         static_cast<uint32_t>(aos::starter::LastStopReason::SET_PRCTL_ERR));
     PLOG(FATAL) << "Could not set PR_SET_PDEATHSIG to SIGKILL";
   }
@@ -116,19 +161,19 @@
     // be set so we change this effective UID back later.
     CHECK(user_);
     if (seteuid(0) == -1) {
-      write_pipe_.Write(
+      status_pipes_.write->Write(
           static_cast<uint32_t>(aos::starter::LastStopReason::SET_GRP_ERR));
       PLOG(FATAL) << "Could not seteuid(0) for " << name_
                   << " in preparation for setting groups";
     }
     if (initgroups(user_name_.c_str(), *group_) == -1) {
-      write_pipe_.Write(
+      status_pipes_.write->Write(
           static_cast<uint32_t>(aos::starter::LastStopReason::SET_GRP_ERR));
       PLOG(FATAL) << "Could not initialize normal groups for " << name_
                   << " as " << user_name_ << " with " << *group_;
     }
     if (setgid(*group_) == -1) {
-      write_pipe_.Write(
+      status_pipes_.write->Write(
           static_cast<uint32_t>(aos::starter::LastStopReason::SET_GRP_ERR));
       PLOG(FATAL) << "Could not set group for " << name_ << " to " << *group_;
     }
@@ -136,31 +181,65 @@
 
   if (user_) {
     if (setuid(*user_) == -1) {
-      write_pipe_.Write(
+      status_pipes_.write->Write(
           static_cast<uint32_t>(aos::starter::LastStopReason::SET_USR_ERR));
       PLOG(FATAL) << "Could not set user for " << name_ << " to " << *user_;
     }
   }
 
-  // argv[0] should be the program name
-  args_.insert(args_.begin(), path_.data());
+  if (capture_stdout_) {
+    PCHECK(STDOUT_FILENO == dup2(stdout_pipes_.write->fd(), STDOUT_FILENO));
+    stdout_pipes_.write.reset();
+  }
 
-  execvp(path_.c_str(), args_.data());
+  if (capture_stderr_) {
+    PCHECK(STDERR_FILENO == dup2(stderr_pipes_.write->fd(), STDERR_FILENO));
+    stderr_pipes_.write.reset();
+  }
+
+  // argv[0] should be the program name
+  args_.insert(args_.begin(), path_);
+
+  std::vector<char *> cargs = CArgs();
+  execvp(path_.c_str(), cargs.data());
 
   // If we got here, something went wrong
-  write_pipe_.Write(
+  status_pipes_.write->Write(
       static_cast<uint32_t>(aos::starter::LastStopReason::EXECV_ERR));
   PLOG(WARNING) << "Could not execute " << name_ << " (" << path_ << ')';
 
   _exit(EXIT_FAILURE);
 }
 
+void Application::FetchOutputs() {
+  if (capture_stdout_) {
+    stdout_pipes_.read->Read(&stdout_);
+  }
+  if (capture_stderr_) {
+    stderr_pipes_.read->Read(&stderr_);
+  }
+}
+
+const std::string &Application::GetStdout() {
+  CHECK(capture_stdout_);
+  FetchOutputs();
+  return stdout_;
+}
+
+const std::string &Application::GetStderr() {
+  CHECK(capture_stderr_);
+  FetchOutputs();
+  return stderr_;
+}
+
 void Application::DoStop(bool restart) {
   // If stop or restart received, the old state of these is no longer applicable
   // so cancel both.
   restart_timer_->Disable();
   start_timer_->Disable();
 
+  FetchOutputs();
+
   switch (status_) {
     case aos::starter::State::STARTING:
     case aos::starter::State::RUNNING: {
@@ -217,14 +296,31 @@
   on_change_();
 }
 
+std::vector<char *> Application::CArgs() {
+  std::vector<char *> cargs;
+  std::transform(args_.begin(), args_.end(), std::back_inserter(cargs),
+                 [](std::string &str) { return str.data(); });
+  cargs.push_back(nullptr);
+  return cargs;
+}
+
 void Application::set_args(
     const flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>> &v) {
   args_.clear();
   std::transform(v.begin(), v.end(), std::back_inserter(args_),
-                 [](const flatbuffers::String *str) {
-                   return const_cast<char *>(str->c_str());
-                 });
-  args_.push_back(nullptr);
+                 [](const flatbuffers::String *str) { return str->str(); });
+}
+
+void Application::set_args(std::vector<std::string> args) {
+  args_ = std::move(args);
+}
+
+void Application::set_capture_stdout(bool capture) {
+  capture_stdout_ = capture;
+}
+
+void Application::set_capture_stderr(bool capture) {
+  capture_stderr_ = capture;
 }
 
 std::optional<uid_t> Application::FindUid(const char *name) {
@@ -257,7 +353,9 @@
   aos::starter::ApplicationStatus::Builder status_builder(*builder);
   status_builder.add_name(name_fbs);
   status_builder.add_state(status_);
-  status_builder.add_last_exit_code(exit_code_);
+  if (exit_code_.has_value()) {
+    status_builder.add_last_exit_code(exit_code_.value());
+  }
   status_builder.add_last_stop_reason(stop_reason_);
   if (pid_ != -1) {
     status_builder.add_pid(pid_);
@@ -326,38 +424,51 @@
     return false;
   }
 
+  start_timer_->Disable();
   exit_time_ = event_loop_->monotonic_now();
   exit_code_ = WIFEXITED(status) ? WEXITSTATUS(status) : WTERMSIG(status);
 
-  if (auto read_result = read_pipe_.Read()) {
+  if (auto read_result = status_pipes_.read->Read()) {
     stop_reason_ = static_cast<aos::starter::LastStopReason>(*read_result);
   }
 
   switch (status_) {
     case aos::starter::State::STARTING: {
-      LOG(WARNING) << "Failed to start '" << name_ << "' on pid " << pid_
-                   << " : Exited with status " << exit_code_;
+      if (exit_code_.value() == 0) {
+        LOG(INFO) << "Application '" << name_ << "' pid " << pid_
+                  << " exited with status " << exit_code_.value();
+      } else {
+        LOG(WARNING) << "Failed to start '" << name_ << "' on pid " << pid_
+                     << " : Exited with status " << exit_code_.value();
+      }
       if (autorestart()) {
         QueueStart();
+      } else {
+        status_ = aos::starter::State::STOPPED;
+        on_change_();
       }
       break;
     }
     case aos::starter::State::RUNNING: {
-      if (exit_code_ == 0) {
+      if (exit_code_.value() == 0) {
         LOG(INFO) << "Application '" << name_ << "' pid " << pid_
-                  << " exited with status " << exit_code_;
+                  << " exited with status " << exit_code_.value();
       } else {
         LOG(WARNING) << "Application '" << name_ << "' pid " << pid_
-                     << " exited unexpectedly with status " << exit_code_;
+                     << " exited unexpectedly with status "
+                     << exit_code_.value();
       }
       if (autorestart()) {
         QueueStart();
+      } else {
+        status_ = aos::starter::State::STOPPED;
+        on_change_();
       }
       break;
     }
     case aos::starter::State::STOPPING: {
       LOG(INFO) << "Successfully stopped '" << name_ << "' pid: " << pid_
-                << " with status " << exit_code_;
+                << " with status " << exit_code_.value();
       status_ = aos::starter::State::STOPPED;
 
       // Disable force stop timer since the process already died
diff --git a/aos/starter/subprocess.h b/aos/starter/subprocess.h
index c1d7c87..9ee9e31 100644
--- a/aos/starter/subprocess.h
+++ b/aos/starter/subprocess.h
@@ -1,7 +1,9 @@
 #ifndef AOS_STARTER_SUBPROCESS_H_
 #define AOS_STARTER_SUBPROCESS_H_
 
+#include <memory>
 #include <string>
+#include <tuple>
 #include <vector>
 
 #include "aos/events/event_loop.h"
@@ -36,11 +38,15 @@
 // automatically.
 class Application {
  public:
-  Application(const aos::Application *application,
+  Application(const aos::Application *application, aos::EventLoop *event_loop,
+              std::function<void()> on_change);
+
+  Application(std::string_view name, std::string_view executable_name,
               aos::EventLoop *event_loop, std::function<void()> on_change);
 
   flatbuffers::Offset<aos::starter::ApplicationStatus> PopulateStatus(
       flatbuffers::FlatBufferBuilder *builder);
+  aos::starter::State status() const { return status_; };
 
   // Returns the last pid of this process. -1 if not started yet.
   pid_t get_pid() const { return pid_; }
@@ -49,6 +55,7 @@
   // process was not the target. Returns true if this Application should be
   // removed.
   bool MaybeHandleSignal();
+  void DisableChildDeathPolling() { child_status_handler_->Disable(); }
 
   // Handles a command. May do nothing if application is already in the desired
   // state.
@@ -60,15 +67,24 @@
 
   void Terminate();
 
-  void set_args(
-      const flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>>
-          &args);
+  void set_args(std::vector<std::string> args);
+  void set_capture_stdout(bool capture);
+  void set_capture_stderr(bool capture);
 
   bool autostart() const { return autostart_; }
 
   bool autorestart() const { return autorestart_; }
 
+  const std::string &GetStdout();
+  const std::string &GetStderr();
+  std::optional<int> exit_code() const { return exit_code_; }
+
  private:
+  typedef aos::util::ScopedPipe::PipePair PipePair;
+  void set_args(
+      const flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>>
+          &args);
+
   void DoStart();
 
   void DoStop(bool restart);
@@ -82,33 +98,48 @@
   static std::optional<uid_t> FindUid(const char *name);
   static std::optional<gid_t> FindPrimaryGidForUser(const char *name);
 
+  void FetchOutputs();
+
+  // Provides an std::vector of the args (such that CArgs().data() ends up being
+  // suitable to pass to execve()).
+  // The points are invalidated when args_ changes (e.g., due to a set_args
+  // call).
+  std::vector<char *> CArgs();
+
   // Next unique id for all applications
   static inline uint64_t next_id_ = 0;
 
   std::string name_;
   std::string path_;
-  std::vector<char *> args_;
+  std::vector<std::string> args_;
   std::string user_name_;
   std::optional<uid_t> user_;
   std::optional<gid_t> group_;
 
+  bool capture_stdout_ = false;
+  PipePair stdout_pipes_;
+  std::string stdout_;
+  bool capture_stderr_ = false;
+  PipePair stderr_pipes_;
+  std::string stderr_;
+
   pid_t pid_ = -1;
-  util::ScopedPipe::ScopedReadPipe read_pipe_;
-  util::ScopedPipe::ScopedWritePipe write_pipe_;
-  uint64_t id_;
-  int exit_code_ = 0;
+  PipePair status_pipes_;
+  uint64_t id_ = 0;
+  std::optional<int> exit_code_;
   aos::monotonic_clock::time_point start_time_, exit_time_;
   bool queue_restart_ = false;
   bool terminating_ = false;
-  bool autostart_ = true;
-  bool autorestart_ = true;
+  bool autostart_ = false;
+  bool autorestart_ = false;
 
   aos::starter::State status_ = aos::starter::State::STOPPED;
   aos::starter::LastStopReason stop_reason_ =
       aos::starter::LastStopReason::STOP_REQUESTED;
 
   aos::EventLoop *event_loop_;
-  aos::TimerHandler *start_timer_, *restart_timer_, *stop_timer_;
+  aos::TimerHandler *start_timer_, *restart_timer_, *stop_timer_, *pipe_timer_,
+      *child_status_handler_;
 
   std::function<void()> on_change_;
 
@@ -116,4 +147,4 @@
 };
 
 }  // namespace aos::starter
-#endif // AOS_STARTER_SUBPROCESS_H_
+#endif  // AOS_STARTER_SUBPROCESS_H_
diff --git a/aos/starter/subprocess_test.cc b/aos/starter/subprocess_test.cc
new file mode 100644
index 0000000..93fbf6a
--- /dev/null
+++ b/aos/starter/subprocess_test.cc
@@ -0,0 +1,97 @@
+#include "aos/starter/subprocess.h"
+
+#include "aos/events/shm_event_loop.h"
+#include "aos/testing/path.h"
+#include "aos/testing/tmpdir.h"
+#include "aos/util/file.h"
+#include "gtest/gtest.h"
+
+namespace aos::starter::testing {
+
+class SubprocessTest : public ::testing::Test {
+ protected:
+  SubprocessTest() : shm_dir_(aos::testing::TestTmpDir() + "/aos") {
+    FLAGS_shm_base = shm_dir_;
+
+    // Nuke the shm dir:
+    aos::util::UnlinkRecursive(shm_dir_);
+  }
+
+  gflags::FlagSaver flag_saver_;
+  std::string shm_dir_;
+};
+
+TEST_F(SubprocessTest, CaptureOutputs) {
+  const std::string config_file =
+      ::aos::testing::ArtifactPath("aos/events/pingpong_config.json");
+
+  aos::FlatbufferDetachedBuffer<aos::Configuration> config =
+      aos::configuration::ReadConfig(config_file);
+  aos::ShmEventLoop event_loop(&config.message());
+  bool observed_stopped = false;
+  Application echo_stdout(
+      "echo", "echo", &event_loop, [&observed_stopped, &echo_stdout]() {
+        if (echo_stdout.status() == aos::starter::State::STOPPED) {
+          observed_stopped = true;
+        }
+      });
+  ASSERT_FALSE(echo_stdout.autorestart());
+  echo_stdout.set_args({"abcdef"});
+  echo_stdout.set_capture_stdout(true);
+  echo_stdout.set_capture_stderr(true);
+
+  echo_stdout.Start();
+  aos::TimerHandler *exit_timer =
+      event_loop.AddTimer([&event_loop]() { event_loop.Exit(); });
+  event_loop.OnRun([&event_loop, exit_timer]() {
+    exit_timer->Setup(event_loop.monotonic_now() + std::chrono::seconds(1));
+  });
+
+  event_loop.Run();
+
+  ASSERT_EQ("abcdef\n", echo_stdout.GetStdout());
+  ASSERT_TRUE(echo_stdout.GetStderr().empty());
+  EXPECT_TRUE(observed_stopped);
+  EXPECT_EQ(aos::starter::State::STOPPED, echo_stdout.status());
+
+  observed_stopped = false;
+
+  // Run again, the output should've been cleared.
+  echo_stdout.set_args({"ghijkl"});
+  echo_stdout.Start();
+  event_loop.Run();
+  ASSERT_EQ("ghijkl\n", echo_stdout.GetStdout());
+  EXPECT_TRUE(observed_stopped);
+}
+
+TEST_F(SubprocessTest, CaptureStderr) {
+  const std::string config_file =
+      ::aos::testing::ArtifactPath("aos/events/pingpong_config.json");
+
+  aos::FlatbufferDetachedBuffer<aos::Configuration> config =
+      aos::configuration::ReadConfig(config_file);
+  aos::ShmEventLoop event_loop(&config.message());
+  bool observed_stopped = false;
+  Application echo_stderr(
+      "echo", "sh", &event_loop, [&observed_stopped, &echo_stderr]() {
+        if (echo_stderr.status() == aos::starter::State::STOPPED) {
+          observed_stopped = true;
+        }
+      });
+  echo_stderr.set_args({"-c", "echo abcdef >&2"});
+  echo_stderr.set_capture_stdout(true);
+  echo_stderr.set_capture_stderr(true);
+
+  echo_stderr.Start();
+  event_loop.AddTimer([&event_loop]() { event_loop.Exit(); })
+      ->Setup(event_loop.monotonic_now() + std::chrono::seconds(1));
+
+  event_loop.Run();
+
+  ASSERT_EQ("abcdef\n", echo_stderr.GetStderr());
+  ASSERT_TRUE(echo_stderr.GetStdout().empty());
+  ASSERT_TRUE(observed_stopped);
+  ASSERT_EQ(aos::starter::State::STOPPED, echo_stderr.status());
+}
+
+}  // namespace aos::starter::testing
diff --git a/aos/util/scoped_pipe.cc b/aos/util/scoped_pipe.cc
index 36bc1b3..d677b07 100644
--- a/aos/util/scoped_pipe.cc
+++ b/aos/util/scoped_pipe.cc
@@ -26,13 +26,21 @@
   return *this;
 }
 
-std::tuple<ScopedPipe::ScopedReadPipe, ScopedPipe::ScopedWritePipe>
-ScopedPipe::MakePipe() {
+ScopedPipe::PipePair ScopedPipe::MakePipe() {
   int fds[2];
   PCHECK(pipe(fds) != -1);
   PCHECK(fcntl(fds[0], F_SETFL, fcntl(fds[0], F_GETFL) | O_NONBLOCK) != -1);
   PCHECK(fcntl(fds[1], F_SETFL, fcntl(fds[1], F_GETFL) | O_NONBLOCK) != -1);
-  return {ScopedReadPipe(fds[0]), ScopedWritePipe(fds[1])};
+  return {std::unique_ptr<ScopedReadPipe>(new ScopedReadPipe(fds[0])),
+          std::unique_ptr<ScopedWritePipe>(new ScopedWritePipe(fds[1]))};
+}
+
+void ScopedPipe::SetCloexec() {
+  // FD_CLOEXEC is the only known file descriptor flag, but call GETFD just in
+  // case.
+  int flags = fcntl(fd(), F_GETFD);
+  PCHECK(flags != -1);
+  PCHECK(fcntl(fd(), F_SETFD, flags | FD_CLOEXEC) != -1);
 }
 
 size_t ScopedPipe::ScopedReadPipe::Read(std::string *buffer) {
diff --git a/aos/util/scoped_pipe.h b/aos/util/scoped_pipe.h
index 6716fb5..fb91e02 100644
--- a/aos/util/scoped_pipe.h
+++ b/aos/util/scoped_pipe.h
@@ -3,8 +3,8 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <optional>
-#include <tuple>
 
 #include "absl/types/span.h"
 
@@ -16,11 +16,18 @@
   class ScopedReadPipe;
   class ScopedWritePipe;
 
-  static std::tuple<ScopedReadPipe, ScopedWritePipe> MakePipe();
+  struct PipePair {
+    std::unique_ptr<ScopedReadPipe> read;
+    std::unique_ptr<ScopedWritePipe> write;
+  };
+
+  static PipePair MakePipe();
 
   virtual ~ScopedPipe();
 
   int fd() const { return fd_; }
+  // Sets FD_CLOEXEC on the file descriptor.
+  void SetCloexec();
 
  private:
   ScopedPipe(int fd = -1);
diff --git a/aos/util/scoped_pipe_test.cc b/aos/util/scoped_pipe_test.cc
index 183688e..c71e272 100644
--- a/aos/util/scoped_pipe_test.cc
+++ b/aos/util/scoped_pipe_test.cc
@@ -1,5 +1,7 @@
 #include "aos/util/scoped_pipe.h"
 
+#include <fcntl.h>
+
 #include <array>
 #include <string>
 
@@ -11,39 +13,45 @@
 
 // Tests using uint32_t read/write methods on the ScopedPipe objects.
 TEST(ScopedPipeTest, IntegerPipe) {
-  std::tuple<ScopedPipe::ScopedReadPipe, ScopedPipe::ScopedWritePipe>
-      pipe = ScopedPipe::MakePipe();
-  ASSERT_FALSE(std::get<0>(pipe).Read().has_value())
+  ScopedPipe::PipePair pipe = ScopedPipe::MakePipe();
+  ASSERT_FALSE(pipe.read->Read().has_value())
       << "Shouldn't get anything on empty read.";
-  std::get<1>(pipe).Write(971);
-  ASSERT_EQ(971, std::get<0>(pipe).Read().value());
+  pipe.write->Write(971);
+  ASSERT_EQ(971, pipe.read->Read().value());
 }
 
 // Tests using string read/write methods on the ScopedPipe objects.
 TEST(ScopedPipeTest, StringPipe) {
-  std::tuple<ScopedPipe::ScopedReadPipe, ScopedPipe::ScopedWritePipe>
-      pipe = ScopedPipe::MakePipe();
+  ScopedPipe::PipePair pipe = ScopedPipe::MakePipe();
   std::string buffer;
-  ASSERT_EQ(0u, std::get<0>(pipe).Read(&buffer))
+  ASSERT_EQ(0u, pipe.read->Read(&buffer))
       << "Shouldn't get anything on empty read.";
   ASSERT_TRUE(buffer.empty());
 
   const char *const kAbc = "abcdef";
-  std::get<1>(pipe).Write(
+  pipe.write->Write(
       absl::Span<const uint8_t>(reinterpret_cast<const uint8_t *>(kAbc), 6));
-  ASSERT_EQ(6u, std::get<0>(pipe).Read(&buffer));
+  ASSERT_EQ(6u, pipe.read->Read(&buffer));
   ASSERT_EQ("abcdef", buffer);
 
   std::array<uint8_t, 10000> large_buffer;
   large_buffer.fill(99);
-  std::get<1>(pipe).Write(
+  pipe.write->Write(
       absl::Span<const uint8_t>(large_buffer.data(), large_buffer.size()));
-  ASSERT_EQ(large_buffer.size(), std::get<0>(pipe).Read(&buffer));
+  ASSERT_EQ(large_buffer.size(), pipe.read->Read(&buffer));
   for (size_t ii = 0; ii < large_buffer.size(); ++ii) {
     ASSERT_EQ(large_buffer[ii], buffer[ii + 6]);
   }
 }
 
+// Tests that calling SetCloexec succeeds and does indeed set FD_CLOEXEC.
+TEST(ScopedPipeTest, SetCloexec) {
+  ScopedPipe::PipePair pipe = ScopedPipe::MakePipe();
+  ASSERT_EQ(0, fcntl(pipe.read->fd(), F_GETFD) & FD_CLOEXEC);
+  pipe.read->SetCloexec();
+  ASSERT_NE(0, fcntl(pipe.read->fd(), F_GETFD) & FD_CLOEXEC);
+}
+
 }  // namespace testing
 }  // namespace util
 }  // namespace aos