update_engine: Add disconnected network type for metrics reporting
On a failed update, correctly report that the network is disconnected
rather than reporting an "unknown" network type, which is set when the
connection type cannot be determined based on the properties of a given
service path.
BUG=chromium:660283
TEST=Added relevant unittests. Also tested manually by updating via cros
flash, unplugging Ethernet after the transfer, and killing the
dev_server. Metrics sends "disconnected" as expected.
Change-Id: Ia681b769208b67fb5c14d41a646d816a42ccf33e
Reviewed-on: https://chromium-review.googlesource.com/1231700
Commit-Ready: Colin Howes <[email protected]>
Tested-by: Colin Howes <[email protected]>
Reviewed-by: Amin Hassani <[email protected]>
diff --git a/connection_manager.cc b/connection_manager.cc
index 4063f24..a048f5f 100644
--- a/connection_manager.cc
+++ b/connection_manager.cc
@@ -138,8 +138,11 @@
if (!default_service_path.IsValid())
return false;
// Shill uses the "/" service path to indicate that it is not connected.
- if (default_service_path.value() == "/")
- return false;
+ if (default_service_path.value() == "/") {
+ *out_type = ConnectionType::kDisconnected;
+ *out_tethering = ConnectionTethering::kUnknown;
+ return true;
+ }
TEST_AND_RETURN_FALSE(
GetServicePathProperties(default_service_path, out_type, out_tethering));
return true;
diff --git a/connection_manager.h b/connection_manager.h
index dc563ef..d8527a3 100644
--- a/connection_manager.h
+++ b/connection_manager.h
@@ -47,8 +47,8 @@
bool IsAllowedConnectionTypesForUpdateSet() const override;
private:
- // Returns (via out_path) the default network path, or empty string if
- // there's no network up. Returns true on success.
+ // Returns (via out_path) the default network path, or "/" if there's no
+ // network up. Returns true on success.
bool GetDefaultServicePath(dbus::ObjectPath* out_path);
bool GetServicePathProperties(const dbus::ObjectPath& path,
diff --git a/connection_manager_unittest.cc b/connection_manager_unittest.cc
index 85b8c57..7cd858d 100644
--- a/connection_manager_unittest.cc
+++ b/connection_manager_unittest.cc
@@ -75,6 +75,9 @@
const char* service_type,
const char* physical_technology,
ConnectionType expected_type);
+
+ void TestWithServiceDisconnected(ConnectionType expected_type);
+
void TestWithServiceTethering(
const char* service_tethering,
ConnectionTethering expected_tethering);
@@ -170,6 +173,18 @@
fake_shill_proxy_->GetManagerProxy());
}
+void ConnectionManagerTest::TestWithServiceDisconnected(
+ ConnectionType expected_type) {
+ SetManagerReply("/", true);
+
+ ConnectionType type;
+ ConnectionTethering tethering;
+ EXPECT_TRUE(cmut_.GetConnectionProperties(&type, &tethering));
+ EXPECT_EQ(expected_type, type);
+ testing::Mock::VerifyAndClearExpectations(
+ fake_shill_proxy_->GetManagerProxy());
+}
+
TEST_F(ConnectionManagerTest, SimpleTest) {
TestWithServiceType(shill::kTypeEthernet, nullptr, ConnectionType::kEthernet);
TestWithServiceType(shill::kTypeWifi, nullptr, ConnectionType::kWifi);
@@ -203,6 +218,10 @@
TestWithServiceType("foo", nullptr, ConnectionType::kUnknown);
}
+TEST_F(ConnectionManagerTest, DisconnectTest) {
+ TestWithServiceDisconnected(ConnectionType::kDisconnected);
+}
+
TEST_F(ConnectionManagerTest, AllowUpdatesOverEthernetTest) {
// Updates over Ethernet are allowed even if there's no policy.
EXPECT_TRUE(cmut_.IsUpdateAllowedOver(ConnectionType::kEthernet,
diff --git a/connection_utils.cc b/connection_utils.cc
index 9b6b526..aeb0163 100644
--- a/connection_utils.cc
+++ b/connection_utils.cc
@@ -18,6 +18,12 @@
#include <shill/dbus-constants.h>
+namespace {
+// Not defined by shill since we don't use this outside of UE.
+constexpr char kTypeDisconnected[] = "Disconnected";
+constexpr char kTypeUnknown[] = "Unknown";
+} // namespace
+
namespace chromeos_update_engine {
namespace connection_utils {
@@ -32,6 +38,8 @@
return ConnectionType::kBluetooth;
} else if (type_str == shill::kTypeCellular) {
return ConnectionType::kCellular;
+ } else if (type_str == kTypeDisconnected) {
+ return ConnectionType::kDisconnected;
}
return ConnectionType::kUnknown;
}
@@ -59,10 +67,12 @@
return shill::kTypeBluetooth;
case ConnectionType::kCellular:
return shill::kTypeCellular;
+ case ConnectionType::kDisconnected:
+ return kTypeDisconnected;
case ConnectionType::kUnknown:
- return "Unknown";
+ return kTypeUnknown;
}
- return "Unknown";
+ return kTypeUnknown;
}
} // namespace connection_utils
diff --git a/connection_utils.h b/connection_utils.h
index e385517..d5133a1 100644
--- a/connection_utils.h
+++ b/connection_utils.h
@@ -22,6 +22,7 @@
namespace chromeos_update_engine {
enum class ConnectionType {
+ kDisconnected,
kEthernet,
kWifi,
kWimax,
diff --git a/metrics_constants.h b/metrics_constants.h
index abec2ad..eabb8fb 100644
--- a/metrics_constants.h
+++ b/metrics_constants.h
@@ -107,14 +107,15 @@
//
// This is used in the UpdateEngine.Attempt.ConnectionType histogram.
enum class ConnectionType {
- kUnknown, // Unknown.
- kEthernet, // Ethernet.
- kWifi, // Wireless.
- kWimax, // WiMax.
- kBluetooth, // Bluetooth.
- kCellular, // Cellular.
- kTetheredEthernet, // Tethered (Ethernet).
- kTetheredWifi, // Tethered (Wifi).
+ kUnknown = 0, // Unknown.
+ kEthernet = 1, // Ethernet.
+ kWifi = 2, // Wireless.
+ kWimax = 3, // WiMax.
+ kBluetooth = 4, // Bluetooth.
+ kCellular = 5, // Cellular.
+ kTetheredEthernet = 6, // Tethered (Ethernet).
+ kTetheredWifi = 7, // Tethered (Wifi).
+ kDisconnected = 8, // Disconnected.
kNumConstants,
kUnset = -1
diff --git a/metrics_utils.cc b/metrics_utils.cc
index b85f257..018f2e4 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -247,6 +247,9 @@
case ConnectionType::kUnknown:
return metrics::ConnectionType::kUnknown;
+ case ConnectionType::kDisconnected:
+ return metrics::ConnectionType::kDisconnected;
+
case ConnectionType::kEthernet:
if (tethering == ConnectionTethering::kConfirmed)
return metrics::ConnectionType::kTetheredEthernet;
diff --git a/metrics_utils_unittest.cc b/metrics_utils_unittest.cc
index edf6bc3..417a72b 100644
--- a/metrics_utils_unittest.cc
+++ b/metrics_utils_unittest.cc
@@ -32,6 +32,9 @@
EXPECT_EQ(metrics::ConnectionType::kUnknown,
GetConnectionType(ConnectionType::kUnknown,
ConnectionTethering::kUnknown));
+ EXPECT_EQ(metrics::ConnectionType::kDisconnected,
+ GetConnectionType(ConnectionType::kDisconnected,
+ ConnectionTethering::kUnknown));
EXPECT_EQ(metrics::ConnectionType::kEthernet,
GetConnectionType(ConnectionType::kEthernet,
ConnectionTethering::kUnknown));
diff --git a/update_manager/boxed_value_unittest.cc b/update_manager/boxed_value_unittest.cc
index 99d578c..212db36 100644
--- a/update_manager/boxed_value_unittest.cc
+++ b/update_manager/boxed_value_unittest.cc
@@ -161,6 +161,9 @@
}
TEST(UmBoxedValueTest, ConnectionTypeToString) {
+ EXPECT_EQ(
+ "Disconnected",
+ BoxedValue(new ConnectionType(ConnectionType::kDisconnected)).ToString());
EXPECT_EQ("ethernet",
BoxedValue(new ConnectionType(ConnectionType::kEthernet))
.ToString());
