Extract RunWithManagedStdio from subprocess.h

https://github.com/google/android-cuttlefish/pull/1432/files#r2226344612 points out that many callers have the same logic around calling the interface. As a first step, extracting to a separate file makes it easier to focus on modifying this interface.

Bug: b/433812518

Author: Databean

base/cvd/cuttlefish/common/libs/utils/BUILD.bazel

@@ -20,6 +20,7 @@ cf_cc_library(
     deps = [
         "//cuttlefish/common/libs/utils:result",
         "//cuttlefish/common/libs/utils:subprocess",
+        "//cuttlefish/common/libs/utils:subprocess_managed_stdio",
         "//libbase",
     ],
 )
@@ -74,6 +75,7 @@ cf_cc_library(
     deps = [
         "//cuttlefish/common/libs/utils:result",
         "//cuttlefish/common/libs/utils:subprocess",
+        "//cuttlefish/common/libs/utils:subprocess_managed_stdio",
         "//libbase",
     ],
 )
@@ -204,6 +206,7 @@ cf_cc_library(
         "//cuttlefish/common/libs/utils:files",
         "//cuttlefish/common/libs/utils:result",
         "//cuttlefish/common/libs/utils:subprocess",
+        "//cuttlefish/common/libs/utils:subprocess_managed_stdio",
         "//libbase",
         "@fmt",
     ],
@@ -341,6 +344,17 @@ cf_cc_library(
     ],
 )
 
+cf_cc_library(
+    name = "subprocess_managed_stdio",
+    srcs = ["subprocess_managed_stdio.cc"],
+    hdrs = ["subprocess_managed_stdio.h"],
+    deps = [
+        "//cuttlefish/common/libs/fs",
+        "//cuttlefish/common/libs/utils:subprocess",
+        "//libbase",
+    ],
+)
+
 cf_cc_library(
     name = "tcp_socket",
     srcs = ["tcp_socket.cpp"],
@@ -445,5 +459,6 @@ cf_cc_library(
         "//cuttlefish/common/libs/utils:files",
         "//cuttlefish/common/libs/utils:result",
         "//cuttlefish/common/libs/utils:subprocess",
+        "//cuttlefish/common/libs/utils:subprocess_managed_stdio",
     ],
 )

base/cvd/cuttlefish/common/libs/utils/archive.cpp

@@ -27,6 +27,7 @@
 
 #include "cuttlefish/common/libs/utils/result.h"
 #include "cuttlefish/common/libs/utils/subprocess.h"
