shill: Fixes disconnect UMA stat
To determine the kernel's disconnect notification behaviour for these
UMA stats, I originally reverse-engineered the disconnect netlink
messages that we get from the kernel (should have looked at the kernel
code which, eventually, I did). I found, erroneously, that all of the
disconnect reasons were found in the disassociate message. It turns out
that the disconnect reasons are from the disassociate message only for
(some!) disconnections instigated by the station. When the AP is the
source of the disconnect, the reason is stored in a disconnect message.
This code, then, extracts the reason code from whatever message type
(disconnect or disassociate) that contains one. The message will still
be a little noisy since some station-started disconnections don't
provide a disassociate message and I've noticed that some AP-started
disconnections seem to send 2 messages. I haven't tracked any of this
down.
BUG=chromium:215808
TEST=unittest (new) and manual - I test using the AP and description
found in chromium:294315.
CQ-DEPEND=Icf79aa729b1ed125743686c4536fe1b59183fed2
Change-Id: If648d530c613f485c074acf58ddb0bca4de22084
Reviewed-on: https://chromium-review.googlesource.com/170926
Reviewed-by: Wade Guthrie <wdg@chromium.org>
Commit-Queue: Wade Guthrie <wdg@chromium.org>
Tested-by: Wade Guthrie <wdg@chromium.org>
diff --git a/callback80211_metrics.cc b/callback80211_metrics.cc
index dedf2ab..7ce42ed 100644
--- a/callback80211_metrics.cc
+++ b/callback80211_metrics.cc
@@ -12,38 +12,80 @@
namespace shill {
-Callback80211Metrics::Callback80211Metrics(
- const NetlinkManager &netlink_manager, Metrics *metrics)
+Callback80211Metrics::Callback80211Metrics(Metrics *metrics)
: metrics_(metrics) {}
+IEEE_80211::WiFiReasonCode Callback80211Metrics::WiFiReasonCodeFromUint16(
+ uint16_t reason) const {
+ IEEE_80211::WiFiReasonCode reason_enum = IEEE_80211::kReasonCodeInvalid;
+ if (reason == IEEE_80211::kReasonCodeReserved0 ||
+ reason == IEEE_80211::kReasonCodeReserved12 ||
+ (reason >= IEEE_80211::kReasonCodeReservedBegin25 &&
+ reason <= IEEE_80211::kReasonCodeReservedEnd31) ||
+ (reason >= IEEE_80211::kReasonCodeReservedBegin40 &&
+ reason <= IEEE_80211::kReasonCodeReservedEnd44) ||
+ reason >= IEEE_80211::kReasonCodeMax) {
+ SLOG(WiFi, 1) << "Invalid reason code in disconnect message";
+ reason_enum = IEEE_80211::kReasonCodeInvalid;
+ } else {
+ reason_enum = static_cast<IEEE_80211::WiFiReasonCode>(reason);
+ }
+ return reason_enum;
+}
+
void Callback80211Metrics::CollectDisconnectStatistics(
const NetlinkMessage &netlink_message) {
- // We only handle deauthenticate messages, which are nl80211 messages.
+ if (!metrics_) {
+ return;
+ }
+ // We only handle disconnect and deauthenticate messages, both of which are
+ // nl80211 messages.
if (netlink_message.message_type() != Nl80211Message::GetMessageType()) {
return;
}
const Nl80211Message &message =
* reinterpret_cast<const Nl80211Message *>(&netlink_message);
- if (metrics_ &&
- message.command() == DeauthenticateMessage::kCommand) {
+ // Station-instigated disconnects provide their information in the
+ // deauthenticate message but AP-instigated disconnects provide it in the
+ // disconnect message.
+ uint16_t reason = IEEE_80211::kReasonCodeUnspecified;
+ if (message.command() == DeauthenticateMessage::kCommand) {
SLOG(WiFi, 3) << "Handling Deauthenticate Message";
- Metrics::WiFiDisconnectByWhom by_whom =
- message.const_attributes()->IsFlagAttributeTrue(
- NL80211_ATTR_DISCONNECTED_BY_AP) ?
- Metrics::kDisconnectedByAp : Metrics::kDisconnectedNotByAp;
- uint16_t reason = static_cast<uint16_t>(
- IEEE_80211::kReasonCodeInvalid);
+ message.Print(3, 3);
+ // If there's no frame, this is probably an AP-caused disconnect and
+ // there'll be a disconnect message to tell us about that.
ByteString rawdata;
- if (message.const_attributes()->GetRawAttributeValue(NL80211_ATTR_FRAME,
+ if (!message.const_attributes()->GetRawAttributeValue(NL80211_ATTR_FRAME,
&rawdata)) {
- Nl80211Frame frame(rawdata);
- reason = frame.reason();
+ SLOG(WiFi, 5) << "No frame in deauthenticate message, ignoring";
+ return;
}
- IEEE_80211::WiFiReasonCode reason_enum =
- static_cast<IEEE_80211::WiFiReasonCode>(reason);
- metrics_->Notify80211Disconnect(by_whom, reason_enum);
+ Nl80211Frame frame(rawdata);
+ reason = frame.reason();
+ } else if (message.command() == DisconnectMessage::kCommand) {
+ SLOG(WiFi, 3) << "Handling Disconnect Message";
+ message.Print(3, 3);
+ // If there's no reason code, this is probably a STA-caused disconnect and
+ // there was be a disconnect message to tell us about that.
+ if (!message.const_attributes()->GetU16AttributeValue(
+ NL80211_ATTR_REASON_CODE, &reason)) {
+ SLOG(WiFi, 5) << "No reason code in disconnect message, ignoring";
+ return;
+ }
+ } else {
+ return;
}
+
+ IEEE_80211::WiFiReasonCode reason_enum = WiFiReasonCodeFromUint16(reason);
+
+ Metrics::WiFiDisconnectByWhom by_whom =
+ message.const_attributes()->IsFlagAttributeTrue(
+ NL80211_ATTR_DISCONNECTED_BY_AP) ? Metrics::kDisconnectedByAp :
+ Metrics::kDisconnectedNotByAp;
+ SLOG(WiFi, 1) << "Notify80211Disconnect by " << (by_whom ? "station" : "AP")
+ << " because:" << reason_enum;
+ metrics_->Notify80211Disconnect(by_whom, reason_enum);
}
} // namespace shill.
diff --git a/callback80211_metrics.h b/callback80211_metrics.h
index 01311cf..0bb80d5 100644
--- a/callback80211_metrics.h
+++ b/callback80211_metrics.h
@@ -11,6 +11,7 @@
#include <base/basictypes.h>
#include <base/memory/weak_ptr.h>
+#include "shill/ieee80211.h"
#include "shill/netlink_manager.h"
namespace shill {
@@ -23,7 +24,7 @@
class Callback80211Metrics :
public base::SupportsWeakPtr<Callback80211Metrics> {
public:
- Callback80211Metrics(const NetlinkManager &netlink_manager, Metrics *metrics);
+ explicit Callback80211Metrics(Metrics *metrics);
// Called with each broadcast netlink message that arrives to NetlinkManager.
// If the message is a deauthenticate message, the method collects the reason
@@ -33,6 +34,8 @@
private:
static const char kMetricLinkDisconnectCount[];
+ IEEE_80211::WiFiReasonCode WiFiReasonCodeFromUint16(uint16_t reason) const;
+
Metrics *metrics_;
DISALLOW_COPY_AND_ASSIGN(Callback80211Metrics);
diff --git a/callback80211_metrics_unittest.cc b/callback80211_metrics_unittest.cc
new file mode 100644
index 0000000..574ffba
--- /dev/null
+++ b/callback80211_metrics_unittest.cc
@@ -0,0 +1,198 @@
+// Copyright (c) 2013 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.
+
+// This file provides tests to verify that Callback80211Metrics sends UMA
+// notifications for appropriate messages and doesn't send them for
+// inappropriate messages.
+
+#include "shill/callback80211_metrics.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "shill/ieee80211.h"
+#include "shill/mock_event_dispatcher.h"
+#include "shill/mock_log.h"
+#include "shill/mock_metrics.h"
+#include "shill/nl80211_message.h"
+#include "shill/refptr_types.h"
+
+using base::Bind;
+using testing::_;
+using testing::Test;
+
+namespace shill {
+
+namespace {
+
+// Unless otherwise specified, these data blocks have been collected by shill
+// using NetlinkManager while, simultaneously (and manually) comparing shill
+// output with that of the 'iw' code from which it was derived. The test
+// strings represent the raw packet data coming from the kernel. The
+// comments above each of these strings is the markup that 'iw' outputs for
+// each of these packets.
+
+// These constants are consistent across the applicable packets, below.
+
+const uint16_t kNl80211FamilyId = 0x13;
+const IEEE_80211::WiFiReasonCode kExpectedDisconnectReason =
+ IEEE_80211::kReasonCodePreviousAuthenticationInvalid;
+
+// NL80211_CMD_DISCONNECT message.
+// wlan0 (phy #0): disconnected (by AP) reason: 2: Previous authentication no
+// longer valid
+
+const unsigned char kDisconnectMessage[] = {
+ 0x30, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x30, 0x01, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x03, 0x00,
+ 0x04, 0x00, 0x00, 0x00, 0x06, 0x00, 0x36, 0x00,
+ 0x02, 0x00, 0x00, 0x00, 0x04, 0x00, 0x47, 0x00,
+};
+
+// NL80211_CMD_DISCONNECT message.
+// Copied from kDisconnectMessage but with most of the payload removed.
+
+const unsigned char kEmptyDisconnectMessage[] = {
+ 0x30, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x30, 0x01, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00,
+ 0x00, 0x00, 0x00, 0x00,
+};
+
+// NL80211_CMD_DEAUTHENTICATE message.
+// wlan0 (phy #0): deauth c0:3f:0e:77:e8:7f -> ff:ff:ff:ff:ff:ff reason 2:
+// Previous authentication no longer valid [frame: c0 00 00 00 ff ff ff ff
+// ff ff c0 3f 0e 77 e8 7f c0 3f 0e 77 e8 7f c0 0e 02 00]
+
+const unsigned char kDeauthenticateMessage[] = {
+ 0x44, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x27, 0x01, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x03, 0x00,
+ 0x04, 0x00, 0x00, 0x00, 0x1e, 0x00, 0x33, 0x00,
+ 0xc0, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xc0, 0x3f, 0x0e, 0x77, 0xe8, 0x7f,
+ 0xc0, 0x3f, 0x0e, 0x77, 0xe8, 0x7f, 0xc0, 0x0e,
+ 0x02, 0x00, 0x00, 0x00,
+};
+
+// NL80211_CMD_DEAUTHENTICATE message.
+// Copied from kDeauthenticateMessage but with most of the payload
+// removed.
+
+const unsigned char kEmptyDeauthenticateMessage[] = {
+ 0x44, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x27, 0x01, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00,
+ 0x00, 0x00, 0x00, 0x00,
+};
+
+// NL80211_CMD_NEW_STATION message.
+// kNewStationMessage is an nl80211 message that's not a deauthenticate or
+// disconnect message. Used to make sure that only those nl80211 messages
+// generate an UMA message.
+// wlan0: new station c0:3f:0e:77:e8:7f
+
+const unsigned char kNewStationMessage[] = {
+ 0x34, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x13, 0x01, 0x00, 0x00, 0x08, 0x00, 0x03, 0x00,
+ 0x04, 0x00, 0x00, 0x00, 0x0a, 0x00, 0x06, 0x00,
+ 0xc0, 0x3f, 0x0e, 0x77, 0xe8, 0x7f, 0x00, 0x00,
+ 0x08, 0x00, 0x2e, 0x00, 0x13, 0x01, 0x00, 0x00,
+ 0x04, 0x00, 0x15, 0x00,
+};
+
+// CTRL_CMD_GETFAMILY message.
+// kGetFamilyMessage is not an nl80211 message. Used to make sure that
+// non-nl80211 messages don't generate an UMA message.
+//
+// Extracted from net.log. It's just a non-nl80211 message (it's actually a
+// message that's sent to the kernel rather than one received from the kernel
+// but the code doesn't differentiate and this message was much shorter than the
+// response).
+
+const unsigned char kGetFamilyMessage[] = {
+ 0x03, 0x01, 0x00, 0x00, 0x0c, 0x00, 0x02, 0x00,
+ 0x6e, 0x6c, 0x38, 0x30, 0x32, 0x31, 0x31, 0x00
+};
+
+} // namespace
+
+class Callback80211MetricsTest : public Test {
+ public:
+ Callback80211MetricsTest() :
+ metrics_(&dispatcher_), callback_(&metrics_) {
+ message_factory_.AddFactoryMethod(
+ kNl80211FamilyId, Bind(&Nl80211Message::CreateMessage));
+ Nl80211Message::SetMessageType(kNl80211FamilyId);
+ }
+
+ protected:
+ MockEventDispatcher dispatcher_;
+ MockMetrics metrics_;
+ NetlinkMessageFactory message_factory_;
+ Callback80211Metrics callback_;
+};
+
+// Make sure that notifications happen for correctly formed messages.
+TEST_F(Callback80211MetricsTest, DisconnectMessage) {
+ scoped_ptr<NetlinkMessage> netlink_message(
+ message_factory_.CreateMessage(const_cast<nlmsghdr *>(
+ reinterpret_cast<const nlmsghdr *>(kDisconnectMessage))));
+ EXPECT_CALL(metrics_, Notify80211Disconnect(Metrics::kDisconnectedByAp,
+ kExpectedDisconnectReason));
+ callback_.CollectDisconnectStatistics(*netlink_message);
+}
+
+TEST_F(Callback80211MetricsTest, DeauthMessage) {
+ scoped_ptr<NetlinkMessage> netlink_message(
+ message_factory_.CreateMessage(const_cast<nlmsghdr *>(
+ reinterpret_cast<const nlmsghdr *>(kDeauthenticateMessage))));
+ EXPECT_CALL(metrics_, Notify80211Disconnect(Metrics::kDisconnectedNotByAp,
+ kExpectedDisconnectReason));
+ callback_.CollectDisconnectStatistics(*netlink_message);
+}
+
+// Make sure there's no notification if there's no reason code in the message.
+TEST_F(Callback80211MetricsTest, EmptyDisconnectMessage) {
+ scoped_ptr<NetlinkMessage> netlink_message(
+ message_factory_.CreateMessage(const_cast<nlmsghdr *>(
+ reinterpret_cast<const nlmsghdr *>(
+ kEmptyDisconnectMessage))));
+ EXPECT_CALL(metrics_, Notify80211Disconnect(_, _)).Times(0);
+ callback_.CollectDisconnectStatistics(*netlink_message);
+}
+
+TEST_F(Callback80211MetricsTest, EmptyDeauthMessage) {
+ scoped_ptr<NetlinkMessage> netlink_message(
+ message_factory_.CreateMessage(const_cast<nlmsghdr *>(
+ reinterpret_cast<const nlmsghdr *>(
+ kEmptyDeauthenticateMessage))));
+ EXPECT_CALL(metrics_, Notify80211Disconnect(_, _)).Times(0);
+ callback_.CollectDisconnectStatistics(*netlink_message);
+}
+
+// Make sure the callback doesn't notify anyone for message of the wrong type.
+TEST_F(Callback80211MetricsTest, Nl80211NotDisconnectDeauthMessage) {
+ scoped_ptr<NetlinkMessage> netlink_message(
+ message_factory_.CreateMessage(const_cast<nlmsghdr *>(
+ reinterpret_cast<const nlmsghdr *>(
+ kNewStationMessage))));
+ EXPECT_CALL(metrics_, Notify80211Disconnect(_, _)).Times(0);
+ callback_.CollectDisconnectStatistics(*netlink_message);
+}
+
+TEST_F(Callback80211MetricsTest, NotNl80211Message) {
+ scoped_ptr<NetlinkMessage> netlink_message(
+ message_factory_.CreateMessage(const_cast<nlmsghdr *>(
+ reinterpret_cast<const nlmsghdr *>(
+ kGetFamilyMessage))));
+ EXPECT_CALL(metrics_, Notify80211Disconnect(_, _)).Times(0);
+ callback_.CollectDisconnectStatistics(*netlink_message);
+}
+
+} // namespace shill
diff --git a/ieee80211.h b/ieee80211.h
index bb78e64..af8e95f 100644
--- a/ieee80211.h
+++ b/ieee80211.h
@@ -80,7 +80,7 @@
// Status/reason code returned by nl80211 messages: Authenticate,
// Deauthenticate, Associate, and Reassociate.
enum WiFiReasonCode {
- // 0 is reserved.
+ kReasonCodeReserved0 = 0, // 0 is reserved.
kReasonCodeUnspecified = 1,
kReasonCodePreviousAuthenticationInvalid = 2,
kReasonCodeSenderHasLeft = 3,
@@ -92,7 +92,7 @@
kReasonCodeReassociationNotAuthenticated = 9,
kReasonCodeUnacceptablePowerCapability = 10,
kReasonCodeUnacceptableSupportedChannelInfo = 11,
- // 12 is reserved.
+ kReasonCodeReserved12 = 12, // 12 is reserved.
kReasonCodeInvalidInfoElement = 13,
kReasonCodeMICFailure = 14,
kReasonCode4WayTimeout = 15,
@@ -105,7 +105,8 @@
kReasonCodeInvalidRsnIeCaps = 22,
kReasonCode8021XAuth = 23,
kReasonCodeCipherSuiteRejected = 24,
- // 25-31 are reserved.
+ kReasonCodeReservedBegin25 = 25, // 25-31 are reserved.
+ kReasonCodeReservedEnd31 = 31,
kReasonCodeUnspecifiedQoS = 32,
kReasonCodeQoSBandwidth = 33,
kReasonCodeiPoorConditions = 34,
@@ -114,6 +115,8 @@
kReasonCodeUnacceptableMechanism = 37,
kReasonCodeSetupRequired = 38,
kReasonCodeTimeout = 39,
+ kReasonCodeReservedBegin40 = 40, // 40-44 are reserved.
+ kReasonCodeReservedEnd44 = 44,
kReasonCodeCipherSuiteNotSupported = 45,
kReasonCodeMax,
kReasonCodeInvalid = UINT16_MAX
diff --git a/metrics.h b/metrics.h
index 4091582..fc214de 100644
--- a/metrics.h
+++ b/metrics.h
@@ -507,8 +507,8 @@
int response_time_milliseconds);
// Notifies this object of WiFi disconnect.
- void Notify80211Disconnect(WiFiDisconnectByWhom by_whom,
- IEEE_80211::WiFiReasonCode reason);
+ virtual void Notify80211Disconnect(WiFiDisconnectByWhom by_whom,
+ IEEE_80211::WiFiReasonCode reason);
// Registers a device with this object so the device can use the timers to
// track state transition metrics.
diff --git a/mock_metrics.h b/mock_metrics.h
index 4309402..567ffec 100644
--- a/mock_metrics.h
+++ b/mock_metrics.h
@@ -22,7 +22,7 @@
void(const Service &service, const std::string &histogram_name,
Service::ConnectState start_state,
Service::ConnectState stop_state));
- MOCK_METHOD1(NotifyDeviceScanStarted, void (int interface_index));
+ MOCK_METHOD1(NotifyDeviceScanStarted, void(int interface_index));
MOCK_METHOD1(NotifyDeviceScanFinished, void(int interface_index));
MOCK_METHOD1(ResetScanTimer, void(int interface_index));
MOCK_METHOD2(NotifyDeviceConnectStarted, void(int interface_index,
@@ -32,6 +32,8 @@
MOCK_METHOD1(NotifyDefaultServiceChanged, void(const Service *service));
MOCK_METHOD2(NotifyServiceStateChanged,
void(const Service &service, Service::ConnectState new_state));
+ MOCK_METHOD2(Notify80211Disconnect, void(WiFiDisconnectByWhom by_whom,
+ IEEE_80211::WiFiReasonCode reason));
MOCK_METHOD0(Notify3GPPRegistrationDelayedDropPosted, void());
MOCK_METHOD0(Notify3GPPRegistrationDelayedDropCanceled, void());
MOCK_METHOD0(NotifyCorruptedProfile, void());
diff --git a/shill.gyp b/shill.gyp
index 3332c28..ed6297c 100644
--- a/shill.gyp
+++ b/shill.gyp
@@ -477,6 +477,7 @@
'arp_packet_unittest.cc',
'async_connection_unittest.cc',
'byte_string_unittest.cc',
+ 'callback80211_metrics_unittest.cc',
'certificate_file_unittest.cc',
'connection_health_checker_unittest.cc',
'connection_info_reader_unittest.cc',
diff --git a/shill_daemon.cc b/shill_daemon.cc
index 2558fe6..9323f76 100644
--- a/shill_daemon.cc
+++ b/shill_daemon.cc
@@ -49,7 +49,7 @@
config->GetRunDirectory(),
config->GetStorageDirectory(),
config->GetUserStorageDirectoryFormat())),
- callback80211_metrics_(*netlink_manager_, metrics_.get()) {
+ callback80211_metrics_(metrics_.get()) {
}
Daemon::~Daemon() { }