shill: Error: Use error strings from service_constants
Move the DBus error message strings out of shill and use the new
definitions in service_constants.h. Nobody used the unadorned
"name" field in Error::Info anyway.
CQ-DEPEND=CL:184385
BUG=chromium:256889
TEST=Unit tests
Change-Id: Ie2a35a30064d2ce2e599d96a325e64cc3a552e57
Reviewed-on: https://chromium-review.googlesource.com/184354
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Reviewed-by: Arman Uguray <armansito@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
diff --git a/error.cc b/error.cc
index ec289fe..87c8a53 100644
--- a/error.cc
+++ b/error.cc
@@ -4,7 +4,7 @@
#include "shill/error.h"
-#include <base/stringprintf.h>
+#include <chromeos/dbus/service_constants.h>
#include <dbus-c++/error.h>
#include "shill/dbus_adaptor.h"
@@ -16,33 +16,33 @@
// static
const Error::Info Error::kInfos[kNumErrors] = {
- { "Success", "Success (no error)" },
- { "Failure", "Operation failed (no other information)" },
- { "AlreadyConnected", "Already connected" },
- { "AlreadyExists", "Already exists" },
- { "OperationInitiated", "Operation initiated" },
- { "InProgress", "In progress" },
- { "InternalError", "Internal error" },
- { "InvalidArguments", "Invalid arguments" },
- { "InvalidNetworkName", "Invalid network name" },
- { "InvalidPassphrase", "Invalid passphrase" },
- { "InvalidProperty", "Invalid property" },
- { "NoCarrier", "No carrier" },
- { "NotConnected", "Not connected" },
- { "NotFound", "Not found" },
- { "NotImplemented", "Not implemented" },
- { "NotOnHomeNetwork", "Not on home network" },
- { "NotRegistered", "Not registered" },
- { "NotSupported", "Not supported" },
- { "OperationAborted", "Operation aborted" },
- { "OperationTimeout", "Operation timeout" },
- { "PassphraseRequired", "Passphrase required" },
- { "IncorrectPin", "Incorrect PIN" },
- { "PinRequired", "SIM PIN is required"},
- { "PinBlocked", "SIM PIN is blocked"},
- { "InvalidApn", "Invalid APN" },
- { "WrongState", "Wrong state" },
- { "PermissionDenied", "Permission denied" }
+ { kErrorResultSuccess, "Success (no error)" },
+ { kErrorResultFailure, "Operation failed (no other information)" },
+ { kErrorResultAlreadyConnected, "Already connected" },
+ { kErrorResultAlreadyExists, "Already exists" },
+ { kErrorResultIncorrectPin, "Incorrect PIN" },
+ { kErrorResultInProgress, "In progress" },
+ { kErrorResultInternalError, "Internal error" },
+ { kErrorResultInvalidApn, "Invalid APN" },
+ { kErrorResultInvalidArguments, "Invalid arguments" },
+ { kErrorResultInvalidNetworkName, "Invalid network name" },
+ { kErrorResultInvalidPassphrase, "Invalid passphrase" },
+ { kErrorResultInvalidProperty, "Invalid property" },
+ { kErrorResultNoCarrier, "No carrier" },
+ { kErrorResultNotConnected, "Not connected" },
+ { kErrorResultNotFound, "Not found" },
+ { kErrorResultNotImplemented, "Not implemented" },
+ { kErrorResultNotOnHomeNetwork, "Not on home network" },
+ { kErrorResultNotRegistered, "Not registered" },
+ { kErrorResultNotSupported, "Not supported" },
+ { kErrorResultOperationAborted, "Operation aborted" },
+ { kErrorResultOperationInitiated, "Operation initiated" },
+ { kErrorResultOperationTimeout, "Operation timeout" },
+ { kErrorResultPassphraseRequired, "Passphrase required" },
+ { kErrorResultPermissionDenied, "Permission denied" },
+ { kErrorResultPinBlocked, "SIM PIN is blocked"},
+ { kErrorResultPinRequired, "SIM PIN is required"},
+ { kErrorResultWrongState, "Wrong state" }
};
Error::Error() {
@@ -79,7 +79,7 @@
bool Error::ToDBusError(::DBus::Error *error) const {
if (IsFailure()) {
- error->set(GetName(type_).c_str(), message_.c_str());
+ error->set(GetDBusResult(type_).c_str(), message_.c_str());
return true;
} else {
return false;
@@ -87,9 +87,9 @@
}
// static
-string Error::GetName(Type type) {
+string Error::GetDBusResult(Type type) {
CHECK(type < kNumErrors) << "Error type out of range: " << type;
- return base::StringPrintf("%s.Error.%s", SHILL_INTERFACE, kInfos[type].name);
+ return kInfos[type].dbus_result;
}
// static
@@ -109,6 +109,6 @@
} // namespace shill
std::ostream &operator<<(std::ostream &stream, const shill::Error &error) {
- stream << error.GetName(error.type()) << ":" << error.message();
+ stream << error.GetDBusResult(error.type()) << ": " << error.message();
return stream;
}
diff --git a/error.h b/error.h
index e22b011..f76062c 100644
--- a/error.h
+++ b/error.h
@@ -22,9 +22,10 @@
kOperationFailed, // failure, otherwise unspecified
kAlreadyConnected,
kAlreadyExists,
- kOperationInitiated,
+ kIncorrectPin,
kInProgress,
kInternalError,
+ kInvalidApn,
kInvalidArguments,
kInvalidNetworkName,
kInvalidPassphrase,
@@ -37,14 +38,13 @@
kNotRegistered,
kNotSupported,
kOperationAborted,
+ kOperationInitiated,
kOperationTimeout,
kPassphraseRequired,
- kIncorrectPin,
- kPinRequired,
- kPinBlocked,
- kInvalidApn,
- kWrongState,
kPermissionDenied,
+ kPinBlocked,
+ kPinRequired,
+ kWrongState,
kNumErrors
};
@@ -71,7 +71,7 @@
bool IsFailure() const { return !IsSuccess() && !IsOngoing(); }
bool IsOngoing() const { return type_ == kOperationInitiated; }
- static std::string GetName(Type type);
+ static std::string GetDBusResult(Type type);
static std::string GetDefaultMessage(Type type);
// Log an error message. If |error| is non-NULL, also populate it.
@@ -80,7 +80,7 @@
private:
struct Info {
- const char *name; // Error type name.
+ const char *dbus_result; // Error type name.
const char *message; // Default Error type message.
};
diff --git a/error_unittest.cc b/error_unittest.cc
index 7736e38..ff784a9 100644
--- a/error_unittest.cc
+++ b/error_unittest.cc
@@ -4,6 +4,7 @@
#include "shill/error.h"
+#include <chromeos/dbus/service_constants.h>
#include <dbus-c++/error.h>
#include <gtest/gtest.h>
@@ -72,7 +73,7 @@
static const char kMessage[] = "Test error message";
Error(Error::kPermissionDenied, kMessage).ToDBusError(&dbus_error);
ASSERT_TRUE(dbus_error.is_set());
- EXPECT_EQ(Error::GetName(Error::kPermissionDenied), dbus_error.name());
+ EXPECT_STREQ(kErrorResultPermissionDenied, dbus_error.name());
EXPECT_STREQ(kMessage, dbus_error.message());
}
@@ -83,15 +84,59 @@
EXPECT_TRUE(Error(Error::kInvalidPassphrase).IsFailure());
}
-TEST_F(ErrorTest, GetName) {
- EXPECT_EQ(SHILL_INTERFACE ".Error.NotFound",
- Error::GetName(Error::kNotFound));
+TEST_F(ErrorTest, GetDBusResult) {
+ // Make sure the Error::Type enum matches up to the Error::Info array.
+ EXPECT_EQ(kErrorResultSuccess, Error::GetDBusResult(Error::kSuccess));
+ EXPECT_EQ(kErrorResultFailure, Error::GetDBusResult(Error::kOperationFailed));
+ EXPECT_EQ(kErrorResultAlreadyConnected,
+ Error::GetDBusResult(Error::kAlreadyConnected));
+ EXPECT_EQ(kErrorResultAlreadyExists,
+ Error::GetDBusResult(Error::kAlreadyExists));
+ EXPECT_EQ(kErrorResultIncorrectPin,
+ Error::GetDBusResult(Error::kIncorrectPin));
+ EXPECT_EQ(kErrorResultInProgress, Error::GetDBusResult(Error::kInProgress));
+ EXPECT_EQ(kErrorResultInternalError,
+ Error::GetDBusResult(Error::kInternalError));
+ EXPECT_EQ(kErrorResultInvalidApn, Error::GetDBusResult(Error::kInvalidApn));
+ EXPECT_EQ(kErrorResultInvalidArguments,
+ Error::GetDBusResult(Error::kInvalidArguments));
+ EXPECT_EQ(kErrorResultInvalidNetworkName,
+ Error::GetDBusResult(Error::kInvalidNetworkName));
+ EXPECT_EQ(kErrorResultInvalidPassphrase,
+ Error::GetDBusResult(Error::kInvalidPassphrase));
+ EXPECT_EQ(kErrorResultInvalidProperty,
+ Error::GetDBusResult(Error::kInvalidProperty));
+ EXPECT_EQ(kErrorResultNoCarrier, Error::GetDBusResult(Error::kNoCarrier));
+ EXPECT_EQ(kErrorResultNotConnected,
+ Error::GetDBusResult(Error::kNotConnected));
+ EXPECT_EQ(kErrorResultNotFound, Error::GetDBusResult(Error::kNotFound));
+ EXPECT_EQ(kErrorResultNotImplemented,
+ Error::GetDBusResult(Error::kNotImplemented));
+ EXPECT_EQ(kErrorResultNotOnHomeNetwork,
+ Error::GetDBusResult(Error::kNotOnHomeNetwork));
+ EXPECT_EQ(kErrorResultNotRegistered,
+ Error::GetDBusResult(Error::kNotRegistered));
+ EXPECT_EQ(kErrorResultNotSupported,
+ Error::GetDBusResult(Error::kNotSupported));
+ EXPECT_EQ(kErrorResultOperationAborted,
+ Error::GetDBusResult(Error::kOperationAborted));
+ EXPECT_EQ(kErrorResultOperationInitiated,
+ Error::GetDBusResult(Error::kOperationInitiated));
+ EXPECT_EQ(kErrorResultOperationTimeout,
+ Error::GetDBusResult(Error::kOperationTimeout));
+ EXPECT_EQ(kErrorResultPassphraseRequired,
+ Error::GetDBusResult(Error::kPassphraseRequired));
+ EXPECT_EQ(kErrorResultPermissionDenied,
+ Error::GetDBusResult(Error::kPermissionDenied));
+ EXPECT_EQ(kErrorResultPinBlocked, Error::GetDBusResult(Error::kPinBlocked));
+ EXPECT_EQ(kErrorResultPinRequired, Error::GetDBusResult(Error::kPinRequired));
+ EXPECT_EQ(kErrorResultWrongState, Error::GetDBusResult(Error::kWrongState));
}
TEST_F(ErrorTest, GetDefaultMessage) {
// Check the last error code to try to prevent off-by-one bugs when adding or
// removing error types.
- ASSERT_EQ(Error::kPermissionDenied, Error::kNumErrors - 1);
+ ASSERT_EQ(Error::kWrongState, Error::kNumErrors - 1);
EXPECT_EQ("Permission denied",
Error::GetDefaultMessage(Error::kPermissionDenied));
}
diff --git a/property_store_unittest.cc b/property_store_unittest.cc
index c281ec2..e43280d 100644
--- a/property_store_unittest.cc
+++ b/property_store_unittest.cc
@@ -10,6 +10,7 @@
#include <vector>
#include <base/basictypes.h>
+#include <chromeos/dbus/service_constants.h>
#include <dbus-c++/dbus.h>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
@@ -71,9 +72,9 @@
DBusAdaptor::Uint64ToVariant(0);
PropertyStoreTest::PropertyStoreTest()
- : internal_error_(Error::GetName(Error::kInternalError)),
- invalid_args_(Error::GetName(Error::kInvalidArguments)),
- invalid_prop_(Error::GetName(Error::kInvalidProperty)),
+ : internal_error_(kErrorResultInternalError),
+ invalid_args_(kErrorResultInvalidArguments),
+ invalid_prop_(kErrorResultInvalidProperty),
path_(dir_.CreateUniqueTempDir() ? dir_.path().value() : ""),
metrics_(dispatcher()),
manager_(control_interface(),
diff --git a/wifi_service.cc b/wifi_service.cc
index 2a55600..a543495 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -326,8 +326,7 @@
SetPassphrase(passphrase, &error);
if (!error.IsSuccess() &&
!(passphrase.empty() && error.type() == Error::kNotSupported)) {
- LOG(ERROR) << "Passphrase could not be set: "
- << Error::GetName(error.type());
+ LOG(ERROR) << "Passphrase could not be set: " << error;
}
}
diff --git a/wifi_service_unittest.cc b/wifi_service_unittest.cc
index 1be7c01..76242da 100644
--- a/wifi_service_unittest.cc
+++ b/wifi_service_unittest.cc
@@ -861,8 +861,8 @@
.WillOnce(DoAll(SetArgumentPointee<2>(string()), Return(true)));
ScopedMockLog log;
EXPECT_CALL(log, Log(logging::LOG_ERROR, _,
- EndsWith("Passphrase could not be set: "
- "org.chromium.flimflam.Error.NotSupported")))
+ HasSubstr("Passphrase could not be set: "
+ "org.chromium.flimflam.Error.NotSupported")))
.Times(1);
EXPECT_TRUE(service->Load(&mock_store));
Mock::VerifyAndClearExpectations(&log);