Implement susped, resume and cancel for the Postinstall action.
This patch sends SIGSTOP/SIGCONT to the running postinstall program to
suspend/resume the child process, and SIGKILL when cancelling it.
Bug: 27272144
TEST=Added unittest to check the signal being sent.
Change-Id: Iebe9bd34448ad1d0a5340c82e1fd839ff8c69dd2
diff --git a/payload_consumer/postinstall_runner_action.cc b/payload_consumer/postinstall_runner_action.cc
index 9ebef53..7fa7009 100644
--- a/payload_consumer/postinstall_runner_action.cc
+++ b/payload_consumer/postinstall_runner_action.cc
@@ -16,13 +16,15 @@
#include "update_engine/payload_consumer/postinstall_runner_action.h"
+#include <signal.h>
#include <stdlib.h>
#include <sys/mount.h>
+#include <sys/types.h>
#include <vector>
-#include <base/bind.h>
#include <base/files/file_path.h>
#include <base/files/file_util.h>
+#include <base/logging.h>
#include <base/strings/string_util.h>
#include "update_engine/common/action_processor.h"
@@ -126,25 +128,31 @@
// Runs the postinstall script asynchronously to free up the main loop while
// it's running.
vector<string> command = {abs_path, partition.target_path};
- if (!Subprocess::Get().Exec(
- command,
- base::Bind(
- &PostinstallRunnerAction::CompletePartitionPostinstall,
- base::Unretained(this)))) {
+ current_command_ = Subprocess::Get().Exec(
+ command,
+ base::Bind(&PostinstallRunnerAction::CompletePartitionPostinstall,
+ base::Unretained(this)));
+ // Subprocess::Exec should never return a negative process id.
+ CHECK_GE(current_command_, 0);
+
+ if (!current_command_)
CompletePartitionPostinstall(1, "Postinstall didn't launch");
- }
}
-void PostinstallRunnerAction::CompletePartitionPostinstall(
- int return_code,
- const string& output) {
+void PostinstallRunnerAction::CleanupMount() {
utils::UnmountFilesystem(fs_mount_dir_);
-#ifndef ANDROID
+#ifndef __ANDROID__
if (!base::DeleteFile(base::FilePath(fs_mount_dir_), false)) {
PLOG(WARNING) << "Not removing temporary mountpoint " << fs_mount_dir_;
}
-#endif // !ANDROID
+#endif // !__ANDROID__
fs_mount_dir_.clear();
+}
+
+void PostinstallRunnerAction::CompletePartitionPostinstall(
+ int return_code, const string& output) {
+ current_command_ = 0;
+ CleanupMount();
if (return_code != 0) {
LOG(ERROR) << "Postinst command failed with code: " << return_code;
@@ -196,4 +204,30 @@
}
}
+void PostinstallRunnerAction::SuspendAction() {
+ if (!current_command_)
+ return;
+ if (kill(current_command_, SIGSTOP) != 0) {
+ PLOG(ERROR) << "Couldn't pause child process " << current_command_;
+ }
+}
+
+void PostinstallRunnerAction::ResumeAction() {
+ if (!current_command_)
+ return;
+ if (kill(current_command_, SIGCONT) != 0) {
+ PLOG(ERROR) << "Couldn't resume child process " << current_command_;
+ }
+}
+
+void PostinstallRunnerAction::TerminateProcessing() {
+ if (!current_command_)
+ return;
+ // Calling KillExec() will discard the callback we registered and therefore
+ // the unretained reference to this object.
+ Subprocess::Get().KillExec(current_command_);
+ current_command_ = 0;
+ CleanupMount();
+}
+
} // namespace chromeos_update_engine
diff --git a/payload_consumer/postinstall_runner_action.h b/payload_consumer/postinstall_runner_action.h
index 08dedd2..a626ce6 100644
--- a/payload_consumer/postinstall_runner_action.h
+++ b/payload_consumer/postinstall_runner_action.h
@@ -34,10 +34,11 @@
explicit PostinstallRunnerAction(BootControlInterface* boot_control)
: PostinstallRunnerAction(boot_control, nullptr) {}
+ // InstallPlanAction overrides.
void PerformAction() override;
-
- // Note that there's no support for terminating this action currently.
- void TerminateProcessing() override { CHECK(false); }
+ void SuspendAction() override;
+ void ResumeAction() override;
+ void TerminateProcessing() override;
// Debugging/logging
static std::string StaticType() { return "PostinstallRunnerAction"; }
@@ -54,6 +55,9 @@
void PerformPartitionPostinstall();
+ // Unmount and remove the mountpoint directory if needed.
+ void CleanupMount();
+
// Subprocess::Exec callback.
void CompletePartitionPostinstall(int return_code,
const std::string& output);
@@ -82,6 +86,9 @@
// file name; used for testing.
const char* powerwash_marker_file_;
+ // Postinstall command currently running, or 0 if no program running.
+ pid_t current_command_{0};
+
DISALLOW_COPY_AND_ASSIGN(PostinstallRunnerAction);
};
diff --git a/payload_consumer/postinstall_runner_action_unittest.cc b/payload_consumer/postinstall_runner_action_unittest.cc
index 01d8d8f..c6c5ecf 100644
--- a/payload_consumer/postinstall_runner_action_unittest.cc
+++ b/payload_consumer/postinstall_runner_action_unittest.cc
@@ -24,6 +24,7 @@
#include <string>
#include <vector>
+#include <base/bind.h>
#include <base/files/file_util.h>
#include <base/message_loop/message_loop.h>
#include <base/strings/string_util.h>
@@ -47,24 +48,30 @@
class PostinstActionProcessorDelegate : public ActionProcessorDelegate {
public:
- PostinstActionProcessorDelegate()
- : code_(ErrorCode::kError),
- code_set_(false) {}
+ PostinstActionProcessorDelegate() = default;
void ProcessingDone(const ActionProcessor* processor,
- ErrorCode code) {
+ ErrorCode code) override {
MessageLoop::current()->BreakLoop();
+ processing_done_called_ = true;
}
+ void ProcessingStopped(const ActionProcessor* processor) override {
+ MessageLoop::current()->BreakLoop();
+ processing_stopped_called_ = true;
+ }
+
void ActionCompleted(ActionProcessor* processor,
AbstractAction* action,
- ErrorCode code) {
+ ErrorCode code) override {
if (action->Type() == PostinstallRunnerAction::StaticType()) {
code_ = code;
code_set_ = true;
}
}
- ErrorCode code_;
- bool code_set_;
+ ErrorCode code_{ErrorCode::kError};
+ bool code_set_{false};
+ bool processing_done_called_{false};
+ bool processing_stopped_called_{false};
};
class PostinstallRunnerActionTest : public ::testing::Test {
@@ -78,9 +85,9 @@
// We use a test-specific powerwash marker file, to avoid race conditions.
powerwash_marker_file_ = working_dir_ + "/factory_install_reset";
// These tests use the postinstall files generated by "generate_images.sh"
- // stored in the "disk_ext2_ue_settings.img" image.
+ // stored in the "disk_ext2_unittest.img" image.
postinstall_image_ = test_utils::GetBuildArtifactsPath()
- .Append("gen/disk_ext2_ue_settings.img")
+ .Append("gen/disk_ext2_unittest.img")
.value();
ASSERT_EQ(0U, getuid()) << "Run these tests as root.";
@@ -97,6 +104,49 @@
const string& postinstall_program,
bool powerwash_required);
+ public:
+ void ResumeRunningAction() {
+ ASSERT_NE(nullptr, postinstall_action_);
+ postinstall_action_->ResumeAction();
+ }
+
+ void SuspendRunningAction() {
+ if (!postinstall_action_ || !postinstall_action_->current_command_ ||
+ test_utils::Readlink(base::StringPrintf(
+ "/proc/%d/fd/0", postinstall_action_->current_command_)) !=
+ "/dev/zero") {
+ // We need to wait for the postinstall command to start and flag that it
+ // is ready by redirecting its input to /dev/zero.
+ loop_.PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&PostinstallRunnerActionTest::SuspendRunningAction,
+ base::Unretained(this)),
+ base::TimeDelta::FromMilliseconds(100));
+ } else {
+ postinstall_action_->SuspendAction();
+ // Schedule to be resumed in a little bit.
+ loop_.PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&PostinstallRunnerActionTest::ResumeRunningAction,
+ base::Unretained(this)),
+ base::TimeDelta::FromMilliseconds(100));
+ }
+ }
+
+ void CancelWhenStarted() {
+ if (!postinstall_action_ || !postinstall_action_->current_command_) {
+ // Wait for the postinstall command to run.
+ loop_.PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&PostinstallRunnerActionTest::CancelWhenStarted,
+ base::Unretained(this)),
+ base::TimeDelta::FromMilliseconds(10));
+ } else {
+ CHECK(processor_);
+ processor_->StopProcessing();
+ }
+ }
+
protected:
base::MessageLoopForIO base_loop_;
brillo::BaseMessageLoop loop_{&base_loop_};
@@ -112,6 +162,10 @@
FakeBootControl fake_boot_control_;
PostinstActionProcessorDelegate delegate_;
+
+ // A pointer to the posinstall_runner action and the processor.
+ PostinstallRunnerAction* postinstall_action_{nullptr};
+ ActionProcessor* processor_{nullptr};
};
void PostinstallRunnerActionTest::RunPosinstallAction(
@@ -119,6 +173,7 @@
const string& postinstall_program,
bool powerwash_required) {
ActionProcessor processor;
+ processor_ = &processor;
ObjectFeederAction<InstallPlan> feeder_action;
InstallPlan::Partition part;
part.name = "part";
@@ -132,6 +187,7 @@
feeder_action.set_obj(install_plan);
PostinstallRunnerAction runner_action(&fake_boot_control_,
powerwash_marker_file_.c_str());
+ postinstall_action_ = &runner_action;
BondActions(&feeder_action, &runner_action);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&runner_action, &collector_action);
@@ -144,15 +200,14 @@
base::Bind([&processor] { processor.StartProcessing(); }));
loop_.Run();
ASSERT_FALSE(processor.IsRunning());
- EXPECT_TRUE(delegate_.code_set_);
-}
-
-// Death tests don't seem to be working on Hardy
-TEST_F(PostinstallRunnerActionTest, DISABLED_RunAsRootDeathTest) {
- ASSERT_EQ(0U, getuid());
- PostinstallRunnerAction runner_action(&fake_boot_control_);
- ASSERT_DEATH({ runner_action.TerminateProcessing(); },
- "postinstall_runner_action.h:.*] Check failed");
+ postinstall_action_ = nullptr;
+ processor_ = nullptr;
+ EXPECT_TRUE(delegate_.processing_stopped_called_ ||
+ delegate_.processing_done_called_);
+ if (delegate_.processing_done_called_) {
+ // Sanity check that the code was set when the processor finishes.
+ EXPECT_TRUE(delegate_.code_set_);
+ }
}
// Test that postinstall succeeds in the simple case of running the default
@@ -161,6 +216,7 @@
ScopedLoopbackDeviceBinder loop(postinstall_image_, false, nullptr);
RunPosinstallAction(loop.dev(), kPostinstallDefaultScript, false);
EXPECT_EQ(ErrorCode::kSuccess, delegate_.code_);
+ EXPECT_TRUE(delegate_.processing_done_called_);
// Since powerwash_required was false, this should not trigger a powerwash.
EXPECT_FALSE(utils::FileExists(powerwash_marker_file_.c_str()));
@@ -229,4 +285,31 @@
}
#endif // __ANDROID__
+// Check that you can suspend/resume postinstall actions.
+TEST_F(PostinstallRunnerActionTest, RunAsRootSuspendResumeActionTest) {
+ ScopedLoopbackDeviceBinder loop(postinstall_image_, false, nullptr);
+
+ // We need to wait for the child to run and setup its signal handler.
+ loop_.PostTask(FROM_HERE,
+ base::Bind(&PostinstallRunnerActionTest::SuspendRunningAction,
+ base::Unretained(this)));
+ RunPosinstallAction(loop.dev(), "bin/postinst_suspend", false);
+ // postinst_suspend returns 0 only if it was suspended at some point.
+ EXPECT_EQ(ErrorCode::kSuccess, delegate_.code_);
+ EXPECT_TRUE(delegate_.processing_done_called_);
+}
+
+// Test that we can cancel a postinstall action while it is running.
+TEST_F(PostinstallRunnerActionTest, RunAsRootCancelPostinstallActionTest) {
+ ScopedLoopbackDeviceBinder loop(postinstall_image_, false, nullptr);
+
+ // Wait for the action to start and then cancel it.
+ CancelWhenStarted();
+ RunPosinstallAction(loop.dev(), "bin/postinst_suspend", false);
+ // When canceling the action, the action never finished and therefore we had
+ // a ProcessingStopped call instead.
+ EXPECT_FALSE(delegate_.code_set_);
+ EXPECT_TRUE(delegate_.processing_stopped_called_);
+}
+
} // namespace chromeos_update_engine