From 11a3aeeae3dc887b889d4086d4d26d95c324c08d Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Thu, 3 Aug 2017 12:54:07 -0700 Subject: [PATCH 1/3] init: introduce Result for return values and error handling init tries to propagate error information up to build context before logging errors. This is a good thing, however too often init has the overly verbose paradigm for error handling, below: bool CalculateResult(const T& input, U* output, std::string* err) bool CalculateAndUseResult(const T& input, std::string* err) { U output; std::string calculate_result_err; if (!CalculateResult(input, &output, &calculate_result_err)) { *err = "CalculateResult " + input + " failed: " + calculate_result_err; return false; } UseResult(output); return true; } Even more common are functions that return only true/false but also require passing a std::string* err in order to see the error message. This change introduces a Result that is use to either hold a successful return value of type T or to hold an error message as a std::string. If the functional only returns success or a failure with an error message, Result may be used. The classes Error and ErrnoError are used to indicate a failed Result. A successful Result is constructed implicitly from any type that can be implicitly converted 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 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. The end result is that the above code snippet is turned into the much clearer example below: Result CalculateResult(const T& input); Result CalculateAndUseResult(const T& input) { auto output = CalculateResult(input); if (!output) { return Error() << "CalculateResult " << input << " failed: " << output.error(); } UseResult(*output); return Success(); } This change also makes this conversion for some of the util.cpp functions that used the old paradigm. Test: boot bullhead, init unit tests Merged-In: I1e7d3a8820a79362245041251057fbeed2f7979b Change-Id: I1e7d3a8820a79362245041251057fbeed2f7979b --- init/Android.bp | 1 + init/builtins.cpp | 58 +++++----- init/init_test.cpp | 5 +- init/parser.cpp | 11 +- init/property_service.cpp | 12 +-- init/result.h | 142 ++++++++++++++++++++++++ init/result_test.cpp | 222 ++++++++++++++++++++++++++++++++++++++ init/selinux.cpp | 5 +- init/service.cpp | 67 ++++++------ init/service_test.cpp | 28 ++--- init/util.cpp | 60 ++++------- init/util.h | 8 +- init/util_test.cpp | 115 ++++++++------------ 13 files changed, 528 insertions(+), 206 deletions(-) create mode 100644 init/result.h create mode 100644 init/result_test.cpp diff --git a/init/Android.bp b/init/Android.bp index db64f71d4..b1f02793f 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -159,6 +159,7 @@ cc_test { "devices_test.cpp", "init_test.cpp", "property_service_test.cpp", + "result_test.cpp", "service_test.cpp", "ueventd_test.cpp", "util_test.cpp", diff --git a/init/builtins.cpp b/init/builtins.cpp index 944fcee22..28f60e6ac 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -151,9 +151,8 @@ static int do_class_restart(const std::vector& args) { } static int do_domainname(const std::vector& args) { - std::string err; - if (!WriteFile("/proc/sys/kernel/domainname", args[1], &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile("/proc/sys/kernel/domainname", args[1]); !result) { + LOG(ERROR) << "Unable to write to /proc/sys/kernel/domainname: " << result.error(); return -1; } return 0; @@ -199,9 +198,8 @@ static int do_export(const std::vector& args) { } static int do_hostname(const std::vector& args) { - std::string err; - if (!WriteFile("/proc/sys/kernel/hostname", args[1], &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile("/proc/sys/kernel/hostname", args[1]); !result) { + LOG(ERROR) << "Unable to write to /proc/sys/kernel/hostname: " << result.error(); return -1; } return 0; @@ -244,22 +242,22 @@ static int do_mkdir(const std::vector& args) { } if (args.size() >= 4) { - uid_t uid; - std::string decode_uid_err; - if (!DecodeUid(args[3], &uid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[3] << "': " << decode_uid_err; + auto uid = DecodeUid(args[3]); + if (!uid) { + LOG(ERROR) << "Unable to decode UID for '" << args[3] << "': " << uid.error(); return -1; } - gid_t gid = -1; + Result gid = -1; if (args.size() == 5) { - if (!DecodeUid(args[4], &gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find GID for '" << args[3] << "': " << decode_uid_err; + gid = DecodeUid(args[4]); + if (!gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[3] << "': " << gid.error(); return -1; } } - if (lchown(args[1].c_str(), uid, gid) == -1) { + if (lchown(args[1].c_str(), *uid, *gid) == -1) { return -errno; } @@ -651,9 +649,8 @@ static int do_verity_update_state(const std::vector& args) { } static int do_write(const std::vector& args) { - std::string err; - if (!WriteFile(args[1], args[2], &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile(args[1], args[2]); !result) { + LOG(ERROR) << "Unable to write to file '" << args[1] << "': " << result.error(); return -1; } return 0; @@ -720,39 +717,38 @@ static int do_readahead(const std::vector& args) { } static int do_copy(const std::vector& args) { - std::string data; - std::string err; - if (!ReadFile(args[1], &data, &err)) { - LOG(ERROR) << err; + auto file_contents = ReadFile(args[1]); + if (!file_contents) { + LOG(ERROR) << "Could not read input file '" << args[1] << "': " << file_contents.error(); return -1; } - if (!WriteFile(args[2], data, &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile(args[2], *file_contents); !result) { + LOG(ERROR) << "Could not write to output file '" << args[2] << "': " << result.error(); return -1; } return 0; } static int do_chown(const std::vector& args) { - uid_t uid; - std::string decode_uid_err; - if (!DecodeUid(args[1], &uid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[1] << "': " << decode_uid_err; + auto uid = DecodeUid(args[1]); + if (!uid) { + LOG(ERROR) << "Unable to decode UID for '" << args[1] << "': " << uid.error(); return -1; } // GID is optional and pushes the index of path out by one if specified. const std::string& path = (args.size() == 4) ? args[3] : args[2]; - gid_t gid = -1; + Result gid = -1; if (args.size() == 4) { - if (!DecodeUid(args[2], &gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find GID for '" << args[2] << "': " << decode_uid_err; + gid = DecodeUid(args[2]); + if (!gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[2] << "': " << gid.error(); return -1; } } - if (lchown(path.c_str(), uid, gid) == -1) return -errno; + if (lchown(path.c_str(), *uid, *gid) == -1) return -errno; return 0; } diff --git a/init/init_test.cpp b/init/init_test.cpp index 20622901c..58e3d758e 100644 --- a/init/init_test.cpp +++ b/init/init_test.cpp @@ -156,10 +156,9 @@ TEST(init, EventTriggerOrderMultipleFiles) { "execute 3"; // clang-format on // WriteFile() ensures the right mode is set - std::string err; - ASSERT_TRUE(WriteFile(std::string(dir.path) + "/a.rc", dir_a_script, &err)); + ASSERT_TRUE(WriteFile(std::string(dir.path) + "/a.rc", dir_a_script)); - ASSERT_TRUE(WriteFile(std::string(dir.path) + "/b.rc", "on boot\nexecute 5", &err)); + ASSERT_TRUE(WriteFile(std::string(dir.path) + "/b.rc", "on boot\nexecute 5")); // clang-format off std::string start_script = "import " + std::string(first_import.path) + "\n" diff --git a/init/parser.cpp b/init/parser.cpp index c6f4f459b..4c34c265b 100644 --- a/init/parser.cpp +++ b/init/parser.cpp @@ -102,15 +102,14 @@ void Parser::ParseData(const std::string& filename, const std::string& data) { bool Parser::ParseConfigFile(const std::string& path) { LOG(INFO) << "Parsing file " << path << "..."; android::base::Timer t; - std::string data; - std::string err; - if (!ReadFile(path, &data, &err)) { - LOG(ERROR) << err; + auto config_contents = ReadFile(path); + if (!config_contents) { + LOG(ERROR) << "Unable to read config file '" << path << "': " << config_contents.error(); return false; } - data.push_back('\n'); // TODO: fix parse_config. - ParseData(path, data); + config_contents->push_back('\n'); // TODO: fix parse_config. + ParseData(path, *config_contents); for (const auto& [section_name, section_parser] : section_parsers_) { section_parser->EndFile(); } diff --git a/init/property_service.cpp b/init/property_service.cpp index 1db8cb7d3..e07550a39 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -588,14 +588,14 @@ static void load_properties(char *data, const char *filter) // "ro.foo.*" is a prefix match, and "ro.foo.bar" is an exact match. static void load_properties_from_file(const char* filename, const char* filter) { Timer t; - std::string data; - std::string err; - if (!ReadFile(filename, &data, &err)) { - PLOG(WARNING) << "Couldn't load property file: " << err; + auto file_contents = ReadFile(filename); + if (!file_contents) { + PLOG(WARNING) << "Couldn't load property file '" << filename + << "': " << file_contents.error(); return; } - data.push_back('\n'); - load_properties(&data[0], filter); + file_contents->push_back('\n'); + load_properties(file_contents->data(), filter); LOG(VERBOSE) << "(Loading properties from " << filename << " took " << t << ".)"; } diff --git a/init/result.h b/init/result.h new file mode 100644 index 000000000..64fa69f26 --- /dev/null +++ b/init/result.h @@ -0,0 +1,142 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// This file contains classes for returning a successful result along with an optional +// arbitrarily typed return value or for returning a failure result along with an optional string +// indicating why the function failed. + +// 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 +// Result::error(). +// +// 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 +// correct way to indicate that a function without a return type has completed successfully. +// +// A successful Result is constructed implicitly from any type that can be implicitly converted +// 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. + +// An example of how to use these is below: +// Result CalculateResult(const T& input) { +// U output; +// if (!SomeOtherCppFunction(input, &output)) { +// return Error() << "SomeOtherCppFunction(" << input << ") failed"; +// } +// if (!c_api_function(output)) { +// return ErrnoError() << "c_api_function(" << output << ") failed"; +// } +// return output; +// } +// +// auto output = CalculateResult(input); +// if (!output) return Error() << "CalculateResult failed: " << output.error(); +// UseOutput(*output); + +#ifndef _INIT_RESULT_H +#define _INIT_RESULT_H + +#include + +#include +#include +#include + +namespace android { +namespace init { + +class Error { + public: + Error() : append_errno_(0) {} + + template + Error&& operator<<(T&& t) { + ss_ << std::forward(t); + return std::move(*this); + } + + const std::string str() const { + if (append_errno_) { + return ss_.str() + ": " + strerror(append_errno_); + } + return ss_.str(); + } + + 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_; +}; + +class ErrnoError : public Error { + public: + ErrnoError() : Error(errno) {} +}; + +template +class Result { + public: + 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()) {} + + bool has_value() const { return contents_.index() == 0; } + + T& value() & { return std::get<0>(contents_); } + const T& value() const & { return std::get<0>(contents_); } + 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_)); } + + explicit operator bool() const { return has_value(); } + + T& operator*() & { return value(); } + const T& operator*() const & { return value(); } + T&& operator*() && { return std::move(value()); } + const T&& operator*() const && { return std::move(value()); } + + T* operator->() { return &value(); } + const T* operator->() const { return &value(); } + + private: + std::variant contents_; +}; + +using Success = std::monostate; + +} // namespace init +} // namespace android + +#endif diff --git a/init/result_test.cpp b/init/result_test.cpp new file mode 100644 index 000000000..ca6501369 --- /dev/null +++ b/init/result_test.cpp @@ -0,0 +1,222 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "result.h" + +#include "errno.h" + +#include + +#include + +using namespace std::string_literals; + +namespace android { +namespace init { + +TEST(result, result_accessors) { + Result result = "success"; + ASSERT_TRUE(result); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ("success", *result); + EXPECT_EQ("success", result.value()); + + EXPECT_EQ('s', result->data()[0]); +} + +TEST(result, result_accessors_rvalue) { + ASSERT_TRUE(Result("success")); + ASSERT_TRUE(Result("success").has_value()); + + EXPECT_EQ("success", *Result("success")); + EXPECT_EQ("success", Result("success").value()); + + EXPECT_EQ('s', Result("success")->data()[0]); +} + +TEST(result, result_success) { + Result result = Success(); + ASSERT_TRUE(result); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ(Success(), *result); + EXPECT_EQ(Success(), result.value()); +} + +TEST(result, result_success_rvalue) { + // Success() doesn't actually create a Result object, but rather an object that can be + // implicitly constructed into a Result object. + + auto MakeRvalueSuccessResult = []() -> Result { return Success(); }; + ASSERT_TRUE(MakeRvalueSuccessResult()); + ASSERT_TRUE(MakeRvalueSuccessResult().has_value()); + + EXPECT_EQ(Success(), *MakeRvalueSuccessResult()); + EXPECT_EQ(Success(), MakeRvalueSuccessResult().value()); +} + +TEST(result, result_error) { + Result result = Error() << "failure" << 1; + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ("failure1", result.error()); +} + +TEST(result, result_error_empty) { + Result result = Error(); + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ("", result.error()); +} + +TEST(result, result_error_rvalue) { + // Error() and ErrnoError() aren't actually used to create a Result object. + // Under the hood, they are an intermediate class that can be implicitly constructed into a + // Result. This is needed both to create the ostream and because Error() itself, by + // definition will not know what the type, T, of the underlying Result object that it would + // create is. + + auto MakeRvalueErrorResult = []() -> Result { return Error() << "failure" << 1; }; + ASSERT_FALSE(MakeRvalueErrorResult()); + ASSERT_FALSE(MakeRvalueErrorResult().has_value()); + + EXPECT_EQ("failure1", MakeRvalueErrorResult().error()); +} + +TEST(result, result_errno_error) { + constexpr int test_errno = 6; + errno = test_errno; + Result result = ErrnoError() << "failure" << 1; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ("failure1: "s + strerror(test_errno), result.error()); +} + +TEST(result, constructor_forwarding) { + auto result = Result(5, 'a'); + + ASSERT_TRUE(result); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ("aaaaa", *result); +} + +struct ConstructorTracker { + static size_t constructor_called; + static size_t copy_constructor_called; + static size_t move_constructor_called; + static size_t copy_assignment_called; + static size_t move_assignment_called; + + template + ConstructorTracker(T&& string) : string(string) { + ++constructor_called; + } + + ConstructorTracker(const ConstructorTracker& ct) { + ++copy_constructor_called; + string = ct.string; + } + ConstructorTracker(ConstructorTracker&& ct) noexcept { + ++move_constructor_called; + string = std::move(ct.string); + } + ConstructorTracker& operator=(const ConstructorTracker& ct) { + ++copy_assignment_called; + string = ct.string; + return *this; + } + ConstructorTracker& operator=(ConstructorTracker&& ct) noexcept { + ++move_assignment_called; + string = std::move(ct.string); + return *this; + } + + std::string string; +}; + +size_t ConstructorTracker::constructor_called = 0; +size_t ConstructorTracker::copy_constructor_called = 0; +size_t ConstructorTracker::move_constructor_called = 0; +size_t ConstructorTracker::copy_assignment_called = 0; +size_t ConstructorTracker::move_assignment_called = 0; + +Result ReturnConstructorTracker(const std::string& in) { + if (in.empty()) { + return "literal string"; + } + if (in == "test2") { + return ConstructorTracker(in + in + "2"); + } + ConstructorTracker result(in + " " + in); + return result; +}; + +TEST(result, no_copy_on_return) { + // If returning parameters that may be used to implicitly construct the type T of Result, + // then those parameters are forwarded to the construction of Result. + + // If returning an prvalue or xvalue, it will be move constructed during the construction of + // Result. + + // This check ensures that that is the case, and particularly that no copy constructors + // are called. + + auto result1 = ReturnConstructorTracker(""); + ASSERT_TRUE(result1); + EXPECT_EQ("literal string", result1->string); + EXPECT_EQ(1U, ConstructorTracker::constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::move_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_assignment_called); + EXPECT_EQ(0U, ConstructorTracker::move_assignment_called); + + auto result2 = ReturnConstructorTracker("test2"); + ASSERT_TRUE(result2); + EXPECT_EQ("test2test22", result2->string); + EXPECT_EQ(2U, ConstructorTracker::constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_constructor_called); + EXPECT_EQ(1U, ConstructorTracker::move_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_assignment_called); + EXPECT_EQ(0U, ConstructorTracker::move_assignment_called); + + auto result3 = ReturnConstructorTracker("test3"); + ASSERT_TRUE(result3); + EXPECT_EQ("test3 test3", result3->string); + EXPECT_EQ(3U, ConstructorTracker::constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_constructor_called); + EXPECT_EQ(2U, ConstructorTracker::move_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_assignment_called); + EXPECT_EQ(0U, ConstructorTracker::move_assignment_called); +} + +TEST(result, die_on_access_failed_result) { + Result result = Error(); + ASSERT_DEATH(*result, ""); +} + +TEST(result, die_on_get_error_succesful_result) { + Result result = "success"; + ASSERT_DEATH(result.error(), ""); +} + +} // namespace init +} // namespace android diff --git a/init/selinux.cpp b/init/selinux.cpp index 3a4a71548..b9305ed47 100644 --- a/init/selinux.cpp +++ b/init/selinux.cpp @@ -339,9 +339,8 @@ void SelinuxInitialize() { } } - std::string err; - if (!WriteFile("/sys/fs/selinux/checkreqprot", "0", &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile("/sys/fs/selinux/checkreqprot", "0"); !result) { + LOG(ERROR) << "Unable to write to /sys/fs/selinux/checkreqprot: " << result.error(); panic(); } diff --git a/init/service.cpp b/init/service.cpp index 6f756fa6b..e37888bf8 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -386,18 +386,20 @@ bool Service::ParseDisabled(const std::vector& args, std::string* e } bool Service::ParseGroup(const std::vector& args, std::string* err) { - std::string decode_uid_err; - if (!DecodeUid(args[1], &gid_, &decode_uid_err)) { - *err = "Unable to find GID for '" + args[1] + "': " + decode_uid_err; + auto gid = DecodeUid(args[1]); + if (!gid) { + *err = "Unable to decode GID for '" + args[1] + "': " + gid.error(); return false; } + gid_ = *gid; + for (std::size_t n = 2; n < args.size(); n++) { - gid_t gid; - if (!DecodeUid(args[n], &gid, &decode_uid_err)) { - *err = "Unable to find GID for '" + args[n] + "': " + decode_uid_err; + gid = DecodeUid(args[n]); + if (!gid) { + *err = "Unable to decode GID for '" + args[n] + "': " + gid.error(); return false; } - supp_gids_.emplace_back(gid); + supp_gids_.emplace_back(*gid); } return true; } @@ -527,26 +529,27 @@ bool Service::ParseShutdown(const std::vector& args, std::string* e template bool Service::AddDescriptor(const std::vector& args, std::string* err) { int perm = args.size() > 3 ? std::strtoul(args[3].c_str(), 0, 8) : -1; - uid_t uid = 0; - gid_t gid = 0; + Result uid = 0; + Result gid = 0; std::string context = args.size() > 6 ? args[6] : ""; - std::string decode_uid_err; if (args.size() > 4) { - if (!DecodeUid(args[4], &uid, &decode_uid_err)) { - *err = "Unable to find UID for '" + args[4] + "': " + decode_uid_err; + uid = DecodeUid(args[4]); + if (!uid) { + *err = "Unable to find UID for '" + args[4] + "': " + uid.error(); return false; } } if (args.size() > 5) { - if (!DecodeUid(args[5], &gid, &decode_uid_err)) { - *err = "Unable to find GID for '" + args[5] + "': " + decode_uid_err; + gid = DecodeUid(args[5]); + if (!gid) { + *err = "Unable to find GID for '" + args[5] + "': " + gid.error(); return false; } } - auto descriptor = std::make_unique(args[1], args[2], uid, gid, perm, context); + auto descriptor = std::make_unique(args[1], args[2], *uid, *gid, perm, context); auto old = std::find_if(descriptors_.begin(), descriptors_.end(), @@ -585,11 +588,12 @@ bool Service::ParseFile(const std::vector& args, std::string* err) } bool Service::ParseUser(const std::vector& args, std::string* err) { - std::string decode_uid_err; - if (!DecodeUid(args[1], &uid_, &decode_uid_err)) { - *err = "Unable to find UID for '" + args[1] + "': " + decode_uid_err; + auto uid = DecodeUid(args[1]); + if (!uid) { + *err = "Unable to find UID for '" + args[1] + "': " + uid.error(); return false; } + uid_ = *uid; return true; } @@ -979,34 +983,35 @@ std::unique_ptr Service::MakeTemporaryOneshotService(const std::vector< if (command_arg > 2 && args[1] != "-") { seclabel = args[1]; } - uid_t uid = 0; + Result uid = 0; if (command_arg > 3) { - std::string decode_uid_err; - if (!DecodeUid(args[2], &uid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[2] << "': " << decode_uid_err; + uid = DecodeUid(args[2]); + if (!uid) { + LOG(ERROR) << "Unable to decode UID for '" << args[2] << "': " << uid.error(); return nullptr; } } - gid_t gid = 0; + Result gid = 0; std::vector supp_gids; if (command_arg > 4) { - std::string decode_uid_err; - if (!DecodeUid(args[3], &gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find GID for '" << args[3] << "': " << decode_uid_err; + gid = DecodeUid(args[3]); + if (!gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[3] << "': " << gid.error(); return nullptr; } std::size_t nr_supp_gids = command_arg - 1 /* -- */ - 4 /* exec SECLABEL UID GID */; for (size_t i = 0; i < nr_supp_gids; ++i) { - gid_t supp_gid; - if (!DecodeUid(args[4 + i], &supp_gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[4 + i] << "': " << decode_uid_err; + auto supp_gid = DecodeUid(args[4 + i]); + if (!supp_gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[4 + i] + << "': " << supp_gid.error(); return nullptr; } - supp_gids.push_back(supp_gid); + supp_gids.push_back(*supp_gid); } } - return std::make_unique(name, flags, uid, gid, supp_gids, no_capabilities, + return std::make_unique(name, flags, *uid, *gid, supp_gids, no_capabilities, namespace_flags, seclabel, str_args); } diff --git a/init/service_test.cpp b/init/service_test.cpp index 62e46f4cd..98d876f51 100644 --- a/init/service_test.cpp +++ b/init/service_test.cpp @@ -132,29 +132,29 @@ static void Test_make_temporary_oneshot_service(bool dash_dash, bool seclabel, b ASSERT_EQ("", svc->seclabel()); } if (uid) { - uid_t decoded_uid; - std::string err; - ASSERT_TRUE(DecodeUid("log", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->uid()); + auto decoded_uid = DecodeUid("log"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->uid()); } else { ASSERT_EQ(0U, svc->uid()); } if (gid) { - uid_t decoded_uid; - std::string err; - ASSERT_TRUE(DecodeUid("shell", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->gid()); + auto decoded_uid = DecodeUid("shell"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->gid()); } else { ASSERT_EQ(0U, svc->gid()); } if (supplementary_gids) { ASSERT_EQ(2U, svc->supp_gids().size()); - uid_t decoded_uid; - std::string err; - ASSERT_TRUE(DecodeUid("system", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->supp_gids()[0]); - ASSERT_TRUE(DecodeUid("adb", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->supp_gids()[1]); + + auto decoded_uid = DecodeUid("system"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->supp_gids()[0]); + + decoded_uid = DecodeUid("adb"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->supp_gids()[1]); } else { ASSERT_EQ(0U, svc->supp_gids().size()); } diff --git a/init/util.cpp b/init/util.cpp index e0379876d..fcf7ca882 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -57,30 +57,20 @@ namespace init { const std::string kDefaultAndroidDtDir("/proc/device-tree/firmware/android/"); // DecodeUid() - decodes and returns the given string, which can be either the -// numeric or name representation, into the integer uid or gid. Returns -// UINT_MAX on error. -bool DecodeUid(const std::string& name, uid_t* uid, std::string* err) { - *uid = UINT_MAX; - *err = ""; - +// numeric or name representation, into the integer uid or gid. +Result DecodeUid(const std::string& name) { if (isalpha(name[0])) { passwd* pwd = getpwnam(name.c_str()); - if (!pwd) { - *err = "getpwnam failed: "s + strerror(errno); - return false; - } - *uid = pwd->pw_uid; - return true; + if (!pwd) return ErrnoError() << "getpwnam failed"; + + return pwd->pw_uid; } errno = 0; uid_t result = static_cast(strtoul(name.c_str(), 0, 0)); - if (errno) { - *err = "strtoul failed: "s + strerror(errno); - return false; - } - *uid = result; - return true; + if (errno) return ErrnoError() << "strtoul failed"; + + return result; } /* @@ -164,50 +154,40 @@ out_unlink: return -1; } -bool ReadFile(const std::string& path, std::string* content, std::string* err) { - content->clear(); - *err = ""; - +Result ReadFile(const std::string& path) { android::base::unique_fd fd( TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC))); if (fd == -1) { - *err = "Unable to open '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "open() failed"; } // For security reasons, disallow world-writable // or group-writable files. struct stat sb; if (fstat(fd, &sb) == -1) { - *err = "fstat failed for '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "fstat failed()"; } if ((sb.st_mode & (S_IWGRP | S_IWOTH)) != 0) { - *err = "Skipping insecure file '" + path + "'"; - return false; + return Error() << "Skipping insecure file"; } - if (!android::base::ReadFdToString(fd, content)) { - *err = "Unable to read '" + path + "': " + strerror(errno); - return false; + std::string content; + if (!android::base::ReadFdToString(fd, &content)) { + return ErrnoError() << "Unable to read file contents"; } - return true; + return content; } -bool WriteFile(const std::string& path, const std::string& content, std::string* err) { - *err = ""; - +Result WriteFile(const std::string& path, const std::string& content) { android::base::unique_fd fd(TEMP_FAILURE_RETRY( open(path.c_str(), O_WRONLY | O_CREAT | O_NOFOLLOW | O_TRUNC | O_CLOEXEC, 0600))); if (fd == -1) { - *err = "Unable to open '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "open() failed"; } if (!android::base::WriteStringToFd(content, fd)) { - *err = "Unable to write to '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "Unable to write file contents"; } - return true; + return Success(); } bool mkdir_recursive(const std::string& path, mode_t mode) { diff --git a/init/util.h b/init/util.h index a81df478a..298aa1cb2 100644 --- a/init/util.h +++ b/init/util.h @@ -28,6 +28,8 @@ #include #include +#include "result.h" + #define COLDBOOT_DONE "/dev/.coldboot_done" using android::base::boot_clock; @@ -39,10 +41,10 @@ namespace init { int CreateSocket(const char* name, int type, bool passcred, mode_t perm, uid_t uid, gid_t gid, const char* socketcon); -bool ReadFile(const std::string& path, std::string* content, std::string* err); -bool WriteFile(const std::string& path, const std::string& content, std::string* err); +Result ReadFile(const std::string& path); +Result WriteFile(const std::string& path, const std::string& content); -bool DecodeUid(const std::string& name, uid_t* uid, std::string* err); +Result DecodeUid(const std::string& name); bool mkdir_recursive(const std::string& pathname, mode_t mode); int wait_for_file(const char *filename, std::chrono::nanoseconds timeout); diff --git a/init/util_test.cpp b/init/util_test.cpp index 5dd271cd0..007d10ee3 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -30,61 +30,51 @@ namespace android { namespace init { TEST(util, ReadFile_ENOENT) { - std::string s("hello"); - std::string err; errno = 0; - EXPECT_FALSE(ReadFile("/proc/does-not-exist", &s, &err)); - EXPECT_EQ("Unable to open '/proc/does-not-exist': No such file or directory", err); + auto file_contents = ReadFile("/proc/does-not-exist"); EXPECT_EQ(ENOENT, errno); - EXPECT_EQ("", s); // s was cleared. + ASSERT_FALSE(file_contents); + EXPECT_EQ("open() failed: No such file or directory", file_contents.error()); } TEST(util, ReadFileGroupWriteable) { std::string s("hello"); TemporaryFile tf; - std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(WriteFile(tf.path, s, &err)) << strerror(errno); - EXPECT_EQ("", err); + EXPECT_TRUE(WriteFile(tf.path, s)) << strerror(errno); EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0620, AT_SYMLINK_NOFOLLOW)) << strerror(errno); - EXPECT_FALSE(ReadFile(tf.path, &s, &err)) << strerror(errno); - EXPECT_EQ("Skipping insecure file '"s + tf.path + "'", err); - EXPECT_EQ("", s); // s was cleared. + auto file_contents = ReadFile(tf.path); + ASSERT_FALSE(file_contents) << strerror(errno); + EXPECT_EQ("Skipping insecure file", file_contents.error()); } TEST(util, ReadFileWorldWiteable) { std::string s("hello"); TemporaryFile tf; - std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(WriteFile(tf.path, s, &err)) << strerror(errno); - EXPECT_EQ("", err); + EXPECT_TRUE(WriteFile(tf.path, s)) << strerror(errno); EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0602, AT_SYMLINK_NOFOLLOW)) << strerror(errno); - EXPECT_FALSE(ReadFile(tf.path, &s, &err)) << strerror(errno); - EXPECT_EQ("Skipping insecure file '"s + tf.path + "'", err); - EXPECT_EQ("", s); // s was cleared. + auto file_contents = ReadFile(tf.path); + ASSERT_FALSE(file_contents) << strerror(errno); + EXPECT_EQ("Skipping insecure file", file_contents.error()); } TEST(util, ReadFileSymbolicLink) { - std::string s("hello"); errno = 0; // lrwxrwxrwx 1 root root 13 1970-01-01 00:00 charger -> /sbin/healthd - std::string err; - EXPECT_FALSE(ReadFile("/charger", &s, &err)); - EXPECT_EQ("Unable to open '/charger': Too many symbolic links encountered", err); + auto file_contents = ReadFile("/charger"); EXPECT_EQ(ELOOP, errno); - EXPECT_EQ("", s); // s was cleared. + ASSERT_FALSE(file_contents); + EXPECT_EQ("open() failed: Too many symbolic links encountered", file_contents.error()); } TEST(util, ReadFileSuccess) { - std::string s("hello"); - std::string err; - EXPECT_TRUE(ReadFile("/proc/version", &s, &err)); - EXPECT_EQ("", err); - EXPECT_GT(s.length(), 6U); - EXPECT_EQ('\n', s[s.length() - 1]); - s[5] = 0; - EXPECT_STREQ("Linux", s.c_str()); + auto file_contents = ReadFile("/proc/version"); + ASSERT_TRUE(file_contents); + EXPECT_GT(file_contents->length(), 6U); + EXPECT_EQ('\n', file_contents->at(file_contents->length() - 1)); + (*file_contents)[5] = 0; + EXPECT_STREQ("Linux", file_contents->c_str()); } TEST(util, WriteFileBinary) { @@ -95,29 +85,23 @@ TEST(util, WriteFileBinary) { ASSERT_EQ(10u, contents.size()); TemporaryFile tf; - std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(WriteFile(tf.path, contents, &err)) << strerror(errno); - EXPECT_EQ("", err); + EXPECT_TRUE(WriteFile(tf.path, contents)) << strerror(errno); - std::string read_back_contents; - EXPECT_TRUE(ReadFile(tf.path, &read_back_contents, &err)) << strerror(errno); - EXPECT_EQ("", err); - EXPECT_EQ(contents, read_back_contents); - EXPECT_EQ(10u, read_back_contents.size()); + auto read_back_contents = ReadFile(tf.path); + ASSERT_TRUE(read_back_contents) << strerror(errno); + EXPECT_EQ(contents, *read_back_contents); + EXPECT_EQ(10u, read_back_contents->size()); } TEST(util, WriteFileNotExist) { std::string s("hello"); - std::string s2("hello"); TemporaryDir test_dir; std::string path = android::base::StringPrintf("%s/does-not-exist", test_dir.path); - std::string err; - EXPECT_TRUE(WriteFile(path, s, &err)); - EXPECT_EQ("", err); - EXPECT_TRUE(ReadFile(path, &s2, &err)); - EXPECT_EQ("", err); - EXPECT_EQ(s, s2); + EXPECT_TRUE(WriteFile(path, s)); + auto file_contents = ReadFile(path); + ASSERT_TRUE(file_contents); + EXPECT_EQ(s, *file_contents); struct stat sb; int fd = open(path.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC); EXPECT_NE(-1, fd); @@ -127,37 +111,30 @@ TEST(util, WriteFileNotExist) { } TEST(util, WriteFileExist) { - std::string s2(""); TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); - std::string err; - EXPECT_TRUE(WriteFile(tf.path, "1hello1", &err)) << strerror(errno); - EXPECT_EQ("", err); - EXPECT_TRUE(ReadFile(tf.path, &s2, &err)); - EXPECT_EQ("", err); - EXPECT_STREQ("1hello1", s2.c_str()); - EXPECT_TRUE(WriteFile(tf.path, "2ll2", &err)); - EXPECT_EQ("", err); - EXPECT_TRUE(ReadFile(tf.path, &s2, &err)); - EXPECT_EQ("", err); - EXPECT_STREQ("2ll2", s2.c_str()); + EXPECT_TRUE(WriteFile(tf.path, "1hello1")) << strerror(errno); + auto file_contents = ReadFile(tf.path); + ASSERT_TRUE(file_contents); + EXPECT_EQ("1hello1", *file_contents); + EXPECT_TRUE(WriteFile(tf.path, "2ll2")); + file_contents = ReadFile(tf.path); + ASSERT_TRUE(file_contents); + EXPECT_EQ("2ll2", *file_contents); } TEST(util, DecodeUid) { - uid_t decoded_uid; - std::string err; + auto decoded_uid = DecodeUid("root"); + EXPECT_TRUE(decoded_uid); + EXPECT_EQ(0U, *decoded_uid); - EXPECT_TRUE(DecodeUid("root", &decoded_uid, &err)); - EXPECT_EQ("", err); - EXPECT_EQ(0U, decoded_uid); + decoded_uid = DecodeUid("toot"); + EXPECT_FALSE(decoded_uid); + EXPECT_EQ("getpwnam failed: No such file or directory", decoded_uid.error()); - EXPECT_FALSE(DecodeUid("toot", &decoded_uid, &err)); - EXPECT_EQ("getpwnam failed: No such file or directory", err); - EXPECT_EQ(UINT_MAX, decoded_uid); - - EXPECT_TRUE(DecodeUid("123", &decoded_uid, &err)); - EXPECT_EQ("", err); - EXPECT_EQ(123U, decoded_uid); + decoded_uid = DecodeUid("123"); + EXPECT_TRUE(decoded_uid); + EXPECT_EQ(123U, *decoded_uid); } TEST(util, is_dir) { From 557946e57c375b05deb5ba07b739f27abc70697e Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 1 Aug 2017 13:50:23 -0700 Subject: [PATCH 2/3] init: use Result for builtin functions We currently throw out the return values from builtin functions and occasionally log errors with no supporting context. This change uses the newly introduced Result class to communicate a successful result or an error back to callers in order to print an error with clear context when a builtin fails. Example: init: Command 'write /sys/class/leds/vibrator/trigger transient' action=init (/init.rc:245) took 0ms and failed: Unable to write to file '/sys/class/leds/vibrator/trigger': open() failed: No such file or directory Test: boot bullhead Merged-In: Idc18f331d2d646629c6093c1e0f2996cf9b42aec Change-Id: Idc18f331d2d646629c6093c1e0f2996cf9b42aec --- init/action.cpp | 13 +- init/action.h | 3 +- init/bootchart.cpp | 48 ++--- init/bootchart.h | 4 +- init/builtins.cpp | 493 +++++++++++++++++++++++---------------------- init/builtins.h | 3 +- init/init.cpp | 24 +-- init/init_test.cpp | 4 +- init/reboot.cpp | 2 +- init/security.cpp | 40 ++-- init/security.h | 8 +- 11 files changed, 323 insertions(+), 319 deletions(-) diff --git a/init/action.cpp b/init/action.cpp index 4ec5f1739..671e285e1 100644 --- a/init/action.cpp +++ b/init/action.cpp @@ -31,14 +31,13 @@ namespace init { Command::Command(BuiltinFunction f, const std::vector& args, int line) : func_(f), args_(args), line_(line) {} -int Command::InvokeFunc() const { +Result Command::InvokeFunc() const { std::vector expanded_args; expanded_args.resize(args_.size()); expanded_args[0] = args_[0]; for (std::size_t i = 1; i < args_.size(); ++i) { if (!expand_props(args_[i], &expanded_args[i])) { - LOG(ERROR) << args_[0] << ": cannot expand '" << args_[i] << "'"; - return -EINVAL; + return Error() << "cannot expand '" << args_[i] << "'"; } } @@ -92,17 +91,17 @@ void Action::ExecuteAllCommands() const { void Action::ExecuteCommand(const Command& command) const { android::base::Timer t; - int result = command.InvokeFunc(); - + auto result = command.InvokeFunc(); auto duration = t.duration(); + // Any action longer than 50ms will be warned to user as slow operation if (duration > 50ms || android::base::GetMinimumLogSeverity() <= android::base::DEBUG) { std::string trigger_name = BuildTriggersString(); std::string cmd_str = command.BuildCommandString(); LOG(INFO) << "Command '" << cmd_str << "' action=" << trigger_name << " (" << filename_ - << ":" << command.line() << ") returned " << result << " took " - << duration.count() << "ms."; + << ":" << command.line() << ") took " << duration.count() << "ms and " + << (result ? "succeeded" : "failed: " + result.error()); } } diff --git a/init/action.h b/init/action.h index 50cae7171..44ecd237b 100644 --- a/init/action.h +++ b/init/action.h @@ -26,6 +26,7 @@ #include "builtins.h" #include "keyword_map.h" #include "parser.h" +#include "result.h" namespace android { namespace init { @@ -34,7 +35,7 @@ class Command { public: Command(BuiltinFunction f, const std::vector& args, int line); - int InvokeFunc() const; + Result InvokeFunc() const; std::string BuildCommandString() const; int line() const { return line_; } diff --git a/init/bootchart.cpp b/init/bootchart.cpp index 4727f92e9..ec84317c3 100644 --- a/init/bootchart.cpp +++ b/init/bootchart.cpp @@ -163,37 +163,37 @@ static void bootchart_thread_main() { LOG(INFO) << "Bootcharting finished"; } -static int do_bootchart_start() { - // We don't care about the content, but we do care that /data/bootchart/enabled actually exists. - std::string start; - if (!android::base::ReadFileToString("/data/bootchart/enabled", &start)) { - LOG(VERBOSE) << "Not bootcharting"; - return 0; - } +static Result do_bootchart_start() { + // We don't care about the content, but we do care that /data/bootchart/enabled actually exists. + std::string start; + if (!android::base::ReadFileToString("/data/bootchart/enabled", &start)) { + LOG(VERBOSE) << "Not bootcharting"; + return Success(); + } - g_bootcharting_thread = new std::thread(bootchart_thread_main); - return 0; + g_bootcharting_thread = new std::thread(bootchart_thread_main); + return Success(); } -static int do_bootchart_stop() { - if (!g_bootcharting_thread) return 0; +static Result do_bootchart_stop() { + if (!g_bootcharting_thread) return Success(); - // Tell the worker thread it's time to quit. - { - std::lock_guard lock(g_bootcharting_finished_mutex); - g_bootcharting_finished = true; - g_bootcharting_finished_cv.notify_one(); - } + // Tell the worker thread it's time to quit. + { + std::lock_guard lock(g_bootcharting_finished_mutex); + g_bootcharting_finished = true; + g_bootcharting_finished_cv.notify_one(); + } - g_bootcharting_thread->join(); - delete g_bootcharting_thread; - g_bootcharting_thread = nullptr; - return 0; + g_bootcharting_thread->join(); + delete g_bootcharting_thread; + g_bootcharting_thread = nullptr; + return Success(); } -int do_bootchart(const std::vector& args) { - if (args[1] == "start") return do_bootchart_start(); - return do_bootchart_stop(); +Result do_bootchart(const std::vector& args) { + if (args[1] == "start") return do_bootchart_start(); + return do_bootchart_stop(); } } // namespace init diff --git a/init/bootchart.h b/init/bootchart.h index e4f7b59b2..f614f712f 100644 --- a/init/bootchart.h +++ b/init/bootchart.h @@ -20,10 +20,12 @@ #include #include +#include "result.h" + namespace android { namespace init { -int do_bootchart(const std::vector& args); +Result do_bootchart(const std::vector& args); } // namespace init } // namespace android diff --git a/init/builtins.cpp b/init/builtins.cpp index 28f60e6ac..f807343ab 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -78,47 +78,13 @@ namespace init { static constexpr std::chrono::nanoseconds kCommandRetryTimeout = 5s; -static int insmod(const char *filename, const char *options, int flags) { - unique_fd fd(TEMP_FAILURE_RETRY(open(filename, O_RDONLY | O_NOFOLLOW | O_CLOEXEC))); - if (fd == -1) { - PLOG(ERROR) << "insmod: open(\"" << filename << "\") failed"; - return -1; - } - int rc = syscall(__NR_finit_module, fd.get(), options, flags); - if (rc == -1) { - PLOG(ERROR) << "finit_module for \"" << filename << "\" failed"; - } - return rc; -} - -static int __ifupdown(const char *interface, int up) { - struct ifreq ifr; - - strlcpy(ifr.ifr_name, interface, IFNAMSIZ); - - unique_fd s(TEMP_FAILURE_RETRY(socket(AF_INET, SOCK_DGRAM, 0))); - if (s < 0) return -1; - - int ret = ioctl(s, SIOCGIFFLAGS, &ifr); - if (ret < 0) return ret; - - if (up) { - ifr.ifr_flags |= IFF_UP; - } else { - ifr.ifr_flags &= ~IFF_UP; - } - - return ioctl(s, SIOCSIFFLAGS, &ifr); -} - -static int reboot_into_recovery(const std::vector& options) { +static Result reboot_into_recovery(const std::vector& options) { std::string err; if (!write_bootloader_message(options, &err)) { - LOG(ERROR) << "failed to set bootloader message: " << err; - return -1; + return Error() << "Failed to set bootloader message: " << err; } property_set("sys.powerctl", "reboot,recovery"); - return 0; + return Success(); } template @@ -128,88 +94,106 @@ static void ForEachServiceInClass(const std::string& classname, F function) { } } -static int do_class_start(const std::vector& args) { +static Result do_class_start(const std::vector& args) { // Starting a class does not start services which are explicitly disabled. // They must be started individually. ForEachServiceInClass(args[1], &Service::StartIfNotDisabled); - return 0; + return Success(); } -static int do_class_stop(const std::vector& args) { +static Result do_class_stop(const std::vector& args) { ForEachServiceInClass(args[1], &Service::Stop); - return 0; + return Success(); } -static int do_class_reset(const std::vector& args) { +static Result do_class_reset(const std::vector& args) { ForEachServiceInClass(args[1], &Service::Reset); - return 0; + return Success(); } -static int do_class_restart(const std::vector& args) { +static Result do_class_restart(const std::vector& args) { ForEachServiceInClass(args[1], &Service::Restart); - return 0; + return Success(); } -static int do_domainname(const std::vector& args) { +static Result do_domainname(const std::vector& args) { if (auto result = WriteFile("/proc/sys/kernel/domainname", args[1]); !result) { - LOG(ERROR) << "Unable to write to /proc/sys/kernel/domainname: " << result.error(); - return -1; + return Error() << "Unable to write to /proc/sys/kernel/domainname: " << result.error(); } - return 0; + return Success(); } -static int do_enable(const std::vector& args) { +static Result do_enable(const std::vector& args) { Service* svc = ServiceList::GetInstance().FindService(args[1]); - if (!svc) { - return -1; - } - return svc->Enable(); + if (!svc) return Error() << "Could not find service"; + + if (!svc->Enable()) return Error() << "Could not enable service"; + + return Success(); } -static int do_exec(const std::vector& args) { +static Result do_exec(const std::vector& args) { auto service = Service::MakeTemporaryOneshotService(args); if (!service) { - LOG(ERROR) << "Failed to create exec service: " << android::base::Join(args, " "); - return -1; + return Error() << "Could not create exec service"; } if (!service->ExecStart()) { - LOG(ERROR) << "Failed to Start exec service"; - return -1; + return Error() << "Could not start exec service"; } + ServiceList::GetInstance().AddService(std::move(service)); - return 0; + return Success(); } -static int do_exec_start(const std::vector& args) { +static Result do_exec_start(const std::vector& args) { Service* service = ServiceList::GetInstance().FindService(args[1]); if (!service) { - LOG(ERROR) << "ExecStart(" << args[1] << "): Service not found"; - return -1; + return Error() << "Service not found"; } + if (!service->ExecStart()) { - LOG(ERROR) << "ExecStart(" << args[1] << "): Could not start Service"; - return -1; + return Error() << "Could not start Service"; } - return 0; + + return Success(); } -static int do_export(const std::vector& args) { - return add_environment(args[1].c_str(), args[2].c_str()); +static Result do_export(const std::vector& args) { + if (!add_environment(args[1].c_str(), args[2].c_str())) { + return Error(); + } + return Success(); } -static int do_hostname(const std::vector& args) { +static Result do_hostname(const std::vector& args) { if (auto result = WriteFile("/proc/sys/kernel/hostname", args[1]); !result) { - LOG(ERROR) << "Unable to write to /proc/sys/kernel/hostname: " << result.error(); - return -1; + return Error() << "Unable to write to /proc/sys/kernel/hostname: " << result.error(); } - return 0; + return Success(); } -static int do_ifup(const std::vector& args) { - return __ifupdown(args[1].c_str(), 1); +static Result do_ifup(const std::vector& args) { + struct ifreq ifr; + + strlcpy(ifr.ifr_name, args[1].c_str(), IFNAMSIZ); + + unique_fd s(TEMP_FAILURE_RETRY(socket(AF_INET, SOCK_DGRAM, 0))); + if (s < 0) return ErrnoError() << "opening socket failed"; + + if (ioctl(s, SIOCGIFFLAGS, &ifr) < 0) { + return ErrnoError() << "ioctl(..., SIOCGIFFLAGS, ...) failed"; + } + + ifr.ifr_flags |= IFF_UP; + + if (ioctl(s, SIOCSIFFLAGS, &ifr) < 0) { + return ErrnoError() << "ioctl(..., SIOCSIFFLAGS, ...) failed"; + } + + return Success(); } -static int do_insmod(const std::vector& args) { +static Result do_insmod(const std::vector& args) { int flags = 0; auto it = args.begin() + 1; @@ -220,11 +204,18 @@ static int do_insmod(const std::vector& args) { std::string filename = *it++; std::string options = android::base::Join(std::vector(it, args.end()), ' '); - return insmod(filename.c_str(), options.c_str(), flags); + + unique_fd fd(TEMP_FAILURE_RETRY(open(filename.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC))); + if (fd == -1) return ErrnoError() << "open(\"" << filename << "\") failed"; + + int rc = syscall(__NR_finit_module, fd.get(), options.c_str(), flags); + if (rc == -1) return ErrnoError() << "finit_module for \"" << filename << "\" failed"; + + return Success(); } // mkdir [mode] [owner] [group] -static int do_mkdir(const std::vector& args) { +static Result do_mkdir(const std::vector& args) { mode_t mode = 0755; if (args.size() >= 3) { mode = std::strtoul(args[2].c_str(), 0, 8); @@ -234,37 +225,35 @@ static int do_mkdir(const std::vector& args) { /* chmod in case the directory already exists */ if (errno == EEXIST) { if (fchmodat(AT_FDCWD, args[1].c_str(), mode, AT_SYMLINK_NOFOLLOW) == -1) { - return -errno; + return ErrnoError() << "fchmodat() failed"; } } else { - return -errno; + return ErrnoError() << "mkdir() failed"; } } if (args.size() >= 4) { auto uid = DecodeUid(args[3]); if (!uid) { - LOG(ERROR) << "Unable to decode UID for '" << args[3] << "': " << uid.error(); - return -1; + return Error() << "Unable to decode UID for '" << args[3] << "': " << uid.error(); } Result gid = -1; if (args.size() == 5) { gid = DecodeUid(args[4]); if (!gid) { - LOG(ERROR) << "Unable to decode GID for '" << args[3] << "': " << gid.error(); - return -1; + return Error() << "Unable to decode GID for '" << args[3] << "': " << gid.error(); } } if (lchown(args[1].c_str(), *uid, *gid) == -1) { - return -errno; + return ErrnoError() << "lchown failed"; } /* chown may have cleared S_ISUID and S_ISGID, chmod again */ if (mode & (S_ISUID | S_ISGID)) { if (fchmodat(AT_FDCWD, args[1].c_str(), mode, AT_SYMLINK_NOFOLLOW) == -1) { - return -errno; + return ErrnoError() << "fchmodat failed"; } } } @@ -275,15 +264,18 @@ static int do_mkdir(const std::vector& args) { "--prompt_and_wipe_data", "--reason=set_policy_failed:"s + args[1]}; reboot_into_recovery(options); - return -1; + return Error() << "reboot into recovery failed"; } } - return 0; + return Success(); } /* umount */ -static int do_umount(const std::vector& args) { - return umount(args[1].c_str()); +static Result do_umount(const std::vector& args) { + if (umount(args[1].c_str()) < 0) { + return ErrnoError() << "umount() failed"; + } + return Success(); } static struct { @@ -311,7 +303,7 @@ static struct { #define DATA_MNT_POINT "/data" /* mount */ -static int do_mount(const std::vector& args) { +static Result do_mount(const std::vector& args) { const char* options = nullptr; unsigned flags = 0; bool wait = false; @@ -342,12 +334,12 @@ static int do_mount(const std::vector& args) { if (android::base::StartsWith(source, "loop@")) { int mode = (flags & MS_RDONLY) ? O_RDONLY : O_RDWR; unique_fd fd(TEMP_FAILURE_RETRY(open(source + 5, mode | O_CLOEXEC))); - if (fd < 0) return -1; + if (fd < 0) return ErrnoError() << "open(" << source + 5 << ", " << mode << ") failed"; for (size_t n = 0;; n++) { std::string tmp = android::base::StringPrintf("/dev/block/loop%zu", n); unique_fd loop(TEMP_FAILURE_RETRY(open(tmp.c_str(), mode | O_CLOEXEC))); - if (loop < 0) return -1; + if (loop < 0) return ErrnoError() << "open(" << tmp << ", " << mode << ") failed"; loop_info info; /* if it is a blank loop device */ @@ -356,26 +348,24 @@ static int do_mount(const std::vector& args) { if (ioctl(loop, LOOP_SET_FD, fd.get()) >= 0) { if (mount(tmp.c_str(), target, system, flags, options) < 0) { ioctl(loop, LOOP_CLR_FD, 0); - return -1; + return ErrnoError() << "mount() failed"; } - return 0; + return Success(); } } } - LOG(ERROR) << "out of loopback devices"; - return -1; + return Error() << "out of loopback devices"; } else { if (wait) wait_for_file(source, kCommandRetryTimeout); if (mount(source, target, system, flags, options) < 0) { - return -1; + return ErrnoError() << "mount() failed"; } } - return 0; - + return Success(); } /* Imports .rc files from the specified paths. Default ones are applied if none is given. @@ -407,9 +397,7 @@ static void import_late(const std::vector& args, size_t start_index * * Call fs_mgr_mount_all() to mount the given fstab */ -static int mount_fstab(const char* fstabfile, int mount_mode) { - int ret = -1; - +static Result mount_fstab(const char* fstabfile, int mount_mode) { /* * Call fs_mgr_mount_all() to mount all filesystems. We fork(2) and * do the call in the child to provide protection to the main init @@ -427,9 +415,9 @@ static int mount_fstab(const char* fstabfile, int mount_mode) { } if (WIFEXITED(status)) { - ret = WEXITSTATUS(status); + return WEXITSTATUS(status); } else { - ret = -1; + return Error() << "child aborted"; } } else if (pid == 0) { /* child, call fs_mgr_mount_all() */ @@ -446,10 +434,8 @@ static int mount_fstab(const char* fstabfile, int mount_mode) { } _exit(child_ret); } else { - /* fork failed, return an error */ - return -1; + return Error() << "fork() failed"; } - return ret; } /* Queue event based on fs_mgr return code. @@ -461,29 +447,33 @@ static int mount_fstab(const char* fstabfile, int mount_mode) { * * return code is processed based on input code */ -static int queue_fs_event(int code) { - int ret = code; +static Result queue_fs_event(int code) { if (code == FS_MGR_MNTALL_DEV_NEEDS_ENCRYPTION) { ActionManager::GetInstance().QueueEventTrigger("encrypt"); + return Success(); } else if (code == FS_MGR_MNTALL_DEV_MIGHT_BE_ENCRYPTED) { property_set("ro.crypto.state", "encrypted"); property_set("ro.crypto.type", "block"); ActionManager::GetInstance().QueueEventTrigger("defaultcrypto"); + return Success(); } else if (code == FS_MGR_MNTALL_DEV_NOT_ENCRYPTED) { property_set("ro.crypto.state", "unencrypted"); ActionManager::GetInstance().QueueEventTrigger("nonencrypted"); + return Success(); } else if (code == FS_MGR_MNTALL_DEV_NOT_ENCRYPTABLE) { property_set("ro.crypto.state", "unsupported"); ActionManager::GetInstance().QueueEventTrigger("nonencrypted"); + return Success(); } else if (code == FS_MGR_MNTALL_DEV_NEEDS_RECOVERY) { /* Setup a wipe via recovery, and reboot into recovery */ PLOG(ERROR) << "fs_mgr_mount_all suggested recovery, so wiping data via recovery."; const std::vector options = {"--wipe_data", "--reason=fs_mgr_mount_all" }; - ret = reboot_into_recovery(options); + reboot_into_recovery(options); + return Error() << "reboot_into_recovery() failed"; /* If reboot worked, there is no return. */ } else if (code == FS_MGR_MNTALL_DEV_FILE_ENCRYPTED) { if (e4crypt_install_keyring()) { - return -1; + return Error() << "e4crypt_install_keyring() failed"; } property_set("ro.crypto.state", "encrypted"); property_set("ro.crypto.type", "file"); @@ -491,12 +481,13 @@ static int queue_fs_event(int code) { // Although encrypted, we have device key, so we do not need to // do anything different from the nonencrypted case. ActionManager::GetInstance().QueueEventTrigger("nonencrypted"); + return Success(); } else if (code > 0) { - PLOG(ERROR) << "fs_mgr_mount_all returned unexpected error " << code; + Error() << "fs_mgr_mount_all() returned unexpected error " << code; } /* else ... < 0: error */ - return ret; + return Error() << "Invalid code: " << code; } /* mount_all [ ]* [--]* @@ -504,7 +495,7 @@ static int queue_fs_event(int code) { * This function might request a reboot, in which case it will * not return. */ -static int do_mount_all(const std::vector& args) { +static Result do_mount_all(const std::vector& args) { std::size_t na = 0; bool import_rc = true; bool queue_event = true; @@ -529,7 +520,10 @@ static int do_mount_all(const std::vector& args) { std::string prop_name = "ro.boottime.init.mount_all."s + prop_post_fix; android::base::Timer t; - int ret = mount_fstab(fstabfile, mount_mode); + auto mount_fstab_return_code = mount_fstab(fstabfile, mount_mode); + if (!mount_fstab_return_code) { + return Error() << "mount_fstab() failed " << mount_fstab_return_code.error(); + } property_set(prop_name, std::to_string(t.duration().count())); if (import_rc) { @@ -540,13 +534,16 @@ static int do_mount_all(const std::vector& args) { if (queue_event) { /* queue_fs_event will queue event based on mount_fstab return code * and return processed return code*/ - ret = queue_fs_event(ret); + auto queue_fs_result = queue_fs_event(*mount_fstab_return_code); + if (!queue_fs_result) { + return Error() << "queue_fs_event() failed: " << queue_fs_result.error(); + } } - return ret; + return Success(); } -static int do_swapon_all(const std::vector& args) { +static Result do_swapon_all(const std::vector& args) { struct fstab *fstab; int ret; @@ -554,89 +551,103 @@ static int do_swapon_all(const std::vector& args) { ret = fs_mgr_swapon_all(fstab); fs_mgr_free_fstab(fstab); - return ret; + if (ret != 0) return Error() << "fs_mgr_swapon_all() failed"; + return Success(); } -static int do_setprop(const std::vector& args) { +static Result do_setprop(const std::vector& args) { property_set(args[1], args[2]); - return 0; + return Success(); } -static int do_setrlimit(const std::vector& args) { - struct rlimit limit; +static Result do_setrlimit(const std::vector& args) { int resource; - if (android::base::ParseInt(args[1], &resource) && - android::base::ParseUint(args[2], &limit.rlim_cur) && - android::base::ParseUint(args[3], &limit.rlim_max)) { - return setrlimit(resource, &limit); + if (!android::base::ParseInt(args[1], &resource)) { + return Error() << "unable to parse resource, " << args[1]; } - LOG(WARNING) << "ignoring setrlimit " << args[1] << " " << args[2] << " " << args[3]; - return -1; + + struct rlimit limit; + if (!android::base::ParseUint(args[2], &limit.rlim_cur)) { + return Error() << "unable to parse rlim_cur, " << args[2]; + } + if (!android::base::ParseUint(args[3], &limit.rlim_max)) { + return Error() << "unable to parse rlim_max, " << args[3]; + } + + if (setrlimit(resource, &limit) == -1) { + return ErrnoError() << "setrlimit failed"; + } + return Success(); } -static int do_start(const std::vector& args) { +static Result do_start(const std::vector& args) { Service* svc = ServiceList::GetInstance().FindService(args[1]); - if (!svc) { - LOG(ERROR) << "do_start: Service " << args[1] << " not found"; - return -1; - } - if (!svc->Start()) - return -1; - return 0; + if (!svc) return Error() << "service " << args[1] << " not found"; + if (!svc->Start()) return Error() << "failed to start service"; + return Success(); } -static int do_stop(const std::vector& args) { +static Result do_stop(const std::vector& args) { Service* svc = ServiceList::GetInstance().FindService(args[1]); - if (!svc) { - LOG(ERROR) << "do_stop: Service " << args[1] << " not found"; - return -1; - } + if (!svc) return Error() << "service " << args[1] << " not found"; svc->Stop(); - return 0; + return Success(); } -static int do_restart(const std::vector& args) { +static Result do_restart(const std::vector& args) { Service* svc = ServiceList::GetInstance().FindService(args[1]); - if (!svc) { - LOG(ERROR) << "do_restart: Service " << args[1] << " not found"; - return -1; - } + if (!svc) return Error() << "service " << args[1] << " not found"; svc->Restart(); - return 0; + return Success(); } -static int do_trigger(const std::vector& args) { +static Result do_trigger(const std::vector& args) { ActionManager::GetInstance().QueueEventTrigger(args[1]); - return 0; + return Success(); } -static int do_symlink(const std::vector& args) { - return symlink(args[1].c_str(), args[2].c_str()); -} - -static int do_rm(const std::vector& args) { - return unlink(args[1].c_str()); -} - -static int do_rmdir(const std::vector& args) { - return rmdir(args[1].c_str()); -} - -static int do_sysclktz(const std::vector& args) { - struct timezone tz = {}; - if (android::base::ParseInt(args[1], &tz.tz_minuteswest) && settimeofday(nullptr, &tz) != -1) { - return 0; +static Result do_symlink(const std::vector& args) { + if (symlink(args[1].c_str(), args[2].c_str()) < 0) { + return ErrnoError() << "symlink() failed"; } - return -1; + return Success(); } -static int do_verity_load_state(const std::vector& args) { +static Result do_rm(const std::vector& args) { + if (unlink(args[1].c_str()) < 0) { + return ErrnoError() << "unlink() failed"; + } + return Success(); +} + +static Result do_rmdir(const std::vector& args) { + if (rmdir(args[1].c_str()) < 0) { + return ErrnoError() << "rmdir() failed"; + } + return Success(); +} + +static Result do_sysclktz(const std::vector& args) { + struct timezone tz = {}; + if (!android::base::ParseInt(args[1], &tz.tz_minuteswest)) { + return Error() << "Unable to parse mins_west_of_gmt"; + } + + if (settimeofday(nullptr, &tz) == -1) { + return ErrnoError() << "settimeofday() failed"; + } + return Success(); +} + +static Result do_verity_load_state(const std::vector& args) { int mode = -1; bool loaded = fs_mgr_load_verity_state(&mode); if (loaded && mode != VERITY_MODE_DEFAULT) { ActionManager::GetInstance().QueueEventTrigger("verity-logging"); } - return loaded ? 0 : 1; + if (!loaded) return Error() << "Could not load verity state"; + + return Success(); } static void verity_update_property(fstab_rec *fstab, const char *mount_point, @@ -644,24 +655,26 @@ static void verity_update_property(fstab_rec *fstab, const char *mount_point, property_set("partition."s + mount_point + ".verified", std::to_string(mode)); } -static int do_verity_update_state(const std::vector& args) { - return fs_mgr_update_verity_state(verity_update_property) ? 0 : 1; -} - -static int do_write(const std::vector& args) { - if (auto result = WriteFile(args[1], args[2]); !result) { - LOG(ERROR) << "Unable to write to file '" << args[1] << "': " << result.error(); - return -1; +static Result do_verity_update_state(const std::vector& args) { + if (!fs_mgr_update_verity_state(verity_update_property)) { + return Error() << "fs_mgr_update_verity_state() failed"; } - return 0; + return Success(); } -static int do_readahead(const std::vector& args) { +static Result do_write(const std::vector& args) { + if (auto result = WriteFile(args[1], args[2]); !result) { + return Error() << "Unable to write to file '" << args[1] << "': " << result.error(); + } + + return Success(); +} + +static Result do_readahead(const std::vector& args) { struct stat sb; if (stat(args[1].c_str(), &sb)) { - PLOG(ERROR) << "Error opening " << args[1]; - return -1; + return ErrnoError() << "Error opening " << args[1]; } // We will do readahead in a forked process in order not to block init @@ -710,30 +723,27 @@ static int do_readahead(const std::vector& args) { LOG(INFO) << "Readahead " << args[1] << " took " << t; _exit(0); } else if (pid < 0) { - PLOG(ERROR) << "Fork failed"; - return -1; + return ErrnoError() << "Fork failed"; } - return 0; + return Success(); } -static int do_copy(const std::vector& args) { +static Result do_copy(const std::vector& args) { auto file_contents = ReadFile(args[1]); if (!file_contents) { - LOG(ERROR) << "Could not read input file '" << args[1] << "': " << file_contents.error(); - return -1; + return Error() << "Could not read input file '" << args[1] << "': " << file_contents.error(); } if (auto result = WriteFile(args[2], *file_contents); !result) { - LOG(ERROR) << "Could not write to output file '" << args[2] << "': " << result.error(); - return -1; + return Error() << "Could not write to output file '" << args[2] << "': " << result.error(); } - return 0; + + return Success(); } -static int do_chown(const std::vector& args) { +static Result do_chown(const std::vector& args) { auto uid = DecodeUid(args[1]); if (!uid) { - LOG(ERROR) << "Unable to decode UID for '" << args[1] << "': " << uid.error(); - return -1; + return Error() << "Unable to decode UID for '" << args[1] << "': " << uid.error(); } // GID is optional and pushes the index of path out by one if specified. @@ -743,14 +753,15 @@ static int do_chown(const std::vector& args) { if (args.size() == 4) { gid = DecodeUid(args[2]); if (!gid) { - LOG(ERROR) << "Unable to decode GID for '" << args[2] << "': " << gid.error(); - return -1; + return Error() << "Unable to decode GID for '" << args[2] << "': " << gid.error(); } } - if (lchown(path.c_str(), *uid, *gid) == -1) return -errno; + if (lchown(path.c_str(), *uid, *gid) == -1) { + return ErrnoError() << "lchown() failed"; + } - return 0; + return Success(); } static mode_t get_mode(const char *s) { @@ -766,15 +777,15 @@ static mode_t get_mode(const char *s) { return mode; } -static int do_chmod(const std::vector& args) { +static Result do_chmod(const std::vector& args) { mode_t mode = get_mode(args[1].c_str()); if (fchmodat(AT_FDCWD, args[2].c_str(), mode, AT_SYMLINK_NOFOLLOW) < 0) { - return -errno; + return ErrnoError() << "fchmodat() failed"; } - return 0; + return Success(); } -static int do_restorecon(const std::vector& args) { +static Result do_restorecon(const std::vector& args) { int ret = 0; struct flag_type {const char* name; int value;}; @@ -791,8 +802,7 @@ static int do_restorecon(const std::vector& args) { for (size_t i = 1; i < args.size(); ++i) { if (android::base::StartsWith(args[i], "--")) { if (!in_flags) { - LOG(ERROR) << "restorecon - flags must precede paths"; - return -1; + return Error() << "flags must precede paths"; } bool found = false; for (size_t j = 0; flags[j].name; ++j) { @@ -803,26 +813,27 @@ static int do_restorecon(const std::vector& args) { } } if (!found) { - LOG(ERROR) << "restorecon - bad flag " << args[i]; - return -1; + return Error() << "bad flag " << args[i]; } } else { in_flags = false; if (selinux_android_restorecon(args[i].c_str(), flag) < 0) { - ret = -errno; + ret = errno; } } } - return ret; + + if (ret) return ErrnoError() << "selinux_android_restorecon() failed"; + return Success(); } -static int do_restorecon_recursive(const std::vector& args) { +static Result do_restorecon_recursive(const std::vector& args) { std::vector non_const_args(args); non_const_args.insert(std::next(non_const_args.begin()), "--recursive"); return do_restorecon(non_const_args); } -static int do_loglevel(const std::vector& args) { +static Result do_loglevel(const std::vector& args) { // TODO: support names instead/as well? int log_level = -1; android::base::ParseInt(args[1], &log_level); @@ -837,77 +848,73 @@ static int do_loglevel(const std::vector& args) { case 1: case 0: severity = android::base::FATAL; break; default: - LOG(ERROR) << "loglevel: invalid log level " << log_level; - return -EINVAL; + return Error() << "invalid log level " << log_level; } android::base::SetMinimumLogSeverity(severity); - return 0; + return Success(); } -static int do_load_persist_props(const std::vector& args) { +static Result do_load_persist_props(const std::vector& args) { load_persist_props(); - return 0; + return Success(); } -static int do_load_system_props(const std::vector& args) { +static Result do_load_system_props(const std::vector& args) { load_system_props(); - return 0; + return Success(); } -static int do_wait(const std::vector& args) { - if (args.size() == 2) { - return wait_for_file(args[1].c_str(), kCommandRetryTimeout); - } else if (args.size() == 3) { - int timeout; - if (android::base::ParseInt(args[2], &timeout)) { - return wait_for_file(args[1].c_str(), std::chrono::seconds(timeout)); +static Result do_wait(const std::vector& args) { + auto timeout = kCommandRetryTimeout; + if (args.size() == 3) { + int timeout_int; + if (!android::base::ParseInt(args[2], &timeout_int)) { + return Error() << "failed to parse timeout"; } + timeout = std::chrono::seconds(timeout_int); } - return -1; + + if (wait_for_file(args[1].c_str(), timeout) != 0) { + return Error() << "wait_for_file() failed"; + } + + return Success(); } -static int do_wait_for_prop(const std::vector& args) { +static Result do_wait_for_prop(const std::vector& args) { const char* name = args[1].c_str(); const char* value = args[2].c_str(); size_t value_len = strlen(value); if (!is_legal_property_name(name)) { - LOG(ERROR) << "do_wait_for_prop(\"" << name << "\", \"" << value - << "\") failed: bad name"; - return -1; + return Error() << "is_legal_property_name(" << name << ") failed"; } if (value_len >= PROP_VALUE_MAX) { - LOG(ERROR) << "do_wait_for_prop(\"" << name << "\", \"" << value - << "\") failed: value too long"; - return -1; + return Error() << "value too long"; } if (!start_waiting_for_property(name, value)) { - LOG(ERROR) << "do_wait_for_prop(\"" << name << "\", \"" << value - << "\") failed: init already in waiting"; - return -1; + return Error() << "already waiting for a property"; } - return 0; + return Success(); } static bool is_file_crypto() { return android::base::GetProperty("ro.crypto.type", "") == "file"; } -static int do_installkey(const std::vector& args) { - if (!is_file_crypto()) { - return 0; - } +static Result do_installkey(const std::vector& args) { + if (!is_file_crypto()) return Success(); + auto unencrypted_dir = args[1] + e4crypt_unencrypted_folder; if (!make_dir(unencrypted_dir, 0700) && errno != EEXIST) { - PLOG(ERROR) << "Failed to create " << unencrypted_dir; - return -1; + return ErrnoError() << "Failed to create " << unencrypted_dir; } std::vector exec_args = {"exec", "/system/bin/vdc", "--wait", "cryptfs", "enablefilecrypto"}; return do_exec(exec_args); } -static int do_init_user0(const std::vector& args) { +static Result do_init_user0(const std::vector& args) { std::vector exec_args = {"exec", "/system/bin/vdc", "--wait", "cryptfs", "init_user0"}; return do_exec(exec_args); diff --git a/init/builtins.h b/init/builtins.h index b110f619f..f66ae1940 100644 --- a/init/builtins.h +++ b/init/builtins.h @@ -23,11 +23,12 @@ #include #include "keyword_map.h" +#include "result.h" namespace android { namespace init { -using BuiltinFunction = std::function&)>; +using BuiltinFunction = std::function(const std::vector&)>; class BuiltinFunctionMap : public KeywordMap { public: BuiltinFunctionMap() {} diff --git a/init/init.cpp b/init/init.cpp index ee3a84c29..c65d8460b 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -238,7 +238,7 @@ void handle_control_message(const std::string& msg, const std::string& name) { } } -static int wait_for_coldboot_done_action(const std::vector& args) { +static Result wait_for_coldboot_done_action(const std::vector& args) { Timer t; LOG(VERBOSE) << "Waiting for " COLDBOOT_DONE "..."; @@ -257,22 +257,20 @@ static int wait_for_coldboot_done_action(const std::vector& args) { } property_set("ro.boottime.init.cold_boot_wait", std::to_string(t.duration().count())); - return 0; + return Success(); } -static int keychord_init_action(const std::vector& args) -{ +static Result keychord_init_action(const std::vector& args) { keychord_init(); - return 0; + return Success(); } -static int console_init_action(const std::vector& args) -{ +static Result console_init_action(const std::vector& args) { std::string console = GetProperty("ro.boot.console", ""); if (!console.empty()) { default_console = "/dev/" + console; } - return 0; + return Success(); } static void import_kernel_nv(const std::string& key, const std::string& value, bool for_emulator) { @@ -354,18 +352,16 @@ static void process_kernel_cmdline() { if (qemu[0]) import_kernel_cmdline(true, import_kernel_nv); } -static int property_enable_triggers_action(const std::vector& args) -{ +static Result property_enable_triggers_action(const std::vector& args) { /* Enable property triggers. */ property_triggers_enabled = 1; - return 0; + return Success(); } -static int queue_property_triggers_action(const std::vector& args) -{ +static Result queue_property_triggers_action(const std::vector& args) { ActionManager::GetInstance().QueueBuiltinAction(property_enable_triggers_action, "enable_property_trigger"); ActionManager::GetInstance().QueueAllPropertyActions(); - return 0; + return Success(); } static void global_seccomp() { diff --git a/init/init_test.cpp b/init/init_test.cpp index 58e3d758e..27659f917 100644 --- a/init/init_test.cpp +++ b/init/init_test.cpp @@ -37,7 +37,7 @@ class TestFunctionMap : public KeywordMap { void Add(const std::string& name, const BuiltinFunctionNoArgs function) { Add(name, 0, 0, [function](const std::vector&) { function(); - return 0; + return Success(); }); } @@ -174,7 +174,7 @@ TEST(init, EventTriggerOrderMultipleFiles) { auto execute_command = [&num_executed](const std::vector& args) { EXPECT_EQ(2U, args.size()); EXPECT_EQ(++num_executed, std::stoi(args[1])); - return 0; + return Success(); }; TestFunctionMap test_function_map; diff --git a/init/reboot.cpp b/init/reboot.cpp index 32816fe40..24ccdfc6a 100644 --- a/init/reboot.cpp +++ b/init/reboot.cpp @@ -520,7 +520,7 @@ bool HandlePowerctlMessage(const std::string& command) { auto shutdown_handler = [cmd, command, reboot_target, run_fsck](const std::vector&) { DoReboot(cmd, command, reboot_target, run_fsck); - return 0; + return Success(); }; ActionManager::GetInstance().QueueBuiltinAction(shutdown_handler, "shutdown_done"); diff --git a/init/security.cpp b/init/security.cpp index 45a5412bf..f8976dec2 100644 --- a/init/security.cpp +++ b/init/security.cpp @@ -45,24 +45,22 @@ namespace init { // devices/configurations where these I/O operations are blocking for a long // time. We do not reboot or halt on failures, as this is a best-effort // attempt. -int MixHwrngIntoLinuxRngAction(const std::vector& args) { +Result MixHwrngIntoLinuxRngAction(const std::vector& args) { unique_fd hwrandom_fd( TEMP_FAILURE_RETRY(open("/dev/hw_random", O_RDONLY | O_NOFOLLOW | O_CLOEXEC))); if (hwrandom_fd == -1) { if (errno == ENOENT) { LOG(INFO) << "/dev/hw_random not found"; // It's not an error to not have a Hardware RNG. - return 0; + return Success(); } - PLOG(ERROR) << "Failed to open /dev/hw_random"; - return -1; + return ErrnoError() << "Failed to open /dev/hw_random"; } unique_fd urandom_fd( TEMP_FAILURE_RETRY(open("/dev/urandom", O_WRONLY | O_NOFOLLOW | O_CLOEXEC))); if (urandom_fd == -1) { - PLOG(ERROR) << "Failed to open /dev/urandom"; - return -1; + return ErrnoError() << "Failed to open /dev/urandom"; } char buf[512]; @@ -71,23 +69,20 @@ int MixHwrngIntoLinuxRngAction(const std::vector& args) { ssize_t chunk_size = TEMP_FAILURE_RETRY(read(hwrandom_fd, buf, sizeof(buf) - total_bytes_written)); if (chunk_size == -1) { - PLOG(ERROR) << "Failed to read from /dev/hw_random"; - return -1; + return ErrnoError() << "Failed to read from /dev/hw_random"; } else if (chunk_size == 0) { - LOG(ERROR) << "Failed to read from /dev/hw_random: EOF"; - return -1; + return Error() << "Failed to read from /dev/hw_random: EOF"; } chunk_size = TEMP_FAILURE_RETRY(write(urandom_fd, buf, chunk_size)); if (chunk_size == -1) { - PLOG(ERROR) << "Failed to write to /dev/urandom"; - return -1; + return ErrnoError() << "Failed to write to /dev/urandom"; } total_bytes_written += chunk_size; } LOG(INFO) << "Mixed " << total_bytes_written << " bytes from /dev/hw_random into /dev/urandom"; - return 0; + return Success(); } static bool SetHighestAvailableOptionValue(std::string path, int min, int max) { @@ -154,38 +149,38 @@ static bool __attribute__((unused)) SetMmapRndBitsMin(int start, int min, bool c // 9e08f57d684a x86: mm: support ARCH_MMAP_RND_BITS // ec9ee4acd97c drivers: char: random: add get_random_long() // 5ef11c35ce86 mm: ASLR: use get_random_long() -int SetMmapRndBitsAction(const std::vector& args) { +Result SetMmapRndBitsAction(const std::vector& args) { // values are arch-dependent #if defined(USER_MODE_LINUX) // uml does not support mmap_rnd_bits - return 0; + return Success(); #elif defined(__aarch64__) // arm64 supports 18 - 33 bits depending on pagesize and VA_SIZE if (SetMmapRndBitsMin(33, 24, false) && SetMmapRndBitsMin(16, 16, true)) { - return 0; + return Success(); } #elif defined(__x86_64__) // x86_64 supports 28 - 32 bits if (SetMmapRndBitsMin(32, 32, false) && SetMmapRndBitsMin(16, 16, true)) { - return 0; + return Success(); } #elif defined(__arm__) || defined(__i386__) // check to see if we're running on 64-bit kernel bool h64 = !access(MMAP_RND_COMPAT_PATH, F_OK); // supported 32-bit architecture must have 16 bits set if (SetMmapRndBitsMin(16, 16, h64)) { - return 0; + return Success(); } #elif defined(__mips__) || defined(__mips64__) // TODO: add mips support b/27788820 - return 0; + return Success(); #else LOG(ERROR) << "Unknown architecture"; #endif LOG(ERROR) << "Unable to set adequate mmap entropy value!"; panic(); - return -1; + return Error(); } #define KPTR_RESTRICT_PATH "/proc/sys/kernel/kptr_restrict" @@ -195,14 +190,15 @@ int SetMmapRndBitsAction(const std::vector& args) { // Set kptr_restrict to the highest available level. // // Aborts if unable to set this to an acceptable value. -int SetKptrRestrictAction(const std::vector& args) { +Result SetKptrRestrictAction(const std::vector& args) { std::string path = KPTR_RESTRICT_PATH; if (!SetHighestAvailableOptionValue(path, KPTR_RESTRICT_MINVALUE, KPTR_RESTRICT_MAXVALUE)) { LOG(ERROR) << "Unable to set adequate kptr_restrict value!"; panic(); + return Error(); } - return 0; + return Success(); } } // namespace init diff --git a/init/security.h b/init/security.h index c489de143..31e5790f0 100644 --- a/init/security.h +++ b/init/security.h @@ -20,12 +20,14 @@ #include #include +#include "result.h" + namespace android { namespace init { -int MixHwrngIntoLinuxRngAction(const std::vector& args); -int SetMmapRndBitsAction(const std::vector& args); -int SetKptrRestrictAction(const std::vector& args); +Result MixHwrngIntoLinuxRngAction(const std::vector& args); +Result SetMmapRndBitsAction(const std::vector& args); +Result SetKptrRestrictAction(const std::vector& args); } // namespace init } // namespace android From 89bcc85edfff4a2b2378f638ab90b2f7e19a8472 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 2 Aug 2017 17:01:36 -0700 Subject: [PATCH 3/3] init: use Result for the parsing functions Test: boot bullhead Merged-In: I7f00c5f0f54dd4fe05df73e1d6a89b56d788e113 Change-Id: I7f00c5f0f54dd4fe05df73e1d6a89b56d788e113 --- init/action.cpp | 73 +++++++-------- init/action.h | 14 +-- init/import_parser.cpp | 12 ++- init/import_parser.h | 4 +- init/keyword_map.h | 29 +++--- init/parser.cpp | 19 ++-- init/parser.h | 10 ++- init/service.cpp | 195 ++++++++++++++++++---------------------- init/service.h | 59 ++++++------ init/ueventd.cpp | 6 +- init/ueventd_parser.cpp | 65 ++++++-------- init/ueventd_parser.h | 16 ++-- 12 files changed, 227 insertions(+), 275 deletions(-) diff --git a/init/action.cpp b/init/action.cpp index 671e285e1..f687074c6 100644 --- a/init/action.cpp +++ b/init/action.cpp @@ -53,19 +53,16 @@ Action::Action(bool oneshot, const std::string& filename, int line) const KeywordMap* Action::function_map_ = nullptr; -bool Action::AddCommand(const std::vector& args, int line, std::string* err) { +Result Action::AddCommand(const std::vector& args, int line) { if (!function_map_) { - *err = "no function map available"; - return false; + return Error() << "no function map available"; } - auto function = function_map_->FindFunction(args, err); - if (!function) { - return false; - } + auto function = function_map_->FindFunction(args); + if (!function) return Error() << function.error(); - AddCommand(function, args, line); - return true; + AddCommand(*function, args, line); + return Success(); } void Action::AddCommand(BuiltinFunction f, const std::vector& args, int line) { @@ -105,67 +102,60 @@ void Action::ExecuteCommand(const Command& command) const { } } -bool Action::ParsePropertyTrigger(const std::string& trigger, std::string* err) { +Result Action::ParsePropertyTrigger(const std::string& trigger) { const static std::string prop_str("property:"); std::string prop_name(trigger.substr(prop_str.length())); size_t equal_pos = prop_name.find('='); if (equal_pos == std::string::npos) { - *err = "property trigger found without matching '='"; - return false; + return Error() << "property trigger found without matching '='"; } std::string prop_value(prop_name.substr(equal_pos + 1)); prop_name.erase(equal_pos); if (auto [it, inserted] = property_triggers_.emplace(prop_name, prop_value); !inserted) { - *err = "multiple property triggers found for same property"; - return false; + return Error() << "multiple property triggers found for same property"; } - return true; + return Success(); } -bool Action::InitTriggers(const std::vector& args, std::string* err) { +Result Action::InitTriggers(const std::vector& args) { const static std::string prop_str("property:"); for (std::size_t i = 0; i < args.size(); ++i) { if (args[i].empty()) { - *err = "empty trigger is not valid"; - return false; + return Error() << "empty trigger is not valid"; } if (i % 2) { if (args[i] != "&&") { - *err = "&& is the only symbol allowed to concatenate actions"; - return false; + return Error() << "&& is the only symbol allowed to concatenate actions"; } else { continue; } } if (!args[i].compare(0, prop_str.length(), prop_str)) { - if (!ParsePropertyTrigger(args[i], err)) { - return false; + if (auto result = ParsePropertyTrigger(args[i]); !result) { + return result; } } else { if (!event_trigger_.empty()) { - *err = "multiple event triggers are not allowed"; - return false; + return Error() << "multiple event triggers are not allowed"; } event_trigger_ = args[i]; } } - return true; + return Success(); } -bool Action::InitSingleTrigger(const std::string& trigger) { +Result Action::InitSingleTrigger(const std::string& trigger) { std::vector name_vector{trigger}; - std::string err; - bool ret = InitTriggers(name_vector, &err); - if (!ret) { - LOG(ERROR) << "InitSingleTrigger failed due to: " << err; + if (auto result = InitTriggers(name_vector); !result) { + return Error() << "InitTriggers() failed: " << result.error(); } - return ret; + return Success(); } // This function checks that all property triggers are satisfied, that is @@ -263,7 +253,8 @@ void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& auto action = std::make_unique(true, "", 0); std::vector name_vector{name}; - if (!action->InitSingleTrigger(name)) { + if (auto result = action->InitSingleTrigger(name); !result) { + LOG(ERROR) << "Cannot queue BuiltinAction for " << name << ": " << result.error(); return; } @@ -332,25 +323,25 @@ void ActionManager::ClearQueue() { current_command_ = 0; } -bool ActionParser::ParseSection(std::vector&& args, const std::string& filename, - int line, std::string* err) { +Result ActionParser::ParseSection(std::vector&& args, + const std::string& filename, int line) { std::vector triggers(args.begin() + 1, args.end()); if (triggers.size() < 1) { - *err = "actions must have a trigger"; - return false; + return Error() << "Actions must have a trigger"; } auto action = std::make_unique(false, filename, line); - if (!action->InitTriggers(triggers, err)) { - return false; + + if (auto result = action->InitTriggers(triggers); !result) { + return Error() << "InitTriggers() failed: " << result.error(); } action_ = std::move(action); - return true; + return Success(); } -bool ActionParser::ParseLineSection(std::vector&& args, int line, std::string* err) { - return action_ ? action_->AddCommand(std::move(args), line, err) : false; +Result ActionParser::ParseLineSection(std::vector&& args, int line) { + return action_ ? action_->AddCommand(std::move(args), line) : Success(); } void ActionParser::EndSection() { diff --git a/init/action.h b/init/action.h index 44ecd237b..d977f827a 100644 --- a/init/action.h +++ b/init/action.h @@ -54,10 +54,10 @@ class Action { public: explicit Action(bool oneshot, const std::string& filename, int line); - bool AddCommand(const std::vector& args, int line, std::string* err); + Result AddCommand(const std::vector& args, int line); void AddCommand(BuiltinFunction f, const std::vector& args, int line); - bool InitTriggers(const std::vector& args, std::string* err); - bool InitSingleTrigger(const std::string& trigger); + Result InitTriggers(const std::vector& args); + Result InitSingleTrigger(const std::string& trigger); std::size_t NumCommands() const; void ExecuteOneCommand(std::size_t command) const; void ExecuteAllCommands() const; @@ -79,7 +79,7 @@ private: void ExecuteCommand(const Command& command) const; bool CheckPropertyTriggers(const std::string& name = "", const std::string& value = "") const; - bool ParsePropertyTrigger(const std::string& trigger, std::string* err); + Result ParsePropertyTrigger(const std::string& trigger); std::map property_triggers_; std::string event_trigger_; @@ -121,9 +121,9 @@ class ActionParser : public SectionParser { public: ActionParser(ActionManager* action_manager) : action_manager_(action_manager), action_(nullptr) {} - bool ParseSection(std::vector&& args, const std::string& filename, int line, - std::string* err) override; - bool ParseLineSection(std::vector&& args, int line, std::string* err) override; + Result ParseSection(std::vector&& args, const std::string& filename, + int line) override; + Result ParseLineSection(std::vector&& args, int line) override; void EndSection() override; private: diff --git a/init/import_parser.cpp b/init/import_parser.cpp index b9fa2cea7..e335fd111 100644 --- a/init/import_parser.cpp +++ b/init/import_parser.cpp @@ -23,24 +23,22 @@ namespace android { namespace init { -bool ImportParser::ParseSection(std::vector&& args, const std::string& filename, - int line, std::string* err) { +Result ImportParser::ParseSection(std::vector&& args, + const std::string& filename, int line) { if (args.size() != 2) { - *err = "single argument needed for import\n"; - return false; + return Error() << "single argument needed for import\n"; } std::string conf_file; bool ret = expand_props(args[1], &conf_file); if (!ret) { - *err = "error while expanding import"; - return false; + return Error() << "error while expanding import"; } LOG(INFO) << "Added '" << conf_file << "' to import list"; if (filename_.empty()) filename_ = filename; imports_.emplace_back(std::move(conf_file), line); - return true; + return Success(); } void ImportParser::EndFile() { diff --git a/init/import_parser.h b/init/import_parser.h index 0d04e0e27..5a2f89498 100644 --- a/init/import_parser.h +++ b/init/import_parser.h @@ -28,8 +28,8 @@ namespace init { class ImportParser : public SectionParser { public: ImportParser(Parser* parser) : parser_(parser) {} - bool ParseSection(std::vector&& args, const std::string& filename, int line, - std::string* err) override; + Result ParseSection(std::vector&& args, const std::string& filename, + int line) override; void EndFile() override; private: diff --git a/init/keyword_map.h b/init/keyword_map.h index 481d63702..c95fc7318 100644 --- a/init/keyword_map.h +++ b/init/keyword_map.h @@ -22,6 +22,8 @@ #include +#include "result.h" + namespace android { namespace init { @@ -34,20 +36,17 @@ class KeywordMap { virtual ~KeywordMap() { } - const Function FindFunction(const std::vector& args, std::string* err) const { + const Result FindFunction(const std::vector& args) const { using android::base::StringPrintf; - if (args.empty()) { - *err = "keyword needed, but not provided"; - return nullptr; - } + if (args.empty()) return Error() << "Keyword needed, but not provided"; + auto& keyword = args[0]; auto num_args = args.size() - 1; auto function_info_it = map().find(keyword); if (function_info_it == map().end()) { - *err = StringPrintf("invalid keyword '%s'", keyword.c_str()); - return nullptr; + return Error() << StringPrintf("Invalid keyword '%s'", keyword.c_str()); } auto function_info = function_info_it->second; @@ -55,22 +54,18 @@ class KeywordMap { auto min_args = std::get<0>(function_info); auto max_args = std::get<1>(function_info); if (min_args == max_args && num_args != min_args) { - *err = StringPrintf("%s requires %zu argument%s", - keyword.c_str(), min_args, - (min_args > 1 || min_args == 0) ? "s" : ""); - return nullptr; + return Error() << StringPrintf("%s requires %zu argument%s", keyword.c_str(), min_args, + (min_args > 1 || min_args == 0) ? "s" : ""); } if (num_args < min_args || num_args > max_args) { if (max_args == std::numeric_limits::max()) { - *err = StringPrintf("%s requires at least %zu argument%s", - keyword.c_str(), min_args, - min_args > 1 ? "s" : ""); + return Error() << StringPrintf("%s requires at least %zu argument%s", + keyword.c_str(), min_args, min_args > 1 ? "s" : ""); } else { - *err = StringPrintf("%s requires between %zu and %zu arguments", - keyword.c_str(), min_args, max_args); + return Error() << StringPrintf("%s requires between %zu and %zu arguments", + keyword.c_str(), min_args, max_args); } - return nullptr; } return std::get(function_info); diff --git a/init/parser.cpp b/init/parser.cpp index 4c34c265b..8a4e798d0 100644 --- a/init/parser.cpp +++ b/init/parser.cpp @@ -67,9 +67,8 @@ void Parser::ParseData(const std::string& filename, const std::string& data) { if (android::base::StartsWith(args[0], prefix.c_str())) { if (section_parser) section_parser->EndSection(); - std::string ret_err; - if (!callback(std::move(args), &ret_err)) { - LOG(ERROR) << filename << ": " << state.line << ": " << ret_err; + if (auto result = callback(std::move(args)); !result) { + LOG(ERROR) << filename << ": " << state.line << ": " << result.error(); } section_parser = nullptr; break; @@ -78,16 +77,16 @@ void Parser::ParseData(const std::string& filename, const std::string& data) { if (section_parsers_.count(args[0])) { if (section_parser) section_parser->EndSection(); section_parser = section_parsers_[args[0]].get(); - std::string ret_err; - if (!section_parser->ParseSection(std::move(args), filename, state.line, - &ret_err)) { - LOG(ERROR) << filename << ": " << state.line << ": " << ret_err; + if (auto result = + section_parser->ParseSection(std::move(args), filename, state.line); + !result) { + LOG(ERROR) << filename << ": " << state.line << ": " << result.error(); section_parser = nullptr; } } else if (section_parser) { - std::string ret_err; - if (!section_parser->ParseLineSection(std::move(args), state.line, &ret_err)) { - LOG(ERROR) << filename << ": " << state.line << ": " << ret_err; + if (auto result = section_parser->ParseLineSection(std::move(args), state.line); + !result) { + LOG(ERROR) << filename << ": " << state.line << ": " << result.error(); } } args.clear(); diff --git a/init/parser.h b/init/parser.h index fd65ad6ff..4ab24a4ee 100644 --- a/init/parser.h +++ b/init/parser.h @@ -22,6 +22,8 @@ #include #include +#include "result.h" + // SectionParser is an interface that can parse a given 'section' in init. // // You can implement up to 4 functions below, with ParseSection() being mandatory. @@ -51,9 +53,9 @@ namespace init { class SectionParser { public: virtual ~SectionParser() {} - virtual bool ParseSection(std::vector&& args, const std::string& filename, - int line, std::string* err) = 0; - virtual bool ParseLineSection(std::vector&&, int, std::string*) { return true; }; + virtual Result ParseSection(std::vector&& args, + const std::string& filename, int line) = 0; + virtual Result ParseLineSection(std::vector&&, int) { return Success(); }; virtual void EndSection(){}; virtual void EndFile(){}; }; @@ -67,7 +69,7 @@ class Parser { // Similar to ParseSection() and ParseLineSection(), this function returns bool with false // indicating a failure and has an std::string* err parameter into which an error string can // be written. - using LineCallback = std::function&&, std::string*)>; + using LineCallback = std::function(std::vector&&)>; Parser(); diff --git a/init/service.cpp b/init/service.cpp index e37888bf8..1b5cc19e4 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -333,12 +333,12 @@ void Service::DumpState() const { [] (const auto& info) { LOG(INFO) << *info; }); } -bool Service::ParseCapabilities(const std::vector& args, std::string* err) { +Result Service::ParseCapabilities(const std::vector& args) { capabilities_ = 0; if (!CapAmbientSupported()) { - *err = "capabilities requested but the kernel does not support ambient capabilities"; - return false; + return Error() + << "capabilities requested but the kernel does not support ambient capabilities"; } unsigned int last_valid_cap = GetLastValidCap(); @@ -350,76 +350,71 @@ bool Service::ParseCapabilities(const std::vector& args, std::strin const std::string& arg = args[i]; int res = LookupCap(arg); if (res < 0) { - *err = StringPrintf("invalid capability '%s'", arg.c_str()); - return false; + return Error() << StringPrintf("invalid capability '%s'", arg.c_str()); } unsigned int cap = static_cast(res); // |res| is >= 0. if (cap > last_valid_cap) { - *err = StringPrintf("capability '%s' not supported by the kernel", arg.c_str()); - return false; + return Error() << StringPrintf("capability '%s' not supported by the kernel", + arg.c_str()); } capabilities_[cap] = true; } - return true; + return Success(); } -bool Service::ParseClass(const std::vector& args, std::string* err) { +Result Service::ParseClass(const std::vector& args) { classnames_ = std::set(args.begin() + 1, args.end()); - return true; + return Success(); } -bool Service::ParseConsole(const std::vector& args, std::string* err) { +Result Service::ParseConsole(const std::vector& args) { flags_ |= SVC_CONSOLE; console_ = args.size() > 1 ? "/dev/" + args[1] : ""; - return true; + return Success(); } -bool Service::ParseCritical(const std::vector& args, std::string* err) { +Result Service::ParseCritical(const std::vector& args) { flags_ |= SVC_CRITICAL; - return true; + return Success(); } -bool Service::ParseDisabled(const std::vector& args, std::string* err) { +Result Service::ParseDisabled(const std::vector& args) { flags_ |= SVC_DISABLED; flags_ |= SVC_RC_DISABLED; - return true; + return Success(); } -bool Service::ParseGroup(const std::vector& args, std::string* err) { +Result Service::ParseGroup(const std::vector& args) { auto gid = DecodeUid(args[1]); if (!gid) { - *err = "Unable to decode GID for '" + args[1] + "': " + gid.error(); - return false; + return Error() << "Unable to decode GID for '" << args[1] << "': " << gid.error(); } gid_ = *gid; for (std::size_t n = 2; n < args.size(); n++) { gid = DecodeUid(args[n]); if (!gid) { - *err = "Unable to decode GID for '" + args[n] + "': " + gid.error(); - return false; + return Error() << "Unable to decode GID for '" << args[n] << "': " << gid.error(); } supp_gids_.emplace_back(*gid); } - return true; + return Success(); } -bool Service::ParsePriority(const std::vector& args, std::string* err) { +Result Service::ParsePriority(const std::vector& args) { priority_ = 0; if (!ParseInt(args[1], &priority_, static_cast(ANDROID_PRIORITY_HIGHEST), // highest is negative static_cast(ANDROID_PRIORITY_LOWEST))) { - *err = StringPrintf("process priority value must be range %d - %d", - ANDROID_PRIORITY_HIGHEST, ANDROID_PRIORITY_LOWEST); - return false; + return Error() << StringPrintf("process priority value must be range %d - %d", + ANDROID_PRIORITY_HIGHEST, ANDROID_PRIORITY_LOWEST); } - return true; + return Success(); } -bool Service::ParseIoprio(const std::vector& args, std::string* err) { +Result Service::ParseIoprio(const std::vector& args) { if (!ParseInt(args[2], &ioprio_pri_, 0, 7)) { - *err = "priority value must be range 0 - 7"; - return false; + return Error() << "priority value must be range 0 - 7"; } if (args[1] == "rt") { @@ -429,14 +424,13 @@ bool Service::ParseIoprio(const std::vector& args, std::string* err } else if (args[1] == "idle") { ioprio_class_ = IoSchedClass_IDLE; } else { - *err = "ioprio option usage: ioprio <0-7>"; - return false; + return Error() << "ioprio option usage: ioprio <0-7>"; } - return true; + return Success(); } -bool Service::ParseKeycodes(const std::vector& args, std::string* err) { +Result Service::ParseKeycodes(const std::vector& args) { for (std::size_t i = 1; i < args.size(); i++) { int code; if (ParseInt(args[i], &code)) { @@ -445,22 +439,24 @@ bool Service::ParseKeycodes(const std::vector& args, std::string* e LOG(WARNING) << "ignoring invalid keycode: " << args[i]; } } - return true; + return Success(); } -bool Service::ParseOneshot(const std::vector& args, std::string* err) { +Result Service::ParseOneshot(const std::vector& args) { flags_ |= SVC_ONESHOT; - return true; + return Success(); } -bool Service::ParseOnrestart(const std::vector& args, std::string* err) { +Result Service::ParseOnrestart(const std::vector& args) { std::vector str_args(args.begin() + 1, args.end()); int line = onrestart_.NumCommands() + 1; - onrestart_.AddCommand(str_args, line, err); - return true; + if (auto result = onrestart_.AddCommand(str_args, line); !result) { + return Error() << "cannot add Onrestart command: " << result.error(); + } + return Success(); } -bool Service::ParseNamespace(const std::vector& args, std::string* err) { +Result Service::ParseNamespace(const std::vector& args) { for (size_t i = 1; i < args.size(); i++) { if (args[i] == "pid") { namespace_flags_ |= CLONE_NEWPID; @@ -469,65 +465,60 @@ bool Service::ParseNamespace(const std::vector& args, std::string* } else if (args[i] == "mnt") { namespace_flags_ |= CLONE_NEWNS; } else { - *err = "namespace must be 'pid' or 'mnt'"; - return false; + return Error() << "namespace must be 'pid' or 'mnt'"; } } - return true; + return Success(); } -bool Service::ParseOomScoreAdjust(const std::vector& args, std::string* err) { +Result Service::ParseOomScoreAdjust(const std::vector& args) { if (!ParseInt(args[1], &oom_score_adjust_, -1000, 1000)) { - *err = "oom_score_adjust value must be in range -1000 - +1000"; - return false; + return Error() << "oom_score_adjust value must be in range -1000 - +1000"; } - return true; + return Success(); } -bool Service::ParseMemcgSwappiness(const std::vector& args, std::string* err) { +Result Service::ParseMemcgSwappiness(const std::vector& args) { if (!ParseInt(args[1], &swappiness_, 0)) { - *err = "swappiness value must be equal or greater than 0"; - return false; + return Error() << "swappiness value must be equal or greater than 0"; } - return true; + return Success(); } -bool Service::ParseMemcgLimitInBytes(const std::vector& args, std::string* err) { +Result Service::ParseMemcgLimitInBytes(const std::vector& args) { if (!ParseInt(args[1], &limit_in_bytes_, 0)) { - *err = "limit_in_bytes value must be equal or greater than 0"; - return false; + return Error() << "limit_in_bytes value must be equal or greater than 0"; } - return true; + return Success(); } -bool Service::ParseMemcgSoftLimitInBytes(const std::vector& args, std::string* err) { +Result Service::ParseMemcgSoftLimitInBytes(const std::vector& args) { if (!ParseInt(args[1], &soft_limit_in_bytes_, 0)) { - *err = "soft_limit_in_bytes value must be equal or greater than 0"; - return false; + return Error() << "soft_limit_in_bytes value must be equal or greater than 0"; } - return true; + return Success(); } -bool Service::ParseSeclabel(const std::vector& args, std::string* err) { +Result Service::ParseSeclabel(const std::vector& args) { seclabel_ = args[1]; - return true; + return Success(); } -bool Service::ParseSetenv(const std::vector& args, std::string* err) { +Result Service::ParseSetenv(const std::vector& args) { envvars_.emplace_back(args[1], args[2]); - return true; + return Success(); } -bool Service::ParseShutdown(const std::vector& args, std::string* err) { +Result Service::ParseShutdown(const std::vector& args) { if (args[1] == "critical") { flags_ |= SVC_SHUTDOWN_CRITICAL; - return true; + return Success(); } - return false; + return Error() << "Invalid shutdown option"; } template -bool Service::AddDescriptor(const std::vector& args, std::string* err) { +Result Service::AddDescriptor(const std::vector& args) { int perm = args.size() > 3 ? std::strtoul(args[3].c_str(), 0, 8) : -1; Result uid = 0; Result gid = 0; @@ -536,16 +527,14 @@ bool Service::AddDescriptor(const std::vector& args, std::string* e if (args.size() > 4) { uid = DecodeUid(args[4]); if (!uid) { - *err = "Unable to find UID for '" + args[4] + "': " + uid.error(); - return false; + return Error() << "Unable to find UID for '" << args[4] << "': " << uid.error(); } } if (args.size() > 5) { gid = DecodeUid(args[5]); if (!gid) { - *err = "Unable to find GID for '" + args[5] + "': " + gid.error(); - return false; + return Error() << "Unable to find GID for '" << args[5] << "': " << gid.error(); } } @@ -556,50 +545,45 @@ bool Service::AddDescriptor(const std::vector& args, std::string* e [&descriptor] (const auto& other) { return descriptor.get() == other.get(); }); if (old != descriptors_.end()) { - *err = "duplicate descriptor " + args[1] + " " + args[2]; - return false; + return Error() << "duplicate descriptor " << args[1] << " " << args[2]; } descriptors_.emplace_back(std::move(descriptor)); - return true; + return Success(); } // name type perm [ uid gid context ] -bool Service::ParseSocket(const std::vector& args, std::string* err) { +Result Service::ParseSocket(const std::vector& args) { if (!StartsWith(args[2], "dgram") && !StartsWith(args[2], "stream") && !StartsWith(args[2], "seqpacket")) { - *err = "socket type must be 'dgram', 'stream' or 'seqpacket'"; - return false; + return Error() << "socket type must be 'dgram', 'stream' or 'seqpacket'"; } - return AddDescriptor(args, err); + return AddDescriptor(args); } // name type perm [ uid gid context ] -bool Service::ParseFile(const std::vector& args, std::string* err) { +Result Service::ParseFile(const std::vector& args) { if (args[2] != "r" && args[2] != "w" && args[2] != "rw") { - *err = "file type must be 'r', 'w' or 'rw'"; - return false; + return Error() << "file type must be 'r', 'w' or 'rw'"; } if ((args[1][0] != '/') || (args[1].find("../") != std::string::npos)) { - *err = "file name must not be relative"; - return false; + return Error() << "file name must not be relative"; } - return AddDescriptor(args, err); + return AddDescriptor(args); } -bool Service::ParseUser(const std::vector& args, std::string* err) { +Result Service::ParseUser(const std::vector& args) { auto uid = DecodeUid(args[1]); if (!uid) { - *err = "Unable to find UID for '" + args[1] + "': " + uid.error(); - return false; + return Error() << "Unable to find UID for '" << args[1] << "': " << uid.error(); } uid_ = *uid; - return true; + return Success(); } -bool Service::ParseWritepid(const std::vector& args, std::string* err) { +Result Service::ParseWritepid(const std::vector& args) { writepid_files_.assign(args.begin() + 1, args.end()); - return true; + return Success(); } class Service::OptionParserMap : public KeywordMap { @@ -647,15 +631,13 @@ const Service::OptionParserMap::Map& Service::OptionParserMap::map() const { return option_parsers; } -bool Service::ParseLine(const std::vector& args, std::string* err) { +Result Service::ParseLine(const std::vector& args) { static const OptionParserMap parser_map; - auto parser = parser_map.FindFunction(args, err); + auto parser = parser_map.FindFunction(args); - if (!parser) { - return false; - } + if (!parser) return Error() << parser.error(); - return (this->*parser)(args, err); + return std::invoke(*parser, this, args); } bool Service::ExecStart() { @@ -1044,32 +1026,29 @@ void ServiceList::DumpState() const { } } -bool ServiceParser::ParseSection(std::vector&& args, const std::string& filename, - int line, std::string* err) { +Result ServiceParser::ParseSection(std::vector&& args, + const std::string& filename, int line) { if (args.size() < 3) { - *err = "services must have a name and a program"; - return false; + return Error() << "services must have a name and a program"; } const std::string& name = args[1]; if (!IsValidName(name)) { - *err = StringPrintf("invalid service name '%s'", name.c_str()); - return false; + return Error() << "invalid service name '" << name << "'"; } Service* old_service = service_list_->FindService(name); if (old_service) { - *err = "ignored duplicate definition of service '" + name + "'"; - return false; + return Error() << "ignored duplicate definition of service '" << name << "'"; } std::vector str_args(args.begin() + 2, args.end()); service_ = std::make_unique(name, str_args); - return true; + return Success(); } -bool ServiceParser::ParseLineSection(std::vector&& args, int line, std::string* err) { - return service_ ? service_->ParseLine(std::move(args), err) : false; +Result ServiceParser::ParseLineSection(std::vector&& args, int line) { + return service_ ? service_->ParseLine(std::move(args)) : Success(); } void ServiceParser::EndSection() { diff --git a/init/service.h b/init/service.h index 6c143cb61..fe851299e 100644 --- a/init/service.h +++ b/init/service.h @@ -76,7 +76,7 @@ class Service { static std::unique_ptr MakeTemporaryOneshotService(const std::vector& args); bool IsRunning() { return (flags_ & SVC_RUNNING) != 0; } - bool ParseLine(const std::vector& args, std::string* err); + Result ParseLine(const std::vector& args); bool ExecStart(); bool Start(); bool StartIfNotDisabled(); @@ -119,8 +119,7 @@ class Service { const std::vector& args() const { return args_; } private: - using OptionParser = bool (Service::*) (const std::vector& args, - std::string* err); + using OptionParser = Result (Service::*)(const std::vector& args); class OptionParserMap; void NotifyStateChange(const std::string& new_state) const; @@ -130,32 +129,32 @@ class Service { void KillProcessGroup(int signal); void SetProcessAttributes(); - bool ParseCapabilities(const std::vector& args, std::string *err); - bool ParseClass(const std::vector& args, std::string* err); - bool ParseConsole(const std::vector& args, std::string* err); - bool ParseCritical(const std::vector& args, std::string* err); - bool ParseDisabled(const std::vector& args, std::string* err); - bool ParseGroup(const std::vector& args, std::string* err); - bool ParsePriority(const std::vector& args, std::string* err); - bool ParseIoprio(const std::vector& args, std::string* err); - bool ParseKeycodes(const std::vector& args, std::string* err); - bool ParseOneshot(const std::vector& args, std::string* err); - bool ParseOnrestart(const std::vector& args, std::string* err); - bool ParseOomScoreAdjust(const std::vector& args, std::string* err); - bool ParseMemcgLimitInBytes(const std::vector& args, std::string* err); - bool ParseMemcgSoftLimitInBytes(const std::vector& args, std::string* err); - bool ParseMemcgSwappiness(const std::vector& args, std::string* err); - bool ParseNamespace(const std::vector& args, std::string* err); - bool ParseSeclabel(const std::vector& args, std::string* err); - bool ParseSetenv(const std::vector& args, std::string* err); - bool ParseShutdown(const std::vector& args, std::string* err); - bool ParseSocket(const std::vector& args, std::string* err); - bool ParseFile(const std::vector& args, std::string* err); - bool ParseUser(const std::vector& args, std::string* err); - bool ParseWritepid(const std::vector& args, std::string* err); + Result ParseCapabilities(const std::vector& args); + Result ParseClass(const std::vector& args); + Result ParseConsole(const std::vector& args); + Result ParseCritical(const std::vector& args); + Result ParseDisabled(const std::vector& args); + Result ParseGroup(const std::vector& args); + Result ParsePriority(const std::vector& args); + Result ParseIoprio(const std::vector& args); + Result ParseKeycodes(const std::vector& args); + Result ParseOneshot(const std::vector& args); + Result ParseOnrestart(const std::vector& args); + Result ParseOomScoreAdjust(const std::vector& args); + Result ParseMemcgLimitInBytes(const std::vector& args); + Result ParseMemcgSoftLimitInBytes(const std::vector& args); + Result ParseMemcgSwappiness(const std::vector& args); + Result ParseNamespace(const std::vector& args); + Result ParseSeclabel(const std::vector& args); + Result ParseSetenv(const std::vector& args); + Result ParseShutdown(const std::vector& args); + Result ParseSocket(const std::vector& args); + Result ParseFile(const std::vector& args); + Result ParseUser(const std::vector& args); + Result ParseWritepid(const std::vector& args); template - bool AddDescriptor(const std::vector& args, std::string* err); + Result AddDescriptor(const std::vector& args); static unsigned long next_start_order_; static bool is_exec_service_running_; @@ -242,9 +241,9 @@ class ServiceList { class ServiceParser : public SectionParser { public: ServiceParser(ServiceList* service_list) : service_list_(service_list), service_(nullptr) {} - bool ParseSection(std::vector&& args, const std::string& filename, int line, - std::string* err) override; - bool ParseLineSection(std::vector&& args, int line, std::string* err) override; + Result ParseSection(std::vector&& args, const std::string& filename, + int line) override; + Result ParseLineSection(std::vector&& args, int line) override; void EndSection() override; private: diff --git a/init/ueventd.cpp b/init/ueventd.cpp index b71945acc..1435d82ef 100644 --- a/init/ueventd.cpp +++ b/init/ueventd.cpp @@ -223,10 +223,10 @@ DeviceHandler CreateDeviceHandler() { using namespace std::placeholders; std::vector sysfs_permissions; std::vector dev_permissions; - parser.AddSingleLineParser( - "/sys/", std::bind(ParsePermissionsLine, _1, _2, &sysfs_permissions, nullptr)); + parser.AddSingleLineParser("/sys/", + std::bind(ParsePermissionsLine, _1, &sysfs_permissions, nullptr)); parser.AddSingleLineParser("/dev/", - std::bind(ParsePermissionsLine, _1, _2, nullptr, &dev_permissions)); + std::bind(ParsePermissionsLine, _1, nullptr, &dev_permissions)); parser.ParseConfig("/ueventd.rc"); parser.ParseConfig("/vendor/ueventd.rc"); diff --git a/init/ueventd_parser.cpp b/init/ueventd_parser.cpp index 02e0d42e5..e831b8b63 100644 --- a/init/ueventd_parser.cpp +++ b/init/ueventd_parser.cpp @@ -24,18 +24,16 @@ namespace android { namespace init { -bool ParsePermissionsLine(std::vector&& args, std::string* err, - std::vector* out_sysfs_permissions, - std::vector* out_dev_permissions) { +Result ParsePermissionsLine(std::vector&& args, + std::vector* out_sysfs_permissions, + std::vector* out_dev_permissions) { bool is_sysfs = out_sysfs_permissions != nullptr; if (is_sysfs && args.size() != 5) { - *err = "/sys/ lines must have 5 entries"; - return false; + return Error() << "/sys/ lines must have 5 entries"; } if (!is_sysfs && args.size() != 4) { - *err = "/dev/ lines must have 4 entries"; - return false; + return Error() << "/dev/ lines must have 4 entries"; } auto it = args.begin(); @@ -49,23 +47,20 @@ bool ParsePermissionsLine(std::vector&& args, std::string* err, char* end_pointer = 0; mode_t perm = strtol(perm_string.c_str(), &end_pointer, 8); if (end_pointer == nullptr || *end_pointer != '\0') { - *err = "invalid mode '" + perm_string + "'"; - return false; + return Error() << "invalid mode '" << perm_string << "'"; } std::string& uid_string = *it++; passwd* pwd = getpwnam(uid_string.c_str()); if (!pwd) { - *err = "invalid uid '" + uid_string + "'"; - return false; + return Error() << "invalid uid '" << uid_string << "'"; } uid_t uid = pwd->pw_uid; std::string& gid_string = *it++; struct group* grp = getgrnam(gid_string.c_str()); if (!grp) { - *err = "invalid gid '" + gid_string + "'"; - return false; + return Error() << "invalid gid '" << gid_string << "'"; } gid_t gid = grp->gr_gid; @@ -74,53 +69,49 @@ bool ParsePermissionsLine(std::vector&& args, std::string* err, } else { out_dev_permissions->emplace_back(name, perm, uid, gid); } - return true; + return Success(); } -bool SubsystemParser::ParseSection(std::vector&& args, const std::string& filename, - int line, std::string* err) { +Result SubsystemParser::ParseSection(std::vector&& args, + const std::string& filename, int line) { if (args.size() != 2) { - *err = "subsystems must have exactly one name"; - return false; + return Error() << "subsystems must have exactly one name"; } if (std::find(subsystems_->begin(), subsystems_->end(), args[1]) != subsystems_->end()) { - *err = "ignoring duplicate subsystem entry"; - return false; + return Error() << "ignoring duplicate subsystem entry"; } subsystem_.name_ = args[1]; - return true; + return Success(); } -bool SubsystemParser::ParseDevName(std::vector&& args, std::string* err) { +Result SubsystemParser::ParseDevName(std::vector&& args) { if (args[1] == "uevent_devname") { subsystem_.devname_source_ = Subsystem::DevnameSource::DEVNAME_UEVENT_DEVNAME; - return true; + return Success(); } if (args[1] == "uevent_devpath") { subsystem_.devname_source_ = Subsystem::DevnameSource::DEVNAME_UEVENT_DEVPATH; - return true; + return Success(); } - *err = "invalid devname '" + args[1] + "'"; - return false; + return Error() << "invalid devname '" << args[1] << "'"; } -bool SubsystemParser::ParseDirName(std::vector&& args, std::string* err) { +Result SubsystemParser::ParseDirName(std::vector&& args) { if (args[1].front() != '/') { - *err = "dirname '" + args[1] + " ' does not start with '/'"; - return false; + return Error() << "dirname '" << args[1] << " ' does not start with '/'"; } subsystem_.dir_name_ = args[1]; - return true; + return Success(); } -bool SubsystemParser::ParseLineSection(std::vector&& args, int line, std::string* err) { - using OptionParser = - bool (SubsystemParser::*)(std::vector && args, std::string * err); +Result SubsystemParser::ParseLineSection(std::vector&& args, int line) { + using OptionParser = Result (SubsystemParser::*)(std::vector && args); + static class OptionParserMap : public KeywordMap { private: const Map& map() const override { @@ -134,13 +125,11 @@ bool SubsystemParser::ParseLineSection(std::vector&& args, int line } } parser_map; - auto parser = parser_map.FindFunction(args, err); + auto parser = parser_map.FindFunction(args); - if (!parser) { - return false; - } + if (!parser) return Error() << parser.error(); - return (this->*parser)(std::move(args), err); + return std::invoke(*parser, this, std::move(args)); } void SubsystemParser::EndSection() { diff --git a/init/ueventd_parser.h b/init/ueventd_parser.h index 51d83ef00..18d1027b4 100644 --- a/init/ueventd_parser.h +++ b/init/ueventd_parser.h @@ -29,22 +29,22 @@ namespace init { class SubsystemParser : public SectionParser { public: SubsystemParser(std::vector* subsystems) : subsystems_(subsystems) {} - bool ParseSection(std::vector&& args, const std::string& filename, int line, - std::string* err) override; - bool ParseLineSection(std::vector&& args, int line, std::string* err) override; + Result ParseSection(std::vector&& args, const std::string& filename, + int line) override; + Result ParseLineSection(std::vector&& args, int line) override; void EndSection() override; private: - bool ParseDevName(std::vector&& args, std::string* err); - bool ParseDirName(std::vector&& args, std::string* err); + Result ParseDevName(std::vector&& args); + Result ParseDirName(std::vector&& args); Subsystem subsystem_; std::vector* subsystems_; }; -bool ParsePermissionsLine(std::vector&& args, std::string* err, - std::vector* out_sysfs_permissions, - std::vector* out_dev_permissions); +Result ParsePermissionsLine(std::vector&& args, + std::vector* out_sysfs_permissions, + std::vector* out_dev_permissions); } // namespace init } // namespace android