shill: WiFiService: Fix-up old-style WiFi Service entries
Add a utility to upgrade WiFi service entries that don't contain
the "Type", "Mode" or "Security" properties, and only contain
this information in the fields of the storage identifier. Use
this utility whenever the WiFi device loads a new profile.
Follow-on CLs will clean up all remaining direct callers of
WiFiServce::ParseStorageIdentifier().
BUG=chromium-os:38048
TEST=Unit Tests, inspect chrome://histograms for new histogram
entries when profiles are updated.
Change-Id: I0f1d4e75abc1edfe6da1bbfe68183cab1b1c0021
Reviewed-on: https://gerrit.chromium.org/gerrit/41802
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
diff --git a/manager.cc b/manager.cc
index be64a6c..9f4878e 100644
--- a/manager.cc
+++ b/manager.cc
@@ -594,6 +594,13 @@
return IsTechnologyInList(props_.link_monitor_technologies, technology);
}
+bool Manager::IsDefaultProfile(const StoreInterface *storage) const {
+ if (profiles_.empty()) {
+ return false;
+ }
+ return storage == profiles_.front()->GetConstStorage();
+}
+
const ProfileRefPtr &Manager::ActiveProfile() const {
DCHECK_NE(profiles_.size(), 0U);
return profiles_.back();
diff --git a/manager.h b/manager.h
index dc888ce..f0b43af 100644
--- a/manager.h
+++ b/manager.h
@@ -40,6 +40,7 @@
class EventDispatcher;
class ManagerAdaptorInterface;
class Resolver;
+class StoreInterface;
class Manager : public base::SupportsWeakPtr<Manager> {
public:
@@ -196,6 +197,9 @@
virtual bool IsTechnologyLinkMonitorEnabled(
Technology::Identifier technology) const;
+ // Return whether |storage| is for the default profile.
+ virtual bool IsDefaultProfile(const StoreInterface *storage) const;
+
std::string CalculateState(Error *error);
virtual int GetPortalCheckInterval() const {
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 4b05a73..e801011 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -2729,6 +2729,20 @@
manager()->IsTechnologyLinkMonitorEnabled(Technology::kCellular));
}
+TEST_F(ManagerTest, IsDefaultProfile) {
+ EXPECT_FALSE(manager()->IsDefaultProfile(NULL));
+ scoped_ptr<MockStore> store0(new MockStore);
+ EXPECT_FALSE(manager()->IsDefaultProfile(store0.get()));
+ scoped_refptr<MockProfile> profile(
+ new MockProfile(control_interface(), manager(), ""));
+ EXPECT_CALL(*profile, GetConstStorage()).WillRepeatedly(Return(store0.get()));
+ AdoptProfile(manager(), profile);
+ EXPECT_TRUE(manager()->IsDefaultProfile(store0.get()));
+ EXPECT_FALSE(manager()->IsDefaultProfile(NULL));
+ scoped_ptr<MockStore> store1(new MockStore);
+ EXPECT_FALSE(manager()->IsDefaultProfile(store1.get()));
+}
+
TEST_F(ManagerTest, EnableTechnology) {
Error error(Error::kOperationInitiated);
ResultCallback callback;
diff --git a/metrics.cc b/metrics.cc
index 688a150..bbbd150 100644
--- a/metrics.cc
+++ b/metrics.cc
@@ -120,6 +120,10 @@
const int Metrics::kMetricTerminationActionTimeMillisecondsMin = 1;
// static
+const char Metrics::kMetricServiceFixupEntries[] =
+ "Network.Shill.%s.ServiceFixupEntries";
+
+// static
const uint16 Metrics::kWiFiBandwidth5MHz = 5;
const uint16 Metrics::kWiFiBandwidth20MHz = 20;
const uint16 Metrics::kWiFiFrequency2412 = 2412;
diff --git a/metrics.h b/metrics.h
index d189df1..baa7613 100644
--- a/metrics.h
+++ b/metrics.h
@@ -140,6 +140,12 @@
kDisconnectedNotByAp
};
+ enum ServiceFixupProfileType {
+ kMetricServiceFixupDefaultProfile,
+ kMetricServiceFixupUserProfile,
+ kMetricServiceFixupMax
+ };
+
enum TerminationActionResult {
kTerminationActionResultSuccess,
kTerminationActionResultFailure,
@@ -245,6 +251,9 @@
static const int kMetricTerminationActionTimeMillisecondsMax;
static const int kMetricTerminationActionTimeMillisecondsMin;
+ // WiFiService Entry Fixup.
+ static const char kMetricServiceFixupEntries[];
+
Metrics();
virtual ~Metrics();
diff --git a/mock_manager.h b/mock_manager.h
index b8836b0..ce21c3f 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -50,6 +50,7 @@
bool(const ServiceConstRefPtr &service));
MOCK_CONST_METHOD1(IsTechnologyLinkMonitorEnabled,
bool(Technology::Identifier tech));
+ MOCK_CONST_METHOD1(IsDefaultProfile, bool(const StoreInterface *storage));
MOCK_CONST_METHOD0(GetPortalCheckURL, const std::string &());
MOCK_CONST_METHOD0(GetPortalCheckInterval, int());
diff --git a/store_interface.h b/store_interface.h
index 32da6b8..2634bed 100644
--- a/store_interface.h
+++ b/store_interface.h
@@ -47,7 +47,8 @@
// Gets a string |value| associated with |group|:|key|. Returns true on
// success and false on failure (including when |group|:|key| is not present
- // in the store).
+ // in the store). It is not an error to pass NULL as |value| to simply
+ // test for the presence of this value.
virtual bool GetString(const std::string &group,
const std::string &key,
std::string *value) const = 0;
@@ -60,7 +61,9 @@
// Gets a boolean |value| associated with |group|:|key|. Returns true on
// success and false on failure (including when the |group|:|key| is not
- // present in the store).
+ // present in the store). It is not an error to pass NULL as |value| to
+ // simply test for the presence of this value.
+
virtual bool GetBool(const std::string &group,
const std::string &key,
bool *value) const = 0;
@@ -73,7 +76,8 @@
// Gets a integer |value| associated with |group|:|key|. Returns true on
// success and false on failure (including when the |group|:|key| is not
- // present in the store).
+ // present in the store). It is not an error to pass NULL as |value| to
+ // simply test for the presence of this value.
virtual bool GetInt(const std::string &group,
const std::string &key,
int *value) const = 0;
@@ -86,7 +90,8 @@
// Gets a 64-bit unsigned integer |value| associated with |group|:|key|.
// Returns true on success and false on failure (including when the
- // |group|:|key| is not present in the store).
+ // |group|:|key| is not present in the store). It is not an error to
+ // pass NULL as |value| to simply test for the presence of this value.
virtual bool GetUint64(const std::string &group,
const std::string &key,
uint64 *value) const = 0;
@@ -99,7 +104,8 @@
// Gets a string list |value| associated with |group|:|key|. Returns true on
// success and false on failure (including when |group|:|key| is not present
- // in the store).
+ // in the store). It is not an error to pass NULL as |value| to simply test
+ // for the presence of this value.
virtual bool GetStringList(const std::string &group,
const std::string &key,
std::vector<std::string> *value) const = 0;
@@ -112,7 +118,8 @@
// Gets and decrypts string |value| associated with |group|:|key|. Returns
// true on success and false on failure (including when |group|:|key| is not
- // present in the store).
+ // present in the store). It is not an error to pass NULL as |value| to
+ // simply test for the presence of this value.
virtual bool GetCryptedString(const std::string &group,
const std::string &key,
std::string *value) = 0;
diff --git a/wifi.cc b/wifi.cc
index 99c122e..a102617 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -934,6 +934,24 @@
}
bool WiFi::LoadHiddenServices(StoreInterface *storage) {
+ // TODO(pstew): This retrofits old (flimflam-based) profile entries for
+ // WiFi Services so they contain the properties that are required to
+ // be searched for by property instead of by group name. I can imagine
+ // a day where we believe that enough users have upgraded so that this
+ // code is no longer necessary. The UMA metric below should help us
+ // understand when this point might be.
+ if (WiFiService::FixupServiceEntries(storage)) {
+ storage->Flush();
+ Metrics::ServiceFixupProfileType profile_type =
+ manager()->IsDefaultProfile(storage) ?
+ Metrics::kMetricServiceFixupDefaultProfile :
+ Metrics::kMetricServiceFixupUserProfile;
+ metrics()->SendEnumToUMA(
+ metrics()->GetFullMetricName(Metrics::kMetricServiceFixupEntries,
+ technology()),
+ profile_type,
+ Metrics::kMetricServiceFixupMax);
+ }
bool created_hidden_service = false;
set<string> groups = storage->GetGroupsWithKey(flimflam::kWifiHiddenSsid);
for (set<string>::iterator it = groups.begin(); it != groups.end(); ++it) {
diff --git a/wifi_service.cc b/wifi_service.cc
index feee026..73cc080 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -721,6 +721,34 @@
}
// static
+bool WiFiService::FixupServiceEntries(StoreInterface *storage) {
+ bool fixed_entry = false;
+ set<string> groups = storage->GetGroups();
+ for (set<string>::const_iterator it = groups.begin(); it != groups.end();
+ ++it) {
+ const string &id = *it;
+ string device_address, network_mode, security;
+ if (!ParseStorageIdentifier(id, &device_address,
+ &network_mode, &security)) {
+ continue;
+ }
+ if (!storage->GetString(id, kStorageType, NULL)) {
+ storage->SetString(id, kStorageType, flimflam::kTypeWifi);
+ fixed_entry = true;
+ }
+ if (!storage->GetString(id, kStorageMode, NULL)) {
+ storage->SetString(id, kStorageMode, network_mode);
+ fixed_entry = true;
+ }
+ if (!storage->GetString(id, kStorageSecurity, NULL)) {
+ storage->SetString(id, kStorageSecurity, security);
+ fixed_entry = true;
+ }
+ }
+ return fixed_entry;
+}
+
+// static
uint8 WiFiService::SignalToStrength(int16 signal_dbm) {
int16 strength;
if (signal_dbm > 0) {
diff --git a/wifi_service.h b/wifi_service.h
index b4ecf47..b052009 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -68,6 +68,11 @@
std::string *mode,
std::string *security);
+ // Iterate over |storage| looking for WiFi servces with "old-style"
+ // properties that don't include explicit type/mode/security, and add
+ // these properties. Returns true if any entries were fixed.
+ static bool FixupServiceEntries(StoreInterface *storage);
+
const std::string &mode() const { return mode_; }
const std::string &key_management() const { return GetEAPKeyManagement(); }
const std::vector<uint8_t> &ssid() const { return ssid_; }
diff --git a/wifi_service_unittest.cc b/wifi_service_unittest.cc
index 03aa47b..dfa0214 100644
--- a/wifi_service_unittest.cc
+++ b/wifi_service_unittest.cc
@@ -5,9 +5,11 @@
#include "shill/wifi_service.h"
#include <map>
+#include <set>
#include <string>
#include <vector>
+#include <base/stringprintf.h>
#include <base/string_util.h>
#include <chromeos/dbus/service_constants.h>
#include <gmock/gmock.h>
@@ -28,6 +30,7 @@
#include "shill/wpa_supplicant.h"
using std::map;
+using std::set;
using std::string;
using std::vector;
using ::testing::_;
@@ -39,6 +42,7 @@
using ::testing::SetArgumentPointee;
using ::testing::StrEq;
using ::testing::StrNe;
+using ::testing::StrictMock;
namespace shill {
@@ -220,6 +224,48 @@
const char *WiFiServiceUpdateFromEndpointsTest::kBadEndpointBssId =
"00:00:00:00:00:03";
+class WiFiServiceFixupStorageTest : public WiFiServiceTest {
+ protected:
+ void AddGroup(string group_name) {
+ groups_.insert(group_name);
+ }
+
+ void AddServiceEntry(bool has_type, bool has_mode, bool has_security) {
+ int index = groups_.size();
+ string id = base::StringPrintf("%s_%d_%d_%s_%s", flimflam::kTypeWifi,
+ index, index, flimflam::kModeManaged,
+ flimflam::kSecurityNone);
+ AddGroup(id);
+ EXPECT_CALL(store_, GetString(id, WiFiService::kStorageType, _))
+ .WillOnce(Return(has_type));
+ if (!has_type) {
+ EXPECT_CALL(store_, SetString(id, WiFiService::kStorageType,
+ flimflam::kTypeWifi));
+ }
+ EXPECT_CALL(store_, GetString(id, WiFiService::kStorageMode, _))
+ .WillOnce(Return(has_mode));
+ if (!has_mode) {
+ EXPECT_CALL(store_, SetString(id, WiFiService::kStorageMode,
+ flimflam::kModeManaged));
+ }
+ EXPECT_CALL(store_, GetString(id, WiFiService::kStorageSecurity, _))
+ .WillOnce(Return(has_security));
+ if (!has_security) {
+ EXPECT_CALL(store_, SetString(id, WiFiService::kStorageSecurity,
+ flimflam::kSecurityNone));
+ }
+ }
+
+ bool FixupServiceEntries() {
+ EXPECT_CALL(store_, GetGroups()).WillOnce(Return(groups_));
+ return WiFiService::FixupServiceEntries(&store_);
+ }
+
+ private:
+ StrictMock<MockStore> store_;
+ set<string> groups_;
+};
+
TEST_F(WiFiServiceTest, StorageId) {
vector<uint8_t> ssid(5);
ssid.push_back(0xff);
@@ -926,6 +972,45 @@
EXPECT_EQ(flimflam::kSecurity8021x, security);
}
+TEST_F(WiFiServiceFixupStorageTest, FixedEntries) {
+ const string kNonWiFiId = "vpn_foo";
+ const string kUnparsableWiFiId = "wifi_foo";
+
+ AddGroup(kNonWiFiId);
+ AddGroup(kUnparsableWiFiId);
+ AddServiceEntry(true, true, true);
+ AddServiceEntry(false, false, false);
+ AddServiceEntry(true, true, true);
+ AddServiceEntry(false, false, false);
+ EXPECT_TRUE(FixupServiceEntries());
+}
+
+TEST_F(WiFiServiceFixupStorageTest, NoFixedEntries) {
+ const string kNonWiFiId = "vpn_foo";
+ const string kUnparsableWiFiId = "wifi_foo";
+
+ AddGroup(kNonWiFiId);
+ AddGroup(kUnparsableWiFiId);
+ AddServiceEntry(true, true, true);
+ EXPECT_FALSE(FixupServiceEntries());
+}
+
+TEST_F(WiFiServiceFixupStorageTest, MissingTypeProperty) {
+ AddServiceEntry(false, true, true);
+ EXPECT_TRUE(FixupServiceEntries());
+}
+
+TEST_F(WiFiServiceFixupStorageTest, MissingModeProperty) {
+ AddServiceEntry(true, false, true);
+ EXPECT_TRUE(FixupServiceEntries());
+}
+
+TEST_F(WiFiServiceFixupStorageTest, MissingSecurityProperty) {
+ AddServiceEntry(true, true, false);
+ EXPECT_TRUE(FixupServiceEntries());
+}
+
+
TEST_F(WiFiServiceTest, Connectable) {
// Open network should be connectable.
EXPECT_TRUE(CheckConnectable(flimflam::kSecurityNone, NULL, NULL));
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index 6e26070..2edcf76 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -55,6 +55,7 @@
#include "shill/nice_mock_control.h"
#include "shill/property_store_unittest.h"
#include "shill/proxy_factory.h"
+#include "shill/technology.h"
#include "shill/wifi_endpoint.h"
#include "shill/wifi_service.h"
#include "shill/wpa_supplicant.h"
@@ -531,6 +532,10 @@
.WillRepeatedly(DoAll(SetArgumentPointee<2>(true), Return(true)));
EXPECT_CALL(*storage, GetString(StrEq(*id), flimflam::kSSIDProperty, _))
.WillRepeatedly(DoAll(SetArgumentPointee<2>(hex_ssid), Return(true)));
+
+ // For embedded WiFiService::FixupServiceEntries call.
+ EXPECT_CALL(*storage, GetGroups()).WillOnce(Return(set<string>()));
+
}
WiFiService *SetupConnectedService(const DBus::Path &network_path) {
@@ -610,6 +615,10 @@
return &control_interface_;
}
+ MockMetrics *metrics() {
+ return &metrics_;
+ }
+
MockManager *manager() {
return &manager_;
}
@@ -1886,11 +1895,48 @@
TEST_F(WiFiMainTest, LoadHiddenServicesFailWithNoGroups) {
StrictMock<MockStore> storage;
+ EXPECT_CALL(storage, GetGroups()).WillOnce(Return(set<string>()));
+ EXPECT_CALL(*metrics(), SendEnumToUMA(_, _, _)).Times(0);
EXPECT_CALL(storage, GetGroupsWithKey(flimflam::kWifiHiddenSsid))
.WillOnce(Return(set<string>()));
EXPECT_FALSE(LoadHiddenServices(&storage));
}
+TEST_F(WiFiMainTest, LoadHiddenServicesWithFixedUpServices) {
+ StrictMock<MockStore> storage;
+ EXPECT_CALL(*manager(), IsDefaultProfile(&storage))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*metrics(), SendEnumToUMA(
+ "Network.Shill.Wifi.ServiceFixupEntries",
+ Metrics::kMetricServiceFixupDefaultProfile,
+ Metrics::kMetricServiceFixupMax)).Times(1);
+ EXPECT_CALL(storage, Flush()).Times(1);
+ const string kGroupId =
+ StringPrintf("%s_%s_0_%s_%s",
+ flimflam::kTypeWifi,
+ kDeviceAddress,
+ flimflam::kModeManaged,
+ flimflam::kSecurityNone);
+ EXPECT_CALL(storage, GetString(kGroupId, _, _)).WillRepeatedly(Return(false));
+ EXPECT_CALL(storage, SetString(kGroupId, _, _)).WillRepeatedly(Return(true));
+ set<string> groups;
+ groups.insert(kGroupId);
+ EXPECT_CALL(storage, GetGroups()).WillRepeatedly(Return(groups));
+ EXPECT_CALL(storage, GetGroupsWithKey(flimflam::kWifiHiddenSsid))
+ .WillRepeatedly(Return(set<string>()));
+ EXPECT_FALSE(LoadHiddenServices(&storage));
+ Mock::VerifyAndClearExpectations(metrics());
+
+ EXPECT_CALL(*manager(), IsDefaultProfile(&storage))
+ .WillOnce(Return(false));
+ EXPECT_CALL(*metrics(), SendEnumToUMA(
+ "Network.Shill.Wifi.ServiceFixupEntries",
+ Metrics::kMetricServiceFixupUserProfile,
+ Metrics::kMetricServiceFixupMax)).Times(1);
+ EXPECT_CALL(storage, Flush()).Times(1);
+ EXPECT_FALSE(LoadHiddenServices(&storage));
+}
+
TEST_F(WiFiMainTest, LoadHiddenServicesFailWithMissingHidden) {
string id;
StrictMock<MockStore> storage;