shill: use profile order in service sorting
Add the ability to sort services based on their loaded profiles.
Loaded profiles should be ranked above ephemeral, then base sort
order on profile priority. This allows user-specific networks to
be preferred over device-wide networks.
BUG=chromium:266107
TEST=Ran shill unit tests
Change-Id: I6412d16e0b5fdb5ece1ae72e17405e3a7cfc0b4a
Reviewed-on: https://chromium-review.googlesource.com/208462
Tested-by: Rebecca Silberstein <silberst@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Commit-Queue: Rebecca Silberstein <silberst@chromium.org>
diff --git a/manager.cc b/manager.cc
index 3961349..6faeabe 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1347,7 +1347,7 @@
}
const bool kCompareConnectivityState = true;
sort(services_.begin(), services_.end(),
- ServiceSorter(kCompareConnectivityState, technology_order_));
+ ServiceSorter(this, kCompareConnectivityState, technology_order_));
if (!services_.empty()) {
ConnectionRefPtr default_connection = default_service->connection();
@@ -1438,8 +1438,9 @@
if (i + 1 < services_.size()) {
const bool kCompareConnectivityState = true;
Service::Compare(
- service, services_[i+1], kCompareConnectivityState,
- technology_order_, &compare_reason);
+ this, service, services_[i+1],
+ kCompareConnectivityState, technology_order_,
+ &compare_reason);
} else {
compare_reason = "last";
}
@@ -1483,7 +1484,7 @@
vector<ServiceRefPtr> services_copy = services_;
const bool kCompareConnectivityState = false;
sort(services_copy.begin(), services_copy.end(),
- ServiceSorter(kCompareConnectivityState, technology_order_));
+ ServiceSorter(this, kCompareConnectivityState, technology_order_));
set<Technology::Identifier> connecting_technologies;
for (const auto &service : services_copy) {
if (!service->connectable()) {
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 35dfeea..424f317 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -2330,6 +2330,34 @@
manager()->UpdateService(mock_service10);
EXPECT_TRUE(ServiceOrderIs(mock_service10, mock_service2));
+
+ // Sort services based on profiles. When comparing two services
+ // with different profiles, prefer the one that is not ephemeral.
+ // If both services have legitimate profiles, prefer the service
+ // with the more recently applied profile.
+ // Compare between an ephemeral and actual profile.
+ Error error;
+ scoped_refptr<MockProfile> default_profile(
+ new MockProfile(control_interface(), metrics(), manager(), ""));
+ string default_profile_name("/profile/default");
+ EXPECT_CALL(*default_profile, GetRpcIdentifier())
+ .WillRepeatedly(Return(default_profile_name));
+ AdoptProfile(manager(), default_profile);
+ mock_service2->set_profile(default_profile);
+ manager()->UpdateService(mock_service2);
+ EXPECT_TRUE(ServiceOrderIs(mock_service2, mock_service10));
+
+ // Compare between two different profiles
+ scoped_refptr<MockProfile> user_profile(
+ new MockProfile(control_interface(), metrics(), manager(), ""));
+ string user_profile_name("/profile/chonos/shill");
+ EXPECT_CALL(*user_profile, GetRpcIdentifier())
+ .WillRepeatedly(Return(user_profile_name));
+ AdoptProfile(manager(), user_profile);
+ mock_service10->set_profile(user_profile);
+ manager()->UpdateService(mock_service10);
+ EXPECT_TRUE(ServiceOrderIs(mock_service10, mock_service2));
+
// Security
mock_service2->SetSecurity(Service::kCryptoAes, true, true);
manager()->UpdateService(mock_service2);
@@ -2341,7 +2369,6 @@
EXPECT_CALL(*mock_service10.get(), technology())
.WillRepeatedly(Return(Technology::kEthernet));
- Error error;
// Default technology ordering should favor Ethernet over WiFi.
manager()->SortServicesTask();
EXPECT_TRUE(ServiceOrderIs(mock_service10, mock_service2));
diff --git a/service.cc b/service.cc
index 7260e5b..108cfaf 100644
--- a/service.cc
+++ b/service.cc
@@ -69,7 +69,9 @@
const char Service::kServiceSortIsFailed[] = "IsFailed";
const char Service::kServiceSortIsPortalled[] = "IsPortal";
const char Service::kServiceSortPriority[] = "Priority";
-const char Service::kServiceSortSecurityEtc[] = "SecurityEtc";
+const char Service::kServiceSortSecurity[] = "Security";
+const char Service::kServiceSortProfileOrder[] = "ProfileOrder";
+const char Service::kServiceSortEtc[] = "Etc";
const char Service::kServiceSortSerialNumber[] = "SerialNumber";
const char Service::kServiceSortTechnology[] = "Technology";
@@ -968,7 +970,8 @@
}
// static
-bool Service::Compare(ServiceRefPtr a,
+bool Service::Compare(Manager *manager,
+ ServiceRefPtr a,
ServiceRefPtr b,
bool compare_connectivity_state,
const vector<Technology::Identifier> &tech_order,
@@ -1040,9 +1043,30 @@
}
}
- if (DecideBetween(a->SecurityLevel(), b->SecurityLevel(), &ret) ||
- DecideBetween(a->strength(), b->strength(), &ret)) {
- *reason = kServiceSortSecurityEtc;
+ if (DecideBetween(a->SecurityLevel(), b->SecurityLevel(), &ret)) {
+ *reason = kServiceSortSecurity;
+ return ret;
+ }
+
+ // If the profiles for the two services are different,
+ // we want to pick the highest priority one. The
+ // ephemeral profile is explicitly tested for since it is not
+ // listed in the manager profiles_ list.
+ if (a->profile() != b->profile()) {
+ *reason = kServiceSortProfileOrder;
+ if (manager->IsServiceEphemeral(b)) {
+ return true;
+ } else if (manager->IsServiceEphemeral(a)) {
+ return false;
+ } else if (manager->IsProfileBefore(b->profile(), a->profile())) {
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+ if (DecideBetween(a->strength(), b->strength(), &ret)) {
+ *reason = kServiceSortEtc;
return ret;
}
diff --git a/service.h b/service.h
index f63d108..5f5dc1c 100644
--- a/service.h
+++ b/service.h
@@ -402,7 +402,8 @@
// |tech_order| to rank services if more decisive criteria do not yield a
// difference. |reason| is populated with the exact criteria used for the
// ultimate comparison.
- static bool Compare(ServiceRefPtr a,
+ static bool Compare(Manager *manager,
+ ServiceRefPtr a,
ServiceRefPtr b,
bool compare_connectivity_state,
const std::vector<Technology::Identifier> &tech_order,
@@ -673,7 +674,9 @@
static const char kServiceSortIsFailed[];
static const char kServiceSortIsPortalled[];
static const char kServiceSortPriority[];
- static const char kServiceSortSecurityEtc[];
+ static const char kServiceSortSecurity[];
+ static const char kServiceSortProfileOrder[];
+ static const char kServiceSortEtc[];
static const char kServiceSortSerialNumber[];
static const char kServiceSortTechnology[];
diff --git a/service_sorter.h b/service_sorter.h
index 2bdf4ef..1fd9545 100644
--- a/service_sorter.h
+++ b/service_sorter.h
@@ -20,17 +20,20 @@
// compare two Service objects at a time.
class ServiceSorter {
public:
- ServiceSorter(bool compare_connectivity_state,
+ ServiceSorter(Manager *manager,
+ bool compare_connectivity_state,
const std::vector<Technology::Identifier> &tech_order)
- : compare_connectivity_state_(compare_connectivity_state),
+ : manager_(manager),
+ compare_connectivity_state_(compare_connectivity_state),
technology_order_(tech_order) {}
bool operator() (ServiceRefPtr a, ServiceRefPtr b) {
const char *reason;
- return Service::Compare(a, b, compare_connectivity_state_,
+ return Service::Compare(manager_, a, b, compare_connectivity_state_,
technology_order_, &reason);
}
private:
+ Manager *manager_;
const bool compare_connectivity_state_;
const std::vector<Technology::Identifier> &technology_order_;
// We can't DISALLOW_COPY_AND_ASSIGN since this is passed by value to STL