shill: Propagate Cellular Activate call errors to the DBus caller.
BUG=chromium-os:19547
TEST=unit tests
Change-Id: I2a0f889f0863c299c3f3ac006a6df5a608407881
Reviewed-on: http://gerrit.chromium.org/gerrit/6616
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Darin Petkov <petkov@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 20cf129..0a2ea86 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -632,8 +632,22 @@
}
}
-void Cellular::Activate(const string &carrier) {
- CHECK_EQ(kTypeCDMA, type_);
+void Cellular::Activate(const string &carrier, Error *error) {
+ if (type_ != kTypeCDMA) {
+ const string kMessage = "Unable to activate non-CDMA modem.";
+ LOG(ERROR) << kMessage;
+ CHECK(error);
+ error->Populate(Error::kInvalidArguments, kMessage);
+ return;
+ }
+ if (state_ != kStateEnabled &&
+ state_ != kStateRegistered) {
+ const string kMessage = "Unable to activate in " + GetStateString(state_);
+ LOG(ERROR) << kMessage;
+ CHECK(error);
+ error->Populate(Error::kInvalidArguments, kMessage);
+ return;
+ }
// Defer connect because we may be in a dbus-c++ callback.
dispatcher_->PostTask(
task_factory_.NewRunnableMethod(&Cellular::ActivateTask, carrier));
@@ -641,7 +655,8 @@
void Cellular::ActivateTask(const string &carrier) {
VLOG(2) << __func__ << "(" << carrier << ")";
- if (state_ != kStateEnabled && state_ != kStateRegistered) {
+ if (state_ != kStateEnabled &&
+ state_ != kStateRegistered) {
LOG(ERROR) << "Unable to activate in " << GetStateString(state_);
return;
}
diff --git a/cellular.h b/cellular.h
index ee226e2..4409a6a 100644
--- a/cellular.h
+++ b/cellular.h
@@ -155,8 +155,9 @@
// failure, leaves it unchanged otherwise.
void Connect(Error *error);
- // Asynchronously activates the modem.
- void Activate(const std::string &carrier);
+ // Asynchronously activates the modem. Populates |error| on failure, leaves it
+ // unchanged otherwise.
+ void Activate(const std::string &carrier, Error *error);
void set_modem_state(ModemState state) { modem_state_ = state; }
ModemState modem_state() const { return modem_state_; }
diff --git a/cellular_service.cc b/cellular_service.cc
index 471ad4c..a6c1140 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -51,8 +51,9 @@
void CellularService::Disconnect() { }
-void CellularService::ActivateCellularModem(const string &carrier) {
- cellular_->Activate(carrier);
+void CellularService::ActivateCellularModem(const string &carrier,
+ Error *error) {
+ cellular_->Activate(carrier, error);
}
string CellularService::GetStorageIdentifier(const string &mac) {
diff --git a/cellular_service.h b/cellular_service.h
index 895a146..67b6a92 100644
--- a/cellular_service.h
+++ b/cellular_service.h
@@ -17,6 +17,7 @@
namespace shill {
class ControlInterface;
+class Error;
class EventDispatcher;
class Manager;
@@ -31,7 +32,7 @@
// Inherited from Service.
virtual void Connect(Error *error);
virtual void Disconnect();
- virtual void ActivateCellularModem(const std::string &carrier);
+ virtual void ActivateCellularModem(const std::string &carrier, Error *error);
// cellular_<MAC>_<Service_Operator_Name>
std::string GetStorageIdentifier(const std::string &mac);
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index ac38b3a..f9914e5 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -546,12 +546,14 @@
}
TEST_F(CellularTest, Activate) {
+ Error error;
device_->type_ = Cellular::kTypeCDMA;
+ device_->state_ = Cellular::kStateEnabled;
EXPECT_CALL(*cdma_proxy_, Activate(kTestCarrier))
.WillOnce(Return(MM_MODEM_CDMA_ACTIVATION_ERROR_NO_ERROR));
- device_->Activate(kTestCarrier);
+ device_->Activate(kTestCarrier, &error);
+ EXPECT_TRUE(error.IsSuccess());
device_->cdma_proxy_.reset(cdma_proxy_.release());
- device_->state_ = Cellular::kStateEnabled;
device_->CreateService();
dispatcher_.DispatchPendingEvents();
EXPECT_EQ(MM_MODEM_CDMA_ACTIVATION_STATE_ACTIVATING,
@@ -562,12 +564,23 @@
}
TEST_F(CellularTest, ActivateError) {
+ Error error;
+ device_->type_ = Cellular::kTypeGSM;
+ device_->Activate(kTestCarrier, &error);
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+
+ error.Populate(Error::kSuccess);
device_->type_ = Cellular::kTypeCDMA;
+ device_->Activate(kTestCarrier, &error);
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+
+ error.Populate(Error::kSuccess);
+ device_->state_ = Cellular::kStateRegistered;
EXPECT_CALL(*cdma_proxy_, Activate(kTestCarrier))
.WillOnce(Return(MM_MODEM_CDMA_ACTIVATION_ERROR_NO_SIGNAL));
- device_->Activate(kTestCarrier);
+ device_->Activate(kTestCarrier, &error);
+ EXPECT_TRUE(error.IsSuccess());
device_->cdma_proxy_.reset(cdma_proxy_.release());
- device_->state_ = Cellular::kStateEnabled;
device_->CreateService();
dispatcher_.DispatchPendingEvents();
EXPECT_EQ(MM_MODEM_CDMA_ACTIVATION_STATE_NOT_ACTIVATED,
diff --git a/error.cc b/error.cc
index c9799ba..030e598 100644
--- a/error.cc
+++ b/error.cc
@@ -49,7 +49,7 @@
Populate(type);
}
-Error::Error(Type type, const std::string &message) {
+Error::Error(Type type, const string &message) {
Populate(type, message);
}
@@ -59,7 +59,7 @@
Populate(type, GetDefaultMessage(type));
}
-void Error::Populate(Type type, const std::string &message) {
+void Error::Populate(Type type, const string &message) {
CHECK(type < kNumErrors) << "Error type out of range: " << type;
type_ = type;
message_ = message;
diff --git a/error_unittest.cc b/error_unittest.cc
index 0a81854..aa84dbc 100644
--- a/error_unittest.cc
+++ b/error_unittest.cc
@@ -52,12 +52,10 @@
TEST_F(ErrorTest, ToDBusError) {
DBus::Error dbus_error;
ASSERT_FALSE(dbus_error.is_set());
- Error e;
- e.ToDBusError(&dbus_error);
+ Error().ToDBusError(&dbus_error);
ASSERT_FALSE(dbus_error.is_set());
static const char kMessage[] = "Test error message";
- e.Populate(Error::kPermissionDenied, kMessage);
- e.ToDBusError(&dbus_error);
+ Error(Error::kPermissionDenied, kMessage).ToDBusError(&dbus_error);
ASSERT_TRUE(dbus_error.is_set());
EXPECT_EQ(Error::GetName(Error::kPermissionDenied), dbus_error.name());
EXPECT_STREQ(kMessage, dbus_error.message());
diff --git a/service.cc b/service.cc
index 1a3c6f7..075bc13 100644
--- a/service.cc
+++ b/service.cc
@@ -151,8 +151,11 @@
Service::~Service() {}
-void Service::ActivateCellularModem(const std::string &carrier) {
- NOTREACHED() << "Attempt to activate a non-cellular service?";
+void Service::ActivateCellularModem(const std::string &carrier, Error *error) {
+ const string kMessage = "Service doesn't support cellular modem activation.";
+ LOG(ERROR) << kMessage;
+ CHECK(error);
+ error->Populate(Error::kInvalidArguments, kMessage);
}
bool Service::IsActive() {
diff --git a/service.h b/service.h
index 3edf054..b4e60a6 100644
--- a/service.h
+++ b/service.h
@@ -90,8 +90,8 @@
virtual void Connect(Error *error) = 0;
virtual void Disconnect() = 0;
- // The default implementation asserts.
- virtual void ActivateCellularModem(const std::string &carrier);
+ // The default implementation sets |error| to kInvalidArguments.
+ virtual void ActivateCellularModem(const std::string &carrier, Error *error);
virtual bool IsActive();
diff --git a/service_dbus_adaptor.cc b/service_dbus_adaptor.cc
index 7aa801b..08b6a89 100644
--- a/service_dbus_adaptor.cc
+++ b/service_dbus_adaptor.cc
@@ -89,7 +89,9 @@
void ServiceDBusAdaptor::ActivateCellularModem(const string &carrier,
::DBus::Error &error) {
- service_->ActivateCellularModem(carrier);
+ Error e;
+ service_->ActivateCellularModem(carrier, &e);
+ e.ToDBusError(&error);
}
} // namespace shill
diff --git a/service_unittest.cc b/service_unittest.cc
index f549750..f90f162 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -250,4 +250,10 @@
EXPECT_EQ(Service::kFailureOutOfRange, service_->failure());
}
+TEST_F(ServiceTest, ActivateCellularModem) {
+ Error error;
+ service_->ActivateCellularModem("Carrier", &error);
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+}
+
} // namespace shill