From 6de21f11125941ea1b94fdeec754bacea3916fd5 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 22 Aug 2017 15:41:03 -0700 Subject: [PATCH 1/4] init: cleanup environment handling Init keep its own copy of the environment that it uses for execve when starting services. This is unnecessary however as libc already has functions that mutate the environment and the environment that init uses is clean for starting services. This change removes init's copy of the environment and uses the libc functions instead. This also makes small clean-up to the way the Service class stores service specific environment variables. Test: boot bullhead Change-Id: I7c98a0b7aac9fa8f195ae33bd6a7515bb56faf78 --- init/builtins.cpp | 4 ++-- init/descriptors.cpp | 3 +-- init/init.cpp | 38 ++------------------------------------ init/init.h | 4 ---- init/selinux.cpp | 4 +--- init/service.cpp | 16 ++++------------ init/service.h | 9 +-------- 7 files changed, 11 insertions(+), 67 deletions(-) diff --git a/init/builtins.cpp b/init/builtins.cpp index f807343ab..593f718f1 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -159,8 +159,8 @@ static Result do_exec_start(const std::vector& args) { } static Result do_export(const std::vector& args) { - if (!add_environment(args[1].c_str(), args[2].c_str())) { - return Error(); + if (setenv(args[1].c_str(), args[2].c_str(), 1) == -1) { + return ErrnoError() << "setenv() failed"; } return Success(); } diff --git a/init/descriptors.cpp b/init/descriptors.cpp index cc5b948e9..62656872f 100644 --- a/init/descriptors.cpp +++ b/init/descriptors.cpp @@ -28,7 +28,6 @@ #include #include -#include "init.h" #include "util.h" namespace android { @@ -62,7 +61,7 @@ void DescriptorInfo::CreateAndPublish(const std::string& globalContext) const { [] (char& c) { c = isalnum(c) ? c : '_'; }); std::string val = std::to_string(fd); - add_environment(publishedName.c_str(), val.c_str()); + setenv(publishedName.c_str(), val.c_str(), 1); // make sure we don't close on exec fcntl(fd, F_SETFD, 0); diff --git a/init/init.cpp b/init/init.cpp index d0afac11d..e1bd3a2cb 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -71,8 +71,6 @@ static char qemu[32]; std::string default_console = "/dev/console"; -const char *ENV[32]; - static int epoll_fd = -1; static std::unique_ptr waiting_for_prop(nullptr); @@ -126,38 +124,6 @@ void register_epoll_handler(int fd, void (*fn)()) { } } -/* add_environment - add "key=value" to the current environment */ -int add_environment(const char *key, const char *val) -{ - size_t n; - size_t key_len = strlen(key); - - /* The last environment entry is reserved to terminate the list */ - for (n = 0; n < (arraysize(ENV) - 1); n++) { - - /* Delete any existing entry for this key */ - if (ENV[n] != NULL) { - size_t entry_key_len = strcspn(ENV[n], "="); - if ((entry_key_len == key_len) && (strncmp(ENV[n], key, entry_key_len) == 0)) { - free((char*)ENV[n]); - ENV[n] = NULL; - } - } - - /* Add entry if a free slot is available */ - if (ENV[n] == NULL) { - char* entry; - asprintf(&entry, "%s=%s", key, val); - ENV[n] = entry; - return 0; - } - } - - LOG(ERROR) << "No env. room to store: '" << key << "':'" << val << "'"; - - return -1; -} - bool start_waiting_for_property(const char *name, const char *value) { if (waiting_for_prop) { @@ -429,8 +395,6 @@ int main(int argc, char** argv) { install_reboot_signal_handlers(); } - add_environment("PATH", _PATH_DEFPATH); - bool is_first_stage = (getenv("INIT_SECOND_STAGE") == nullptr); if (is_first_stage) { @@ -439,6 +403,8 @@ int main(int argc, char** argv) { // Clear the umask. umask(0); + clearenv(); + setenv("PATH", _PATH_DEFPATH, 1); // Get the basic filesystem setup we need put together in the initramdisk // on / and then we'll let the rc file figure out the rest. mount("tmpfs", "/dev", "tmpfs", MS_NOSUID, "mode=0755"); diff --git a/init/init.h b/init/init.h index 50a7c8352..b757c1d65 100644 --- a/init/init.h +++ b/init/init.h @@ -30,9 +30,7 @@ namespace init { // Note: These globals are *only* valid in init, so they should not be used in ueventd, // watchdogd, or any files that may be included in those, such as devices.cpp and util.cpp. // TODO: Have an Init class and remove all globals. -extern const char *ENV[32]; extern std::string default_console; - extern std::vector late_import_paths; Parser CreateParser(ActionManager& action_manager, ServiceList& service_list); @@ -43,8 +41,6 @@ void property_changed(const std::string& name, const std::string& value); void register_epoll_handler(int fd, void (*fn)()); -int add_environment(const char* key, const char* val); - bool start_waiting_for_property(const char *name, const char *value); void DumpState(); diff --git a/init/selinux.cpp b/init/selinux.cpp index c8240286d..ef59164e3 100644 --- a/init/selinux.cpp +++ b/init/selinux.cpp @@ -48,7 +48,6 @@ #include "selinux.h" #include -#include #include #include #include @@ -126,8 +125,7 @@ bool ForkExecveAndWaitForCompletion(const char* filename, char* const argv[]) { } TEMP_FAILURE_RETRY(close(pipe_fds[1])); - const char* envp[] = {_PATH_DEFPATH, nullptr}; - if (execve(filename, argv, (char**)envp) == -1) { + if (execv(filename, argv) == -1) { PLOG(ERROR) << "Failed to execve " << filename; return false; } diff --git a/init/service.cpp b/init/service.cpp index 6ab60e3ca..de9538fbe 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -147,14 +147,6 @@ static void ExpandArgs(const std::vector& args, std::vector* strs->push_back(nullptr); } -ServiceEnvironmentInfo::ServiceEnvironmentInfo() { -} - -ServiceEnvironmentInfo::ServiceEnvironmentInfo(const std::string& name, - const std::string& value) - : name(name), value(value) { -} - unsigned long Service::next_start_order_ = 1; bool Service::is_exec_service_running_ = false; @@ -507,7 +499,7 @@ Result Service::ParseSeclabel(const std::vector& args) { } Result Service::ParseSetenv(const std::vector& args) { - envvars_.emplace_back(args[1], args[2]); + environment_vars_.emplace_back(args[1], args[2]); return Success(); } @@ -723,8 +715,8 @@ bool Service::Start() { SetUpPidNamespace(name_); } - for (const auto& ei : envvars_) { - add_environment(ei.name.c_str(), ei.value.c_str()); + for (const auto& [key, value] : environment_vars_) { + setenv(key.c_str(), value.c_str(), 1); } std::for_each(descriptors_.begin(), descriptors_.end(), @@ -779,7 +771,7 @@ bool Service::Start() { std::vector strs; ExpandArgs(args_, &strs); - if (execve(strs[0], (char**) &strs[0], (char**) ENV) < 0) { + if (execv(strs[0], (char**)&strs[0]) < 0) { PLOG(ERROR) << "cannot execve('" << strs[0] << "')"; } diff --git a/init/service.h b/init/service.h index fe851299e..be173cd64 100644 --- a/init/service.h +++ b/init/service.h @@ -57,13 +57,6 @@ namespace android { namespace init { -struct ServiceEnvironmentInfo { - ServiceEnvironmentInfo(); - ServiceEnvironmentInfo(const std::string& name, const std::string& value); - std::string name; - std::string value; -}; - class Service { public: Service(const std::string& name, const std::vector& args); @@ -178,7 +171,7 @@ class Service { std::string seclabel_; std::vector> descriptors_; - std::vector envvars_; + std::vector> environment_vars_; Action onrestart_; // Commands to execute on restart. From 130e3d7204d2b2d3d2ba956c3243fbc0fb1cabe4 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 22 Aug 2017 16:07:15 -0700 Subject: [PATCH 2/4] init: pass errors from one Result to another better Result currently has two problems, 1) A failing Result cannot be easily constructed from a Result's error. 2) errno is lost when passing .error() through multiple Result's This change fixes both problems having Result::error() return a ResultError class that contains the std::string error message and int errno. It additionally has ostream operators to continue to allow printing the error string directly to an ostream and also to pass the errno through to another Result class via Error() creation. Lastly, it provides a new constructor for Result for ResultError, such that a Result can be constructed from Result::error(). Test: boot bullhead, init unit tests Change-Id: Id9614b727cdabd2f5498b0da0e598e9aff7d9ae0 --- init/action.cpp | 2 +- init/result.h | 111 +++++++++++++++++++++++++++++++++---------- init/result_test.cpp | 78 ++++++++++++++++++++++++++++-- init/util_test.cpp | 10 ++-- 4 files changed, 166 insertions(+), 35 deletions(-) diff --git a/init/action.cpp b/init/action.cpp index f687074c6..a7aa7567a 100644 --- a/init/action.cpp +++ b/init/action.cpp @@ -98,7 +98,7 @@ void Action::ExecuteCommand(const Command& command) const { LOG(INFO) << "Command '" << cmd_str << "' action=" << trigger_name << " (" << filename_ << ":" << command.line() << ") took " << duration.count() << "ms and " - << (result ? "succeeded" : "failed: " + result.error()); + << (result ? "succeeded" : "failed: " + result.error_string()); } } diff --git a/init/result.h b/init/result.h index 64fa69f26..36c3b8324 100644 --- a/init/result.h +++ b/init/result.h @@ -21,9 +21,13 @@ // There are 3 classes that implement this functionality and one additional helper type. // // Result either contains a member of type T that can be accessed using similar semantics as -// std::optional or it contains a std::string describing an error, which can be accessed via +// std::optional or it contains a ResultError describing an error, which can be accessed via // Result::error(). // +// ResultError is a type that contains both a std::string describing the error and a copy of errno +// from when the error occurred. ResultError can be used in an ostream directly to print its +// string value. +// // Success is a typedef that aids in creating Result that do not contain a return value. // Result is the correct return type for a function that either returns successfully or // returns an error value. Returning Success() from a function that returns Result is the @@ -33,10 +37,20 @@ // to T or from the constructor arguments for T. This allows you to return a type T directly from // a function that returns Result. // -// Error and ErrnoError are used to construct a Result that has failed. Each of these classes -// take an ostream as an input and are implicitly cast to a Result containing that failure. -// ErrnoError() additionally appends ": " + strerror(errno) to the end of the failure string to aid -// in interacting with C APIs. +// Error and ErrnoError are used to construct a Result that has failed. The Error class takes +// an ostream as an input and are implicitly cast to a Result containing that failure. +// ErrnoError() is a helper function to create an Error class that appends ": " + strerror(errno) +// to the end of the failure string to aid in interacting with C APIs. Alternatively, an errno +// value can be directly specified via the Error() constructor. +// +// ResultError can be used in the ostream when using Error to construct a Result. In this case, +// the string that the ResultError takes is passed through the stream normally, but the errno is +// passed to the Result. This can be used to pass errno from a failing C function up multiple +// callers. +// +// ResultError can also directly construct a Result. This is particularly useful if you have a +// function that return Result but you have a Result and want to return its error. In this +// case, you can return the .error() from the Result to construct the Result. // An example of how to use these is below: // Result CalculateResult(const T& input) { @@ -66,9 +80,29 @@ namespace android { namespace init { +struct ResultError { + template + ResultError(T&& error_string, int error_errno) + : error_string(std::forward(error_string)), error_errno(error_errno) {} + + std::string error_string; + int error_errno; +}; + +inline std::ostream& operator<<(std::ostream& os, const ResultError& t) { + os << t.error_string; + return os; +} + +inline std::ostream& operator<<(std::ostream& os, ResultError&& t) { + os << std::move(t.error_string); + return os; +} + class Error { public: - Error() : append_errno_(0) {} + Error() : errno_(0), append_errno_(false) {} + Error(int errno_to_append) : errno_(errno_to_append), append_errno_(true) {} template Error&& operator<<(T&& t) { @@ -76,30 +110,45 @@ class Error { return std::move(*this); } - const std::string str() const { - if (append_errno_) { - return ss_.str() + ": " + strerror(append_errno_); - } - return ss_.str(); + Error&& operator<<(const ResultError& result_error) { + ss_ << result_error.error_string; + errno_ = result_error.error_errno; + return std::move(*this); } + Error&& operator<<(ResultError&& result_error) { + ss_ << std::move(result_error.error_string); + errno_ = result_error.error_errno; + return std::move(*this); + } + + const std::string str() const { + std::string str = ss_.str(); + if (append_errno_) { + if (str.empty()) { + return strerror(errno_); + } + return str + ": " + strerror(errno_); + } + return str; + } + + int get_errno() const { return errno_; } + Error(const Error&) = delete; Error(Error&&) = delete; Error& operator=(const Error&) = delete; Error& operator=(Error&&) = delete; - protected: - Error(int append_errno) : append_errno_(append_errno) {} - private: std::stringstream ss_; - int append_errno_; + int errno_; + bool append_errno_; }; -class ErrnoError : public Error { - public: - ErrnoError() : Error(errno) {} -}; +inline Error ErrnoError() { + return Error(errno); +} template class Result { @@ -107,7 +156,13 @@ class Result { template Result(U&&... result) : contents_(std::in_place_index_t<0>(), std::forward(result)...) {} - Result(Error&& fb) : contents_(std::in_place_index_t<1>(), fb.str()) {} + Result(Error&& error) : contents_(std::in_place_index_t<1>(), error.str(), error.get_errno()) {} + Result(const ResultError& result_error) + : contents_(std::in_place_index_t<1>(), result_error.error_string, + result_error.error_errno) {} + Result(ResultError&& result_error) + : contents_(std::in_place_index_t<1>(), std::move(result_error.error_string), + result_error.error_errno) {} bool has_value() const { return contents_.index() == 0; } @@ -116,9 +171,17 @@ class Result { T&& value() && { return std::get<0>(std::move(contents_)); } const T&& value() const && { return std::get<0>(std::move(contents_)); } - const std::string& error() const & { return std::get<1>(contents_); } - std::string&& error() && { return std::get<1>(std::move(contents_)); } - const std::string&& error() const && { return std::get<1>(std::move(contents_)); } + const ResultError& error() const & { return std::get<1>(contents_); } + ResultError&& error() && { return std::get<1>(std::move(contents_)); } + const ResultError&& error() const && { return std::get<1>(std::move(contents_)); } + + const std::string& error_string() const & { return std::get<1>(contents_).error_string; } + std::string&& error_string() && { return std::get<1>(std::move(contents_)).error_string; } + const std::string&& error_string() const && { + return std::get<1>(std::move(contents_)).error_string; + } + + int error_errno() const { return std::get<1>(contents_).error_errno; } explicit operator bool() const { return has_value(); } @@ -131,7 +194,7 @@ class Result { const T* operator->() const { return &value(); } private: - std::variant contents_; + std::variant contents_; }; using Success = std::monostate; diff --git a/init/result_test.cpp b/init/result_test.cpp index ca6501369..19caaf5b5 100644 --- a/init/result_test.cpp +++ b/init/result_test.cpp @@ -74,7 +74,8 @@ TEST(result, result_error) { ASSERT_FALSE(result); ASSERT_FALSE(result.has_value()); - EXPECT_EQ("failure1", result.error()); + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ("failure1", result.error_string()); } TEST(result, result_error_empty) { @@ -82,7 +83,8 @@ TEST(result, result_error_empty) { ASSERT_FALSE(result); ASSERT_FALSE(result.has_value()); - EXPECT_EQ("", result.error()); + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ("", result.error_string()); } TEST(result, result_error_rvalue) { @@ -96,7 +98,8 @@ TEST(result, result_error_rvalue) { ASSERT_FALSE(MakeRvalueErrorResult()); ASSERT_FALSE(MakeRvalueErrorResult().has_value()); - EXPECT_EQ("failure1", MakeRvalueErrorResult().error()); + EXPECT_EQ(0, MakeRvalueErrorResult().error_errno()); + EXPECT_EQ("failure1", MakeRvalueErrorResult().error_string()); } TEST(result, result_errno_error) { @@ -107,7 +110,72 @@ TEST(result, result_errno_error) { ASSERT_FALSE(result); ASSERT_FALSE(result.has_value()); - EXPECT_EQ("failure1: "s + strerror(test_errno), result.error()); + EXPECT_EQ(test_errno, result.error_errno()); + EXPECT_EQ("failure1: "s + strerror(test_errno), result.error_string()); +} + +TEST(result, result_errno_error_no_text) { + constexpr int test_errno = 6; + errno = test_errno; + Result result = ErrnoError(); + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ(test_errno, result.error_errno()); + EXPECT_EQ(strerror(test_errno), result.error_string()); +} + +TEST(result, result_error_from_other_result) { + auto error_text = "test error"s; + Result result = Error() << error_text; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + Result result2 = result.error(); + + ASSERT_FALSE(result2); + ASSERT_FALSE(result2.has_value()); + + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ(error_text, result.error_string()); +} + +TEST(result, result_error_through_ostream) { + auto error_text = "test error"s; + Result result = Error() << error_text; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + Result result2 = Error() << result.error(); + + ASSERT_FALSE(result2); + ASSERT_FALSE(result2.has_value()); + + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ(error_text, result.error_string()); +} + +TEST(result, result_errno_error_through_ostream) { + auto error_text = "test error"s; + constexpr int test_errno = 6; + errno = 6; + Result result = ErrnoError() << error_text; + + errno = 0; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + Result result2 = Error() << result.error(); + + ASSERT_FALSE(result2); + ASSERT_FALSE(result2.has_value()); + + EXPECT_EQ(test_errno, result.error_errno()); + EXPECT_EQ(error_text + ": " + strerror(test_errno), result.error_string()); } TEST(result, constructor_forwarding) { @@ -215,7 +283,7 @@ TEST(result, die_on_access_failed_result) { TEST(result, die_on_get_error_succesful_result) { Result result = "success"; - ASSERT_DEATH(result.error(), ""); + ASSERT_DEATH(result.error_string(), ""); } } // namespace init diff --git a/init/util_test.cpp b/init/util_test.cpp index 007d10ee3..3ae53a481 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -34,7 +34,7 @@ TEST(util, ReadFile_ENOENT) { auto file_contents = ReadFile("/proc/does-not-exist"); EXPECT_EQ(ENOENT, errno); ASSERT_FALSE(file_contents); - EXPECT_EQ("open() failed: No such file or directory", file_contents.error()); + EXPECT_EQ("open() failed: No such file or directory", file_contents.error_string()); } TEST(util, ReadFileGroupWriteable) { @@ -45,7 +45,7 @@ TEST(util, ReadFileGroupWriteable) { EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0620, AT_SYMLINK_NOFOLLOW)) << strerror(errno); auto file_contents = ReadFile(tf.path); ASSERT_FALSE(file_contents) << strerror(errno); - EXPECT_EQ("Skipping insecure file", file_contents.error()); + EXPECT_EQ("Skipping insecure file", file_contents.error_string()); } TEST(util, ReadFileWorldWiteable) { @@ -56,7 +56,7 @@ TEST(util, ReadFileWorldWiteable) { EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0602, AT_SYMLINK_NOFOLLOW)) << strerror(errno); auto file_contents = ReadFile(tf.path); ASSERT_FALSE(file_contents) << strerror(errno); - EXPECT_EQ("Skipping insecure file", file_contents.error()); + EXPECT_EQ("Skipping insecure file", file_contents.error_string()); } TEST(util, ReadFileSymbolicLink) { @@ -65,7 +65,7 @@ TEST(util, ReadFileSymbolicLink) { auto file_contents = ReadFile("/charger"); EXPECT_EQ(ELOOP, errno); ASSERT_FALSE(file_contents); - EXPECT_EQ("open() failed: Too many symbolic links encountered", file_contents.error()); + EXPECT_EQ("open() failed: Too many symbolic links encountered", file_contents.error_string()); } TEST(util, ReadFileSuccess) { @@ -130,7 +130,7 @@ TEST(util, DecodeUid) { decoded_uid = DecodeUid("toot"); EXPECT_FALSE(decoded_uid); - EXPECT_EQ("getpwnam failed: No such file or directory", decoded_uid.error()); + EXPECT_EQ("getpwnam failed: No such file or directory", decoded_uid.error_string()); decoded_uid = DecodeUid("123"); EXPECT_TRUE(decoded_uid); From 76af7e6a0c4ce7759e6cc5994b5496ddb09035ee Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 22 Aug 2017 16:13:59 -0700 Subject: [PATCH 3/4] init: log Service failures via Result Log Service failures via Result such that their context can be captured when interacting with services through builtin functions. Test: boot bullhead Change-Id: I4d99744d64008d4a06a404e3c9817182c6e177bc --- init/builtins.cpp | 16 ++++++++----- init/service.cpp | 61 ++++++++++++++++++++++------------------------- init/service.h | 8 +++---- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/init/builtins.cpp b/init/builtins.cpp index 593f718f1..3d0ba5547 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -127,7 +127,9 @@ static Result do_enable(const std::vector& args) { Service* svc = ServiceList::GetInstance().FindService(args[1]); if (!svc) return Error() << "Could not find service"; - if (!svc->Enable()) return Error() << "Could not enable service"; + if (auto result = svc->Enable(); !result) { + return Error() << "Could not enable service: " << result.error(); + } return Success(); } @@ -137,8 +139,8 @@ static Result do_exec(const std::vector& args) { if (!service) { return Error() << "Could not create exec service"; } - if (!service->ExecStart()) { - return Error() << "Could not start exec service"; + if (auto result = service->ExecStart(); !result) { + return Error() << "Could not start exec service: " << result.error(); } ServiceList::GetInstance().AddService(std::move(service)); @@ -151,8 +153,8 @@ static Result do_exec_start(const std::vector& args) { return Error() << "Service not found"; } - if (!service->ExecStart()) { - return Error() << "Could not start Service"; + if (auto result = service->ExecStart(); !result) { + return Error() << "Could not start exec service: " << result.error(); } return Success(); @@ -583,7 +585,9 @@ static Result do_setrlimit(const std::vector& args) { static Result do_start(const std::vector& args) { Service* svc = ServiceList::GetInstance().FindService(args[1]); if (!svc) return Error() << "service " << args[1] << " not found"; - if (!svc->Start()) return Error() << "failed to start service"; + if (auto result = svc->Start(); !result) { + return Error() << "Could not start service: " << result.error(); + } return Success(); } diff --git a/init/service.cpp b/init/service.cpp index de9538fbe..d3c9f9236 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -57,22 +57,20 @@ using android::base::WriteStringToFile; namespace android { namespace init { -static std::string ComputeContextFromExecutable(std::string& service_name, - const std::string& service_path) { +static Result ComputeContextFromExecutable(std::string& service_name, + const std::string& service_path) { std::string computed_context; char* raw_con = nullptr; char* raw_filecon = nullptr; if (getcon(&raw_con) == -1) { - LOG(ERROR) << "could not get context while starting '" << service_name << "'"; - return ""; + return Error() << "Could not get security context"; } std::unique_ptr mycon(raw_con); if (getfilecon(service_path.c_str(), &raw_filecon) == -1) { - LOG(ERROR) << "could not get file context while starting '" << service_name << "'"; - return ""; + return Error() << "Could not get file context"; } std::unique_ptr filecon(raw_filecon); @@ -84,12 +82,10 @@ static std::string ComputeContextFromExecutable(std::string& service_name, free(new_con); } if (rc == 0 && computed_context == mycon.get()) { - LOG(ERROR) << "service " << service_name << " does not have a SELinux domain defined"; - return ""; + return Error() << "Service does not have an SELinux domain defined"; } if (rc < 0) { - LOG(ERROR) << "could not get context while starting '" << service_name << "'"; - return ""; + return Error() << "Could not get process context"; } return computed_context; } @@ -629,16 +625,16 @@ Result Service::ParseLine(const std::vector& args) { static const OptionParserMap parser_map; auto parser = parser_map.FindFunction(args); - if (!parser) return Error() << parser.error(); + if (!parser) return parser.error(); return std::invoke(*parser, this, args); } -bool Service::ExecStart() { +Result Service::ExecStart() { flags_ |= SVC_ONESHOT; - if (!Start()) { - return false; + if (auto result = Start(); !result) { + return result; } flags_ |= SVC_EXEC; @@ -648,10 +644,10 @@ bool Service::ExecStart() { << supp_gids_.size() << " context " << (!seclabel_.empty() ? seclabel_ : "default") << ") started; waiting..."; - return true; + return Success(); } -bool Service::Start() { +Result Service::Start() { // Starting a service removes it from the disabled or reset state and // immediately takes it out of the restarting state if it was in there. flags_ &= (~(SVC_DISABLED|SVC_RESTARTING|SVC_RESET|SVC_RESTART|SVC_DISABLED_START)); @@ -660,7 +656,8 @@ bool Service::Start() { // process of exiting, we've ensured that they will immediately restart // on exit, unless they are ONESHOT. if (flags_ & SVC_RUNNING) { - return false; + // It is not an error to try to start a service that is already running. + return Success(); } bool needs_console = (flags_ & SVC_CONSOLE); @@ -673,28 +670,27 @@ bool Service::Start() { // properly registered for the device node int console_fd = open(console_.c_str(), O_RDWR | O_CLOEXEC); if (console_fd < 0) { - PLOG(ERROR) << "service '" << name_ << "' couldn't open console '" << console_ << "'"; flags_ |= SVC_DISABLED; - return false; + return ErrnoError() << "Couldn't open console '" << console_ << "'"; } close(console_fd); } struct stat sb; if (stat(args_[0].c_str(), &sb) == -1) { - PLOG(ERROR) << "cannot find '" << args_[0] << "', disabling '" << name_ << "'"; flags_ |= SVC_DISABLED; - return false; + return ErrnoError() << "Cannot find '" << args_[0] << "'"; } std::string scon; if (!seclabel_.empty()) { scon = seclabel_; } else { - scon = ComputeContextFromExecutable(name_, args_[0]); - if (scon == "") { - return false; + auto result = ComputeContextFromExecutable(name_, args_[0]); + if (!result) { + return result.error(); } + scon = *result; } LOG(INFO) << "starting service '" << name_ << "'..."; @@ -779,9 +775,8 @@ bool Service::Start() { } if (pid < 0) { - PLOG(ERROR) << "failed to fork for '" << name_ << "'"; pid_ = 0; - return false; + return ErrnoError() << "Failed to fork"; } if (oom_score_adjust_ != -1000) { @@ -823,24 +818,24 @@ bool Service::Start() { } NotifyStateChange("running"); - return true; + return Success(); } -bool Service::StartIfNotDisabled() { +Result Service::StartIfNotDisabled() { if (!(flags_ & SVC_DISABLED)) { return Start(); } else { flags_ |= SVC_DISABLED_START; } - return true; + return Success(); } -bool Service::Enable() { +Result Service::Enable() { flags_ &= ~(SVC_DISABLED | SVC_RC_DISABLED); if (flags_ & SVC_DISABLED_START) { return Start(); } - return true; + return Success(); } void Service::Reset() { @@ -866,7 +861,9 @@ void Service::Restart() { StopOrReset(SVC_RESTART); } else if (!(flags_ & SVC_RESTARTING)) { /* Just start the service since it's not running. */ - Start(); + if (auto result = Start(); !result) { + LOG(ERROR) << "Could not restart '" << name_ << "': " << result.error(); + } } /* else: Service is restarting anyways. */ } diff --git a/init/service.h b/init/service.h index be173cd64..1f2c44f93 100644 --- a/init/service.h +++ b/init/service.h @@ -70,10 +70,10 @@ class Service { bool IsRunning() { return (flags_ & SVC_RUNNING) != 0; } Result ParseLine(const std::vector& args); - bool ExecStart(); - bool Start(); - bool StartIfNotDisabled(); - bool Enable(); + Result ExecStart(); + Result Start(); + Result StartIfNotDisabled(); + Result Enable(); void Reset(); void Stop(); void Terminate(); From 68f2a4614518468f1320ad3e62a6db554e509fb1 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 22 Aug 2017 16:15:26 -0700 Subject: [PATCH 4/4] init: enable error reporting of builtin functions Enable error reporting when builtin functions fail. These errors are now reported with full context including the source file and line number, e.g. init: Command 'write /sys/module/subsystem_restart/parameters/enable_debug ${persist.sys.ssr.enable_debug}' action=early-boot (/init.bullhead.rc:84) took 0ms and failed: cannot expand '${persist.sys.ssr.enable_debug}' There are two small caveats: 1) There are nearly 200 reports of builtins failure due to "No such file or directory". Many of these are due to legacy paths included in rootdir/init.rc. Until they are cleaned up, reporting of these failures is disabled. 2) Similarly, symlink is often used to create backwards compatible symlinks. By their very nature, these calls are expected to fail on newer systems that do already use the new path. Due to this, failures of symlink due to EEXIST are not reported. Bug: 38038887 Test: boot bullhead, only see true errors reported from builtins. Change-Id: I316c13e3adc992cacc6d79ffee987adc8738fca0 --- init/action.cpp | 13 ++++++++++++- init/builtins.cpp | 5 +++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/init/action.cpp b/init/action.cpp index a7aa7567a..60204a8d8 100644 --- a/init/action.cpp +++ b/init/action.cpp @@ -91,8 +91,19 @@ void Action::ExecuteCommand(const Command& command) const { auto result = command.InvokeFunc(); auto duration = t.duration(); + // There are many legacy paths in rootdir/init.rc that will virtually never exist on a new + // device, such as '/sys/class/leds/jogball-backlight/brightness'. As of this writing, there + // are 198 such failures on bullhead. Instead of spamming the log reporting them, we do not + // report such failures unless we're running at the DEBUG log level. + bool report_failure = !result.has_value(); + if (report_failure && android::base::GetMinimumLogSeverity() > android::base::DEBUG && + result.error_errno() == ENOENT) { + report_failure = false; + } + // Any action longer than 50ms will be warned to user as slow operation - if (duration > 50ms || android::base::GetMinimumLogSeverity() <= android::base::DEBUG) { + if (report_failure || duration > 50ms || + android::base::GetMinimumLogSeverity() <= android::base::DEBUG) { std::string trigger_name = BuildTriggersString(); std::string cmd_str = command.BuildCommandString(); diff --git a/init/builtins.cpp b/init/builtins.cpp index 3d0ba5547..54ccf091e 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -612,6 +612,11 @@ static Result do_trigger(const std::vector& args) { static Result do_symlink(const std::vector& args) { if (symlink(args[1].c_str(), args[2].c_str()) < 0) { + // The symlink builtin is often used to create symlinks for older devices to be backwards + // compatible with new paths, therefore we skip reporting this error. + if (errno == EEXIST && android::base::GetMinimumLogSeverity() > android::base::DEBUG) { + return Success(); + } return ErrnoError() << "symlink() failed"; } return Success();