Make message realated methods of class Error shorter and less diverse (#5410)

Summary:
Pull Request resolved: https://github.com/facebook/osquery/pull/5410

 - get rid of *Short* methods
 - getFullMessage -> getNonRecursiveMessage
 - getFullMessageRecursive -> getMessage

Reviewed By: mkareta

Differential Revision: D13897854

fbshipit-source-id: 3e97ceefb2a48a16cd400f7ba7dd730724957ef0
This commit is contained in:
Alexander Kindyakov 2019-02-01 07:31:03 -08:00 committed by Facebook Github Bot
parent da91d8cfe8
commit 79cd575790
22 changed files with 54 additions and 73 deletions

View File

@ -475,7 +475,7 @@ Status Config::refresh() {
if (FLAGS_config_enable_backup && is_first_time_refresh.exchange(false)) {
const auto result = restoreConfigBackup();
if (!result) {
return Status::failure(result.getError().getFullMessageRecursive());
return Status::failure(result.getError().getMessage());
} else {
update(*result);
}

View File

@ -88,7 +88,7 @@ class Database {
const std::vector<std::pair<std::string, std::string>>& data) = 0;
void panic(const Error<DatabaseError>& error) {
LOG(ERROR) << "Database did panic: " << error.getFullMessageRecursive();
LOG(ERROR) << "Database did panic: " << error.getMessage();
debug_only::fail("Database did panic");
}

View File

@ -96,8 +96,8 @@ Expected<T, DatabaseError> InMemoryDatabase::getValue(const std::string& domain,
createError(DatabaseError::KeyNotFound, "Requested wrong type for: ")
<< domain << ":" << key << " stored type: " << value.type().name()
<< " requested type " << boost::core::demangle(typeid(T).name());
LOG(ERROR) << error.getFullMessageRecursive();
debug_only::fail(error.getFullMessageRecursive().c_str());
LOG(ERROR) << error.getMessage();
debug_only::fail(error.getMessage().c_str());
return std::move(error);
}
}

View File

@ -292,7 +292,7 @@ Expected<std::string, DatabaseError> RocksdbDatabase::getString(
if (BOOST_UNLIKELY(validateInt32StorageBuffer(result_str))) {
auto type_error = createError(RocksdbError::UnexpectedValueType,
"Fetching string as integer");
LOG(ERROR) << type_error.getFullMessageRecursive().c_str();
LOG(ERROR) << type_error.getMessage().c_str();
assert(false);
return createError(DatabaseError::KeyNotFound, "", std::move(type_error));
}
@ -325,9 +325,9 @@ Expected<int32_t, DatabaseError> RocksdbDatabase::getInt32(
"Fetching string as integer");
auto error =
createError(DatabaseError::KeyNotFound, "", std::move(type_error));
assert(false && error.getFullMessageRecursive().c_str());
LOG(ERROR) << error.getFullMessageRecursive();
debug_only::fail(error.getFullMessageRecursive().c_str());
assert(false && error.getMessage().c_str());
LOG(ERROR) << error.getMessage();
debug_only::fail(error.getMessage().c_str());
return std::move(error);
}
}

View File

@ -107,7 +107,7 @@ void EbpfTracepoint::forceUnload() {
auto const exp = unload();
if (exp.isError()) {
LOG(ERROR) << "Could not unload perf tracepoint "
<< boost::io::quoted(exp.getError().getFullMessage());
<< boost::io::quoted(exp.getError().getMessage());
}
}

View File

@ -55,7 +55,7 @@ bool Killswitch::isNewCodeEnabled(const std::string& key) {
if (result) {
return *result;
} else {
VLOG(1) << result.getError().getFullMessageRecursive();
VLOG(1) << result.getError().getMessage();
return true;
}
}

View File

@ -76,7 +76,7 @@ Status KillswitchPlugin::call(const PluginRequest& request,
response.push_back({{Killswitch::isEnabled_, std::to_string(*result)}});
return Status::success();
} else {
return Status::failure(result.getError().getFullMessageRecursive());
return Status::failure(result.getError().getMessage());
}
}
return Status(1, "Could not find appropriate action mapping");

View File

@ -59,7 +59,7 @@ Status KillswitchRefreshablePlugin::call(const PluginRequest& request,
if (result) {
return Status::success();
} else {
return Status::failure(result.getError().getFullMessageRecursive());
return Status::failure(result.getError().getMessage());
}
} else {
return KillswitchPlugin::call(request, response);

View File

@ -51,7 +51,7 @@ KillswitchFilesystem::refresh() {
return Success();
} else {
return createError(KillswitchRefreshablePlugin::RefreshError::ParsingError,
result.getError().getFullMessageRecursive());
result.getError().getMessage());
}
}

