shill: (wifi) only register endpoints for which we have a service.
When we process a BSSRemoved event, we check that we know which Service the
Endpoint corresponds to. Previously, however, this check could fail.
The failure occurred in cases where the Endpoint was registered, but no
corresponding Service was found (or created). Resolve this problem by
registering the Endpoint only after we have a corresponding Service.
Also, remove some special-case code which is no longer needed.
BUG=chromium-os:26159
TEST=new unit tests
Change-Id: Ib4c13f9e426f69cd979afb1efdd4faad80fd853d
Reviewed-on: https://gerrit.chromium.org/gerrit/15697
Commit-Ready: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/wifi.cc b/wifi.cc
index 4694f20..bed02af 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -268,7 +268,7 @@
void WiFi::ConnectTo(WiFiService *service,
map<string, DBus::Variant> service_params) {
- CHECK(service);
+ CHECK(service) << "Can't connect to NULL service.";
DBus::Path network_path;
// TODO(quiche): Handle cases where already connected.
@@ -778,8 +778,8 @@
// means that if an AP reuses the same BSSID for multiple SSIDs, we
// lose.
WiFiEndpointRefPtr endpoint(new WiFiEndpoint(properties));
- endpoint_by_rpcid_[path] = endpoint;
LOG(INFO) << "Found endpoint. "
+ << "RPC path: " << path << ", "
<< "ssid: " << endpoint->ssid_string() << ", "
<< "bssid: " << endpoint->bssid_string() << ", "
<< "signal: " << endpoint->signal_strength() << ", "
@@ -810,16 +810,18 @@
DCHECK_EQ(1, service->NumEndpoints()); // Expect registered by now if >1.
manager()->RegisterService(service);
}
-
- return;
+ } else {
+ const bool hidden_ssid = false;
+ service = CreateServiceForEndpoint(*endpoint, hidden_ssid);
+ LOG(INFO) << "New service " << service->GetRpcIdentifier()
+ << " (" << service->friendly_name() << ")";
+ service->AddEndpoint(endpoint);
+ manager()->RegisterService(service);
}
- const bool hidden_ssid = false;
- service = CreateServiceForEndpoint(*endpoint, hidden_ssid);
- LOG(INFO) << "New service " << service->GetRpcIdentifier()
- << " (" << service->friendly_name() << ")";
- service->AddEndpoint(endpoint);
- manager()->RegisterService(service);
+ // Do this last, to maintain the invariant that any Endpoint we
+ // know about has a corresponding Service.
+ endpoint_by_rpcid_[path] = endpoint;
}
void WiFi::BSSRemovedTask(const ::DBus::Path &path) {
@@ -835,14 +837,10 @@
CHECK(endpoint);
endpoint_by_rpcid_.erase(i);
- if (endpoint->ssid_string().empty()) {
- // In BSSAdded, we don't create Services for Endpoints with empty
- // SSIDs. So don't bother looking for a Service to update.
- return;
- }
-
WiFiServiceRefPtr service = FindServiceForEndpoint(*endpoint);
- CHECK(service);
+ CHECK(service) << "Can't find Service for Endpoint "
+ << path << " "
+ << "(with BSSID " << endpoint->bssid_string() << ").";
VLOG(2) << "Removing Endpoint " << endpoint->bssid_string()
<< " from Service " << service->friendly_name();
service->RemoveEndpoint(endpoint);
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index 384b000..d3bcfde 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -578,10 +578,15 @@
ReportScanDone();
EXPECT_EQ(3, GetServices().size());
+ // BSSes with SSIDs that start with NULL should be filtered.
ReportBSS("bss3", string(1, 0), "00:00:00:00:00:03", 3, 0, kNetworkModeAdHoc);
- EXPECT_EQ(4, GetEndpointMap().size());
+ EXPECT_EQ(3, GetEndpointMap().size());
EXPECT_EQ(3, GetServices().size());
+ // BSSes with empty SSIDs should be filtered.
+ ReportBSS("bss3", string(), "00:00:00:00:00:03", 3, 0, kNetworkModeAdHoc);
+ EXPECT_EQ(3, GetEndpointMap().size());
+ EXPECT_EQ(3, GetServices().size());
}
TEST_F(WiFiMainTest, EndpointGroupingTogether) {
@@ -625,6 +630,22 @@
EXPECT_EQ(0, GetServices().size());
}
+TEST_F(WiFiMainTest, BSSWithEmptySSIDRemoved) {
+ // Removal of BSS with an empty SSID should not cause a crash.
+ ReportBSS("bss", string(), "00:00:00:00:00:01", 0, 0, kNetworkModeAdHoc);
+ StartWiFi();
+ RemoveBSS("bss");
+ EXPECT_EQ(0, GetServices().size());
+}
+
+TEST_F(WiFiMainTest, BSSWithNullSSIDRemoved) {
+ // Removal of BSS with a NULL SSID should not cause a crash.
+ ReportBSS("bss", string(1, 0), "00:00:00:00:00:01", 0, 0, kNetworkModeAdHoc);
+ StartWiFi();
+ RemoveBSS("bss");
+ EXPECT_EQ(0, GetServices().size());
+}
+
TEST_F(WiFiMainTest, LoneBSSRemoved) {
EXPECT_CALL(*manager(), RegisterService(_)).Times(AnyNumber());
StartWiFi();