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