+#include "cuttlefish/common/libs/utils/subprocess_managed_stdio.h"
 
 namespace cuttlefish {
 namespace {

base/cvd/cuttlefish/common/libs/utils/disk_usage.cpp

@@ -24,6 +24,7 @@
 
 #include "cuttlefish/common/libs/utils/result.h"
 #include "cuttlefish/common/libs/utils/subprocess.h"
+#include "cuttlefish/common/libs/utils/subprocess_managed_stdio.h"
 
 namespace cuttlefish {
 namespace {

base/cvd/cuttlefish/common/libs/utils/network.cpp

@@ -46,6 +46,7 @@
 #include "cuttlefish/common/libs/utils/files.h"
 #include "cuttlefish/common/libs/utils/result.h"
 #include "cuttlefish/common/libs/utils/subprocess.h"
+#include "cuttlefish/common/libs/utils/subprocess_managed_stdio.h"
 
 namespace cuttlefish {
 namespace {

base/cvd/cuttlefish/common/libs/utils/subprocess.cpp

@@ -37,8 +37,6 @@
 #include <set>
 #include <sstream>
 #include <string>
-#include <thread>
-#include <type_traits>
 #include <unordered_map>
 #include <unordered_set>
 #include <utility>
@@ -47,7 +45,6 @@
 #include <android-base/logging.h>
 #include <android-base/strings.h>
 
-#include "cuttlefish/common/libs/fs/shared_buf.h"
 #include "cuttlefish/common/libs/utils/contains.h"
 #include "cuttlefish/common/libs/utils/files.h"
 
@@ -525,109 +522,6 @@ std::string Command::AsBashScript(
   return contents;
 }
 
-// A class that waits for threads to exit in its destructor.
-class ThreadJoiner {
-std::vector<std::thread*> threads_;
-public:
-  ThreadJoiner(const std::vector<std::thread*> threads) : threads_(threads) {}
-  ~ThreadJoiner() {
-    for (auto& thread : threads_) {
-      if (thread->joinable()) {
-        thread->join();
-      }
-    }
-  }
-};
-
-int RunWithManagedStdio(Command&& cmd_tmp, const std::string* stdin_str,
-                        std::string* stdout_str, std::string* stderr_str,
-                        SubprocessOptions options) {
-  /*
-   * The order of these declarations is necessary for safety. If the function
-   * returns at any point, the Command will be destroyed first, closing all
-   * of its references to SharedFDs. This will cause the thread internals to fail
-   * their reads or writes. The ThreadJoiner then waits for the threads to
-   * complete, as running the destructor of an active std::thread crashes the
-   * program.
-   *
-   * C++ scoping rules dictate that objects are descoped in reverse order to
-   * construction, so this behavior is predictable.
-   */
-  std::thread stdin_thread, stdout_thread, stderr_thread;
-  ThreadJoiner thread_joiner({&stdin_thread, &stdout_thread, &stderr_thread});
-  Command cmd = std::move(cmd_tmp);
-  bool io_error = false;
-  if (stdin_str != nullptr) {
-    SharedFD pipe_read, pipe_write;
-    if (!SharedFD::Pipe(&pipe_read, &pipe_write)) {
-      LOG(ERROR) << "Could not create a pipe to write the stdin of \""
-                << cmd.GetShortName() << "\"";
-      return -1;
-    }
-    cmd.RedirectStdIO(Subprocess::StdIOChannel::kStdIn, pipe_read);
-    stdin_thread = std::thread([pipe_write, stdin_str, &io_error]() {
-      int written = WriteAll(pipe_write, *stdin_str);
-      if (written < 0) {
-        io_error = true;
-        LOG(ERROR) << "Error in writing stdin to process";
-      }
-    });
-  }
-  if (stdout_str != nullptr) {
-    SharedFD pipe_read, pipe_write;
-    if (!SharedFD::Pipe(&pipe_read, &pipe_write)) {
-      LOG(ERROR) << "Could not create a pipe to read the stdout of \""
-                << cmd.GetShortName() << "\"";
-      return -1;
-    }
-    cmd.RedirectStdIO(Subprocess::StdIOChannel::kStdOut, pipe_write);
-    stdout_thread = std::thread([pipe_read, stdout_str, &io_error]() {
-      int read = ReadAll(pipe_read, stdout_str);
-      if (read < 0) {
-        io_error = true;
-        LOG(ERROR) << "Error in reading stdout from process";
-      }
-    });
-  }
-  if (stderr_str != nullptr) {
-    SharedFD pipe_read, pipe_write;
-    if (!SharedFD::Pipe(&pipe_read, &pipe_write)) {
-      LOG(ERROR) << "Could not create a pipe to read the stderr of \""
-                << cmd.GetShortName() << "\"";
-      return -1;
-    }
-    cmd.RedirectStdIO(Subprocess::StdIOChannel::kStdErr, pipe_write);
-    stderr_thread = std::thread([pipe_read, stderr_str, &io_error]() {
-      int read = ReadAll(pipe_read, stderr_str);
-      if (read < 0) {
-        io_error = true;
-        LOG(ERROR) << "Error in reading stderr from process";
-      }
-    });
-  }
-
-  auto subprocess = cmd.Start(std::move(options));
-  if (!subprocess.Started()) {
-    return -1;
-  }
-  auto cmd_short_name = cmd.GetShortName();
-  {
-    // Force the destructor to run by moving it into a smaller scope.
-    // This is necessary to close the write end of the pipe.
-    Command forceDelete = std::move(cmd);
-  }
-
-  int code = subprocess.Wait();
-  {
-    auto join_threads = std::move(thread_joiner);
-  }
-  if (io_error) {
-    LOG(ERROR) << "IO error communicating with " << cmd_short_name;
-    return -1;
-  }
-  return code;
-}
-
 namespace {
 
 struct ExtraParam {

base/cvd/cuttlefish/common/libs/utils/subprocess.h

@@ -325,23 +325,6 @@ class Command {
 };
 std::ostream& operator<<(std::ostream& out, const Command& command);
 
-
-/*
- * Consumes a Command and runs it, optionally managing the stdio channels.
- *
- * If `stdin` is set, the subprocess stdin will be pipe providing its contents.
- * If `stdout` is set, the subprocess stdout will be captured and saved to it.
- * If `stderr` is set, the subprocess stderr will be captured and saved to it.
- *
- * If `command` exits normally, the lower 8 bits of the return code will be
- * returned in a value between 0 and 255.
- * If some setup fails, `command` fails to start, or `command` exits due to a
- * signal, the return value will be negative.
- */
-int RunWithManagedStdio(Command&& cmd_tmp, const std::string* stdin,
-                        std::string* stdout, std::string* stderr,
-                        SubprocessOptions options = SubprocessOptions());
-
 /**
  * Returns the exit status on success, negative values on error
  *

base/cvd/cuttlefish/common/libs/utils/subprocess_managed_stdio.cc

@@ -0,0 +1,143 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "cuttlefish/common/libs/utils/subprocess_managed_stdio.h"
+
+#include <cerrno>
+#include <cstring>
+#include <ostream>
+#include <string>
+#include <thread>
+#include <utility>
+#include <vector>
+
+#include <android-base/logging.h>
+#include <android-base/strings.h>
+
+#include "cuttlefish/common/libs/fs/shared_buf.h"
+#include "cuttlefish/common/libs/fs/shared_fd.h"
+#include "cuttlefish/common/libs/utils/subprocess.h"
+
+namespace cuttlefish {
+namespace {
+
+// A class that waits for threads to exit in its destructor.
+class ThreadJoiner {
+  std::vector<std::thread*> threads_;
+
+ public:
+  ThreadJoiner(const std::vector<std::thread*> threads) : threads_(threads) {}
+  ~ThreadJoiner() {
+    for (auto& thread : threads_) {
+      if (thread->joinable()) {
+        thread->join();
+      }
+    }
+  }
+};
+
+}  // namespace
+
+int RunWithManagedStdio(Command&& cmd_tmp, const std::string* stdin_str,
+                        std::string* stdout_str, std::string* stderr_str,
+                        SubprocessOptions options) {
+  /*
+   * The order of these declarations is necessary for safety. If the function
+   * returns at any point, the Command will be destroyed first, closing all
+   * of its references to SharedFDs. This will cause the thread internals to
+   * fail their reads or writes. The ThreadJoiner then waits for the threads to
+   * complete, as running the destructor of an active std::thread crashes the
+   * program.
+   *
+   * C++ scoping rules dictate that objects are descoped in reverse order to
+   * construction, so this behavior is predictable.
+   */
+  std::thread stdin_thread, stdout_thread, stderr_thread;
+  ThreadJoiner thread_joiner({&stdin_thread, &stdout_thread, &stderr_thread});
+  Command cmd = std::move(cmd_tmp);
+  bool io_error = false;
+  if (stdin_str != nullptr) {
+    SharedFD pipe_read, pipe_write;
+    if (!SharedFD::Pipe(&pipe_read, &pipe_write)) {
+      LOG(ERROR) << "Could not create a pipe to write the stdin of \""
+                 << cmd.GetShortName() << "\"";
+      return -1;
+    }
+    cmd.RedirectStdIO(Subprocess::StdIOChannel::kStdIn, pipe_read);
+    stdin_thread = std::thread([pipe_write, stdin_str, &io_error]() {
+      int written = WriteAll(pipe_write, *stdin_str);
+      if (written < 0) {
+        io_error = true;
+        LOG(ERROR) << "Error in writing stdin to process";
+      }
+    });
+  }
+  if (stdout_str != nullptr) {
+    SharedFD pipe_read, pipe_write;
+    if (!SharedFD::Pipe(&pipe_read, &pipe_write)) {
+      LOG(ERROR) << "Could not create a pipe to read the stdout of \""
+                 << cmd.GetShortName() << "\"";
+      return -1;
+    }
+    cmd.RedirectStdIO(Subprocess::StdIOChannel::kStdOut, pipe_write);
+    stdout_thread = std::thread([pipe_read, stdout_str, &io_error]() {
+      int read = ReadAll(pipe_read, stdout_str);
+      if (read < 0) {
+        io_error = true;
+        LOG(ERROR) << "Error in reading stdout from process";
+      }
+    });
+  }
+  if (stderr_str != nullptr) {
+    SharedFD pipe_read, pipe_write;
+    if (!SharedFD::Pipe(&pipe_read, &pipe_write)) {
+      LOG(ERROR) << "Could not create a pipe to read the stderr of \""
+                 << cmd.GetShortName() << "\"";
+      return -1;
+    }
+    cmd.RedirectStdIO(Subprocess::StdIOChannel::kStdErr, pipe_write);
+    stderr_thread = std::thread([pipe_read, stderr_str, &io_error]() {
+      int read = ReadAll(pipe_read, stderr_str);
+      if (read < 0) {
+        io_error = true;
+        LOG(ERROR) << "Error in reading stderr from process";
+      }
+    });
+  }
+
+  auto subprocess = cmd.Start(std::move(options));
+  if (!subprocess.Started()) {
+    return -1;
+  }
+  auto cmd_short_name = cmd.GetShortName();
+  {
+    // Force the destructor to run by moving it into a smaller scope.
+    // This is necessary to close the write end of the pipe.
+    Command forceDelete = std::move(cmd);
+  }
+
+  int code = subprocess.Wait();
+  {
+    auto join_threads = std::move(thread_joiner);
+  }
+  if (io_error) {
+    LOG(ERROR) << "IO error communicating with " << cmd_short_name;
+    return -1;
+  }
+  return code;
+}
+
+}  // namespace cuttlefish