diff --git a/init/action.cpp b/init/action.cpp index f687074c6..a7aa7567a 100644 --- a/init/action.cpp +++ b/init/action.cpp @@ -98,7 +98,7 @@ void Action::ExecuteCommand(const Command& command) const { LOG(INFO) << "Command '" << cmd_str << "' action=" << trigger_name << " (" << filename_ << ":" << command.line() << ") took " << duration.count() << "ms and " - << (result ? "succeeded" : "failed: " + result.error()); + << (result ? "succeeded" : "failed: " + result.error_string()); } } diff --git a/init/result.h b/init/result.h index 64fa69f26..36c3b8324 100644 --- a/init/result.h +++ b/init/result.h @@ -21,9 +21,13 @@ // There are 3 classes that implement this functionality and one additional helper type. // // Result either contains a member of type T that can be accessed using similar semantics as -// std::optional or it contains a std::string describing an error, which can be accessed via +// std::optional or it contains a ResultError describing an error, which can be accessed via // Result::error(). // +// ResultError is a type that contains both a std::string describing the error and a copy of errno +// from when the error occurred. ResultError can be used in an ostream directly to print its +// string value. +// // Success is a typedef that aids in creating Result that do not contain a return value. // Result is the correct return type for a function that either returns successfully or // returns an error value. Returning Success() from a function that returns Result is the @@ -33,10 +37,20 @@ // to T or from the constructor arguments for T. This allows you to return a type T directly from // a function that returns Result. // -// Error and ErrnoError are used to construct a Result that has failed. Each of these classes -// take an ostream as an input and are implicitly cast to a Result containing that failure. -// ErrnoError() additionally appends ": " + strerror(errno) to the end of the failure string to aid -// in interacting with C APIs. +// Error and ErrnoError are used to construct a Result that has failed. The Error class takes +// an ostream as an input and are implicitly cast to a Result containing that failure. +// ErrnoError() is a helper function to create an Error class that appends ": " + strerror(errno) +// to the end of the failure string to aid in interacting with C APIs. Alternatively, an errno +// value can be directly specified via the Error() constructor. +// +// ResultError can be used in the ostream when using Error to construct a Result. In this case, +// the string that the ResultError takes is passed through the stream normally, but the errno is +// passed to the Result. This can be used to pass errno from a failing C function up multiple +// callers. +// +// ResultError can also directly construct a Result. This is particularly useful if you have a +// function that return Result but you have a Result and want to return its error. In this +// case, you can return the .error() from the Result to construct the Result. // An example of how to use these is below: // Result CalculateResult(const T& input) { @@ -66,9 +80,29 @@ namespace android { namespace init { +struct ResultError { + template + ResultError(T&& error_string, int error_errno) + : error_string(std::forward(error_string)), error_errno(error_errno) {} + + std::string error_string; + int error_errno; +}; + +inline std::ostream& operator<<(std::ostream& os, const ResultError& t) { + os << t.error_string; + return os; +} + +inline std::ostream& operator<<(std::ostream& os, ResultError&& t) { + os << std::move(t.error_string); + return os; +} + class Error { public: - Error() : append_errno_(0) {} + Error() : errno_(0), append_errno_(false) {} + Error(int errno_to_append) : errno_(errno_to_append), append_errno_(true) {} template Error&& operator<<(T&& t) { @@ -76,30 +110,45 @@ class Error { return std::move(*this); } - const std::string str() const { - if (append_errno_) { - return ss_.str() + ": " + strerror(append_errno_); - } - return ss_.str(); + Error&& operator<<(const ResultError& result_error) { + ss_ << result_error.error_string; + errno_ = result_error.error_errno; + return std::move(*this); } + Error&& operator<<(ResultError&& result_error) { + ss_ << std::move(result_error.error_string); + errno_ = result_error.error_errno; + return std::move(*this); + } + + const std::string str() const { + std::string str = ss_.str(); + if (append_errno_) { + if (str.empty()) { + return strerror(errno_); + } + return str + ": " + strerror(errno_); + } + return str; + } + + int get_errno() const { return errno_; } + Error(const Error&) = delete; Error(Error&&) = delete; Error& operator=(const Error&) = delete; Error& operator=(Error&&) = delete; - protected: - Error(int append_errno) : append_errno_(append_errno) {} - private: std::stringstream ss_; - int append_errno_; + int errno_; + bool append_errno_; }; -class ErrnoError : public Error { - public: - ErrnoError() : Error(errno) {} -}; +inline Error ErrnoError() { + return Error(errno); +} template class Result { @@ -107,7 +156,13 @@ class Result { template Result(U&&... result) : contents_(std::in_place_index_t<0>(), std::forward(result)...) {} - Result(Error&& fb) : contents_(std::in_place_index_t<1>(), fb.str()) {} + Result(Error&& error) : contents_(std::in_place_index_t<1>(), error.str(), error.get_errno()) {} + Result(const ResultError& result_error) + : contents_(std::in_place_index_t<1>(), result_error.error_string, + result_error.error_errno) {} + Result(ResultError&& result_error) + : contents_(std::in_place_index_t<1>(), std::move(result_error.error_string), + result_error.error_errno) {} bool has_value() const { return contents_.index() == 0; } @@ -116,9 +171,17 @@ class Result { T&& value() && { return std::get<0>(std::move(contents_)); } const T&& value() const && { return std::get<0>(std::move(contents_)); } - const std::string& error() const & { return std::get<1>(contents_); } - std::string&& error() && { return std::get<1>(std::move(contents_)); } - const std::string&& error() const && { return std::get<1>(std::move(contents_)); } + const ResultError& error() const & { return std::get<1>(contents_); } + ResultError&& error() && { return std::get<1>(std::move(contents_)); } + const ResultError&& error() const && { return std::get<1>(std::move(contents_)); } + + const std::string& error_string() const & { return std::get<1>(contents_).error_string; } + std::string&& error_string() && { return std::get<1>(std::move(contents_)).error_string; } + const std::string&& error_string() const && { + return std::get<1>(std::move(contents_)).error_string; + } + + int error_errno() const { return std::get<1>(contents_).error_errno; } explicit operator bool() const { return has_value(); } @@ -131,7 +194,7 @@ class Result { const T* operator->() const { return &value(); } private: - std::variant contents_; + std::variant contents_; }; using Success = std::monostate; diff --git a/init/result_test.cpp b/init/result_test.cpp index ca6501369..19caaf5b5 100644 --- a/init/result_test.cpp +++ b/init/result_test.cpp @@ -74,7 +74,8 @@ TEST(result, result_error) { ASSERT_FALSE(result); ASSERT_FALSE(result.has_value()); - EXPECT_EQ("failure1", result.error()); + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ("failure1", result.error_string()); } TEST(result, result_error_empty) { @@ -82,7 +83,8 @@ TEST(result, result_error_empty) { ASSERT_FALSE(result); ASSERT_FALSE(result.has_value()); - EXPECT_EQ("", result.error()); + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ("", result.error_string()); } TEST(result, result_error_rvalue) { @@ -96,7 +98,8 @@ TEST(result, result_error_rvalue) { ASSERT_FALSE(MakeRvalueErrorResult()); ASSERT_FALSE(MakeRvalueErrorResult().has_value()); - EXPECT_EQ("failure1", MakeRvalueErrorResult().error()); + EXPECT_EQ(0, MakeRvalueErrorResult().error_errno()); + EXPECT_EQ("failure1", MakeRvalueErrorResult().error_string()); } TEST(result, result_errno_error) { @@ -107,7 +110,72 @@ TEST(result, result_errno_error) { ASSERT_FALSE(result); ASSERT_FALSE(result.has_value()); - EXPECT_EQ("failure1: "s + strerror(test_errno), result.error()); + EXPECT_EQ(test_errno, result.error_errno()); + EXPECT_EQ("failure1: "s + strerror(test_errno), result.error_string()); +} + +TEST(result, result_errno_error_no_text) { + constexpr int test_errno = 6; + errno = test_errno; + Result result = ErrnoError(); + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ(test_errno, result.error_errno()); + EXPECT_EQ(strerror(test_errno), result.error_string()); +} + +TEST(result, result_error_from_other_result) { + auto error_text = "test error"s; + Result result = Error() << error_text; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + Result result2 = result.error(); + + ASSERT_FALSE(result2); + ASSERT_FALSE(result2.has_value()); + + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ(error_text, result.error_string()); +} + +TEST(result, result_error_through_ostream) { + auto error_text = "test error"s; + Result result = Error() << error_text; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + Result result2 = Error() << result.error(); + + ASSERT_FALSE(result2); + ASSERT_FALSE(result2.has_value()); + + EXPECT_EQ(0, result.error_errno()); + EXPECT_EQ(error_text, result.error_string()); +} + +TEST(result, result_errno_error_through_ostream) { + auto error_text = "test error"s; + constexpr int test_errno = 6; + errno = 6; + Result result = ErrnoError() << error_text; + + errno = 0; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + Result result2 = Error() << result.error(); + + ASSERT_FALSE(result2); + ASSERT_FALSE(result2.has_value()); + + EXPECT_EQ(test_errno, result.error_errno()); + EXPECT_EQ(error_text + ": " + strerror(test_errno), result.error_string()); } TEST(result, constructor_forwarding) { @@ -215,7 +283,7 @@ TEST(result, die_on_access_failed_result) { TEST(result, die_on_get_error_succesful_result) { Result result = "success"; - ASSERT_DEATH(result.error(), ""); + ASSERT_DEATH(result.error_string(), ""); } } // namespace init diff --git a/init/util_test.cpp b/init/util_test.cpp index 007d10ee3..3ae53a481 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -34,7 +34,7 @@ TEST(util, ReadFile_ENOENT) { auto file_contents = ReadFile("/proc/does-not-exist"); EXPECT_EQ(ENOENT, errno); ASSERT_FALSE(file_contents); - EXPECT_EQ("open() failed: No such file or directory", file_contents.error()); + EXPECT_EQ("open() failed: No such file or directory", file_contents.error_string()); } TEST(util, ReadFileGroupWriteable) { @@ -45,7 +45,7 @@ TEST(util, ReadFileGroupWriteable) { EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0620, AT_SYMLINK_NOFOLLOW)) << strerror(errno); auto file_contents = ReadFile(tf.path); ASSERT_FALSE(file_contents) << strerror(errno); - EXPECT_EQ("Skipping insecure file", file_contents.error()); + EXPECT_EQ("Skipping insecure file", file_contents.error_string()); } TEST(util, ReadFileWorldWiteable) { @@ -56,7 +56,7 @@ TEST(util, ReadFileWorldWiteable) { EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0602, AT_SYMLINK_NOFOLLOW)) << strerror(errno); auto file_contents = ReadFile(tf.path); ASSERT_FALSE(file_contents) << strerror(errno); - EXPECT_EQ("Skipping insecure file", file_contents.error()); + EXPECT_EQ("Skipping insecure file", file_contents.error_string()); } TEST(util, ReadFileSymbolicLink) { @@ -65,7 +65,7 @@ TEST(util, ReadFileSymbolicLink) { auto file_contents = ReadFile("/charger"); EXPECT_EQ(ELOOP, errno); ASSERT_FALSE(file_contents); - EXPECT_EQ("open() failed: Too many symbolic links encountered", file_contents.error()); + EXPECT_EQ("open() failed: Too many symbolic links encountered", file_contents.error_string()); } TEST(util, ReadFileSuccess) { @@ -130,7 +130,7 @@ TEST(util, DecodeUid) { decoded_uid = DecodeUid("toot"); EXPECT_FALSE(decoded_uid); - EXPECT_EQ("getpwnam failed: No such file or directory", decoded_uid.error()); + EXPECT_EQ("getpwnam failed: No such file or directory", decoded_uid.error_string()); decoded_uid = DecodeUid("123"); EXPECT_TRUE(decoded_uid);