View File

@ -97,7 +97,7 @@ TLSKillswitchPlugin::refresh() {
return Success();
} else {
return createError(KillswitchRefreshablePlugin::RefreshError::ParsingError,
result.getError().getFullMessageRecursive());
result.getError().getMessage());
}
}
} // namespace osquery

View File

@ -43,8 +43,7 @@ int getUidFromSid(PSID sid) {
if (uid_exp.isError()) {
LocalFree(sidString);
VLOG(1) << "failed to parse PSID "
<< uid_exp.getError().getFullMessageRecursive();
VLOG(1) << "failed to parse PSID " << uid_exp.getError().getMessage();
return uid_default;
}

View File

@ -148,12 +148,12 @@ CodeProfiler::~CodeProfiler() {
auto rusage_start = code_profiler_data_->takeRusageData();
if (!rusage_start) {
LOG(ERROR) << "rusage_start error: "
<< rusage_start.getError().getFullMessageRecursive();
<< rusage_start.getError().getMessage();
} else {
auto rusage_end = code_profiler_data_end.takeRusageData();
if (!rusage_end) {
LOG(ERROR) << "rusage_end error: "
<< rusage_end.getError().getFullMessageRecursive();
<< rusage_end.getError().getMessage();
} else {
recordRusageStatDifference(names_, *rusage_start, *rusage_end);
}

View File

@ -21,10 +21,8 @@ namespace osquery {
class ErrorBase {
public:
virtual std::string getShortMessage() const = 0;
virtual std::string getFullMessage() const = 0;
virtual std::string getShortMessageRecursive() const = 0;
virtual std::string getFullMessageRecursive() const = 0;
virtual std::string getNonRecursiveMessage() const = 0;
virtual std::string getMessage() const = 0;
virtual ~ErrorBase() = default;
ErrorBase() = default;
ErrorBase(const ErrorBase& other) = default;
@ -66,30 +64,18 @@ class Error final : public ErrorBase {
return std::move(underlyingError_);
}
std::string getShortMessage() const override {
return to<std::string>(errorCode_);
}
std::string getFullMessage() const override {
std::string full_message = getShortMessage();
std::string getNonRecursiveMessage() const override {
std::string full_message = to<std::string>(errorCode_);
if (message_.size() > 0) {
full_message += " (" + message_ + ")";
}
return full_message;
}
std::string getShortMessageRecursive() const override {
std::string full_message = getShortMessage();
std::string getMessage() const override {
std::string full_message = getNonRecursiveMessage();
if (underlyingError_) {
full_message += " <- " + underlyingError_->getShortMessageRecursive();
}
return full_message;
}
std::string getFullMessageRecursive() const override {
std::string full_message = getFullMessage();
if (underlyingError_) {
full_message += " <- " + underlyingError_->getFullMessageRecursive();
full_message += " <- " + underlyingError_->getMessage();
}
return full_message;
}
@ -130,7 +116,7 @@ inline bool operator==(const ErrorBase& lhs, const T rhs) {
}
inline std::ostream& operator<<(std::ostream& out, const ErrorBase& error) {
out << error.getFullMessageRecursive();
out << error.getMessage();
return out;
}

View File

@ -24,10 +24,10 @@ GTEST_TEST(ErrorTest, initialization) {
EXPECT_FALSE(error.hasUnderlyingError());
EXPECT_TRUE(error == TestError::SomeError);
auto shortMsg = error.getShortMessageRecursive();
auto shortMsg = error.getNonRecursiveMessage();
EXPECT_NE(std::string::npos, shortMsg.find("TestError[1]"));
auto fullMsg = error.getFullMessageRecursive();
auto fullMsg = error.getMessage();
EXPECT_NE(std::string::npos, fullMsg.find("TestError[1]"));
EXPECT_NE(std::string::npos, fullMsg.find("TestMessage"));
}
@ -39,11 +39,11 @@ GTEST_TEST(ErrorTest, recursive) {
TestError::AnotherError, "TestMessage", std::move(orignalError));
EXPECT_TRUE(error.hasUnderlyingError());
auto shortMsg = error.getShortMessageRecursive();
EXPECT_NE(std::string::npos, shortMsg.find("TestError[1]"));
auto shortMsg = error.getNonRecursiveMessage();
EXPECT_EQ(std::string::npos, shortMsg.find("TestError[1]"));
EXPECT_NE(std::string::npos, shortMsg.find("TestError[2]"));
auto fullMsg = error.getFullMessageRecursive();
auto fullMsg = error.getMessage();
EXPECT_NE(std::string::npos, fullMsg.find("TestError[1]"));
EXPECT_NE(std::string::npos, fullMsg.find("SuperTestMessage"));
EXPECT_NE(std::string::npos, fullMsg.find("TestError[2]"));
@ -61,7 +61,7 @@ GTEST_TEST(ErrorTest, createErrorSimple) {
EXPECT_EQ(TestError::AnotherError, err.getErrorCode());
EXPECT_FALSE(err.hasUnderlyingError());
auto shortMsg = err.getFullMessageRecursive();
auto shortMsg = err.getMessage();
EXPECT_PRED2(stringContains, shortMsg, "TestError");
EXPECT_PRED2(stringContains, shortMsg, msg);
}
@ -73,14 +73,14 @@ GTEST_TEST(ErrorTest, createErrorFromOtherError) {
EXPECT_EQ(TestError::SomeError, firstErr.getErrorCode());
EXPECT_FALSE(firstErr.hasUnderlyingError());
EXPECT_PRED2(stringContains, firstErr.getFullMessageRecursive(), firstMsg);
EXPECT_PRED2(stringContains, firstErr.getMessage(), firstMsg);
const auto secondMsg = std::string{"what's wrong with the first message?!"};
auto secondErr = osquery::createError(
TestError::AnotherError, secondMsg, std::move(firstErr));
EXPECT_EQ(TestError::AnotherError, secondErr.getErrorCode());
EXPECT_TRUE(secondErr.hasUnderlyingError());
auto secondShortMsg = secondErr.getFullMessageRecursive();
auto secondShortMsg = secondErr.getMessage();
EXPECT_PRED2(stringContains, secondShortMsg, "TestError");
EXPECT_PRED2(stringContains, secondShortMsg, firstMsg);
EXPECT_PRED2(stringContains, secondShortMsg, secondMsg);
@ -96,7 +96,7 @@ GTEST_TEST(ErrorTest, createErrorAndStreamToIt) {
<< "-La"
<< "-Si La" << boost::io::quoted(a4) << ' ' << 440 << " Hz";
EXPECT_EQ(TestError::MusicError, err.getErrorCode());
auto fullMsg = err.getFullMessageRecursive();
auto fullMsg = err.getMessage();
EXPECT_PRED2(
stringContains, fullMsg, "Do-Re-Mi-Fa-Sol-La-Si La\"A4\" 440 Hz");
}

View File

@ -52,7 +52,7 @@ GTEST_TEST(ExpectedTest, failure_error_str_contructor_initialization) {
EXPECT_FALSE(expected);
EXPECT_TRUE(expected.isError());
EXPECT_EQ(expected.getErrorCode(), TestError::Some);
auto fullMsg = expected.getError().getFullMessage();
auto fullMsg = expected.getError().getMessage();
EXPECT_PRED2(stringContains, fullMsg, msg);
}
@ -147,7 +147,7 @@ GTEST_TEST(ExpectedTest, nested_errors_example) {
ASSERT_TRUE(ret.isError());
EXPECT_EQ(ret.getErrorCode(), TestError::Runtime);
ASSERT_TRUE(ret.getError().hasUnderlyingError());
EXPECT_PRED2(stringContains, ret.getError().getFullMessage(), msg);
EXPECT_PRED2(stringContains, ret.getError().getMessage(), msg);
}
GTEST_TEST(ExpectedTest, error_handling_example) {

View File

@ -55,8 +55,7 @@ class Status {
*/
Status(int c, std::string m) : code_(c), message_(std::move(m)) {}
Status(const ErrorBase& error)
: code_(1), message_(error.getFullMessageRecursive()) {}
Status(const ErrorBase& error) : code_(1), message_(error.getMessage()) {}
public:
/**
@ -165,9 +164,8 @@ template <typename ToType, typename ValueType, typename ErrorCodeEnumType>
inline
typename std::enable_if<std::is_same<ToType, Status>::value, Status>::type
to(const Expected<ValueType, ErrorCodeEnumType>& expected) {
return expected
? Status::success()
: Status::failure(expected.getError().getFullMessageRecursive());
return expected ? Status::success()
: Status::failure(expected.getError().getMessage());
}
} // namespace osquery

View File

@ -143,7 +143,7 @@ void PerfOutput<MessageType>::forceUnload() {
auto const exp = unload();
if (exp.isError()) {
LOG(ERROR) << "Could not unload perf event output point "
<< boost::io::quoted(exp.getError().getFullMessage());
<< boost::io::quoted(exp.getError().getMessage());
}
}

View File

@ -17,7 +17,7 @@ class EbpfTests : public testing::Test {};
TEST_F(EbpfTests, isSupportedBySystem) {
auto const exp = ebpf::isSupportedBySystem();
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
}
TEST_F(EbpfTests, sysEbpf_null_attr) {
@ -29,7 +29,7 @@ TEST_F(EbpfTests, sysEbpf_null_attr) {
TEST_F(EbpfTests, sysEbpf_create_map) {
auto const is_supported_exp = ebpf::isSupportedBySystem();
EXPECT_TRUE(is_supported_exp.isValue())
<< is_supported_exp.getError().getFullMessageRecursive();
<< is_supported_exp.getError().getMessage();
if (is_supported_exp.get()) {
union bpf_attr attr;
memset(&attr, 0, sizeof(union bpf_attr));

View File

@ -26,8 +26,7 @@ TEST_F(EbpfMapTests, int_key_int_value) {
}
auto const size = std::size_t{12};
auto map_exp = ebpf::createMap<int, int, BPF_MAP_TYPE_HASH>(size);
ASSERT_TRUE(map_exp.isValue())
<< map_exp.getError().getFullMessageRecursive();
ASSERT_TRUE(map_exp.isValue()) << map_exp.getError().getMessage();
auto map = map_exp.take();
ASSERT_EQ(map.size(), size);
{
@ -42,27 +41,27 @@ TEST_F(EbpfMapTests, int_key_int_value) {
}
{
auto exp = map.updateElement(5, 53);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
}
{
auto exp = map.lookupElement(5);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
ASSERT_EQ(exp.get(), 53);
}
{
// key could be greater a size, because it is a hash map
auto exp = map.updateElement(207, 8042);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
}
{
auto exp = map.lookupElement(207);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
ASSERT_EQ(exp.get(), 8042);
}
{
// let's try to delete some existing key
auto exp = map.deleteElement(207);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
}
{
auto exp = map.lookupElement(207);
@ -92,11 +91,11 @@ TEST_F(EbpfMapTests, int_key_struct_value) {
.right = 2781,
};
auto exp = map.updateElement(72, v);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
}
{
auto exp = map.lookupElement(72);
ASSERT_TRUE(exp.isValue()) << exp.getError().getFullMessageRecursive();
ASSERT_TRUE(exp.isValue()) << exp.getError().getMessage();
EXPECT_EQ(exp.get().left, -9287);
EXPECT_EQ(exp.get().right, 2781);
}

View File

@ -141,7 +141,7 @@ TEST_F(PerfOutputTests,
buf.size() + sizeof(WrappedMessage),
buf.size(),
dst);
ASSERT_FALSE(status.isError()) << status.getError().getFullMessageRecursive();
ASSERT_FALSE(status.isError()) << status.getError().getMessage();
ASSERT_EQ(dst.size(), test_size);
for (std::size_t i = 0; i < test_size; ++i) {
EXPECT_EQ(dst[i].c_, 'y') << i;
@ -194,7 +194,7 @@ TEST_F(PerfOutputTests,
auto status =
ebpf::impl::consumeWrappedMessagesFromCircularBuffer<WrappedMessage>(
&buf[0], tail, head, buf.size(), dst);
ASSERT_FALSE(status.isError()) << status.getError().getFullMessageRecursive();
ASSERT_FALSE(status.isError()) << status.getError().getMessage();
ASSERT_EQ(dst.size(), test_size);
for (std::size_t i = 0; i < test_size; ++i) {
EXPECT_EQ(dst[i].c_, 't');

View File

@ -27,8 +27,7 @@ bool const kDebug =
TEST_F(EbpfProgramTests, empty_debug) {
auto const ebpf_exp = ebpf::isSupportedBySystem();
EXPECT_TRUE(ebpf_exp.isValue())
<< ebpf_exp.getError().getFullMessageRecursive();
EXPECT_TRUE(ebpf_exp.isValue()) << ebpf_exp.getError().getMessage();
if (!ebpf_exp.get()) {
LOG(WARNING) << "This system does not support eBPF of required vesion, "
"test will be skipped";

View File

@ -30,7 +30,7 @@ NativeEvent::~NativeEvent() {
auto const exp = enable(false);
if (exp.isError()) {
LOG(WARNING) << "Disabling system event " << event_path_
<< " failed: " << exp.getError().getFullMessage();
<< " failed: " << exp.getError().getMessage();
}
}