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/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) {