refactor subprocess to make it easier for windows port

Rather than tracking stdout/stderr explicitly, just keep an opaque
pointer to a platform-specific 'stream' type.  Also provide API
to get at the process output.

diff --git a/src/build.cc b/src/build.cc
index f6b9b64..03c692c 100644 (file)
--- a/src/build.cc
+++ b/src/build.cc
@@ -319,9 +319,8 @@ Edge* RealCommandRunner::NextFinishedCommand(bool* success) {
   Edge* edge = i->second;
   subproc_to_edge_.erase(i);
 
-  if (!*success ||
-      !subproc->stdout_.buf_.empty() ||
-      !subproc->stderr_.buf_.empty()) {
+  string output = subproc->GetOutput();
+  if (!*success || !output.empty()) {
     // Print the command that is spewing before printing its output.
     // Print the full command when it failed, otherwise the short name if
     // available.
@@ -333,10 +332,8 @@ Edge* RealCommandRunner::NextFinishedCommand(bool* success) {
     }
 
     printf("\n%s%s\n", *success ? "" : "FAILED: ", to_print.c_str());
-    if (!subproc->stdout_.buf_.empty())
-      printf("%s\n", subproc->stdout_.buf_.c_str());
-    if (!subproc->stderr_.buf_.empty())
-      printf("%s\n", subproc->stderr_.buf_.c_str());
+    if (!output.empty())
+      printf("%s\n", output.c_str());
   }
 
   delete subproc;

diff --git a/src/subprocess.cc b/src/subprocess.cc
index a4957ad..597fcac 100644 (file)
--- a/src/subprocess.cc
+++ b/src/subprocess.cc
@@ -27,40 +27,45 @@
 
 #include "util.h"
 
+struct Subprocess::Stream {
+  Stream();
+  ~Stream();
+  string buf_;
+
+  int fd_;
+};
+
 Subprocess::Stream::Stream() : fd_(-1) {}
 Subprocess::Stream::~Stream() {
   if (fd_ >= 0)
     close(fd_);
 }
 
-Subprocess::Subprocess() : pid_(-1) {}
+Subprocess::Subprocess() : pid_(-1) {
+  stream_ = new Stream;
+}
 Subprocess::~Subprocess() {
   // Reap child if forgotten.
   if (pid_ != -1)
     Finish();
+  delete stream_;
 }
 
 bool Subprocess::Start(const string& command) {
-  int stdout_pipe[2];
-  if (pipe(stdout_pipe) < 0)
-    Fatal("pipe: %s", strerror(errno));
-  stdout_.fd_ = stdout_pipe[0];
-
-  int stderr_pipe[2];
-  if (pipe(stderr_pipe) < 0)
+  int output_pipe[2];
+  if (pipe(output_pipe) < 0)
     Fatal("pipe: %s", strerror(errno));
-  stderr_.fd_ = stderr_pipe[0];
+  stream_->fd_ = output_pipe[0];
 
   pid_ = fork();
   if (pid_ < 0)
     Fatal("fork: %s", strerror(errno));
 
   if (pid_ == 0) {
-    close(stdout_pipe[0]);
-    close(stderr_pipe[0]);
+    close(output_pipe[0]);
 
     // Track which fd we use to report errors on.
-    int error_pipe = stderr_pipe[1];
+    int error_pipe = output_pipe[1];
     do {
       // Open /dev/null over stdin.
       int devnull = open("/dev/null", O_WRONLY);
@@ -70,14 +75,13 @@ bool Subprocess::Start(const string& command) {
         break;
       close(devnull);
 
-      if (dup2(stdout_pipe[1], 1) < 0 ||
-          dup2(stderr_pipe[1], 2) < 0)
+      if (dup2(output_pipe[1], 1) < 0 ||
+          dup2(output_pipe[1], 2) < 0)
         break;
 
       // Now can use stderr for errors.
       error_pipe = 2;
-      close(stdout_pipe[1]);
-      close(stderr_pipe[1]);
+      close(output_pipe[1]);
 
       execl("/bin/sh", "/bin/sh", "-c", command.c_str(), NULL);
     } while (false);
@@ -90,22 +94,20 @@ bool Subprocess::Start(const string& command) {
     _exit(1);
   }
 
-  close(stdout_pipe[1]);
-  close(stderr_pipe[1]);
+  close(output_pipe[1]);
   return true;
 }
 
-void Subprocess::OnFDReady(int fd) {
+void Subprocess::OnFDReady() {
   char buf[4 << 10];
-  ssize_t len = read(fd, buf, sizeof(buf));
-  Stream* stream = fd == stdout_.fd_ ? &stdout_ : &stderr_;
+  ssize_t len = read(stream_->fd_, buf, sizeof(buf));
   if (len > 0) {
-    stream->buf_.append(buf, len);
+    stream_->buf_.append(buf, len);
   } else {
     if (len < 0)
       Fatal("read: %s", strerror(errno));
-    close(stream->fd_);
-    stream->fd_ = -1;
+    close(stream_->fd_);
+    stream_->fd_ = -1;
   }
 }
 
@@ -124,6 +126,14 @@ bool Subprocess::Finish() {
   return false;
 }
 
+bool Subprocess::Done() const {
+  return stream_->fd_ == -1;
+}
+
+const string& Subprocess::GetOutput() const {
+  return stream_->buf_;
+}
+
 void SubprocessSet::Add(Subprocess* subprocess) {
   running_.push_back(subprocess);
 }
@@ -134,16 +144,7 @@ void SubprocessSet::DoWork() {
   map<int, Subprocess*> fd_to_subprocess;
   for (vector<Subprocess*>::iterator i = running_.begin();
        i != running_.end(); ++i) {
-    int fd = (*i)->stdout_.fd_;
-    if (fd >= 0) {
-      fd_to_subprocess[fd] = *i;
-      fds.resize(fds.size() + 1);
-      pollfd* newfd = &fds.back();
-      newfd->fd = fd;
-      newfd->events = POLLIN;
-      newfd->revents = 0;
-    }
-    fd = (*i)->stderr_.fd_;
+    int fd = (*i)->stream_->fd_;
     if (fd >= 0) {
       fd_to_subprocess[fd] = *i;
       fds.resize(fds.size() + 1);
@@ -164,13 +165,11 @@ void SubprocessSet::DoWork() {
   for (size_t i = 0; i < fds.size(); ++i) {
     if (fds[i].revents) {
       Subprocess* subproc = fd_to_subprocess[fds[i].fd];
-      if (fds[i].revents) {
-        subproc->OnFDReady(fds[i].fd);
-        if (subproc->done()) {
-          finished_.push(subproc);
-          std::remove(running_.begin(), running_.end(), subproc);
-          running_.resize(running_.size() - 1);
-        }
+      subproc->OnFDReady();
+      if (subproc->Done()) {
+        finished_.push(subproc);
+        std::remove(running_.begin(), running_.end(), subproc);
+        running_.resize(running_.size() - 1);
       }
     }
   }

diff --git a/src/subprocess.h b/src/subprocess.h
index a1edb0d..048bacb 100644 (file)
--- a/src/subprocess.h
+++ b/src/subprocess.h
@@ -28,21 +28,16 @@ struct Subprocess {
   Subprocess();
   ~Subprocess();
   bool Start(const string& command);
-  void OnFDReady(int fd);
+  void OnFDReady();
   /// Returns true on successful process exit.
   bool Finish();
 
-  bool done() const {
-    return stdout_.fd_ == -1 && stderr_.fd_ == -1;
-  }
-
-  struct Stream {
-    Stream();
-    ~Stream();
-    int fd_;
-    string buf_;
-  };
-  Stream stdout_, stderr_;
+  bool Done() const;
+
+  const string& GetOutput() const;
+
+  struct Stream;
+  Stream* stream_;
   pid_t pid_;
 };
 

diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc
index 4eb878b..5ee3eb7 100644 (file)
--- a/src/subprocess_test.cc
+++ b/src/subprocess_test.cc
@@ -21,11 +21,10 @@ TEST(Subprocess, Ls) {
   EXPECT_TRUE(ls.Start("ls /"));
 
   // Pretend we discovered that stdout was ready for writing.
-  ls.OnFDReady(ls.stdout_.fd_);
+  ls.OnFDReady();
 
   EXPECT_TRUE(ls.Finish());
-  EXPECT_NE("", ls.stdout_.buf_);
-  EXPECT_EQ("", ls.stderr_.buf_);
+  EXPECT_NE("", ls.GetOutput());
 }
 
 TEST(Subprocess, BadCommand) {
@@ -33,11 +32,10 @@ TEST(Subprocess, BadCommand) {
   EXPECT_TRUE(subproc.Start("ninja_no_such_command"));
 
   // Pretend we discovered that stderr was ready for writing.
-  subproc.OnFDReady(subproc.stderr_.fd_);
+  subproc.OnFDReady();
 
   EXPECT_FALSE(subproc.Finish());
-  EXPECT_EQ("", subproc.stdout_.buf_);
-  EXPECT_NE("", subproc.stderr_.buf_);
+  EXPECT_NE("", subproc.GetOutput());
 }
 
 TEST(SubprocessSet, Single) {
@@ -46,11 +44,11 @@ TEST(SubprocessSet, Single) {
   EXPECT_TRUE(ls->Start("ls /"));
   subprocs.Add(ls);
 
-  while (!ls->done()) {
+  while (!ls->Done()) {
     subprocs.DoWork();
   }
   ASSERT_TRUE(ls->Finish());
-  ASSERT_NE("", ls->stdout_.buf_);
+  ASSERT_NE("", ls->GetOutput());
 
   ASSERT_EQ(1, subprocs.finished_.size());
 }
@@ -72,13 +70,12 @@ TEST(SubprocessSet, Multi) {
 
   ASSERT_EQ(3, subprocs.running_.size());
   for (int i = 0; i < 3; ++i) {
-    ASSERT_FALSE(processes[i]->done());
-    ASSERT_EQ("", processes[i]->stdout_.buf_);
-    ASSERT_EQ("", processes[i]->stderr_.buf_);
+    ASSERT_FALSE(processes[i]->Done());
+    ASSERT_EQ("", processes[i]->GetOutput());
   }
 
-  while (!processes[0]->done() || !processes[1]->done() ||
-         !processes[2]->done()) {
+  while (!processes[0]->Done() || !processes[1]->Done() ||
+         !processes[2]->Done()) {
     ASSERT_GT(subprocs.running_.size(), 0);
     subprocs.DoWork();
   }
@@ -88,8 +85,7 @@ TEST(SubprocessSet, Multi) {
 
   for (int i = 0; i < 3; ++i) {
     ASSERT_TRUE(processes[i]->Finish());
-    ASSERT_NE("", processes[i]->stdout_.buf_);
-    ASSERT_EQ("", processes[i]->stderr_.buf_);
+    ASSERT_NE("", processes[i]->GetOutput());
     delete processes[i];
   }
 }