shill: improve conformance with WiFiRoaming tests
This patch makes several changes to get shill passing more of
the WiFiRoaming suite. shill now passes 13 of the 18 tests.
Overview of changes:
- Clean up Services when their last Endpoint disappers.
- Make a new WiFi connection request pre-empt an existing one.
- Make Device::SelectService no-op if new service is same as old.
(part of resolving crosbug.com/23703)
- Move auto-connect logic to its own function, and run that function
in a deferred task (partly resolves crosbug.com/24276)
- Add a non-deferred variant of Service::Connect (part of resolving
crosbug.com/24276).
- Have service sort order reflect whether or not services are
connecting, failed, connectable, and configured for auto-connect.
BUG=chromium-os:24276,chromium-os:23223,chromium-os:22943,chromium-os:23703
TEST=WiFiRoaming, unit tests, manual
Manual testing: per https://gerrit.chromium.org/gerrit/12963
Collateral changes:
- updated TESTING
- added crosbug.com/24461 for problem with autotest and profiles
- declared some functions as const
- removed some useless comments
Change-Id: I36d6eb7108a377dbf409d3e5673deffb85c0633e
Reviewed-on: https://gerrit.chromium.org/gerrit/12687
Reviewed-by: Thieu Le <thieule@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
Commit-Ready: mukesh agrawal <quiche@chromium.org>
diff --git a/TESTING b/TESTING
index b1ad1e2..16cdca2 100644
--- a/TESTING
+++ b/TESTING
@@ -1,4 +1,4 @@
-Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
Use of this source code is governed by a BSD-style license that can be
found in the LICENSE file.
@@ -64,7 +64,7 @@
- stop flimflam, and start shill
localhost / # stop flimflam
localhost / # shill --foreground --v=1000 \
- --device-black-list=managed0,managed1,mon.managed0,mon.managed1
+ --device-black-list=managed0,managed1
(the black list keep shill from configuring the devices used by the hostapd
processes.)
diff --git a/cellular_service.cc b/cellular_service.cc
index b7b3b8a..7a9a7d4 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
diff --git a/device.cc b/device.cc
index 68ab81b..e5b6824 100644
--- a/device.cc
+++ b/device.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -359,6 +359,13 @@
service->UniqueName().c_str(),
service->friendly_name().c_str()) :
"*reset*");
+
+ if (selected_service_.get() == service.get()) {
+ // No change to |selected_service_|. Return early to avoid
+ // changing its state.
+ return;
+ }
+
if (selected_service_.get()) {
if (selected_service_->state() != Service::kStateFailure) {
selected_service_->SetState(Service::kStateIdle);
diff --git a/manager.cc b/manager.cc
index 559b237..393c43a 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -55,17 +55,19 @@
const string &run_directory,
const string &storage_directory,
const string &user_storage_format)
- : run_path_(FilePath(run_directory)),
- storage_path_(FilePath(storage_directory)),
- user_storage_format_(user_storage_format),
- adaptor_(control_interface->CreateManagerAdaptor(this)),
- device_info_(control_interface, dispatcher, this),
- modem_info_(control_interface, dispatcher, this, glib),
- running_(false),
- connect_profiles_to_rpc_(true),
- ephemeral_profile_(new EphemeralProfile(control_interface, this)),
- control_interface_(control_interface),
- glib_(glib) {
+ : dispatcher_(dispatcher),
+ task_factory_(this),
+ run_path_(FilePath(run_directory)),
+ storage_path_(FilePath(storage_directory)),
+ user_storage_format_(user_storage_format),
+ adaptor_(control_interface->CreateManagerAdaptor(this)),
+ device_info_(control_interface, dispatcher, this),
+ modem_info_(control_interface, dispatcher, this, glib),
+ running_(false),
+ connect_profiles_to_rpc_(true),
+ ephemeral_profile_(new EphemeralProfile(control_interface, this)),
+ control_interface_(control_interface),
+ glib_(glib) {
HelpRegisterDerivedString(flimflam::kActiveProfileProperty,
&Manager::GetActiveProfileName,
NULL);
@@ -215,6 +217,7 @@
// to avoid leaving permanent side effects to devices under test. This
// whole thing will need to be reworked in order to allow that to happen,
// or the autotests (or their expectations) will need to change.
+ // crosbug.com/24461
Error::PopulateAndLog(error, Error::kInvalidArguments,
"Cannot load non-default global profile " + name);
return;
@@ -422,7 +425,8 @@
<< " state: " << Service::ConnectStateToString(to_update->state())
<< " failure: "
<< Service::ConnectFailureToString(to_update->failure());
- LOG(INFO) << "IsConnected(): " << to_update->IsConnected();
+ VLOG(2) << "IsConnected(): " << to_update->IsConnected();
+ VLOG(2) << "IsConnecting(): " << to_update->IsConnecting();
if (to_update->IsConnected()) {
to_update->MakeFavorite();
if (to_update->profile().get() == ephemeral_profile_.get()) {
@@ -508,13 +512,48 @@
}
}
+ AutoConnect();
+}
+
+void Manager::AutoConnect() {
+ // We might be called in the middle of another request (e.g., as a
+ // consequence of Service::SetState calling UpdateService). To avoid
+ // re-entrancy issues in dbus-c++, defer to the event loop.
+ dispatcher_->PostTask(
+ task_factory_.NewRunnableMethod(&Manager::AutoConnectTask));
+ return;
+}
+
+void Manager::AutoConnectTask() {
+ if (services_.empty()) {
+ LOG(INFO) << "No services.";
+ return;
+ }
+
+ if (VLOG_IS_ON(4)) {
+ for (vector<ServiceRefPtr>::const_iterator it = services_.begin();
+ it != services_.end(); ++it) {
+ VLOG(4) << "Sorted service list: ";
+ VLOG(4) << "Service " << (*it)->friendly_name()
+ << " IsConnected: " << (*it)->IsConnected()
+ << " IsConnecting: " << (*it)->IsConnecting()
+ << " IsFailed: " << (*it)->IsFailed()
+ << " connectable: " << (*it)->connectable()
+ << " auto_connect: " << (*it)->auto_connect()
+ << " favorite: " << (*it)->favorite()
+ << " priority: " << (*it)->priority()
+ << " security_level: " << (*it)->security_level()
+ << " strength: " << (*it)->strength()
+ << " UniqueName: " << (*it)->UniqueName();
+ }
+ }
+
// Perform auto-connect.
- for (it = services_.begin(); it != services_.end(); ++it) {
- if ((*it)->auto_connect() && (*it)->IsAutoConnectable()) {
- Error error;
- (*it)->Connect(&error);
- // We intentionally ignore the error returned by Connect() here since it
- // should not prevent us from trying to connect a different service.
+ for (vector<ServiceRefPtr>::iterator it = services_.begin();
+ it != services_.end(); ++it) {
+ if ((*it)->auto_connect()) {
+ LOG(INFO) << "Initiating connect to " << (*it)->friendly_name() << ".";
+ (*it)->AutoConnect();
}
}
}
diff --git a/manager.h b/manager.h
index 63da1bc..a5fd702 100644
--- a/manager.h
+++ b/manager.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -61,7 +61,11 @@
void DeregisterDevice(const DeviceRefPtr &to_forget);
virtual bool HasService(const ServiceRefPtr &service);
+ // Register a Service with the Manager. Manager may choose to
+ // connect to it immediately.
virtual void RegisterService(const ServiceRefPtr &to_manage);
+ // Deregister a Service from the Manager. Caller is responsible
+ // for disconnecting the Service before-hand.
virtual void DeregisterService(const ServiceRefPtr &to_forget);
virtual void UpdateService(const ServiceRefPtr &to_update);
@@ -116,6 +120,8 @@
static const char kManagerErrorNoDevice[];
std::string CalculateState(Error *error);
+ void AutoConnect();
+ void AutoConnectTask();
std::vector<std::string> AvailableTechnologies(Error *error);
std::vector<std::string> ConnectedTechnologies(Error *error);
std::string DefaultTechnology(Error *error);
@@ -138,6 +144,8 @@
bool OrderServices(ServiceRefPtr a, ServiceRefPtr b);
void SortServices();
+ EventDispatcher *dispatcher_;
+ ScopedRunnableMethodFactory<Manager> task_factory_;
const FilePath run_path_;
const FilePath storage_path_;
const std::string user_storage_format_;
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 8c75c8e..b8f5ffc 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -163,7 +163,19 @@
manager->PushProfile(name, &path, &error);
return error.type();
}
+
protected:
+ typedef scoped_refptr<MockService> MockServiceRefPtr;
+
+ MockServiceRefPtr MakeAutoConnectableService() {
+ MockServiceRefPtr service = new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ manager());
+ service->MakeFavorite();
+ service->set_connectable(true);
+ return service;
+ }
+
scoped_refptr<MockWiFi> mock_wifi_;
vector<scoped_refptr<MockDevice> > mock_devices_;
scoped_ptr<MockDeviceInfo> device_info_;
@@ -347,6 +359,18 @@
manager.Stop();
}
+TEST_F(ManagerTest, DeregisterUnregisteredService) {
+ // WiFi assumes that it can deregister a service that is not
+ // registered. (E.g. a hidden service can be deregistered when it
+ // loses its last endpoint, and again when WiFi is Stop()-ed.)
+ //
+ // So test that doing so doesn't cause a crash.
+ MockServiceRefPtr service = new NiceMock<MockService>(control_interface(),
+ dispatcher(),
+ manager());
+ manager()->DeregisterService(service);
+}
+
TEST_F(ManagerTest, GetProperties) {
ProfileRefPtr profile(new MockProfile(control_interface(), manager(), ""));
AdoptProfile(manager(), profile);
@@ -733,27 +757,54 @@
manager()->UpdateService(mock_service0);
EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
- // Favorite
+ // Favorite.
mock_service1->MakeFavorite();
manager()->UpdateService(mock_service1);
EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
- // Connecting.
- EXPECT_CALL(*mock_service0.get(), state())
- .WillRepeatedly(Return(Service::kStateAssociating));
- EXPECT_CALL(*mock_service0.get(), IsConnecting())
- .WillRepeatedly(Return(true));
+ // Auto-connect.
+ mock_service0->set_auto_connect(true);
manager()->UpdateService(mock_service0);
+ mock_service1->set_auto_connect(false);
+ manager()->UpdateService(mock_service1);
EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
- // Connected.
+ // Connectable.
+ mock_service1->set_connectable(true);
+ manager()->UpdateService(mock_service1);
+ mock_service0->set_connectable(false);
+ manager()->UpdateService(mock_service0);
+ EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
+
+ // IsFailed.
+ EXPECT_CALL(*mock_service0.get(), state())
+ .WillRepeatedly(Return(Service::kStateIdle));
+ EXPECT_CALL(*mock_service0.get(), IsFailed())
+ .WillRepeatedly(Return(false));
+ manager()->UpdateService(mock_service0);
+ EXPECT_CALL(*mock_service0.get(), state())
+ .WillRepeatedly(Return(Service::kStateFailure));
+ EXPECT_CALL(*mock_service1.get(), IsFailed())
+ .WillRepeatedly(Return(true));
+ manager()->UpdateService(mock_service1);
+ EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
+ // Connecting.
EXPECT_CALL(*mock_service1.get(), state())
- .WillRepeatedly(Return(Service::kStateConnected));
- EXPECT_CALL(*mock_service1.get(), IsConnected())
+ .WillRepeatedly(Return(Service::kStateAssociating));
+ EXPECT_CALL(*mock_service1.get(), IsConnecting())
.WillRepeatedly(Return(true));
manager()->UpdateService(mock_service1);
EXPECT_TRUE(ServiceOrderIs(mock_service1, mock_service0));
+ // Connected.
+ EXPECT_CALL(*mock_service0.get(), state())
+ .WillRepeatedly(Return(Service::kStateConnected));
+ EXPECT_CALL(*mock_service0.get(), IsConnected())
+ .WillRepeatedly(Return(true));
+ manager()->UpdateService(mock_service0);
+ EXPECT_TRUE(ServiceOrderIs(mock_service0, mock_service1));
+
manager()->DeregisterService(mock_service0);
manager()->DeregisterService(mock_service1);
}
@@ -977,4 +1028,45 @@
manager()->UpdateService(service);
}
+TEST_F(ManagerTest, AutoConnectOnRegister) {
+ MockServiceRefPtr service = MakeAutoConnectableService();
+ EXPECT_CALL(*service.get(), AutoConnect());
+ manager()->RegisterService(service);
+ dispatcher()->DispatchPendingEvents();
+}
+
+TEST_F(ManagerTest, AutoConnectOnUpdate) {
+ MockServiceRefPtr service1 = MakeAutoConnectableService();
+ service1->set_priority(1);
+ MockServiceRefPtr service2 = MakeAutoConnectableService();
+ service2->set_priority(2);
+ manager()->RegisterService(service1);
+ manager()->RegisterService(service2);
+ dispatcher()->DispatchPendingEvents();
+
+ EXPECT_CALL(*service1.get(), AutoConnect());
+ EXPECT_CALL(*service2.get(), state())
+ .WillRepeatedly(Return(Service::kStateFailure));
+ EXPECT_CALL(*service2.get(), IsFailed())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(*service2.get(), IsConnected())
+ .WillRepeatedly(Return(false));
+ manager()->UpdateService(service2);
+ dispatcher()->DispatchPendingEvents();
+}
+
+TEST_F(ManagerTest, AutoConnectOnDeregister) {
+ MockServiceRefPtr service1 = MakeAutoConnectableService();
+ service1->set_priority(1);
+ MockServiceRefPtr service2 = MakeAutoConnectableService();
+ service2->set_priority(2);
+ manager()->RegisterService(service1);
+ manager()->RegisterService(service2);
+ dispatcher()->DispatchPendingEvents();
+
+ EXPECT_CALL(*service1.get(), AutoConnect());
+ manager()->DeregisterService(service2);
+ dispatcher()->DispatchPendingEvents();
+}
+
} // namespace shill
diff --git a/mock_service.cc b/mock_service.cc
index 7ad7911..e8f54c4 100644
--- a/mock_service.cc
+++ b/mock_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
diff --git a/mock_service.h b/mock_service.h
index d03e6c2..e5d3df4 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -25,6 +25,7 @@
Manager *manager);
virtual ~MockService();
+ MOCK_METHOD0(AutoConnect, void());
MOCK_METHOD1(Connect, void(Error *error));
MOCK_METHOD1(Disconnect, void(Error *error));
MOCK_METHOD1(CalculateState, std::string(Error *error));
@@ -34,6 +35,7 @@
MOCK_METHOD1(SetState, void(ConnectState state));
MOCK_CONST_METHOD0(IsConnected, bool());
MOCK_CONST_METHOD0(IsConnecting, bool());
+ MOCK_CONST_METHOD0(IsFailed, bool());
MOCK_METHOD1(SetFailure, void(ConnectFailure failure));
MOCK_CONST_METHOD0(failure, ConnectFailure());
MOCK_METHOD1(GetDeviceRpcId, std::string(Error *error));
diff --git a/mock_wifi.h b/mock_wifi.h
index 4870309..45ce307 100644
--- a/mock_wifi.h
+++ b/mock_wifi.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -40,6 +40,7 @@
MOCK_METHOD2(ConnectTo,
void(WiFiService *service,
std::map<std::string, ::DBus::Variant> service_params));
+ MOCK_CONST_METHOD0(IsIdle, bool());
private:
DISALLOW_COPY_AND_ASSIGN(MockWiFi);
diff --git a/service.cc b/service.cc
index c571461..15acf0a 100644
--- a/service.cc
+++ b/service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -174,6 +174,13 @@
metrics_->DeregisterService(this);
}
+void Service::AutoConnect() {
+ if (this->IsAutoConnectable()) {
+ Error error;
+ Connect(&error);
+ }
+}
+
void Service::ActivateCellularModem(const string &/*carrier*/, Error *error) {
const string kMessage = "Service doesn't support cellular modem activation.";
LOG(ERROR) << kMessage;
@@ -424,9 +431,15 @@
if (DecideBetween(a->IsConnecting(), b->IsConnecting(), &ret)) {
return ret;
}
+
+ if (DecideBetween(!a->IsFailed(), !b->IsFailed(), &ret)) {
+ return ret;
+ }
}
- if (DecideBetween(a->favorite(), b->favorite(), &ret) ||
+ if (DecideBetween(a->connectable(), b->connectable(), &ret) ||
+ DecideBetween(a->auto_connect(), b->auto_connect(), &ret) ||
+ DecideBetween(a->favorite(), b->favorite(), &ret) ||
DecideBetween(a->priority(), b->priority(), &ret)) {
return ret;
}
diff --git a/service.h b/service.h
index 096ed8a..374ec66 100644
--- a/service.h
+++ b/service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -98,6 +98,15 @@
Technology::Identifier technology);
virtual ~Service();
+ // AutoConnect MAY choose to ignore the connection request in some
+ // cases. For example, if the corresponding Device only supports one
+ // concurrent connection, and another Service is already connected
+ // or connecting.
+ //
+ // AutoConnect MAY issue RPCs immediately. So AutoConnect MUST NOT
+ // 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;
@@ -109,12 +118,6 @@
virtual bool IsActive(Error *error);
- // Returns whether this service is in a state conducive to auto-connect.
- // This should include any tests used for computing connectable(),
- // as well as other critera such as whether the device associated with
- // this service is busy with another connection.
- virtual bool IsAutoConnectable() { return connectable(); }
-
virtual ConnectState state() const { return state_; }
// Updates the state of the Service and alerts the manager. Also
// clears |failure_| if the new state isn't a failure.
@@ -125,6 +128,9 @@
virtual bool IsConnecting() const {
return state() == kStateAssociating || state() == kStateConfiguring;
}
+ virtual bool IsFailed() const {
+ return state() == kStateFailure;
+ }
virtual ConnectFailure failure() const { return failure_; }
// Records the failure mode, and sets the Service state to "Failure".
@@ -224,6 +230,12 @@
virtual std::string CalculateState(Error *error);
+ // Returns whether this service is in a state conducive to auto-connect.
+ // This should include any tests used for computing connectable(),
+ // as well as other critera such as whether the device associated with
+ // this service is busy with another connection.
+ virtual bool IsAutoConnectable() const { return connectable(); }
+
void HelpRegisterDerivedBool(
const std::string &name,
bool(Service::*get)(Error *error),
diff --git a/supplicant_interface_proxy.cc b/supplicant_interface_proxy.cc
index ac182cf..93252d9 100644
--- a/supplicant_interface_proxy.cc
+++ b/supplicant_interface_proxy.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -98,7 +98,8 @@
void SupplicantInterfaceProxy::Proxy::NetworkRemoved(
const ::DBus::Path &/*network*/) {
LOG(INFO) << __func__;
- // XXX
+ // TODO(quiche): Pass this up to WiFi, so that it can clean its
+ // rpcid_by_service_ map. crosbug.com/24699
}
void SupplicantInterfaceProxy::Proxy::NetworkSelected(
diff --git a/wifi.cc b/wifi.cc
index 6206c70..cc8fd00 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -10,6 +10,7 @@
#include <netinet/ether.h>
#include <linux/if.h> // Needs definitions from netinet/ether.h
+#include <algorithm>
#include <map>
#include <set>
#include <string>
@@ -65,7 +66,6 @@
"service mode is unsupported";
const char WiFi::kInterfaceStateUnknown[] = "shill-unknown";
-// NB: we assume supplicant is already running. [quiche.20110518]
WiFi::WiFi(ControlInterface *control_interface,
EventDispatcher *dispatcher,
Manager *manager,
@@ -202,6 +202,12 @@
return type == Technology::kWifi;
}
+bool WiFi::IsConnectingTo(const WiFiService &service) const {
+ return pending_service_ == &service
+ || (current_service_ == &service
+ && service.state() != Service::kStateConnected);
+}
+
void WiFi::LinkEvent(unsigned int flags, unsigned int change) {
// TODO(quiche): Figure out how to relate these events to supplicant
// events. E.g., may be we can ignore LinkEvent, in favor of events
@@ -264,8 +270,17 @@
DBus::Path network_path;
// TODO(quiche): Handle cases where already connected.
- // TODO(quiche): Handle case where there's already a pending
- // connection attempt.
+ if (pending_service_ && pending_service_ == service) {
+ // TODO(quiche): Return an error to the caller. crosbug.com/23832
+ LOG(INFO) << "WiFi " << link_name() << " ignoring ConnectTo "
+ << service->friendly_name()
+ << ", which is already pending.";
+ return;
+ }
+
+ if (pending_service_ && pending_service_ != service) {
+ DisconnectFrom(pending_service_);
+ }
try {
// TODO(quiche): Set a timeout here. In the normal case, we expect
@@ -277,7 +292,6 @@
append_uint32(scan_ssid);
network_path =
supplicant_interface_proxy_->AddNetwork(service_params);
- // TODO(quiche): Figure out when to remove services from this map.
rpcid_by_service_[service] = network_path;
} catch (const DBus::Error e) { // NOLINT
LOG(ERROR) << "exception while adding network: " << e.what();
@@ -286,6 +300,9 @@
supplicant_interface_proxy_->SelectNetwork(network_path);
+ pending_service_ = service;
+ CHECK(current_service_.get() != pending_service_.get());
+
// SelectService here (instead of in LinkEvent, like Ethernet), so
// that, if we fail to bring up L2, we can attribute failure correctly.
//
@@ -293,8 +310,6 @@
// reconsider if this is the right place to change the selected service.
// see discussion in crosbug.com/20191.
SelectService(service);
- pending_service_ = service;
- CHECK(current_service_.get() != pending_service_.get());
}
void WiFi::DisconnectFrom(WiFiService *service) {
@@ -353,7 +368,7 @@
|| current_service_.get() != pending_service_.get());
}
-bool WiFi::IsIdle() {
+bool WiFi::IsIdle() const {
return !current_service_ && !pending_service_;
}
@@ -385,6 +400,8 @@
}
SelectService(current_service_);
+ // Invariant check: a Service can either be current, or pending, but
+ // not both.
CHECK(current_service_.get() != pending_service_.get() ||
current_service_.get() == NULL);
@@ -423,6 +440,9 @@
// TODO(quiche): Reconsider giving up immediately. Maybe give
// wpa_supplicant some time to retry, first.
supplicant_interface_proxy_->RemoveNetwork(rpcid_it->second);
+ // TODO(quiche): If we initated the disconnect, we should probably
+ // go to the idle state instead. crosbug.com/24700
+ affected_service->SetFailure(Service::kFailureUnknown);
if (affected_service == pending_service_.get()) {
// The attempt to connect to |pending_service_| failed. Clear
@@ -528,9 +548,9 @@
<< " new current Endpoint "
<< endpoint.bssid_string()
<< (current_service_.get() ?
- StringPrintf("%s is not part of current service ",
+ StringPrintf(" is not part of current service %s",
current_service_->friendly_name().c_str()) :
- "with no current service");
+ " with no current service");
// We didn't expect to be here, but let's cope as well as we
// can. Update |current_service_| to keep it in sync with
// supplicant.
@@ -743,12 +763,40 @@
WiFiServiceRefPtr service = FindServiceForEndpoint(*endpoint);
CHECK(service);
+ VLOG(2) << "Removing Endpoint " << endpoint->bssid_string()
+ << " from Service " << service->friendly_name();
service->RemoveEndpoint(endpoint);
- manager()->UpdateService(service);
- // TODO(quiche): If the Service no longer has any Endpoints, we may
- // want to unregister it from the Manager, and possibly our own
- // |services_| list as well. (crosbug.com/23703)
+ bool disconnect_service = !service->HasEndpoints() &&
+ (service->IsConnecting() || service->IsConnected());
+ bool forget_service =
+ // Forget Services without Endpoints, except that we always keep
+ // hidden services around. (We need them around to populate the
+ // hidden SSIDs list.)
+ !service->HasEndpoints() && !service->hidden_ssid();
+ bool deregister_service =
+ // Only deregister a Service if we're forgetting it. Otherwise,
+ // Manager can't keep our configuration up-to-date (as Profiles
+ // change).
+ forget_service;
+
+ if (disconnect_service) {
+ DisconnectFrom(service);
+ }
+
+ if (deregister_service) {
+ manager()->DeregisterService(service);
+ } else {
+ manager()->UpdateService(service);
+ }
+
+ if (forget_service) {
+ vector<WiFiServiceRefPtr>::iterator it;
+ it = std::find(services_.begin(), services_.end(), service);
+ if (it != services_.end()) {
+ services_.erase(it);
+ }
+ }
}
void WiFi::PropertiesChangedTask(
@@ -826,6 +874,10 @@
// IP connectivity.
affected_service->SetState(Service::kStateConfiguring);
} else if (new_state == wpa_supplicant::kInterfaceStateAssociated) {
+ // TODO(quiche): Resolve race with LinkEvent. It's possible for us
+ // to receive this state notification after LinkEvent. In that case,
+ // our state transitions are Associating -> Configuring ->
+ // Associating -> Configuring -> Ready. (crosbug.com/22831)
affected_service->SetState(Service::kStateAssociating);
} else if (new_state == wpa_supplicant::kInterfaceStateAuthenticating ||
new_state == wpa_supplicant::kInterfaceStateAssociating ||
@@ -838,7 +890,7 @@
// TOOD(quiche): On backwards transitions, we should probably set
// a timeout for getting back into the completed state. At present,
// we depend on wpa_supplicant eventually reporting that CurrentBSS
- // has changed. But there may be cases where that signal is sent.
+ // has changed. But there may be cases where that signal is not sent.
// (crosbug.com/23207)
} else {
// Other transitions do not affect Service state.
diff --git a/wifi.h b/wifi.h
index 572bd5c..f4da2e1 100644
--- a/wifi.h
+++ b/wifi.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -41,6 +41,7 @@
virtual bool Load(StoreInterface *storage);
virtual void Scan(Error *error);
virtual bool TechnologyIs(const Technology::Identifier type) const;
+ virtual bool IsConnectingTo(const WiFiService &service) const;
virtual void LinkEvent(unsigned int flags, unsigned int change);
// Called by SupplicantInterfaceProxy, in response to events from
@@ -57,7 +58,7 @@
WiFiService *service,
std::map<std::string, ::DBus::Variant> service_params);
virtual void DisconnectFrom(WiFiService *service);
- virtual bool IsIdle();
+ virtual bool IsIdle() const;
// Called by Manager.
virtual WiFiServiceRefPtr GetService(const KeyValueStore &args, Error *error);
diff --git a/wifi_endpoint.cc b/wifi_endpoint.cc
index 4234513..12d9e59 100644
--- a/wifi_endpoint.cc
+++ b/wifi_endpoint.cc
@@ -95,6 +95,30 @@
}
// static
+WiFiEndpoint *WiFiEndpoint::MakeOpenEndpoint(
+ const string &ssid, const string &bssid) {
+ map <string, ::DBus::Variant> args;
+ ::DBus::MessageIter writer;
+
+ writer = args[wpa_supplicant::kBSSPropertySSID].writer();
+ writer << vector<uint8_t>(ssid.begin(), ssid.end());
+
+ string bssid_nosep;
+ RemoveChars(bssid, ":", &bssid_nosep);
+ vector<uint8_t> bssid_bytes;
+ base::HexStringToBytes(bssid_nosep, &bssid_bytes);
+ writer = args[wpa_supplicant::kBSSPropertyBSSID].writer();
+ writer << bssid_bytes;
+
+ args[wpa_supplicant::kBSSPropertySignal].writer().append_int16(0);
+ args[wpa_supplicant::kBSSPropertyMode].writer().append_string(
+ wpa_supplicant::kNetworkModeInfrastructure);
+ // We indicate this is an open BSS by leaving out all security properties.
+
+ return new WiFiEndpoint(args);
+}
+
+// static
const char *WiFiEndpoint::ParseMode(const string &mode_string) {
if (mode_string == wpa_supplicant::kNetworkModeInfrastructure) {
return flimflam::kModeManaged;
diff --git a/wifi_endpoint.h b/wifi_endpoint.h
index 0093425..9dd0ba8 100644
--- a/wifi_endpoint.h
+++ b/wifi_endpoint.h
@@ -39,6 +39,8 @@
private:
friend class WiFiEndpointTest;
+ friend class WiFiMainTest; // for MakeOpenEndpoint
+ friend class WiFiServiceTest; // for MakeOpenEndpoint
// these test cases need access to the KeyManagement enum
FRIEND_TEST(WiFiEndpointTest, ParseKeyManagementMethodsEAP);
FRIEND_TEST(WiFiEndpointTest, ParseKeyManagementMethodsPSK);
@@ -49,6 +51,9 @@
kKeyManagementPSK
};
+ // Build a simple WiFiEndpoint, for testing purposes.
+ static WiFiEndpoint *MakeOpenEndpoint(
+ const std::string &ssid, const std::string &bssid);
// Maps mode strings from supplicant into flimflam's nomenclature, as defined
// in chromeos/dbus/service_constants.h
static const char *ParseMode(const std::string &mode_string);
diff --git a/wifi_service.cc b/wifi_service.cc
index 995394f..4882ccf 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -113,6 +113,22 @@
LOG(INFO) << __func__;
}
+void WiFiService::AutoConnect() {
+ if (IsAutoConnectable()) {
+ // Execute immediately, for two reasons:
+ //
+ // 1. We need IsAutoConnectable to return the correct value for
+ // other WiFiServices, and that depends on WiFi's state.
+ //
+ // 2. We should probably limit the extent to which we queue up
+ // actions (such as AutoConnect) which depend on current state.
+ // If we queued AutoConnects, we could build a long queue of
+ // useless work (one AutoConnect per Service), which blocks
+ // more timely work.
+ ConnectTask();
+ }
+}
+
void WiFiService::Connect(Error */*error*/) {
LOG(INFO) << __func__;
// Defer handling, since dbus-c++ does not permit us to send an
@@ -133,10 +149,22 @@
return wifi_->TechnologyIs(type);
}
-bool WiFiService::IsAutoConnectable() {
- // TODO(quiche): Need to also handle the case where there might be
- // another service that has posted a Connect task. crosbug.com/24276
- return connectable() && wifi_->IsIdle();
+bool WiFiService::IsAutoConnectable() const {
+ return connectable()
+ // Only auto-connect to Services which have visible Endpoints.
+ // (Needed because hidden Services may remain registered with
+ // Manager even without visible Endpoints.)
+ && HasEndpoints()
+ // Do not preempt other connections (whether pending, or
+ // connected).
+ && wifi_->IsIdle();
+}
+
+bool WiFiService::IsConnecting() const {
+ // WiFi does not move us into the associating state until it gets
+ // feedback from wpa_supplicant. So, to answer whether or
+ // not we're connecting, we consult with |wifi_|.
+ return wifi_->IsConnectingTo(*this);
}
void WiFiService::AddEndpoint(WiFiEndpointConstRefPtr endpoint) {
@@ -185,12 +213,10 @@
}
bool WiFiService::IsVisible() const {
- const bool is_visible_in_scan = !endpoints_.empty();
-
// WiFi Services should be displayed only if they are in range (have
// endpoints that have shown up in a scan) or if the service is actively
// being connected.
- return is_visible_in_scan || IsConnected() || IsConnecting();
+ return HasEndpoints() || IsConnected() || IsConnecting();
}
bool WiFiService::Load(StoreInterface *storage) {
diff --git a/wifi_service.h b/wifi_service.h
index 13b0a9d..460598c 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -34,11 +34,12 @@
~WiFiService();
// Inherited from Service.
+ virtual void AutoConnect();
virtual void Connect(Error *error);
virtual void Disconnect(Error *error);
virtual bool TechnologyIs(const Technology::Identifier type) const;
- virtual bool IsAutoConnectable();
+ virtual bool IsConnecting() const;
virtual void AddEndpoint(WiFiEndpointConstRefPtr endpoint);
virtual void RemoveEndpoint(WiFiEndpointConstRefPtr endpoint);
@@ -63,6 +64,7 @@
virtual bool Load(StoreInterface *storage);
virtual bool Save(StoreInterface *storage);
+ virtual bool HasEndpoints() const { return !endpoints_.empty(); }
virtual bool IsVisible() const;
bool IsSecurityMatch(const std::string &security) const;
bool hidden_ssid() const { return hidden_ssid_; }
@@ -70,13 +72,18 @@
virtual void InitializeCustomMetrics() const;
virtual void SendPostReadyStateMetrics() const;
+ protected:
+ virtual bool IsAutoConnectable() const;
+
private:
friend class WiFiServiceSecurityTest;
FRIEND_TEST(MetricsTest, WiFiServiceChannel);
+ FRIEND_TEST(WiFiServiceTest, AutoConnect);
FRIEND_TEST(WiFiServiceTest, ConnectTaskRSN);
FRIEND_TEST(WiFiServiceTest, ConnectTaskWPA);
FRIEND_TEST(WiFiServiceTest, ConnectTaskPSK);
FRIEND_TEST(WiFiServiceTest, ConnectTaskWEP);
+ FRIEND_TEST(WiFiServiceTest, IsAutoConnectable);
FRIEND_TEST(WiFiServiceTest, LoadHidden);
static const char kStorageHiddenSSID[];
diff --git a/wifi_service_unittest.cc b/wifi_service_unittest.cc
index 244b957..7254bd0 100644
--- a/wifi_service_unittest.cc
+++ b/wifi_service_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -21,6 +21,8 @@
#include "shill/mock_store.h"
#include "shill/mock_wifi.h"
#include "shill/property_store_unittest.h"
+#include "shill/refptr_types.h"
+#include "shill/wifi_endpoint.h"
#include "shill/wpa_supplicant.h"
using std::map;
@@ -64,6 +66,9 @@
service->SetPassphrase(passphrase, &error);
return service->connectable();
}
+ WiFiEndpoint *MakeEndpoint(const string &ssid, const string &bssid) {
+ return WiFiEndpoint::MakeOpenEndpoint(ssid, bssid);
+ }
scoped_refptr<MockWiFi> wifi() { return wifi_; }
private:
@@ -474,4 +479,59 @@
EXPECT_FALSE(CheckConnectable(flimflam::kSecurity8021x, NULL));
}
+TEST_F(WiFiServiceTest, IsAutoConnectable) {
+ vector<uint8_t> ssid(1, 'a');
+ WiFiServiceRefPtr service = new WiFiService(control_interface(),
+ dispatcher(),
+ manager(),
+ wifi(),
+ ssid,
+ flimflam::kModeManaged,
+ flimflam::kSecurityNone,
+ false);
+ EXPECT_CALL(*wifi(), IsIdle())
+ .WillRepeatedly(Return(true));
+ EXPECT_FALSE(service->HasEndpoints());
+ EXPECT_FALSE(service->IsAutoConnectable());
+
+ WiFiEndpointRefPtr endpoint = MakeEndpoint("a", "00:00:00:00:00:01");
+ service->AddEndpoint(endpoint);
+ EXPECT_CALL(*wifi(), IsIdle())
+ .WillRepeatedly(Return(true));
+ EXPECT_TRUE(service->HasEndpoints());
+ EXPECT_TRUE(service->IsAutoConnectable());
+
+ // WiFi only supports connecting to one Service at a time. So, to
+ // avoid disrupting connectivity, we only allow auto-connection to
+ // a WiFiService when the corresponding WiFi is idle.
+ EXPECT_CALL(*wifi(), IsIdle())
+ .WillRepeatedly(Return(false));
+ EXPECT_TRUE(service->HasEndpoints());
+ EXPECT_FALSE(service->IsAutoConnectable());
+}
+
+TEST_F(WiFiServiceTest, AutoConnect) {
+ vector<uint8_t> ssid(1, 'a');
+ WiFiServiceRefPtr service = new WiFiService(control_interface(),
+ dispatcher(),
+ manager(),
+ wifi(),
+ ssid,
+ flimflam::kModeManaged,
+ flimflam::kSecurityNone,
+ false);
+ EXPECT_FALSE(service->IsAutoConnectable());
+ EXPECT_CALL(*wifi(), ConnectTo(_, _))
+ .Times(0);
+ service->AutoConnect();
+
+ WiFiEndpointRefPtr endpoint = MakeEndpoint("a", "00:00:00:00:00:01");
+ service->AddEndpoint(endpoint);
+ EXPECT_CALL(*wifi(), IsIdle())
+ .WillRepeatedly(Return(true));
+ EXPECT_TRUE(service->IsAutoConnectable());
+ EXPECT_CALL(*wifi(), ConnectTo(_, _));
+ service->AutoConnect();
+}
+
} // namespace shill
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index 5598988..649ee95 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -206,8 +206,9 @@
SupplicantProcessProxyInterface *GetSupplicantProcessProxy() {
return wifi_->supplicant_process_proxy_.get();
}
- SupplicantInterfaceProxyInterface *GetSupplicantInterfaceProxy() {
- return wifi_->supplicant_interface_proxy_.get();
+ MockSupplicantInterfaceProxy *GetSupplicantInterfaceProxy() {
+ return dynamic_cast<MockSupplicantInterfaceProxy *>(
+ wifi_->supplicant_interface_proxy_.get());
}
const string &GetSupplicantState() {
return wifi_->supplicant_state_;
@@ -223,25 +224,7 @@
return wifi_->link_up_;
}
WiFiEndpointRefPtr MakeEndpoint(const string &ssid, const string &bssid) {
- map <string, ::DBus::Variant> args;
- ::DBus::MessageIter writer;
-
- writer = args[wpa_supplicant::kBSSPropertySSID].writer();
- writer << vector<uint8_t>(ssid.begin(), ssid.end());
-
- string bssid_nosep;
- RemoveChars(bssid, ":", &bssid_nosep);
- vector<uint8_t> bssid_bytes;
- base::HexStringToBytes(bssid_nosep, &bssid_bytes);
- writer = args[wpa_supplicant::kBSSPropertyBSSID].writer();
- writer << bssid_bytes;
-
- args[wpa_supplicant::kBSSPropertySignal].writer().append_int16(0);
- args[wpa_supplicant::kBSSPropertyMode].writer().append_string(
- wpa_supplicant::kNetworkModeInfrastructure);
- // We indicate this is an open BSS by leaving out all security properties.
-
- return new WiFiEndpoint(args);
+ return WiFiEndpoint::MakeOpenEndpoint(ssid, bssid);
}
MockWiFiServiceRefPtr MakeMockService() {
vector<uint8_t> ssid(1, 'a');
@@ -554,10 +537,45 @@
EXPECT_EQ(1, GetServices().size());
EXPECT_TRUE(GetServices().front()->IsVisible());
- EXPECT_CALL(*manager(), UpdateService(_));
+ EXPECT_CALL(*manager(), DeregisterService(_));
RemoveBSS("bss0");
- EXPECT_FALSE(GetServices().front()->IsVisible());
+ EXPECT_TRUE(GetServices().empty());
+}
+
+TEST_F(WiFiMainTest, LoneBSSRemovedWhileConnected) {
+ StartWiFi();
+ ReportBSS("bss0", "ssid", "00:00:00:00:00:00", 0, kNetworkModeAdHoc);
+ ReportScanDone();
+ ReportCurrentBSSChanged("bss0");
+
+ EXPECT_CALL(*GetSupplicantInterfaceProxy(), Disconnect());
+ EXPECT_CALL(*manager(), DeregisterService(_));
+ RemoveBSS("bss0");
+ EXPECT_TRUE(GetServices().empty());
+}
+
+TEST_F(WiFiMainTest, LoneBSSRemovedWhileConnectedToHidden) {
+ StartWiFi();
+
+ Error e;
+ WiFiServiceRefPtr service =
+ GetServiceInner(flimflam::kTypeWifi, "ssid", flimflam::kModeManaged,
+ NULL, NULL, true, &e);
EXPECT_EQ(1, GetServices().size());
+
+ ReportBSS("bss", "ssid", "00:00:00:00:00:01", 0, kNetworkModeInfrastructure);
+ ReportScanDone();
+ ReportCurrentBSSChanged("bss");
+ EXPECT_EQ(1, GetServices().size());
+
+ EXPECT_CALL(*GetSupplicantInterfaceProxy(), Disconnect());
+ EXPECT_CALL(*manager(), UpdateService(_));
+ RemoveBSS("bss");
+ EXPECT_TRUE(manager()->HasService(service));
+ EXPECT_EQ(1, GetServices().size());
+ // Verify expectations now, because WiFi may call UpdateService when
+ // WiFi is Stop()-ed (during TearDown()).
+ Mock::VerifyAndClearExpectations(manager());
}
TEST_F(WiFiMainTest, NonSolitaryBSSRemoved) {
@@ -1183,7 +1201,7 @@
EXPECT_EQ(NULL, GetPendingService().get());
ReportCurrentBSSChanged(wpa_supplicant::kCurrentBSSNull);
- EXPECT_EQ(Service::kStateIdle, service->state());
+ EXPECT_EQ(Service::kStateFailure, service->state());
EXPECT_EQ(NULL, GetCurrentService().get());
EXPECT_EQ(NULL, GetPendingService().get());
}
@@ -1245,4 +1263,36 @@
kNetworkModeInfrastructure);
}
+TEST_F(WiFiMainTest, NewConnectPreemptsPending) {
+ WiFiEndpointRefPtr ap1 = MakeEndpoint("an_ssid", "00:01:02:03:04:05");
+ WiFiEndpointRefPtr ap2 = MakeEndpoint("another_ssid", "01:02:03:04:05:06");
+ WiFiServiceRefPtr service1 = CreateServiceForEndpoint(*ap1);
+ WiFiServiceRefPtr service2 = CreateServiceForEndpoint(*ap2);
+
+ StartWiFi();
+ ReportBSS("ap1", ap1->ssid_string(), ap1->bssid_string(), 0,
+ kNetworkModeInfrastructure);
+ ReportBSS("ap2", ap2->ssid_string(), ap2->bssid_string(), 0,
+ kNetworkModeInfrastructure);
+ InitiateConnect(service1);
+ EXPECT_EQ(service1.get(), GetPendingService().get());
+
+ EXPECT_CALL(*GetSupplicantInterfaceProxy(), Disconnect());
+ EXPECT_CALL(*GetSupplicantInterfaceProxy(), AddNetwork(_));
+ InitiateConnect(service2);
+ EXPECT_EQ(service2.get(), GetPendingService().get());
+}
+
+TEST_F(WiFiMainTest, IsIdle) {
+ StartWiFi();
+ EXPECT_TRUE(wifi()->IsIdle());
+
+ WiFiEndpointRefPtr ap = MakeEndpoint("an_ssid", "00:01:02:03:04:05");
+ WiFiServiceRefPtr service = CreateServiceForEndpoint(*ap);
+ Error error;
+ service->AddEndpoint(ap);
+ service->AutoConnect();
+ EXPECT_FALSE(wifi()->IsIdle());
+}
+
} // namespace shill