shill: Disable autoconnect on activating cellular services
Don't allow services undergoing activation to be connected by the
manager. Implement this by asking the capability (through the device)
whether it is currently in the process of activation. This works around
a huge mess where Chrome forces autoconnect off, and then has retrying
reconnect logic after activation.
BUG=chromium-os:36301
TEST=Activations work as before. The real magic will happen when we
stop toggling the autoconnect bit from the mobile activator in Chrome.
Change-Id: I92cb6780712d341605f51e4a98041bb3642ab6fe
Reviewed-on: https://gerrit.chromium.org/gerrit/38314
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Tested-by: Christopher Wiley <wiley@chromium.org>
Commit-Ready: Christopher Wiley <wiley@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index ef4b15a..9b3e04b 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -629,6 +629,10 @@
}
}
+bool Cellular::IsActivating() const {
+ return capability_->IsActivating();
+}
+
void Cellular::SetAllowRoaming(const bool &value, Error */*error*/) {
SLOG(Cellular, 2) << __func__
<< "(" << allow_roaming_ << "->" << value << ")";
diff --git a/cellular.h b/cellular.h
index bd65f14..82d6d0a 100644
--- a/cellular.h
+++ b/cellular.h
@@ -215,6 +215,8 @@
// accessor to read the allow roaming property
bool allow_roaming_property() const { return allow_roaming_; }
+ // Is the underlying device in the process of activating?
+ bool IsActivating() const;
private:
friend class CellularTest;
diff --git a/cellular_capability.cc b/cellular_capability.cc
index 13e076d..b0e5b00 100644
--- a/cellular_capability.cc
+++ b/cellular_capability.cc
@@ -92,4 +92,8 @@
OnUnsupportedOperation(__func__, error);
}
+bool CellularCapability::IsActivating() const {
+ return false;
+}
+
} // namespace shill
diff --git a/cellular_capability.h b/cellular_capability.h
index 6772608..bc369ac 100644
--- a/cellular_capability.h
+++ b/cellular_capability.h
@@ -181,6 +181,8 @@
// property.
virtual bool AllowRoaming() = 0;
+ virtual bool IsActivating() const;
+
protected:
// Releases all proxies held by the object. This is most useful
// during unit tests.
diff --git a/cellular_capability_cdma.cc b/cellular_capability_cdma.cc
index 44fc8cc..2505fc9 100644
--- a/cellular_capability_cdma.cc
+++ b/cellular_capability_cdma.cc
@@ -32,6 +32,7 @@
ProxyFactory *proxy_factory)
: CellularCapabilityClassic(cellular, proxy_factory),
weak_ptr_factory_(this),
+ activation_starting_(false),
activation_state_(MM_MODEM_CDMA_ACTIVATION_STATE_NOT_ACTIVATED),
registration_state_evdo_(MM_MODEM_CDMA_REGISTRATION_STATE_UNKNOWN),
registration_state_1x_(MM_MODEM_CDMA_REGISTRATION_STATE_UNKNOWN),
@@ -135,6 +136,8 @@
Error *error,
const ResultCallback &callback) {
SLOG(Cellular, 2) << __func__ << "(" << carrier << ")";
+ // We're going to trigger something which leads to an activation.
+ activation_starting_ = true;
if (cellular()->state() == Cellular::kStateEnabled ||
cellular()->state() == Cellular::kStateRegistered) {
ActivationResultCallback cb =
@@ -154,6 +157,7 @@
Error::PopulateAndLog(error, Error::kInvalidArguments,
"Unable to activate in " +
Cellular::GetStateString(cellular()->state()));
+ activation_starting_ = false;
}
}
@@ -218,6 +222,11 @@
callback.Run(Error());
}
+bool CellularCapabilityCDMA::IsActivating() const {
+ return activation_starting_ ||
+ activation_state_ == MM_MODEM_CDMA_ACTIVATION_STATE_ACTIVATING;
+}
+
bool CellularCapabilityCDMA::IsRegistered() {
return registration_state_evdo_ != MM_MODEM_CDMA_REGISTRATION_STATE_UNKNOWN ||
registration_state_1x_ != MM_MODEM_CDMA_REGISTRATION_STATE_UNKNOWN;
@@ -290,6 +299,7 @@
void CellularCapabilityCDMA::OnActivateReply(
const ResultCallback &callback, uint32 status, const Error &error) {
+ activation_starting_ = false;
if (error.IsSuccess()) {
if (status == MM_MODEM_CDMA_ACTIVATION_ERROR_NO_ERROR) {
activation_state_ = MM_MODEM_CDMA_ACTIVATION_STATE_ACTIVATING;
@@ -314,6 +324,7 @@
LOG(WARNING) << "Tried to disconnect before activating cellular service"
<< " and failed.";
HandleNewActivationState(MM_MODEM_CDMA_ACTIVATION_ERROR_UNKNOWN);
+ activation_starting_ = false;
}
}
diff --git a/cellular_capability_cdma.h b/cellular_capability_cdma.h
index 83c6eac..07e15c4 100644
--- a/cellular_capability_cdma.h
+++ b/cellular_capability_cdma.h
@@ -30,6 +30,7 @@
virtual void SetupConnectProperties(DBusPropertiesMap *properties);
virtual void Activate(const std::string &carrier, Error *error,
const ResultCallback &callback);
+ virtual bool IsActivating() const;
virtual bool IsRegistered();
virtual void SetUnregistered(bool searching);
virtual std::string CreateFriendlyServiceName();
@@ -61,6 +62,7 @@
FRIEND_TEST(CellularCapabilityCDMATest, CreateFriendlyServiceName);
FRIEND_TEST(CellularCapabilityCDMATest, GetActivationStateString);
FRIEND_TEST(CellularCapabilityCDMATest, GetActivationErrorString);
+ FRIEND_TEST(CellularServiceTest, IsAutoConnectable);
FRIEND_TEST(CellularTest, CreateService);
static const char kPhoneNumber[];
@@ -97,6 +99,7 @@
scoped_ptr<ModemCDMAProxyInterface> proxy_;
base::WeakPtrFactory<CellularCapabilityCDMA> weak_ptr_factory_;
+ bool activation_starting_;
uint32 activation_state_;
uint32 registration_state_evdo_;
uint32 registration_state_1x_;
diff --git a/cellular_capability_cdma_unittest.cc b/cellular_capability_cdma_unittest.cc
index d1c5a31..2df1940 100644
--- a/cellular_capability_cdma_unittest.cc
+++ b/cellular_capability_cdma_unittest.cc
@@ -106,6 +106,10 @@
static const char kTestCarrier[];
static const unsigned int kStrength;
+ bool IsActivationStarting() const {
+ return capability_->activation_starting_;
+ }
+
void SetRegistrationStateEVDO(uint32 state) {
capability_->registration_state_evdo_ = state;
}
@@ -194,6 +198,30 @@
EXPECT_EQ(flimflam::kActivationStateActivating,
cellular_->service()->activation_state());
EXPECT_EQ("", cellular_->service()->error());
+ EXPECT_FALSE(IsActivationStarting());
+ EXPECT_TRUE(capability_->IsActivating());
+}
+
+TEST_F(CellularCapabilityCDMATest, MidActivateWhileConnected) {
+ SetDeviceState(Cellular::kStateConnected);
+ {
+ InSequence dummy;
+
+ EXPECT_CALL(*cellular_, DisconnectWithCallback(_,_))
+ .WillOnce(Invoke(cellular_.get(),
+ &MockCellular::RealDisconnectWithCallback));
+ EXPECT_CALL(*classic_proxy_,
+ Disconnect(_, _, CellularCapability::kTimeoutDisconnect));
+ }
+ SetProxy();
+ SetService();
+ Error error;
+ capability_->Activate(kTestCarrier, &error,
+ Bind(&CellularCapabilityCDMATest::TestCallback, Unretained(this)));
+ EXPECT_EQ(MM_MODEM_CDMA_ACTIVATION_STATE_NOT_ACTIVATED,
+ capability_->activation_state());
+ EXPECT_TRUE(IsActivationStarting());
+ EXPECT_TRUE(capability_->IsActivating());
}
TEST_F(CellularCapabilityCDMATest, ActivateWhileConnectedButFail) {
@@ -223,6 +251,8 @@
cellular_->service()->activation_state());
EXPECT_EQ(flimflam::kErrorActivationFailed,
cellular_->service()->error());
+ EXPECT_FALSE(IsActivationStarting());
+ EXPECT_FALSE(capability_->IsActivating());
}
TEST_F(CellularCapabilityCDMATest, ActivateError) {
diff --git a/cellular_service.cc b/cellular_service.cc
index 3a32ed6..8912f9a 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -18,6 +18,7 @@
namespace shill {
+const char CellularService::kAutoConnActivating[] = "activating";
const char CellularService::kAutoConnDeviceDisabled[] = "device disabled";
const char CellularService::kStorageAPN[] = "Cellular.APN";
const char CellularService::kStorageLastGoodAPN[] = "Cellular.LastGoodAPN";
@@ -122,6 +123,10 @@
*reason = kAutoConnDeviceDisabled;
return false;
}
+ if (cellular_->IsActivating()) {
+ *reason = kAutoConnActivating;
+ return false;
+ }
return Service::IsAutoConnectable(reason);
}
diff --git a/cellular_service.h b/cellular_service.h
index 63c9b2f..d8153f6 100644
--- a/cellular_service.h
+++ b/cellular_service.h
@@ -114,6 +114,7 @@
FRIEND_TEST(CellularServiceTest, LastGoodApn);
FRIEND_TEST(CellularServiceTest, IsAutoConnectable);
+ static const char kAutoConnActivating[];
static const char kAutoConnDeviceDisabled[];
void HelpRegisterDerivedStringmap(
diff --git a/cellular_service_unittest.cc b/cellular_service_unittest.cc
index e01556a..f699931 100644
--- a/cellular_service_unittest.cc
+++ b/cellular_service_unittest.cc
@@ -6,6 +6,7 @@
#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
+#include <mm/mm-modem.h>
#include "shill/cellular_capability.h"
#include "shill/cellular_capability_cdma.h"
@@ -319,6 +320,27 @@
device_->running_ = true;
+ // If we're waiting on a disconnect before an activation, don't auto-connect.
+ GetCapabilityCDMA()->activation_starting_ = true;
+ EXPECT_FALSE(service_->IsAutoConnectable(&reason));
+
+ // If we're waiting on an activation, also don't auto-connect.
+ GetCapabilityCDMA()->activation_starting_ = false;
+ GetCapabilityCDMA()->activation_state_ =
+ MM_MODEM_CDMA_ACTIVATION_STATE_ACTIVATING;
+ EXPECT_FALSE(service_->IsAutoConnectable(&reason));
+
+ // But other activation states are fine.
+ GetCapabilityCDMA()->activation_state_ =
+ MM_MODEM_CDMA_ACTIVATION_STATE_ACTIVATED;
+ EXPECT_TRUE(service_->IsAutoConnectable(&reason));
+ GetCapabilityCDMA()->activation_state_ =
+ MM_MODEM_CDMA_ACTIVATION_STATE_NOT_ACTIVATED;
+ EXPECT_TRUE(service_->IsAutoConnectable(&reason));
+ GetCapabilityCDMA()->activation_state_ =
+ MM_MODEM_CDMA_ACTIVATION_STATE_PARTIALLY_ACTIVATED;
+ EXPECT_TRUE(service_->IsAutoConnectable(&reason));
+
// The following test cases are copied from ServiceTest.IsAutoConnectable
service_->set_connectable(true);