update_engine: Use libchromeos to launch subprocesses.
The Subprocess class handles the execution of suprocesses in the
update_engine such as the post-install script and bspatch operations.
This patch migrates this class from using glib functions to use
libchromeos classes with equivalent functionality.
Callsites and unittests were updated to match the new interface.
BUG=chromium:499886
TEST=Unittest still pass. Deployed on link and cros flash another image
using a delta payload.
Change-Id: Ia64d39734e220675113f393a6049e9a9b0fe8409
Reviewed-on: https://chromium-review.googlesource.com/288837
Trybot-Ready: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
diff --git a/delta_performer.cc b/delta_performer.cc
index 725ca00..81477de 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -894,10 +894,8 @@
int return_code = 0;
TEST_AND_RETURN_FALSE(
- Subprocess::SynchronousExecFlags(cmd,
- G_SPAWN_LEAVE_DESCRIPTORS_OPEN,
- &return_code,
- nullptr));
+ Subprocess::SynchronousExecFlags(cmd, Subprocess::kSearchPath,
+ &return_code, nullptr));
TEST_AND_RETURN_FALSE(return_code == 0);
if (operation.dst_length() % block_size_) {
@@ -965,10 +963,8 @@
int return_code = 0;
TEST_AND_RETURN_FALSE(
- Subprocess::SynchronousExecFlags(cmd,
- G_SPAWN_LEAVE_DESCRIPTORS_OPEN,
- &return_code,
- nullptr));
+ Subprocess::SynchronousExecFlags(cmd, Subprocess::kSearchPath,
+ &return_code, nullptr));
TEST_AND_RETURN_FALSE(return_code == 0);
return true;
}
diff --git a/filesystem_verifier_action.cc b/filesystem_verifier_action.cc
index 70156d9..5c352b0 100644
--- a/filesystem_verifier_action.cc
+++ b/filesystem_verifier_action.cc
@@ -17,7 +17,6 @@
#include <base/posix/eintr_wrapper.h>
#include "update_engine/hardware_interface.h"
-#include "update_engine/subprocess.h"
#include "update_engine/system_state.h"
#include "update_engine/utils.h"
diff --git a/main.cc b/main.cc
index 51a04ec..cf06f2a 100644
--- a/main.cc
+++ b/main.cc
@@ -170,7 +170,6 @@
dbus_threads_init_default();
base::AtExitManager exit_manager; // Required for base/rand_util.h.
chromeos_update_engine::Terminator::Init();
- chromeos_update_engine::Subprocess::Init();
chromeos::FlagHelper::Init(argc, argv, "Chromium OS Update Engine");
chromeos_update_engine::SetupLogging(FLAGS_logtostderr);
if (!FLAGS_foreground)
@@ -192,6 +191,10 @@
chromeos::GlibMessageLoop loop;
loop.SetAsCurrent();
+ // The Subprocess class requires a chromeos::MessageLoop setup.
+ chromeos_update_engine::Subprocess subprocess;
+ subprocess.Init();
+
// Wait up to 2 minutes for DBus to be ready.
LOG_IF(FATAL, !chromeos_update_engine::WaitForDBusSystem(
base::TimeDelta::FromSeconds(kDBusSystemMaxWaitSeconds)))
diff --git a/p2p_manager.cc b/p2p_manager.cc
index c72a023..b1b4438 100644
--- a/p2p_manager.cc
+++ b/p2p_manager.cc
@@ -369,8 +369,8 @@
~LookupData() {
if (timeout_task_ != MessageLoop::kTaskIdNull)
MessageLoop::current()->CancelTask(timeout_task_);
- if (child_tag_)
- Subprocess::Get().KillExec(child_tag_);
+ if (child_pid_)
+ Subprocess::Get().KillExec(child_pid_);
}
void InitiateLookup(const vector<string>& cmd, TimeDelta timeout) {
@@ -380,11 +380,11 @@
// guarantee is useful for testing).
// We expect to run just "p2p-client" and find it in the path.
- child_tag_ = Subprocess::Get().ExecFlags(
- cmd, G_SPAWN_SEARCH_PATH, false /* redirect stderr */, OnLookupDone,
- this);
+ child_pid_ = Subprocess::Get().ExecFlags(
+ cmd, Subprocess::kSearchPath,
+ Bind(&LookupData::OnLookupDone, base::Unretained(this)));
- if (!child_tag_) {
+ if (!child_pid_) {
LOG(ERROR) << "Error spawning " << utils::StringVectorToString(cmd);
ReportErrorAndDeleteInIdle();
return;
@@ -442,19 +442,16 @@
reported_ = true;
}
- static void OnLookupDone(int return_code,
- const string& output,
- void *user_data) {
- LookupData *lookup_data = reinterpret_cast<LookupData*>(user_data);
- lookup_data->child_tag_ = 0;
+ void OnLookupDone(int return_code, const string& output) {
+ child_pid_ = 0;
if (return_code != 0) {
LOG(INFO) << "Child exited with non-zero exit code "
<< return_code;
- lookup_data->ReportError();
+ ReportError();
} else {
- lookup_data->ReportSuccess(output);
+ ReportSuccess(output);
}
- delete lookup_data;
+ delete this;
}
void OnTimeout() {
@@ -467,7 +464,7 @@
// The Subprocess tag of the running process. A value of 0 means that the
// process is not running.
- uint32_t child_tag_{0};
+ pid_t child_pid_{0};
// The timeout task_id we are waiting on, if any.
MessageLoop::TaskId timeout_task_{MessageLoop::kTaskIdNull};
diff --git a/p2p_manager_unittest.cc b/p2p_manager_unittest.cc
index 8667578..ec93bc6 100644
--- a/p2p_manager_unittest.cc
+++ b/p2p_manager_unittest.cc
@@ -57,6 +57,7 @@
// Derived from testing::Test.
void SetUp() override {
loop_.SetAsCurrent();
+ subprocess_.Init();
test_conf_ = new FakeP2PManagerConfiguration();
// Allocate and install a mock policy implementation in the fake Update
@@ -71,14 +72,11 @@
TimeDelta::FromDays(5)));
}
- void TearDown() override {
- EXPECT_EQ(0, chromeos::MessageLoopRunMaxIterations(&loop_, 1));
- }
-
// TODO(deymo): Replace this with a FakeMessageLoop. P2PManager uses glib to
// interact with the p2p-client tool, so we need to run a GlibMessageLoop
// here.
chromeos::GlibMessageLoop loop_;
+ Subprocess subprocess_;
// The P2PManager::Configuration instance used for testing.
FakeP2PManagerConfiguration *test_conf_;
diff --git a/payload_generator/generate_delta_main.cc b/payload_generator/generate_delta_main.cc
index fee3dbb..1873331 100644
--- a/payload_generator/generate_delta_main.cc
+++ b/payload_generator/generate_delta_main.cc
@@ -25,7 +25,6 @@
#include "update_engine/payload_generator/payload_signer.h"
#include "update_engine/payload_verifier.h"
#include "update_engine/prefs.h"
-#include "update_engine/subprocess.h"
#include "update_engine/terminator.h"
#include "update_engine/update_metadata.pb.h"
#include "update_engine/utils.h"
@@ -297,7 +296,6 @@
"image is provided. It also provides debugging options to apply, sign\n"
"and verify payloads.");
Terminator::Init();
- Subprocess::Init();
logging::LoggingSettings log_settings;
log_settings.log_file = "delta_generator.log";
diff --git a/postinstall_runner_action.cc b/postinstall_runner_action.cc
index f27aace..0760bd5 100644
--- a/postinstall_runner_action.cc
+++ b/postinstall_runner_action.cc
@@ -8,6 +8,8 @@
#include <sys/mount.h>
#include <vector>
+#include <base/bind.h>
+
#include "update_engine/action_processor.h"
#include "update_engine/subprocess.h"
#include "update_engine/utils.h"
@@ -83,12 +85,16 @@
command.push_back(kPostinstallScript);
}
command.push_back(install_device);
- if (!Subprocess::Get().Exec(command, StaticCompletePostinstall, this)) {
- CompletePostinstall(1);
+ if (!Subprocess::Get().Exec(command,
+ base::Bind(
+ &PostinstallRunnerAction::CompletePostinstall,
+ base::Unretained(this)))) {
+ CompletePostinstall(1, "Postinstall didn't launch");
}
}
-void PostinstallRunnerAction::CompletePostinstall(int return_code) {
+void PostinstallRunnerAction::CompletePostinstall(int return_code,
+ const string& output) {
ScopedActionCompleter completer(processor_, this);
ScopedTempUnmounter temp_unmounter(temp_rootfs_dir_);
if (return_code != 0) {
@@ -123,11 +129,4 @@
completer.set_code(ErrorCode::kSuccess);
}
-void PostinstallRunnerAction::StaticCompletePostinstall(int return_code,
- const string& output,
- void* p) {
- reinterpret_cast<PostinstallRunnerAction*>(p)->CompletePostinstall(
- return_code);
-}
-
} // namespace chromeos_update_engine
diff --git a/postinstall_runner_action.h b/postinstall_runner_action.h
index e7ac1c5..18a3691 100644
--- a/postinstall_runner_action.h
+++ b/postinstall_runner_action.h
@@ -32,10 +32,8 @@
private:
// Subprocess::Exec callback.
- void CompletePostinstall(int return_code);
- static void StaticCompletePostinstall(int return_code,
- const std::string& output,
- void* p);
+ void CompletePostinstall(int return_code,
+ const std::string& output);
InstallPlan install_plan_;
std::string temp_rootfs_dir_;
diff --git a/postinstall_runner_action_unittest.cc b/postinstall_runner_action_unittest.cc
index 4549272..ff64128 100644
--- a/postinstall_runner_action_unittest.cc
+++ b/postinstall_runner_action_unittest.cc
@@ -37,10 +37,7 @@
protected:
void SetUp() override {
loop_.SetAsCurrent();
- }
-
- void TearDown() override {
- EXPECT_EQ(0, chromeos::MessageLoopRunMaxIterations(&loop_, 1));
+ subprocess_.Init();
}
// DoTest with various combinations of do_losetup, err_code and
@@ -51,6 +48,7 @@
static const char* kImageMountPointTemplate;
chromeos::GlibMessageLoop loop_;
+ Subprocess subprocess_;
};
class PostinstActionProcessorDelegate : public ActionProcessorDelegate {
diff --git a/subprocess.cc b/subprocess.cc
index db66eb9..6282b2c 100644
--- a/subprocess.cc
+++ b/subprocess.cc
@@ -18,47 +18,81 @@
#include <base/posix/eintr_wrapper.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
-
-#include "update_engine/glib_utils.h"
+#include <chromeos/process.h>
+#include <chromeos/secure_blob.h>
using chromeos::MessageLoop;
-using std::shared_ptr;
using std::string;
using std::unique_ptr;
using std::vector;
namespace chromeos_update_engine {
-void Subprocess::GChildExitedCallback(GPid pid, gint status, gpointer data) {
- SubprocessRecord* record = reinterpret_cast<SubprocessRecord*>(data);
+namespace {
- // Make sure we read any remaining process output and then close the pipe.
- OnStdoutReady(record);
+bool SetupChild(const std::map<string, string>& env, uint32_t flags) {
+ // Setup the environment variables.
+ clearenv();
+ for (const auto& key_value : env) {
+ setenv(key_value.first.c_str(), key_value.second.c_str(), 0);
+ }
- MessageLoop::current()->CancelTask(record->task_id);
- record->task_id = MessageLoop::kTaskIdNull;
- if (IGNORE_EINTR(close(record->stdout_fd)) != 0) {
- PLOG(ERROR) << "Error closing fd " << record->stdout_fd;
+ if ((flags & Subprocess::kRedirectStderrToStdout) != 0) {
+ if (HANDLE_EINTR(dup2(STDOUT_FILENO, STDERR_FILENO)) != STDERR_FILENO)
+ return false;
}
- g_spawn_close_pid(pid);
- gint use_status = status;
- if (WIFEXITED(status))
- use_status = WEXITSTATUS(status);
- if (status) {
- LOG(INFO) << "Subprocess status: " << use_status;
- }
- if (!record->stdout.empty()) {
- LOG(INFO) << "Subprocess output:\n" << record->stdout;
- }
- if (record->callback) {
- record->callback(use_status, record->stdout, record->callback_data);
- }
- Get().subprocess_records_.erase(record->tag);
+ int fd = HANDLE_EINTR(open("/dev/null", O_RDONLY));
+ if (fd < 0)
+ return false;
+ if (HANDLE_EINTR(dup2(fd, STDIN_FILENO)) != STDIN_FILENO)
+ return false;
+ IGNORE_EINTR(close(fd));
+
+ return true;
}
-void Subprocess::GRedirectStderrToStdout(gpointer user_data) {
- dup2(1, 2);
+// Helper function to launch a process with the given Subprocess::Flags.
+// This function only sets up and starts the process according to the |flags|.
+// The caller is responsible for watching the termination of the subprocess.
+// Return whether the process was successfully launched and fills in the |proc|
+// Process.
+bool LaunchProcess(const vector<string>& cmd,
+ uint32_t flags,
+ chromeos::Process* proc) {
+ for (const string& arg : cmd)
+ proc->AddArg(arg);
+ proc->SetSearchPath((flags & Subprocess::kSearchPath) != 0);
+
+ // Create an environment for the child process with just the required PATHs.
+ std::map<string, string> env;
+ for (const char* key : {"LD_LIBRARY_PATH", "PATH"}) {
+ const char* value = getenv(key);
+ if (value)
+ env.emplace(key, value);
+ }
+
+ proc->RedirectUsingPipe(STDOUT_FILENO, false);
+ proc->SetPreExecCallback(base::Bind(&SetupChild, env, flags));
+
+ return proc->Start();
+}
+
+} // namespace
+
+void Subprocess::Init() {
+ if (subprocess_singleton_ == this)
+ return;
+ CHECK(subprocess_singleton_ == nullptr);
+ subprocess_singleton_ = this;
+
+ async_signal_handler_.Init();
+ process_reaper_.Register(&async_signal_handler_);
+}
+
+Subprocess::~Subprocess() {
+ if (subprocess_singleton_ == this)
+ subprocess_singleton_ = nullptr;
}
void Subprocess::OnStdoutReady(SubprocessRecord* record) {
@@ -71,205 +105,150 @@
// input as we are in non-blocking mode.
if (errno != EWOULDBLOCK && errno != EAGAIN) {
PLOG(ERROR) << "Error reading fd " << record->stdout_fd;
+ MessageLoop::current()->CancelTask(record->stdout_task_id);
+ record->stdout_task_id = MessageLoop::kTaskIdNull;
}
} else if (rc == 0) {
// A value of 0 means that the child closed its end of the pipe and there
// is nothing else to read from stdout.
- MessageLoop::current()->CancelTask(record->task_id);
- record->task_id = MessageLoop::kTaskIdNull;
+ MessageLoop::current()->CancelTask(record->stdout_task_id);
+ record->stdout_task_id = MessageLoop::kTaskIdNull;
} else {
record->stdout.append(buf, rc);
}
} while (rc > 0);
}
-namespace {
-void FreeArgv(char** argv) {
- for (int i = 0; argv[i]; i++) {
- free(argv[i]);
- argv[i] = nullptr;
+void Subprocess::ChildExitedCallback(const siginfo_t& info) {
+ auto pid_record = subprocess_records_.find(info.si_pid);
+ if (pid_record == subprocess_records_.end())
+ return;
+ SubprocessRecord* record = pid_record->second.get();
+
+ // Make sure we read any remaining process output and then close the pipe.
+ OnStdoutReady(record);
+
+ MessageLoop::current()->CancelTask(record->stdout_task_id);
+ record->stdout_task_id = MessageLoop::kTaskIdNull;
+
+ // Release and close all the pipes now.
+ record->proc.Release();
+ record->proc.Reset(0);
+
+ // Don't print any log if the subprocess exited with exit code 0.
+ if (info.si_code != CLD_EXITED) {
+ LOG(INFO) << "Subprocess terminated with si_code " << info.si_code;
+ } else if (info.si_status != 0) {
+ LOG(INFO) << "Subprocess exited with si_status: " << info.si_status;
}
-}
-void FreeArgvInError(char** argv) {
- FreeArgv(argv);
- LOG(ERROR) << "Ran out of memory copying args.";
-}
-
-// Note: Caller responsible for free()ing the returned value!
-// Will return null on failure and free any allocated memory.
-char** ArgPointer() {
- const char* keys[] = {"LD_LIBRARY_PATH", "PATH"};
- char** ret = new char*[arraysize(keys) + 1];
- int pointer = 0;
- for (size_t i = 0; i < arraysize(keys); i++) {
- if (getenv(keys[i])) {
- ret[pointer] = strdup(base::StringPrintf("%s=%s", keys[i],
- getenv(keys[i])).c_str());
- if (!ret[pointer]) {
- FreeArgv(ret);
- delete [] ret;
- return nullptr;
- }
- ++pointer;
- }
+ if (!record->stdout.empty()) {
+ LOG(INFO) << "Subprocess output:\n" << record->stdout;
}
- ret[pointer] = nullptr;
- return ret;
-}
-
-class ScopedFreeArgPointer {
- public:
- explicit ScopedFreeArgPointer(char** arr) : arr_(arr) {}
- ~ScopedFreeArgPointer() {
- if (!arr_)
- return;
- for (int i = 0; arr_[i]; i++)
- free(arr_[i]);
- delete[] arr_;
+ if (!record->callback.is_null()) {
+ record->callback.Run(info.si_status, record->stdout);
}
- private:
- char** arr_;
- DISALLOW_COPY_AND_ASSIGN(ScopedFreeArgPointer);
-};
-} // namespace
-
-uint32_t Subprocess::Exec(const vector<string>& cmd,
- ExecCallback callback,
- void* p) {
- return ExecFlags(cmd, static_cast<GSpawnFlags>(0), true, callback, p);
+ subprocess_records_.erase(pid_record);
}
-uint32_t Subprocess::ExecFlags(const vector<string>& cmd,
- GSpawnFlags flags,
- bool redirect_stderr_to_stdout,
- ExecCallback callback,
- void* p) {
- unique_ptr<gchar*, utils::GLibStrvFreeDeleter> argv(
- utils::StringVectorToGStrv(cmd));
+pid_t Subprocess::Exec(const vector<string>& cmd,
+ const ExecCallback& callback) {
+ return ExecFlags(cmd, kRedirectStderrToStdout, callback);
+}
- char** argp = ArgPointer();
- if (!argp) {
- FreeArgvInError(argv.get()); // null in argv[i] terminates argv.
+pid_t Subprocess::ExecFlags(const vector<string>& cmd,
+ uint32_t flags,
+ const ExecCallback& callback) {
+ unique_ptr<SubprocessRecord> record(new SubprocessRecord(callback));
+
+ if (!LaunchProcess(cmd, flags, &record->proc)) {
+ LOG(ERROR) << "Failed to launch subprocess";
return 0;
}
- ScopedFreeArgPointer argp_free(argp);
- shared_ptr<SubprocessRecord> record(new SubprocessRecord);
- record->callback = callback;
- record->callback_data = p;
- gint stdout_fd = -1;
- GError* error = nullptr;
- bool success = g_spawn_async_with_pipes(
- nullptr, // working directory
- argv.get(),
- argp,
- static_cast<GSpawnFlags>(flags | G_SPAWN_DO_NOT_REAP_CHILD), // flags
- // child setup function:
- redirect_stderr_to_stdout ? GRedirectStderrToStdout : nullptr,
- nullptr, // child setup data pointer
- &record->pid,
- nullptr,
- &stdout_fd,
- nullptr,
- &error);
- if (!success) {
- LOG(ERROR) << "g_spawn_async failed: " << utils::GetAndFreeGError(&error);
- return 0;
- }
- record->tag =
- g_child_watch_add(record->pid, GChildExitedCallback, record.get());
- record->stdout_fd = stdout_fd;
- subprocess_records_[record->tag] = record;
+ pid_t pid = record->proc.pid();
+ CHECK(process_reaper_.WatchForChild(FROM_HERE, pid, base::Bind(
+ &Subprocess::ChildExitedCallback,
+ base::Unretained(this))));
+ record->stdout_fd = record->proc.GetPipe(STDOUT_FILENO);
// Capture the subprocess output. Make our end of the pipe non-blocking.
- int fd_flags = fcntl(stdout_fd, F_GETFL, 0) | O_NONBLOCK;
+ int fd_flags = fcntl(record->stdout_fd, F_GETFL, 0) | O_NONBLOCK;
if (HANDLE_EINTR(fcntl(record->stdout_fd, F_SETFL, fd_flags)) < 0) {
LOG(ERROR) << "Unable to set non-blocking I/O mode on fd "
<< record->stdout_fd << ".";
}
- record->task_id = MessageLoop::current()->WatchFileDescriptor(
+ record->stdout_task_id = MessageLoop::current()->WatchFileDescriptor(
FROM_HERE,
record->stdout_fd,
MessageLoop::WatchMode::kWatchRead,
true,
base::Bind(&Subprocess::OnStdoutReady, record.get()));
- return record->tag;
+ subprocess_records_[pid].reset(record.release());
+ return pid;
}
-void Subprocess::KillExec(uint32_t tag) {
- const auto& record = subprocess_records_.find(tag);
- if (record == subprocess_records_.end())
+void Subprocess::KillExec(pid_t pid) {
+ auto pid_record = subprocess_records_.find(pid);
+ if (pid_record == subprocess_records_.end())
return;
- record->second->callback = nullptr;
- kill(record->second->pid, SIGTERM);
-}
-
-bool Subprocess::SynchronousExecFlags(const vector<string>& cmd,
- GSpawnFlags flags,
- int* return_code,
- string* stdout) {
- if (stdout) {
- *stdout = "";
- }
- GError* err = nullptr;
- unique_ptr<char*[]> argv(new char*[cmd.size() + 1]);
- for (unsigned int i = 0; i < cmd.size(); i++) {
- argv[i] = strdup(cmd[i].c_str());
- if (!argv[i]) {
- FreeArgvInError(argv.get()); // null in argv[i] terminates argv.
- return false;
- }
- }
- argv[cmd.size()] = nullptr;
-
- char** argp = ArgPointer();
- if (!argp) {
- FreeArgvInError(argv.get()); // null in argv[i] terminates argv.
- return false;
- }
- ScopedFreeArgPointer argp_free(argp);
-
- char* child_stdout;
- bool success = g_spawn_sync(
- nullptr, // working directory
- argv.get(),
- argp,
- static_cast<GSpawnFlags>(G_SPAWN_STDERR_TO_DEV_NULL |
- G_SPAWN_SEARCH_PATH | flags), // flags
- GRedirectStderrToStdout, // child setup function
- nullptr, // data for child setup function
- &child_stdout,
- nullptr,
- return_code,
- &err);
- FreeArgv(argv.get());
- LOG_IF(INFO, err) << utils::GetAndFreeGError(&err);
- if (child_stdout) {
- if (stdout) {
- *stdout = child_stdout;
- } else if (*child_stdout) {
- LOG(INFO) << "Subprocess output:\n" << child_stdout;
- }
- g_free(child_stdout);
- }
- return success;
+ pid_record->second->callback.Reset();
+ kill(pid, SIGTERM);
}
bool Subprocess::SynchronousExec(const vector<string>& cmd,
int* return_code,
string* stdout) {
- return SynchronousExecFlags(cmd,
- static_cast<GSpawnFlags>(0),
- return_code,
- stdout);
+ // The default for SynchronousExec is to use kSearchPath since the code relies
+ // on that.
+ return SynchronousExecFlags(
+ cmd,
+ kRedirectStderrToStdout | kSearchPath,
+ return_code,
+ stdout);
+}
+
+bool Subprocess::SynchronousExecFlags(const vector<string>& cmd,
+ uint32_t flags,
+ int* return_code,
+ string* stdout) {
+ chromeos::ProcessImpl proc;
+ if (!LaunchProcess(cmd, flags, &proc)) {
+ LOG(ERROR) << "Failed to launch subprocess";
+ return false;
+ }
+
+ if (stdout) {
+ stdout->clear();
+ }
+
+ int fd = proc.GetPipe(STDOUT_FILENO);
+ vector<char> buffer(32 * 1024);
+ while (true) {
+ int rc = HANDLE_EINTR(read(fd, buffer.data(), buffer.size()));
+ if (rc < 0) {
+ PLOG(ERROR) << "Reading from child's output";
+ break;
+ } else if (rc == 0) {
+ break;
+ } else {
+ if (stdout)
+ stdout->append(buffer.data(), rc);
+ }
+ }
+ // At this point, the subprocess already closed the output, so we only need to
+ // wait for it to finish.
+ int proc_return_code = proc.Wait();
+ if (return_code)
+ *return_code = proc_return_code;
+ return proc_return_code != chromeos::Process::kErrorExitStatus;
}
bool Subprocess::SubprocessInFlight() {
- for (const auto& tag_record_pair : subprocess_records_) {
- if (tag_record_pair.second->callback)
+ for (const auto& pid_record : subprocess_records_) {
+ if (!pid_record.second->callback.is_null())
return true;
}
return false;
diff --git a/subprocess.h b/subprocess.h
index 13f7eb7..c26ea60 100644
--- a/subprocess.h
+++ b/subprocess.h
@@ -5,16 +5,20 @@
#ifndef UPDATE_ENGINE_SUBPROCESS_H_
#define UPDATE_ENGINE_SUBPROCESS_H_
-#include <glib.h>
+#include <unistd.h>
#include <map>
#include <memory>
#include <string>
#include <vector>
+#include <base/callback.h>
#include <base/logging.h>
#include <base/macros.h>
+#include <chromeos/asynchronous_signal_handler.h>
#include <chromeos/message_loops/message_loop.h>
+#include <chromeos/process.h>
+#include <chromeos/process_reaper.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
// The Subprocess class is a singleton. It's used to spawn off a subprocess
@@ -23,42 +27,51 @@
// you know you won't call KillExec(), you may safely lose the return value
// from Exec().
+// To create the Subprocess singleton just instantiate it with and call Init().
+// You can't have two Subprocess instances initialized at the same time.
+
namespace chromeos_update_engine {
class Subprocess {
public:
- typedef void(*ExecCallback)(int return_code,
- const std::string& output,
- void *p);
+ enum Flags {
+ kSearchPath = 1 << 0,
+ kRedirectStderrToStdout = 1 << 1,
+ };
- static void Init() {
- CHECK(!subprocess_singleton_);
- subprocess_singleton_ = new Subprocess;
- }
+ // Callback type used when an async process terminates. It receives the exit
+ // code and the stdout output (and stderr if redirected).
+ using ExecCallback = base::Callback<void(int, const std::string&)>;
- // Returns a tag > 0 on success.
- uint32_t Exec(const std::vector<std::string>& cmd,
- ExecCallback callback,
- void* p);
- uint32_t ExecFlags(const std::vector<std::string>& cmd,
- GSpawnFlags flags,
- bool redirect_stderr_to_stdout,
- ExecCallback callback,
- void* p);
+ Subprocess() = default;
+
+ // Destroy and unregister the Subprocess singleton.
+ ~Subprocess();
+
+ // Initialize and register the Subprocess singleton.
+ void Init();
+
+ // Launches a process in the background and calls the passed |callback| when
+ // the process exits.
+ // Returns the process id of the new launched process or 0 in case of failure.
+ pid_t Exec(const std::vector<std::string>& cmd, const ExecCallback& callback);
+ pid_t ExecFlags(const std::vector<std::string>& cmd,
+ uint32_t flags,
+ const ExecCallback& callback);
// Kills the running process with SIGTERM and ignores the callback.
- void KillExec(uint32_t tag);
+ void KillExec(pid_t tag);
// Executes a command synchronously. Returns true on success. If |stdout| is
// non-null, the process output is stored in it, otherwise the output is
// logged. Note that stderr is redirected to stdout.
- static bool SynchronousExecFlags(const std::vector<std::string>& cmd,
- GSpawnFlags flags,
- int* return_code,
- std::string* stdout);
static bool SynchronousExec(const std::vector<std::string>& cmd,
int* return_code,
std::string* stdout);
+ static bool SynchronousExecFlags(const std::vector<std::string>& cmd,
+ uint32_t flags,
+ int* return_code,
+ std::string* stdout);
// Gets the one instance.
static Subprocess& Get() {
@@ -72,40 +85,42 @@
FRIEND_TEST(SubprocessTest, CancelTest);
struct SubprocessRecord {
- SubprocessRecord() = default;
+ explicit SubprocessRecord(const ExecCallback& callback)
+ : callback(callback) {}
- uint32_t tag{0};
- chromeos::MessageLoop::TaskId task_id{chromeos::MessageLoop::kTaskIdNull};
+ // The callback supplied by the caller.
+ ExecCallback callback;
- ExecCallback callback{nullptr};
- void* callback_data{nullptr};
+ // The ProcessImpl instance managing the child process. Destroying this
+ // will close our end of the pipes we have open.
+ chromeos::ProcessImpl proc;
- GPid pid;
-
+ // These are used to monitor the stdout of the running process, including
+ // the stderr if it was redirected.
+ chromeos::MessageLoop::TaskId stdout_task_id{
+ chromeos::MessageLoop::kTaskIdNull};
int stdout_fd{-1};
std::string stdout;
};
- Subprocess() {}
-
- // Callback for when any subprocess terminates. This calls the user
- // requested callback.
- static void GChildExitedCallback(GPid pid, gint status, gpointer data);
-
- // Callback which runs in the child before exec to redirect stderr onto
- // stdout.
- static void GRedirectStderrToStdout(gpointer user_data);
-
// Callback which runs whenever there is input available on the subprocess
// stdout pipe.
static void OnStdoutReady(SubprocessRecord* record);
+ // Callback for when any subprocess terminates. This calls the user
+ // requested callback.
+ void ChildExitedCallback(const siginfo_t& info);
+
// The global instance.
static Subprocess* subprocess_singleton_;
// A map from the asynchronous subprocess tag (see Exec) to the subprocess
// record structure for all active asynchronous subprocesses.
- std::map<uint32_t, std::shared_ptr<SubprocessRecord>> subprocess_records_;
+ std::map<pid_t, std::unique_ptr<SubprocessRecord>> subprocess_records_;
+
+ // Used to watch for child processes.
+ chromeos::AsynchronousSignalHandler async_signal_handler_;
+ chromeos::ProcessReaper process_reaper_;
DISALLOW_COPY_AND_ASSIGN(Subprocess);
};
diff --git a/subprocess_unittest.cc b/subprocess_unittest.cc
index dba9e6c..eb95133 100644
--- a/subprocess_unittest.cc
+++ b/subprocess_unittest.cc
@@ -13,6 +13,7 @@
#include <sys/types.h>
#include <unistd.h>
+#include <set>
#include <string>
#include <vector>
@@ -25,7 +26,7 @@
#include <chromeos/message_loops/glib_message_loop.h>
#include <chromeos/message_loops/message_loop.h>
#include <chromeos/message_loops/message_loop_utils.h>
-#include <glib.h>
+#include <chromeos/strings/string_utils.h>
#include <gtest/gtest.h>
#include "update_engine/test_utils.h"
@@ -42,69 +43,83 @@
protected:
void SetUp() override {
loop_.SetAsCurrent();
- }
-
- void TearDown() override {
- EXPECT_EQ(0, chromeos::MessageLoopRunMaxIterations(&loop_, 1));
+ subprocess_.Init();
}
// TODO(deymo): Replace this with a FakeMessageLoop. Subprocess uses glib to
// asynchronously spawn a process, so we need to run a GlibMessageLoop here.
chromeos::GlibMessageLoop loop_;
+ Subprocess subprocess_;
};
namespace {
+
int local_server_port = 0;
-void Callback(int return_code, const string& output, void* /* unused */) {
- EXPECT_EQ(1, return_code);
+void ExpectedResults(int expected_return_code, const string& expected_output,
+ int return_code, const string& output) {
+ EXPECT_EQ(expected_return_code, return_code);
+ EXPECT_EQ(expected_output, output);
MessageLoop::current()->BreakLoop();
}
-void CallbackEcho(int return_code, const string& output, void* /* unused */) {
+void ExpectedEnvVars(int return_code, const string& output) {
EXPECT_EQ(0, return_code);
- EXPECT_NE(string::npos, output.find("this is stdout"));
- EXPECT_NE(string::npos, output.find("this is stderr"));
- MessageLoop::current()->BreakLoop();
-}
-
-void CallbackStdoutOnlyEcho(int return_code,
- const string& output,
- void* /* unused */) {
- EXPECT_EQ(0, return_code);
- EXPECT_NE(string::npos, output.find("on stdout"));
- EXPECT_EQ(string::npos, output.find("on stderr"));
+ const std::set<string> allowed_envs = {"LD_LIBRARY_PATH", "PATH"};
+ for (string key_value : chromeos::string_utils::Split(output, "\n")) {
+ auto key_value_pair = chromeos::string_utils::SplitAtFirst(
+ key_value, "=", true);
+ EXPECT_NE(allowed_envs.end(), allowed_envs.find(key_value_pair.first));
+ }
MessageLoop::current()->BreakLoop();
}
} // namespace
+TEST_F(SubprocessTest, IsASingleton) {
+ EXPECT_EQ(&subprocess_, &Subprocess::Get());
+}
+
+TEST_F(SubprocessTest, InactiveInstancesDontChangeTheSingleton) {
+ std::unique_ptr<Subprocess> another_subprocess(new Subprocess());
+ EXPECT_EQ(&subprocess_, &Subprocess::Get());
+ another_subprocess.reset();
+ EXPECT_EQ(&subprocess_, &Subprocess::Get());
+}
+
TEST_F(SubprocessTest, SimpleTest) {
- Subprocess::Get().Exec(vector<string>{"/bin/false"}, Callback, nullptr);
+ EXPECT_TRUE(subprocess_.Exec({"/bin/false"},
+ base::Bind(&ExpectedResults, 1, "")));
loop_.Run();
}
TEST_F(SubprocessTest, EchoTest) {
- Subprocess::Get().Exec(
- vector<string>{
- "/bin/sh",
- "-c",
- "echo this is stdout; echo this is stderr > /dev/stderr"},
- CallbackEcho,
- nullptr);
+ EXPECT_TRUE(subprocess_.Exec(
+ {"/bin/sh", "-c", "echo this is stdout; echo this is stderr >&2"},
+ base::Bind(&ExpectedResults, 0, "this is stdout\nthis is stderr\n")));
loop_.Run();
}
TEST_F(SubprocessTest, StderrNotIncludedInOutputTest) {
- Subprocess::Get().ExecFlags(
- vector<string>{"/bin/sh", "-c", "echo on stdout; echo on stderr >&2"},
- static_cast<GSpawnFlags>(0),
- false, // don't redirect stderr
- CallbackStdoutOnlyEcho,
- nullptr);
+ EXPECT_TRUE(subprocess_.ExecFlags(
+ {"/bin/sh", "-c", "echo on stdout; echo on stderr >&2"},
+ 0,
+ base::Bind(&ExpectedResults, 0, "on stdout\n")));
loop_.Run();
}
+TEST_F(SubprocessTest, EnvVarsAreFiltered) {
+ EXPECT_TRUE(subprocess_.Exec({"/usr/bin/env"}, base::Bind(&ExpectedEnvVars)));
+ loop_.Run();
+}
+
+TEST_F(SubprocessTest, SynchronousTrueSearchsOnPath) {
+ int rc = -1;
+ EXPECT_TRUE(Subprocess::SynchronousExecFlags(
+ {"true"}, Subprocess::kSearchPath, &rc, nullptr));
+ EXPECT_EQ(0, rc);
+}
+
TEST_F(SubprocessTest, SynchronousEchoTest) {
vector<string> cmd = {
"/bin/sh",
@@ -118,15 +133,16 @@
}
TEST_F(SubprocessTest, SynchronousEchoNoOutputTest) {
- vector<string> cmd = {"/bin/sh", "-c", "echo test"};
int rc = -1;
- ASSERT_TRUE(Subprocess::SynchronousExec(cmd, &rc, nullptr));
+ ASSERT_TRUE(Subprocess::SynchronousExec(
+ {"/bin/sh", "-c", "echo test"},
+ &rc, nullptr));
EXPECT_EQ(0, rc);
}
namespace {
-void CallbackBad(int return_code, const string& output, void* p) {
- CHECK(false) << "should never be called.";
+void CallbackBad(int return_code, const string& output) {
+ ADD_FAILURE() << "should never be called.";
}
// TODO(garnold) this test method uses test_http_server as a representative for
@@ -148,7 +164,7 @@
vector<string> cmd;
cmd.push_back("./test_http_server");
cmd.push_back(temp_file_name);
- uint32_t tag = Subprocess::Get().Exec(cmd, CallbackBad, nullptr);
+ uint32_t tag = Subprocess::Get().Exec(cmd, base::Bind(&CallbackBad));
EXPECT_NE(0, tag);
*spawned = true;
printf("test http server spawned\n");
diff --git a/testrunner.cc b/testrunner.cc
index 4786c74..3defa63 100644
--- a/testrunner.cc
+++ b/testrunner.cc
@@ -14,7 +14,6 @@
#include <glib-object.h>
#include <gtest/gtest.h>
-#include "update_engine/subprocess.h"
#include "update_engine/terminator.h"
int main(int argc, char **argv) {
@@ -28,7 +27,6 @@
// the default exit status of 1. Corresponding reverts are necessary in
// terminator_unittest.cc.
chromeos_update_engine::Terminator::Init(2);
- chromeos_update_engine::Subprocess::Init();
LOG(INFO) << "parsing command line arguments";
base::CommandLine::Init(argc, argv);
LOG(INFO) << "initializing gtest";
diff --git a/update_attempter.cc b/update_attempter.cc
index a1b2f11..3afe2ac 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -1220,13 +1220,21 @@
// the script runtime.
update_boot_flags_running_ = true;
LOG(INFO) << "Updating boot flags...";
- vector<string> cmd{set_good_kernel_cmd_};
- if (!Subprocess::Get().Exec(cmd, StaticCompleteUpdateBootFlags, this)) {
- CompleteUpdateBootFlags(1);
+ vector<string> cmd{"/usr/sbin/chromeos-setgoodkernel"};
+ if (skip_set_good_kernel_) {
+ CompleteUpdateBootFlags(1, "Skipping the call to set");
+ } else {
+ if (!Subprocess::Get().Exec(cmd,
+ Bind(&UpdateAttempter::CompleteUpdateBootFlags,
+ base::Unretained(this)))) {
+ CompleteUpdateBootFlags(
+ 1, "Failed to launch process to mark kernel as good");
+ }
}
}
-void UpdateAttempter::CompleteUpdateBootFlags(int return_code) {
+void UpdateAttempter::CompleteUpdateBootFlags(int return_code,
+ const string& output) {
update_boot_flags_running_ = false;
updated_boot_flags_ = true;
if (start_action_processor_) {
@@ -1234,13 +1242,6 @@
}
}
-void UpdateAttempter::StaticCompleteUpdateBootFlags(
- int return_code,
- const string& output,
- void* p) {
- reinterpret_cast<UpdateAttempter*>(p)->CompleteUpdateBootFlags(return_code);
-}
-
void UpdateAttempter::BroadcastStatus() {
if (!dbus_service_) {
return;
diff --git a/update_attempter.h b/update_attempter.h
index 1326365..48d344f 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -112,10 +112,7 @@
void UpdateBootFlags();
// Subprocess::Exec callback.
- void CompleteUpdateBootFlags(int return_code);
- static void StaticCompleteUpdateBootFlags(int return_code,
- const std::string& output,
- void* p);
+ void CompleteUpdateBootFlags(int return_code, const std::string& output);
UpdateStatus status() const { return status_; }
@@ -469,8 +466,9 @@
// True if UpdateBootFlags is running.
bool update_boot_flags_running_ = false;
- // The command to run to set the current kernel as good.
- std::string set_good_kernel_cmd_ = "/usr/sbin/chromeos-setgoodkernel";
+ // Whether we should skip the async call to "setgoodkernel" command. Used in
+ // unittests.
+ bool skip_set_good_kernel_ = false;
// True if the action processor needs to be started by the boot flag updater.
bool start_action_processor_ = false;
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 41a8232..8c6fa3d 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -110,12 +110,8 @@
// Finish initializing the attempter.
attempter_.Init();
-
- // We set the set_good_kernel command to a non-existent path so it fails to
- // run it. This avoids the async call to the command and continues the
- // update process right away. Tests testing that behavior can override the
- // default set_good_kernel command if needed.
- attempter_.set_good_kernel_cmd_ = "/path/to/non-existent/command";
+ // Don't run setgoodkernel command.
+ attempter_.skip_set_good_kernel_ = true;
}
void SetUp() override {
@@ -165,7 +161,6 @@
void TearDown() override {
test_utils::RecursiveUnlinkDir(test_dir_);
- EXPECT_EQ(0, MessageLoopRunMaxIterations(&loop_, 1));
}
public:
@@ -514,9 +509,7 @@
}
TEST_F(UpdateAttempterTest, UpdateTest) {
- loop_.PostTask(FROM_HERE,
- base::Bind(&UpdateAttempterTest::UpdateTestStart,
- base::Unretained(this)));
+ UpdateTestStart();
loop_.Run();
}