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/Android.mk b/Android.mk
index 20b9b5e..c8bf904 100644
--- a/Android.mk
+++ b/Android.mk
@@ -711,7 +711,7 @@
$(call ue-unittest-sample-image,disk_ext2_1k.img)
$(call ue-unittest-sample-image,disk_ext2_4k.img)
$(call ue-unittest-sample-image,disk_ext2_4k_empty.img)
-$(call ue-unittest-sample-image,disk_ext2_ue_settings.img)
+$(call ue-unittest-sample-image,disk_ext2_unittest.img)
# test_http_server (type: executable)
# ========================================================
@@ -747,7 +747,7 @@
ue_unittest_disk_ext2_1k.img \
ue_unittest_disk_ext2_4k.img \
ue_unittest_disk_ext2_4k_empty.img \
- ue_unittest_disk_ext2_ue_settings.img
+ ue_unittest_disk_ext2_unittest.img
LOCAL_MODULE_CLASS := EXECUTABLES
LOCAL_CPP_EXTENSION := .cc
LOCAL_CLANG := true
diff --git a/common/subprocess.cc b/common/subprocess.cc
index f43aaac..61f2d54 100644
--- a/common/subprocess.cc
+++ b/common/subprocess.cc
@@ -207,7 +207,12 @@
if (pid_record == subprocess_records_.end())
return;
pid_record->second->callback.Reset();
- kill(pid, SIGTERM);
+ if (kill(pid, SIGTERM) != 0) {
+ PLOG(WARNING) << "Error sending SIGTERM to " << pid;
+ }
+ // Release the pid now so we don't try to kill it if Subprocess is destroyed
+ // before the corresponding ChildExitedCallback() is called.
+ pid_record->second->proc.Release();
}
bool Subprocess::SynchronousExec(const vector<string>& cmd,
diff --git a/common/test_utils.cc b/common/test_utils.cc
index 0e7a8ef..4ab1663 100644
--- a/common/test_utils.cc
+++ b/common/test_utils.cc
@@ -103,6 +103,15 @@
0xbe, 0x9f, 0xa3, 0x5d,
};
+string Readlink(const string& path) {
+ vector<char> buf(PATH_MAX + 1);
+ ssize_t r = readlink(path.c_str(), buf.data(), buf.size());
+ if (r < 0)
+ return "";
+ CHECK_LT(r, static_cast<ssize_t>(buf.size()));
+ return string(buf.begin(), buf.begin() + r);
+}
+
bool IsXAttrSupported(const base::FilePath& dir_path) {
char *path = strdup(dir_path.Append("xattr_test_XXXXXX").value().c_str());
diff --git a/common/test_utils.h b/common/test_utils.h
index 2c8a6de..60ec90e 100644
--- a/common/test_utils.h
+++ b/common/test_utils.h
@@ -90,6 +90,9 @@
return chdir(path.c_str());
}
+// Reads a symlink from disk. Returns empty string on failure.
+std::string Readlink(const std::string& path);
+
// Checks if xattr is supported in the directory specified by
// |dir_path| which must be writable. Returns true if the feature is
// supported, false if not or if an error occurred.
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
diff --git a/payload_generator/ext2_filesystem_unittest.cc b/payload_generator/ext2_filesystem_unittest.cc
index f7d25da..17c72d6 100644
--- a/payload_generator/ext2_filesystem_unittest.cc
+++ b/payload_generator/ext2_filesystem_unittest.cc
@@ -189,8 +189,8 @@
}
TEST_F(Ext2FilesystemTest, LoadSettingsWorksTest) {
- base::FilePath path = test_utils::GetBuildArtifactsPath().Append(
- "gen/disk_ext2_ue_settings.img");
+ base::FilePath path =
+ test_utils::GetBuildArtifactsPath().Append("gen/disk_ext2_unittest.img");
unique_ptr<Ext2Filesystem> fs = Ext2Filesystem::CreateFromFile(path.value());
ASSERT_NE(nullptr, fs.get());
diff --git a/sample_images/generate_images.sh b/sample_images/generate_images.sh
index c9936c1..b461175 100755
--- a/sample_images/generate_images.sh
+++ b/sample_images/generate_images.sh
@@ -150,6 +150,22 @@
exit 1
EOF
+ # A program that succeeds if it is suspended during the first 5 minutes.
+ sudo tee "${mntdir}"/bin/postinst_suspend >/dev/null <<EOF
+#!/etc/../bin/sh
+trap "{ echo Got a SIGCONT; exit 0; }" CONT
+# Signal that we are ready to receive the signal by redirecting our stdin to
+# /dev/zero, the test can detect that.
+exec </dev/zero
+# Allow the signal handler to run every 100 ms.
+i=3000
+while [ \$i -ge 0 ]; do
+ sleep 0.1
+ i=\$((i-1))
+done
+exit 1
+EOF
+
# A postinstall bash program.
sudo tee "${mntdir}"/bin/self_check_context >/dev/null <<EOF
#!/etc/../bin/sh
@@ -193,7 +209,7 @@
sudo mount "${filename}" "${mntdir}" -o loop
case "${kind}" in
- ue_settings)
+ unittest)
add_files_ue_settings "${mntdir}" "${block_size}"
add_files_postinstall "${mntdir}" "${block_size}"
;;
@@ -223,7 +239,7 @@
generate_image disk_ext2_1k default 16777216 1024
generate_image disk_ext2_4k default 16777216 4096
generate_image disk_ext2_4k_empty empty $((1024 * 4096)) 4096
- generate_image disk_ext2_ue_settings ue_settings $((1024 * 4096)) 4096
+ generate_image disk_ext2_unittest unittest $((1024 * 4096)) 4096
# Generate the tarball and delete temporary images.
echo "Packing tar file sample_images.tar.bz2"
diff --git a/sample_images/sample_images.tar.bz2 b/sample_images/sample_images.tar.bz2
index d77ae93..20a698b 100644
--- a/sample_images/sample_images.tar.bz2
+++ b/sample_images/sample_images.tar.bz2
Binary files differ
