shill: LinkMonitor: Fixup local/remote order
ARP responses will have the "local" fields of the ARP
header populated with the "remote" addresses, from the
perspective of the recipient. Fix this misunderstanding.
BUG=chromium-os:32600
TEST=Manual -- ran on real system (after following CLs)
Change-Id: I0e34fdada9b0f1b149d0569cf1d942ca67466ea5
Reviewed-on: https://gerrit.chromium.org/gerrit/29793
Commit-Ready: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/link_monitor.cc b/link_monitor.cc
index 1d8dd1b..26cb0dd 100644
--- a/link_monitor.cc
+++ b/link_monitor.cc
@@ -88,7 +88,7 @@
timerclear(&sent_request_at_);
}
-unsigned int LinkMonitor::GetResponseTimeMilliseconds() {
+unsigned int LinkMonitor::GetResponseTimeMilliseconds() const {
return response_sample_count_ ?
response_sample_bucket_ / response_sample_count_ : 0;
}
@@ -125,6 +125,8 @@
if (!arp_client_->Start()) {
return false;
}
+ SLOG(Link, 4) << "Created ARP client; listening on socket "
+ << arp_client_->socket() << ".";
receive_response_handler_.reset(
dispatcher_->CreateReadyHandler(
arp_client_->socket(),
@@ -169,6 +171,10 @@
return false;
}
+bool LinkMonitor::IsGatewayFound() const {
+ return !gateway_mac_address_.IsZero();
+}
+
void LinkMonitor::ReceiveResponse(int fd) {
SLOG(Link, 2) << "In " << __func__ << ".";
ArpPacket packet;
@@ -177,17 +183,19 @@
return;
}
- if (!connection_->local().Equals(packet.local_ip_address())) {
+ if (!connection_->local().address().Equals(
+ packet.remote_ip_address().address())) {
SLOG(Link, 4) << "Response is not for our IP address.";
return;
}
- if (!local_mac_address_.Equals(packet.local_mac_address())) {
+ if (!local_mac_address_.Equals(packet.remote_mac_address())) {
SLOG(Link, 4) << "Response is not for our MAC address.";
return;
}
- if (!connection_->gateway().Equals(packet.remote_ip_address())) {
+ if (!connection_->gateway().address().Equals(
+ packet.local_ip_address().address())) {
SLOG(Link, 4) << "Response is not from the gateway IP address.";
return;
}
@@ -208,9 +216,9 @@
broadcast_failure_count_ = 0;
}
- if (!gateway_mac_address_.Equals(packet.remote_mac_address())) {
- const ByteString &new_mac_address = packet.remote_mac_address();
- if (gateway_mac_address_.IsZero()) {
+ if (!gateway_mac_address_.Equals(packet.local_mac_address())) {
+ const ByteString &new_mac_address = packet.local_mac_address();
+ if (!IsGatewayFound()) {
SLOG(Link, 2) << "Found gateway at "
<< HardwareAddressToString(new_mac_address);
} else {
@@ -248,7 +256,7 @@
}
ByteString destination_mac_address(gateway_mac_address_.GetLength());
- if (gateway_mac_address_.IsZero()) {
+ if (!IsGatewayFound()) {
// The remote MAC addess is set by convention to be all-zeroes in the
// ARP header if not known. The ArpClient will translate an all-zeroes
// remote address into a send to the broadcast (all-ones) address in
diff --git a/link_monitor.h b/link_monitor.h
index 60a4057..3a420cf 100644
--- a/link_monitor.h
+++ b/link_monitor.h
@@ -56,7 +56,11 @@
// time. Returns zero if no samples are available. For each
// missed ARP response, the sample is assumed to be the full
// test period.
- virtual unsigned int GetResponseTimeMilliseconds();
+ virtual unsigned int GetResponseTimeMilliseconds() const;
+
+ // Returns true if the LinkMonitor was ever able to find the default
+ // gateway via broadcast ARP.
+ virtual bool IsGatewayFound() const;
private:
friend class LinkMonitorForTest;
diff --git a/link_monitor_unittest.cc b/link_monitor_unittest.cc
index 27aaf52..de3dc61 100644
--- a/link_monitor_unittest.cc
+++ b/link_monitor_unittest.cc
@@ -221,7 +221,7 @@
monitor_.ReceiveResponse(0);
}
void ReceiveCorrectResponse() {
- ReceiveResponse(local_ip_, local_mac_, gateway_ip_, gateway_mac_);
+ ReceiveResponse(gateway_ip_, gateway_mac_, local_ip_, local_mac_);
}
MockEventDispatcher dispatcher_;
@@ -316,17 +316,17 @@
EXPECT_CALL(log, Log(_, _, _)).Times(AnyNumber());
EXPECT_CALL(log, Log(_, _, HasSubstr("not for our IP"))).Times(1);
- ReceiveResponse(gateway_ip_, local_mac_, gateway_ip_, gateway_mac_);
+ ReceiveResponse(gateway_ip_, gateway_mac_, gateway_ip_, local_mac_);
Mock::VerifyAndClearExpectations(&log);
EXPECT_CALL(log, Log(_, _, _)).Times(AnyNumber());
EXPECT_CALL(log, Log(_, _, HasSubstr("not for our MAC"))).Times(1);
- ReceiveResponse(local_ip_, gateway_mac_, gateway_ip_, gateway_mac_);
+ ReceiveResponse(gateway_ip_, gateway_mac_, local_ip_, gateway_mac_);
Mock::VerifyAndClearExpectations(&log);
EXPECT_CALL(log, Log(_, _, _)).Times(AnyNumber());
EXPECT_CALL(log, Log(_, _, HasSubstr("not from the gateway"))).Times(1);
- ReceiveResponse(local_ip_, local_mac_, local_ip_, gateway_mac_);
+ ReceiveResponse(local_ip_, gateway_mac_, local_ip_, local_mac_);
Mock::VerifyAndClearExpectations(&log);
EXPECT_TRUE(GetArpClient());
diff --git a/mock_link_monitor.h b/mock_link_monitor.h
index 083b5d2..b76137d 100644
--- a/mock_link_monitor.h
+++ b/mock_link_monitor.h
@@ -18,7 +18,8 @@
MOCK_METHOD0(Start, bool());
MOCK_METHOD0(Stop, void());
- MOCK_METHOD0(GetResponseTimeMilliseconds, unsigned int());
+ MOCK_CONST_METHOD0(GetResponseTimeMilliseconds, unsigned int());
+ MOCK_CONST_METHOD0(IsGatewayFound, bool());
DISALLOW_COPY_AND_ASSIGN(MockLinkMonitor);
};