shill: don't auto-connect to a disconnected service
This addresses part of the problem with the 003AsciiKeyWEP40
failure. That test establishes a connection, disconnects,
reconfigures the (WEP) password on an AP, and then tries
to reconnect.
Before this patch, the test failed as follows:
1. shill immediately tries to reconnect after the disconnect.
This reconnect uses the old password (because the test
hasn't had a chance to supply a new password).
2. The test code provides the correct password, and then
initiates an explicit connect. However, the explicit
connect is ignored, because a connection is already in
progress (to the same service).
3. The automatic reconnect fails, because it is using the
wrong password.
BUG=chromium-os:25153,chromium-os:25157
TEST=unit tests, WiFiRoaming, WiFiSecMat.*WEP*, WiFiManager
Testing notes:
- WiFiRoaming had some failures, but no regressions
- WiFiSecMat.*WEP* passed except 003CheckWEP_8021x
(previously, that failed in addition to 000StaticKeyWEP40,
001StaticKeyWEP104, 005SharedKeyWEP40, and 006SharedKeyWEP104.)
- WiFiManager passed
(previously, 003AsciiKeyWEP40 and 004AsciiKeyWEP104 failed.)
Change-Id: I2dd6312006865c914d4193ce3f7d7c5443b84deb
Reviewed-on: https://gerrit.chromium.org/gerrit/14408
Commit-Ready: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/cellular_service.cc b/cellular_service.cc
index c79329a..43aa917 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -48,11 +48,10 @@
CellularService::~CellularService() { }
void CellularService::Connect(Error *error) {
+ Service::Connect(error);
cellular_->Connect(error);
}
-void CellularService::Disconnect(Error */*error*/) { }
-
void CellularService::ActivateCellularModem(const string &carrier,
Error *error) {
cellular_->Activate(carrier, error);
diff --git a/cellular_service.h b/cellular_service.h
index 048db57..246a274 100644
--- a/cellular_service.h
+++ b/cellular_service.h
@@ -33,7 +33,6 @@
// Inherited from Service.
virtual void Connect(Error *error);
- virtual void Disconnect(Error *error);
virtual void ActivateCellularModem(const std::string &carrier, Error *error);
virtual bool TechnologyIs(const Technology::Identifier type) const;
diff --git a/ethernet_service.cc b/ethernet_service.cc
index eadf46c..f2c88cb 100644
--- a/ethernet_service.cc
+++ b/ethernet_service.cc
@@ -44,10 +44,6 @@
EthernetService::~EthernetService() { }
-void EthernetService::Connect(Error */*error*/) { }
-
-void EthernetService::Disconnect(Error */*error*/) { }
-
bool EthernetService::TechnologyIs(const Technology::Identifier type) const {
return ethernet_->TechnologyIs(type);
}
diff --git a/ethernet_service.h b/ethernet_service.h
index 6dbcf1b..d61c91f 100644
--- a/ethernet_service.h
+++ b/ethernet_service.h
@@ -28,8 +28,6 @@
~EthernetService();
// Inherited from Service.
- virtual void Connect(Error *error);
- virtual void Disconnect(Error *error);
virtual bool TechnologyIs(const Technology::Identifier type) const;
// ethernet_<MAC>
diff --git a/service.cc b/service.cc
index 952f0aa..21a8a13 100644
--- a/service.cc
+++ b/service.cc
@@ -78,6 +78,7 @@
auto_connect_(false),
check_portal_(kCheckPortalAuto),
connectable_(false),
+ explicitly_disconnected_(false),
favorite_(false),
priority_(kPriorityNone),
security_level_(0),
@@ -184,6 +185,14 @@
}
}
+void Service::Connect(Error */*error*/) {
+ explicitly_disconnected_ = false;
+}
+
+void Service::Disconnect(Error */*error*/) {
+ explicitly_disconnected_ = true;
+}
+
void Service::ActivateCellularModem(const string &/*carrier*/, Error *error) {
const string kMessage = "Service doesn't support cellular modem activation.";
LOG(ERROR) << kMessage;
@@ -264,6 +273,7 @@
// "APN"
// "LastGoodAPN"
+ explicitly_disconnected_ = false;
favorite_ = true;
return true;
@@ -501,7 +511,8 @@
}
bool Service::IsAutoConnectable() const {
- return connectable() && !IsConnected() && !IsConnecting();
+ return connectable() && !IsConnected() && !IsConnecting() &&
+ !explicitly_disconnected_;
}
void Service::HelpRegisterDerivedBool(
diff --git a/service.h b/service.h
index 38a4eb9..944ce6b 100644
--- a/service.h
+++ b/service.h
@@ -108,8 +108,10 @@
// be called from a D-Bus signal handler context.
virtual void AutoConnect();
// Queue up a connection attempt.
- virtual void Connect(Error *error) = 0;
- virtual void Disconnect(Error *error) = 0;
+ virtual void Connect(Error *error);
+ // Disconnect this service. The service will not be eligible for
+ // auto-connect until a subsequent call to Connect, or Load.
+ virtual void Disconnect(Error *error);
// The default implementation sets |error| to kInvalidArguments.
virtual void ActivateCellularModem(const std::string &carrier, Error *error);
@@ -329,6 +331,7 @@
std::string check_portal_;
bool connectable_;
std::string error_;
+ bool explicitly_disconnected_;
bool favorite_;
int32 priority_;
int32 security_level_;
diff --git a/service_under_test.cc b/service_under_test.cc
index f3137d1..4fdd6a4 100644
--- a/service_under_test.cc
+++ b/service_under_test.cc
@@ -27,10 +27,6 @@
ServiceUnderTest::~ServiceUnderTest() {}
-void ServiceUnderTest::Connect(Error */*error*/) {}
-
-void ServiceUnderTest::Disconnect(Error */*error*/) {}
-
string ServiceUnderTest::CalculateState(Error */*error*/) { return ""; }
string ServiceUnderTest::GetRpcIdentifier() const {
diff --git a/service_under_test.h b/service_under_test.h
index bafdf44..0e32223 100644
--- a/service_under_test.h
+++ b/service_under_test.h
@@ -26,8 +26,6 @@
Manager *manager);
virtual ~ServiceUnderTest();
- virtual void Connect(Error *error);
- virtual void Disconnect(Error *error);
virtual std::string CalculateState(Error *error);
virtual std::string GetRpcIdentifier() const;
virtual std::string GetDeviceRpcId(Error *error);
diff --git a/service_unittest.cc b/service_unittest.cc
index eb0c9b6..51f9d12 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -281,6 +281,28 @@
service_->set_connectable(true);
EXPECT_TRUE(service_->IsAutoConnectable());
+ // We should not auto-connect to a Service that a user has
+ // deliberately disconnected.
+ Error error;
+ service_->Disconnect(&error);
+ EXPECT_FALSE(service_->IsAutoConnectable());
+
+ // But if the Service is reloaded, it is eligible for auto-connect
+ // again.
+ NiceMock<MockStore> storage;
+ EXPECT_CALL(storage, ContainsGroup(storage_id_)).WillOnce(Return(true));
+ EXPECT_TRUE(service_->Load(&storage));
+ EXPECT_TRUE(service_->IsAutoConnectable());
+
+ // A deliberate Connect should also re-enable auto-connect.
+ service_->Disconnect(&error);
+ EXPECT_FALSE(service_->IsAutoConnectable());
+ service_->Connect(&error);
+ EXPECT_TRUE(service_->IsAutoConnectable());
+
+ // TODO(quiche): After we have resume handling in place, test that
+ // we re-enable auto-connect on resume. crosbug.com/25213
+
service_->SetState(Service::kStateConnected);
EXPECT_FALSE(service_->IsAutoConnectable());
diff --git a/wifi_service.cc b/wifi_service.cc
index 263775c..f0ee9e5 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -135,8 +135,9 @@
task_factory_.NewRunnableMethod(&WiFiService::ConnectTask));
}
-void WiFiService::Disconnect(Error */*error*/) {
+void WiFiService::Disconnect(Error *error) {
LOG(INFO) << __func__;
+ Service::Disconnect(error);
// Defer handling, since dbus-c++ does not permit us to send an
// outbound request while processing an inbound one.
dispatcher()->PostTask(
@@ -148,14 +149,14 @@
}
bool WiFiService::IsAutoConnectable() const {
- return connectable()
+ return Service::IsAutoConnectable() &&
// Only auto-connect to Services which have visible Endpoints.
// (Needed because hidden Services may remain registered with
// Manager even without visible Endpoints.)
- && HasEndpoints()
+ HasEndpoints() &&
// Do not preempt an existing connection (whether pending, or
// connected, and whether to this service, or another).
- && wifi_->IsIdle();
+ wifi_->IsIdle();
}
bool WiFiService::IsConnecting() const {
diff --git a/wifi_service_unittest.cc b/wifi_service_unittest.cc
index 3631f2d..5f226ac 100644
--- a/wifi_service_unittest.cc
+++ b/wifi_service_unittest.cc
@@ -588,6 +588,10 @@
EXPECT_TRUE(service->IsAutoConnectable());
EXPECT_CALL(*wifi(), ConnectTo(_, _));
service->AutoConnect();
+
+ Error error;
+ service->Disconnect(&error);
+ EXPECT_FALSE(service->IsAutoConnectable());
}
} // namespace shill