QBONE TUN exchanger async refactor: Remove now-defuct StatsInterface visitor This visitor type is redundant in capabilities with the now-expanded quic::QboneClientPacketExchanger::Visitor. Silly to have two similar visitor types for the same logic. If anything wants to track stats, they can do so via Visitor. Also, very slight change to make Visitor now required, to better replace the was-required StatsInterface. PiperOrigin-RevId: 959914406
diff --git a/build/source_list.bzl b/build/source_list.bzl index a94ca22..066b955 100644 --- a/build/source_list.bzl +++ b/build/source_list.bzl
@@ -1745,7 +1745,6 @@ "quic/qbone/bonnet/icmp_reachable.h", "quic/qbone/bonnet/icmp_reachable_interface.h", "quic/qbone/bonnet/mock_icmp_reachable.h", - "quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h", "quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h", "quic/qbone/bonnet/mock_qbone_tunnel.h", "quic/qbone/bonnet/mock_tun_device.h",
diff --git a/build/source_list.gni b/build/source_list.gni index 3e66444..87af639 100644 --- a/build/source_list.gni +++ b/build/source_list.gni
@@ -1751,7 +1751,6 @@ "src/quiche/quic/qbone/bonnet/icmp_reachable.h", "src/quiche/quic/qbone/bonnet/icmp_reachable_interface.h", "src/quiche/quic/qbone/bonnet/mock_icmp_reachable.h", - "src/quiche/quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h", "src/quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h", "src/quiche/quic/qbone/bonnet/mock_qbone_tunnel.h", "src/quiche/quic/qbone/bonnet/mock_tun_device.h",
diff --git a/build/source_list.json b/build/source_list.json index 17e3ed6..231fdb0 100644 --- a/build/source_list.json +++ b/build/source_list.json
@@ -1750,7 +1750,6 @@ "quiche/quic/qbone/bonnet/icmp_reachable.h", "quiche/quic/qbone/bonnet/icmp_reachable_interface.h", "quiche/quic/qbone/bonnet/mock_icmp_reachable.h", - "quiche/quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h", "quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h", "quiche/quic/qbone/bonnet/mock_qbone_tunnel.h", "quiche/quic/qbone/bonnet/mock_tun_device.h",
diff --git a/quiche/quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h b/quiche/quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h deleted file mode 100644 index 5cd1ad7..0000000 --- a/quiche/quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h +++ /dev/null
@@ -1,30 +0,0 @@ -// Copyright (c) 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef QUICHE_QUIC_QBONE_BONNET_MOCK_PACKET_EXCHANGER_STATS_INTERFACE_H_ -#define QUICHE_QUIC_QBONE_BONNET_MOCK_PACKET_EXCHANGER_STATS_INTERFACE_H_ - -#include <cstddef> - -#include "absl/time/time.h" -#include "quiche/quic/platform/api/quic_test.h" -#include "quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h" - -namespace quic { - -class MockPacketExchangerStatsInterface - : public TunDevicePacketExchanger::StatsInterface { - public: - MOCK_METHOD(void, OnPacketRead, (size_t, absl::Duration), (override)); - MOCK_METHOD(void, OnPacketWritten, (size_t, absl::Duration), (override)); - MOCK_METHOD(void, OnReadError, (absl::string_view), (override)); - MOCK_METHOD(void, OnWriteError, (absl::string_view), (override)); - - MOCK_METHOD(int64_t, PacketsRead, (), (const, override)); - MOCK_METHOD(int64_t, PacketsWritten, (), (const, override)); -}; - -} // namespace quic - -#endif // QUICHE_QUIC_QBONE_BONNET_MOCK_PACKET_EXCHANGER_STATS_INTERFACE_H_
diff --git a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc index f6f0c2d..ccb2770 100644 --- a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc +++ b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc
@@ -26,7 +26,6 @@ #include "absl/time/clock.h" #include "absl/time/time.h" #include "absl/types/span.h" -#include "quiche/quic/core/quic_packets.h" #include "quiche/quic/platform/api/quic_ip_address.h" #include "quiche/quic/platform/api/quic_logging.h" #include "quiche/quic/qbone/platform/icmp_packet.h" @@ -42,17 +41,15 @@ TunDevicePacketExchanger::TunDevicePacketExchanger( size_t mtu, KernelInterface* kernel, NetlinkInterface* netlink, - Visitor* absl_nullable visitor, bool is_tap, StatsInterface* stats, - absl::string_view ifname) + Visitor* absl_nonnull visitor, bool is_tap, absl::string_view ifname) : kernel_(kernel), netlink_(netlink), - visitor_(visitor), + visitor_(*visitor), ifname_(ifname), // Reading on a TUN device returns a packet at a time. If the packet is // longer than the buffer, it's truncated. read_buffer_(mtu), - is_tap_(is_tap), - stats_(stats) {} + is_tap_(is_tap) {} bool TunDevicePacketExchanger::ReadAndDeliverPacket( QboneClientInterface* qbone_client) { @@ -60,11 +57,7 @@ absl::Status error = absl::InternalError( absl::StrCat("Invalid file descriptor of the TUN device: ", read_fd_)); QUIC_LOG_EVERY_N_SEC(ERROR, 60) << "Packet read failed: " << error; - stats_->OnReadError(absl::StrCat(absl::StatusCodeToString(error.code()), - ": ", error.message())); - if (visitor_) { - visitor_->OnRead(std::move(error)); - } + visitor_.OnRead(std::move(error)); return false; } @@ -91,11 +84,7 @@ if (!status.ok()) { QUIC_LOG_EVERY_N_SEC(ERROR, 60) << "Packet read failed: " << status; - stats_->OnReadError(absl::StrCat(absl::StatusCodeToString(status.code()), - ": ", status.message())); - if (visitor_) { - visitor_->OnRead(std::move(status)); - } + visitor_.OnRead(std::move(status)); return false; } @@ -104,11 +93,7 @@ absl::Status error = absl::InternalError(absl::StrCat("Invalid packet size.")); QUIC_LOG_EVERY_N_SEC(ERROR, 60) << "Packet read failed: " << error; - stats_->OnReadError(absl::StrCat(absl::StatusCodeToString(error.code()), - ": ", error.message())); - if (visitor_) { - visitor_->OnRead(std::move(error)); - } + visitor_.OnRead(std::move(error)); return false; } absl::Span<const std::byte> l3_packet = @@ -118,11 +103,7 @@ switch (ValidateL2Headers(eth_header, l3_packet)) { case L2ValidationResult::kInvalid: { absl::Status error = absl::InvalidArgumentError("Invalid L2 headers."); - stats_->OnReadError(absl::StrCat(absl::StatusCodeToString(error.code()), - ": ", error.message())); - if (visitor_) { - visitor_->OnRead(std::move(error)); - } + visitor_.OnRead(std::move(error)); return false; } case L2ValidationResult::kValidLinkLocal: @@ -138,11 +119,8 @@ } } - if (visitor_) { - visitor_->OnRead(std::vector<ReadResult>{ - ReadResult{.packet = l3_packet, .latency = latency}}); - } - stats_->OnPacketRead(l3_packet.size(), latency); + visitor_.OnRead(std::vector<ReadResult>{ + ReadResult{.packet = l3_packet, .latency = latency}}); qbone_client->ProcessPacketFromNetwork(absl::string_view( reinterpret_cast<const char*>(l3_packet.data()), l3_packet.size())); return true; @@ -154,11 +132,7 @@ absl::Status error = absl::InternalError( absl::StrCat("Invalid file descriptor of the TUN device: ", write_fd_)); QUIC_LOG_EVERY_N_SEC(ERROR, 60) << "Packet write failed: " << error; - stats_->OnWriteError(absl::StrCat(absl::StatusCodeToString(error.code()), - ": ", error.message())); - if (visitor_) { - visitor_->OnWrite(std::move(error)); - } + visitor_.OnWrite(std::move(error)); return; } @@ -181,21 +155,14 @@ if (!status.ok()) { QUIC_LOG_EVERY_N_SEC(ERROR, 60) << "Packet write failed: " << status; - stats_->OnWriteError(absl::StrCat(absl::StatusCodeToString(status.code()), - ": ", status.message())); - if (visitor_) { - visitor_->OnWrite(std::move(status)); - } + visitor_.OnWrite(std::move(status)); return; } - if (visitor_) { - visitor_->OnWrite(std::vector<WriteResult>{WriteResult{ - .packet = - absl::MakeSpan(reinterpret_cast<const std::byte*>(packet), size), - .latency = latency}}); - } - stats_->OnPacketWritten(result, latency); + visitor_.OnWrite(std::vector<WriteResult>{WriteResult{ + .packet = + absl::MakeSpan(reinterpret_cast<const std::byte*>(packet), size), + .latency = latency}}); } void TunDevicePacketExchanger::set_read_file_descriptor(int fd) { @@ -205,11 +172,6 @@ write_fd_ = fd; } -const TunDevicePacketExchanger::StatsInterface* -TunDevicePacketExchanger::stats_interface() const { - return stats_; -} - void TunDevicePacketExchanger::InitializeEthHdr() { if (!eth_hdr_initialized_) { NetlinkInterface::LinkInfo link_info{};
diff --git a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h index 09a392f..0ad7bbf 100644 --- a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h +++ b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h
@@ -8,16 +8,13 @@ #include <linux/if_ether.h> #include <cstddef> -#include <cstdint> #include <string> #include <vector> #include "absl/base/attributes.h" #include "absl/base/nullability.h" #include "absl/strings/string_view.h" -#include "absl/time/time.h" #include "absl/types/span.h" -#include "quiche/quic/core/quic_packets.h" #include "quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h" #include "quiche/quic/qbone/platform/kernel_interface.h" #include "quiche/quic/qbone/platform/netlink_interface.h" @@ -27,43 +24,18 @@ class TunDevicePacketExchanger : public QboneClientPacketExchanger { public: - class StatsInterface { - public: - StatsInterface() = default; - - StatsInterface(const StatsInterface&) = delete; - StatsInterface& operator=(const StatsInterface&) = delete; - - StatsInterface(StatsInterface&&) = delete; - StatsInterface& operator=(StatsInterface&&) = delete; - - virtual ~StatsInterface() = default; - - virtual void OnPacketRead(size_t length, absl::Duration latency) = 0; - virtual void OnPacketWritten(size_t length, absl::Duration latency) = 0; - virtual void OnReadError(absl::string_view error) = 0; - virtual void OnWriteError(absl::string_view error) = 0; - - ABSL_MUST_USE_RESULT virtual int64_t PacketsRead() const = 0; - ABSL_MUST_USE_RESULT virtual int64_t PacketsWritten() const = 0; - }; - // |mtu| is the mtu of the TUN device. // |kernel| is not owned but should out live objects of this class. // |visitor| is not owned but should out live objects of this class. - // |stats| is notified about packet read/write statistics. It is not owned, - // but should outlive objects of this class. - TunDevicePacketExchanger( - size_t mtu, KernelInterface* kernel, NetlinkInterface* netlink, - QboneClientPacketExchanger::Visitor* absl_nullable visitor - ABSL_ATTRIBUTE_LIFETIME_BOUND, - bool is_tap, StatsInterface* stats, absl::string_view ifname); + TunDevicePacketExchanger(size_t mtu, KernelInterface* kernel, + NetlinkInterface* netlink, + QboneClientPacketExchanger::Visitor* absl_nonnull + visitor ABSL_ATTRIBUTE_LIFETIME_BOUND, + bool is_tap, absl::string_view ifname); void set_read_file_descriptor(int fd); void set_write_file_descriptor(int fd); - ABSL_MUST_USE_RESULT const StatsInterface* stats_interface() const; - // QboneClientPacketExchanger: bool ReadAndDeliverPacket(QboneClientInterface* qbone_client) override; void WritePacketToNetwork(const char* packet, size_t size) override; @@ -91,7 +63,7 @@ int write_fd_ = -1; KernelInterface* kernel_; NetlinkInterface* netlink_; - QboneClientPacketExchanger::Visitor* const absl_nullable visitor_; + QboneClientPacketExchanger::Visitor& visitor_; const std::string ifname_; std::vector<std::byte> read_buffer_; @@ -99,8 +71,6 @@ const bool is_tap_; ethhdr eth_hdr_ = {}; bool eth_hdr_initialized_ = false; - - StatsInterface* stats_; }; } // namespace quic
diff --git a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc index d390851..6102d65 100644 --- a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc +++ b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc
@@ -18,7 +18,6 @@ #include "absl/status/status.h" #include "absl/strings/string_view.h" #include "quiche/quic/platform/api/quic_test.h" -#include "quiche/quic/qbone/bonnet/mock_packet_exchanger_stats_interface.h" #include "quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h" #include "quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h" #include "quiche/quic/qbone/mock_qbone_client.h" @@ -51,7 +50,7 @@ protected: TunDevicePacketExchangerTest() : exchanger_(kMtu, &mock_kernel_, nullptr, &mock_visitor_, false, - &mock_stats_, absl::string_view()) { + absl::string_view()) { exchanger_.set_read_file_descriptor(kReadFd); exchanger_.set_write_file_descriptor(kWriteFd); } @@ -61,7 +60,6 @@ MockKernel mock_kernel_; StrictMock<MockQboneClientPacketExchanger::MockVisitor> mock_visitor_; StrictMock<MockQboneClient> mock_client_; - StrictMock<MockPacketExchangerStatsInterface> mock_stats_; TunDevicePacketExchanger exchanger_; }; @@ -79,7 +77,6 @@ }); EXPECT_CALL(mock_visitor_, OnWrite(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_CALL(mock_stats_, OnWriteError(_)); exchanger_.WritePacketToNetwork(packet.data(), packet.size()); } @@ -96,7 +93,6 @@ return -1; }); - EXPECT_CALL(mock_stats_, OnWriteError(_)); EXPECT_CALL(mock_visitor_, OnWrite(StatusIs(Ne(absl::StatusCode::kOk)))); exchanger_.WritePacketToNetwork(packet.data(), packet.size()); } @@ -114,7 +110,6 @@ return packet.size(); }); - EXPECT_CALL(mock_stats_, OnPacketWritten(packet.size(), _)); EXPECT_CALL( mock_visitor_, OnWrite(IsOkAndHolds(ElementsAre(Field( @@ -129,10 +124,9 @@ StrictMock<MockKernel> mock_kernel; StrictMock<MockNetlink> mock_netlink; StrictMock<MockQboneClientPacketExchanger::MockVisitor> mock_visitor; - StrictMock<MockPacketExchangerStatsInterface> mock_stats; TunDevicePacketExchanger tap_exchanger(kMtu, &mock_kernel, &mock_netlink, &mock_visitor, /*is_tap=*/true, - &mock_stats, "tap0"); + "tap0"); tap_exchanger.set_write_file_descriptor(kWriteFd); std::string packet = "fake packet"; @@ -170,7 +164,6 @@ return ETH_HLEN + packet.length(); }); - EXPECT_CALL(mock_stats, OnPacketWritten(ETH_HLEN + packet.size(), _)); EXPECT_CALL( mock_visitor, OnWrite(IsOkAndHolds(ElementsAre(Field( @@ -188,7 +181,6 @@ return -1; }); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_CALL(mock_stats_, OnReadError(_)); EXPECT_FALSE(exchanger_.ReadAndDeliverPacket(&mock_client_)); } @@ -198,7 +190,6 @@ errno = EAGAIN; return -1; }); - EXPECT_CALL(mock_stats_, OnReadError(_)); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); EXPECT_FALSE(exchanger_.ReadAndDeliverPacket(&mock_client_)); } @@ -213,7 +204,6 @@ return packet.size(); }); EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(packet))); - EXPECT_CALL(mock_stats_, OnPacketRead(_, _)); EXPECT_CALL( mock_visitor_, OnRead(IsOkAndHolds(ElementsAre(Field( @@ -223,90 +213,11 @@ EXPECT_TRUE(exchanger_.ReadAndDeliverPacket(&mock_client_)); } -TEST_F(TunDevicePacketExchangerTest, WriteWithNullVisitor) { - TunDevicePacketExchanger exchanger(kMtu, &mock_kernel_, nullptr, - /*visitor=*/nullptr, false, &mock_stats_, - absl::string_view()); - exchanger.set_write_file_descriptor(kWriteFd); - - std::string packet = "fake packet"; - EXPECT_CALL(mock_kernel_, writev(kWriteFd, _, 2)) - .WillOnce( - [&packet](int fd, const struct iovec* iov, int iovcnt) -> ssize_t { - EXPECT_EQ(iov[0].iov_base, nullptr); - EXPECT_EQ(iov[0].iov_len, 0); - EXPECT_THAT(reinterpret_cast<const char*>(iov[1].iov_base), - StrEq(packet)); - EXPECT_EQ(iov[1].iov_len, packet.size()); - return packet.size(); - }); - - EXPECT_CALL(mock_stats_, OnPacketWritten(packet.size(), _)).Times(1); - exchanger.WritePacketToNetwork(packet.data(), packet.size()); -} - -TEST_F(TunDevicePacketExchangerTest, WritePacketErrorWithNullVisitor) { - TunDevicePacketExchanger exchanger(kMtu, &mock_kernel_, nullptr, - /*visitor=*/nullptr, false, &mock_stats_, - absl::string_view()); - exchanger.set_write_file_descriptor(kWriteFd); - - std::string packet = "fake packet"; - EXPECT_CALL(mock_kernel_, writev(kWriteFd, _, 2)) - .WillOnce([](int fd, const struct iovec* iov, int iovcnt) -> ssize_t { - EXPECT_EQ(iov[0].iov_base, nullptr); - EXPECT_EQ(iov[0].iov_len, 0); - EXPECT_THAT(reinterpret_cast<const char*>(iov[1].iov_base), - testing::StrEq("fake packet")); - EXPECT_EQ(iov[1].iov_len, 11); - errno = ECOMM; - return -1; - }); - - EXPECT_CALL(mock_stats_, OnWriteError(_)); - - exchanger.WritePacketToNetwork(packet.data(), packet.size()); -} - -TEST_F(TunDevicePacketExchangerTest, ReadPacketWithNullVisitor) { - TunDevicePacketExchanger exchanger(kMtu, &mock_kernel_, nullptr, - /*visitor=*/nullptr, false, &mock_stats_, - absl::string_view()); - exchanger.set_read_file_descriptor(kReadFd); - - std::string packet = "fake_packet"; - EXPECT_CALL(mock_kernel_, readv(kReadFd, _, 2)) - .WillOnce([packet](int fd, const struct iovec* iov, int iovcnt) { - EXPECT_EQ(iov[0].iov_len, 0); - EXPECT_EQ(iov[1].iov_len, kMtu); - memcpy(iov[1].iov_base, packet.data(), packet.size()); - return packet.size(); - }); - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(packet))); - EXPECT_CALL(mock_stats_, OnPacketRead(_, _)).Times(1); - EXPECT_TRUE(exchanger.ReadAndDeliverPacket(&mock_client_)); -} - -TEST_F(TunDevicePacketExchangerTest, ReadPacketErrorWithNullVisitor) { - TunDevicePacketExchanger exchanger(kMtu, &mock_kernel_, nullptr, - /*visitor=*/nullptr, false, &mock_stats_, - absl::string_view()); - exchanger.set_read_file_descriptor(kReadFd); - - EXPECT_CALL(mock_kernel_, readv(kReadFd, _, 2)) - .WillOnce([](int fd, const struct iovec* iov, int iovcnt) { - errno = ECOMM; - return -1; - }); - EXPECT_CALL(mock_stats_, OnReadError(_)); - EXPECT_FALSE(exchanger.ReadAndDeliverPacket(&mock_client_)); -} - class TunDevicePacketExchangerTapTest : public QuicTest { protected: TunDevicePacketExchangerTapTest() : exchanger_(kMtu, &mock_kernel_, &mock_netlink_, &mock_visitor_, true, - &mock_stats_, "tap0") { + "tap0") { exchanger_.set_read_file_descriptor(kReadFd); exchanger_.set_write_file_descriptor(kWriteFd); } @@ -317,7 +228,6 @@ StrictMock<MockNetlink> mock_netlink_; StrictMock<MockQboneClientPacketExchanger::MockVisitor> mock_visitor_; StrictMock<MockQboneClient> mock_client_; - StrictMock<MockPacketExchangerStatsInterface> mock_stats_; TunDevicePacketExchanger exchanger_; }; @@ -345,7 +255,6 @@ }); EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(l3_packet))); - EXPECT_CALL(mock_stats_, OnPacketRead(l3_packet.size(), _)); EXPECT_CALL( mock_visitor_, OnRead(IsOkAndHolds(ElementsAre(Field( @@ -366,7 +275,6 @@ }); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_CALL(mock_stats_, OnReadError(_)); EXPECT_FALSE(exchanger_.ReadAndDeliverPacket(&mock_client_)); } @@ -412,7 +320,6 @@ .WillOnce([](int fd, const struct iovec* iov, int iovcnt) -> ssize_t { return iov[0].iov_len + iov[1].iov_len; }); - EXPECT_CALL(mock_stats_, OnPacketWritten(_, _)); EXPECT_CALL(mock_visitor_, OnWrite(IsOkAndHolds(SizeIs(1)))); // ReadAndDeliverPacket should return false because packet was handled