execute_capture_output -> RunWithManagedStdio
RunWithManagedStdio is a more general function that can be applied to
any cvd::Command, which allows using the full set of configuration
options that cvd::Command supports.
The function supports writing a string to the subprocess stdin, reading
the subprocess stdout to a string, and/or reading the subprocess stderr
to a string.
Test: Run launch_cvd --help, fetch_cvd
Bug: 141889929
Change-Id: I2845add9d22c21e6853651f1ea93f011e6c434fe
diff --git a/common/libs/utils/archive.cpp b/common/libs/utils/archive.cpp
index ace5ab9..a22db4f 100644
--- a/common/libs/utils/archive.cpp
+++ b/common/libs/utils/archive.cpp
@@ -33,10 +33,12 @@
}
std::vector<std::string> Archive::Contents() {
- std::string bsdtar_output;
- auto bsdtar_ret =
- cvd::execute_capture_output({"/usr/bin/bsdtar", "-tf", file},
- &bsdtar_output);
+ cvd::Command bsdtar_cmd("/usr/bin/bsdtar");
+ bsdtar_cmd.AddParameter("-tf");
+ bsdtar_cmd.AddParameter(file);
+ std::string bsdtar_input, bsdtar_output;
+ auto bsdtar_ret = cvd::RunWithManagedStdio(std::move(bsdtar_cmd), &bsdtar_input,
+ &bsdtar_output, nullptr);
if (bsdtar_ret != 0) {
LOG(ERROR) << "`bsdtar -tf \"" << file << "\"` returned " << bsdtar_ret;
}
diff --git a/common/libs/utils/subprocess.cpp b/common/libs/utils/subprocess.cpp
index 4685eeb..d3ebf1d 100644
--- a/common/libs/utils/subprocess.cpp
+++ b/common/libs/utils/subprocess.cpp
@@ -25,6 +25,7 @@
#include <map>
#include <set>
+#include <thread>
#include <glog/logging.h>
@@ -306,6 +307,123 @@
return StartHelper(with_control_socket, true);
}
+// 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(cvd::Command&& cmd_tmp, const std::string* stdin,
+ std::string* stdout, std::string* stderr) {
+ /*
+ * The order of these declarations is necessary for safety. If the function
+ * returns at any point, the cvd::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});
+ cvd::Command cmd = std::move(cmd_tmp);
+ bool io_error = false;
+ if (stdin != nullptr) {
+ cvd::SharedFD pipe_read, pipe_write;
+ if (!cvd::SharedFD::Pipe(&pipe_read, &pipe_write)) {
+ LOG(ERROR) << "Could not create a pipe to write the stdin of \""
+ << cmd.GetShortName() << "\"";
+ return -1;
+ }
+ if (!cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdIn, pipe_read)) {
+ LOG(ERROR) << "Could not set stdout of \"" << cmd.GetShortName()
+ << "\", was already set.";
+ return -1;
+ }
+ stdin_thread = std::thread([pipe_write, stdin, &io_error]() {
+ int written = cvd::WriteAll(pipe_write, *stdin);
+ if (written < 0) {
+ io_error = true;
+ LOG(ERROR) << "Error in writing stdin to process";
+ }
+ });
+ }
+ if (stdout != nullptr) {
+ cvd::SharedFD pipe_read, pipe_write;
+ if (!cvd::SharedFD::Pipe(&pipe_read, &pipe_write)) {
+ LOG(ERROR) << "Could not create a pipe to read the stdout of \""
+ << cmd.GetShortName() << "\"";
+ return -1;
+ }
+ if (!cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdOut, pipe_write)) {
+ LOG(ERROR) << "Could not set stdout of \"" << cmd.GetShortName()
+ << "\", was already set.";
+ return -1;
+ }
+ stdout_thread = std::thread([pipe_read, stdout, &io_error]() {
+ int read = cvd::ReadAll(pipe_read, stdout);
+ if (read < 0) {
+ io_error = true;
+ LOG(ERROR) << "Error in reading stdout from process";
+ }
+ });
+ }
+ if (stderr != nullptr) {
+ cvd::SharedFD pipe_read, pipe_write;
+ if (!cvd::SharedFD::Pipe(&pipe_read, &pipe_write)) {
+ LOG(ERROR) << "Could not create a pipe to read the stderr of \""
+ << cmd.GetShortName() << "\"";
+ return -1;
+ }
+ if (!cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdErr, pipe_write)) {
+ LOG(ERROR) << "Could not set stderr of \"" << cmd.GetShortName()
+ << "\", was already set.";
+ return -1;
+ }
+ stderr_thread = std::thread([pipe_read, stderr, &io_error]() {
+ int read = cvd::ReadAll(pipe_read, stderr);
+ if (read < 0) {
+ io_error = true;
+ LOG(ERROR) << "Error in reading stderr from process";
+ }
+ });
+ }
+
+ auto subprocess = cmd.Start();
+ if (!subprocess.Started()) {
+ return -1;
+ }
+ {
+ // Force the destructor to run by moving it into a smaller scope.
+ // This is necessary to close the write end of the pipe.
+ cvd::Command forceDelete = std::move(cmd);
+ }
+ int wstatus;
+ subprocess.Wait(&wstatus, 0);
+ if (WIFSIGNALED(wstatus)) {
+ LOG(ERROR) << "Command was interrupted by a signal: " << WTERMSIG(wstatus);
+ return -1;
+ }
+ {
+ auto join_threads = std::move(thread_joiner);
+ }
+ if (io_error) {
+ LOG(ERROR) << "IO error communicating with " << cmd.GetShortName();
+ return -1;
+ }
+ return WEXITSTATUS(wstatus);
+}
+
int execute(const std::vector<std::string>& command,
const std::vector<std::string>& env) {
Command cmd(command[0]);
@@ -331,33 +449,4 @@
return subprocess.Wait();
}
-int execute_capture_output(const std::vector<std::string>& command,
- std::string* output) {
- Command cmd(command[0]);
- for (size_t i = 1; i < command.size(); ++i) {
- cmd.AddParameter(command[i]);
- }
- cvd::SharedFD pipe_read, pipe_write;
- cvd::SharedFD::Pipe(&pipe_read, &pipe_write);
- cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdOut, pipe_write);
- cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdErr, pipe_write);
-
- auto subprocess = cmd.Start();
- if (!subprocess.Started()) {
- return -1;
- }
- {
- pipe_write->Close();
- // Force the destructor to run by moving it into a smaller scope.
- // This is necessary to close the write end of the pipe.
- cvd::Command forceDelete = std::move(cmd);
- }
-
- int read = cvd::ReadAll(pipe_read, output);
- if (read < 0) {
- LOG(FATAL) << "Could not read from pipe in execute_capture_output";
- }
- return subprocess.Wait();
-}
-
} // namespace cvd
diff --git a/common/libs/utils/subprocess.h b/common/libs/utils/subprocess.h
index 06539fd..44976f9 100644
--- a/common/libs/utils/subprocess.h
+++ b/common/libs/utils/subprocess.h
@@ -187,6 +187,21 @@
bool verbose_;
};
+/*
+ * Consumes a cvd::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(cvd::Command&& command, const std::string* stdin,
+ std::string* stdout, std::string* stderr);
+
// Convenience wrapper around Command and Subprocess class, allows to easily
// execute a command and wait for it to complete. The version without the env
// parameter starts the command with the same environment as the parent. Returns
@@ -195,8 +210,4 @@
const std::vector<std::string>& env);
int execute(const std::vector<std::string>& command);
-// Like execute, but captures stdout and stderr and returns it in "output".
-int execute_capture_output(const std::vector<std::string>& command,
- std::string* output);
-
} // namespace cvd
diff --git a/host/commands/fetcher/install_zip.cc b/host/commands/fetcher/install_zip.cc
index c862fcb..bd6294f 100644
--- a/host/commands/fetcher/install_zip.cc
+++ b/host/commands/fetcher/install_zip.cc
@@ -47,9 +47,11 @@
}
std::string extracted_file = target_directory + "/" + file;
- std::string file_output;
- auto file_ret = cvd::execute_capture_output(
- {"/usr/bin/file", extracted_file}, &file_output);
+ std::string file_input, file_output;
+ cvd::Command file_cmd("/usr/bin/file");
+ file_cmd.AddParameter(extracted_file);
+ auto file_ret = cvd::RunWithManagedStdio(std::move(file_cmd), &file_input,
+ &file_output, nullptr);
if (file_ret != 0) {
LOG(ERROR) << "Unable to run file on " << file << ", returned" << file_ret;
extraction_success = false;
diff --git a/host/commands/launch/flag_forwarder.cc b/host/commands/launch/flag_forwarder.cc
index d310833..9a8e6a2 100644
--- a/host/commands/launch/flag_forwarder.cc
+++ b/host/commands/launch/flag_forwarder.cc
@@ -143,49 +143,6 @@
return output;
}
-std::string CaptureSubprocessStdout(cvd::Command cmd) {
- // TODO(b/141889929): Unify this with execute_capture_output.
- cmd.SetVerbose(false);
- auto dev_null = cvd::SharedFD::Open("/dev/null", O_RDONLY);
- if (!dev_null->IsOpen()) {
- LOG(ERROR) << "Failed to open /dev/null: " << dev_null->StrError();
- return "";
- }
- cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdIn, dev_null);
- cvd::SharedFD pipe_read, pipe_write;
- cvd::SharedFD::Pipe(&pipe_read, &pipe_write);
- cmd.RedirectStdIO(cvd::Subprocess::StdIOChannel::kStdOut, pipe_write);
- auto subprocess = cmd.Start();
- if (!subprocess.Started()) {
- LOG(ERROR) << "Could not start subprocess";
- return "";
- }
- {
- pipe_write->Close();
- // Force the destructor to run by moving it into a smaller scope.
- // This is necessary to close the write end of the pipe.
- cvd::Command forceDelete = std::move(cmd);
- }
- std::string output;
- int read = cvd::ReadAll(pipe_read, &output);
- if (read < 0) {
- LOG(ERROR) << "Could not read from pipe in CaptureSubprocessStdout";
- return "";
- }
- int status;
- subprocess.Wait(&status, 0);
- if (!WIFEXITED(status)) {
- LOG(ERROR) << "Subprocess exited with abnormal conditions.";
- return "";
- }
- if (WEXITSTATUS(status) != 1) {
- LOG(ERROR) << "Subprocess exited with status " << WEXITSTATUS(status)
- << ", expected 1";
- return "";
- }
- return output;
-}
-
/**
* Creates a dynamic flag
*/
@@ -278,8 +235,16 @@
for (const auto& subprocess : subprocesses_) {
cvd::Command cmd(subprocess);
+ cmd.SetVerbose(false);
cmd.AddParameter("--helpxml");
- std::string helpxml_output = CaptureSubprocessStdout(std::move(cmd));
+ std::string helpxml_input, helpxml_output, helpxml_error;
+ int helpxml_ret = cvd::RunWithManagedStdio(std::move(cmd), &helpxml_input,
+ &helpxml_output, &helpxml_error);
+ if (helpxml_ret != 1) {
+ LOG(FATAL) << subprocess << " --helpxml returned unexpected response "
+ << helpxml_ret << ". Stderr was " << helpxml_error;
+ return;
+ }
auto subprocess_flags = FlagsForSubprocess(helpxml_output);
for (const auto& flag : subprocess_flags) {
@@ -307,6 +272,7 @@
for (const auto& subprocess : subprocesses_) {
cvd::Command cmd(subprocess);
+ cmd.SetVerbose(false);
std::vector<std::string> invocation = {subprocess};
for (const auto& flag : ArgvForSubprocess(subprocess)) {
cmd.AddParameter(flag);
@@ -322,7 +288,14 @@
cmd.AddParameter("--noversion");
// Ensure this is set on by putting it at the end.
cmd.AddParameter("--helpxml");
- std::string helpxml_output = CaptureSubprocessStdout(std::move(cmd));
+ std::string helpxml_input, helpxml_output, helpxml_error;
+ int helpxml_ret = cvd::RunWithManagedStdio(std::move(cmd), &helpxml_input,
+ &helpxml_output, &helpxml_error);
+ if (helpxml_ret != 1) {
+ LOG(FATAL) << subprocess << " --helpxml returned unexpected response "
+ << helpxml_ret << ". Stderr was " << helpxml_error;
+ return;
+ }
auto subprocess_flags = FlagsForSubprocess(helpxml_output);
for (const auto& flag : subprocess_flags) {
