QBONE TUN exchanger async refactor: Cleanup read write calls interface Remove passing the QboneClient into the Read call in favor of directly using the Visitor because that passed client was acting as a sneaky third visitor type, and we now want everything unified into Visitor. The call to the client now occurs inside GfeTunDevicePacketExchanger in its visitor impl next to the stats collection. And while poking at the read/write function signatures, modernize write to use byte spans. Also move all epoll callback logic from QboneTunnel to inside GfeTunDevicePacketExchanger to better organize the interactions and remove any need for QboneTunnel to interact with a Read() call of any sort. And since both of these changes require GfeTunDevicePacketExchanger having slightly different interface from the underlying QboneClientPacketExchanger, convert it into being a wrapper of the exchanger rather than being a derived exchanger type itself. No real reason that class and its contained logic actually needs to be an exchanger. PiperOrigin-RevId: 972099683
diff --git a/quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h b/quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h index 7168d42..13e96d4 100644 --- a/quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h +++ b/quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h
@@ -9,9 +9,9 @@ #include <vector> #include "absl/status/statusor.h" +#include "absl/types/span.h" #include "quiche/quic/platform/api/quic_test.h" #include "quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h" -#include "quiche/quic/qbone/qbone_client_interface.h" namespace quic::test { @@ -31,10 +31,9 @@ MOCK_METHOD(void, Start, (int read_fd, int write_fd), (override)); MOCK_METHOD(void, Stop, (), (override)); - MOCK_METHOD(int, OnReadFromNetworkReady, - (int max_packets_to_read, QboneClientInterface* qbone_client), + MOCK_METHOD(int, OnReadFromNetworkReady, (int max_packets_to_read), (override)); - MOCK_METHOD(void, WritePacketToNetwork, (const char* packet, size_t size), + MOCK_METHOD(void, WritePacketToNetwork, (absl::Span<const std::byte> packet), (override)); };
diff --git a/quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h b/quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h index 2f15da9..1755d75 100644 --- a/quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h +++ b/quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h
@@ -11,7 +11,6 @@ #include "absl/status/statusor.h" #include "absl/time/time.h" #include "absl/types/span.h" -#include "quiche/quic/qbone/qbone_client_interface.h" namespace quic { @@ -53,12 +52,11 @@ // packets synchronously read from the socket (not number of valid packets // processed to client and visitor, and not useful if implementation handles // reads asynchronously). Must not be called before Start() or after Stop(). - virtual int OnReadFromNetworkReady(int max_packets_to_read, - QboneClientInterface* qbone_client) = 0; + virtual int OnReadFromNetworkReady(int max_packets_to_read) = 0; // Writes a packet to the local network. If the write would be blocked, the // packet is dropped. Must not be called before Start() or after Stop(). - virtual void WritePacketToNetwork(const char* packet, size_t size) = 0; + virtual void WritePacketToNetwork(absl::Span<const std::byte> packet) = 0; }; } // namespace quic
diff --git a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc index 0c804de..f002ece 100644 --- a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc +++ b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.cc
@@ -32,7 +32,6 @@ #include "quiche/quic/qbone/platform/icmp_packet.h" #include "quiche/quic/qbone/platform/kernel_interface.h" #include "quiche/quic/qbone/platform/netlink_interface.h" -#include "quiche/quic/qbone/qbone_client_interface.h" #include "quiche/quic/qbone/qbone_constants.h" #include "quiche/common/platform/api/quiche_logging.h" #include "quiche/common/quiche_endian.h" @@ -79,8 +78,7 @@ write_fd_ = -1; } -int TunDevicePacketExchanger::OnReadFromNetworkReady( - int max_packets_to_read, QboneClientInterface* qbone_client) { +int TunDevicePacketExchanger::OnReadFromNetworkReady(int max_packets_to_read) { if (read_fd_ < 0) { QUIC_BUG(qbone_tun_device_packet_exchanger_read_with_invalid_fd) << "Invalid file descriptor of the TUN device: " << read_fd_; @@ -94,7 +92,7 @@ // errors are just the signal that there are no more packets to read. bool exchange_blocked_error = packets_read == 0; - if (ReadAndExchangeSinglePacket(qbone_client, exchange_blocked_error)) { + if (ReadAndExchangeSinglePacket(exchange_blocked_error)) { packets_read++; } else { break; @@ -104,8 +102,8 @@ return packets_read; } -void TunDevicePacketExchanger::WritePacketToNetwork(const char* packet, - size_t size) { +void TunDevicePacketExchanger::WritePacketToNetwork( + absl::Span<const std::byte> packet) { if (write_fd_ < 0) { QUIC_BUG(qbone_tun_device_packet_exchanger_write_with_invalid_fd) << "Invalid file descriptor of the TUN device: " << write_fd_; @@ -118,8 +116,8 @@ struct iovec iov[2]; iov[0].iov_base = is_tap_ ? ð_hdr_ : nullptr; iov[0].iov_len = is_tap_ ? ETH_HLEN : 0; - iov[1].iov_base = const_cast<char*>(packet); - iov[1].iov_len = size; + iov[1].iov_base = const_cast<std::byte*>(packet.data()); + iov[1].iov_len = packet.size(); absl::Status status = absl::OkStatus(); absl::Time start = absl::Now(); @@ -135,14 +133,12 @@ return; } - visitor_.OnWrite(std::vector<WriteResult>{WriteResult{ - .packet = - absl::MakeSpan(reinterpret_cast<const std::byte*>(packet), size), - .latency = latency}}); + visitor_.OnWrite(std::vector<WriteResult>{ + WriteResult{.packet = std::move(packet), .latency = latency}}); } bool TunDevicePacketExchanger::ReadAndExchangeSinglePacket( - QboneClientInterface* qbone_client, bool exchange_blocked_error) { + bool exchange_blocked_error) { QUICHE_DCHECK_GE(read_fd_, 0); // TODO(ericorth): Consider allocating these buffers once and reusing rather @@ -212,8 +208,6 @@ 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; } @@ -315,7 +309,9 @@ CreateIcmpPacket(ip_hdr->ip6_src, ip_hdr->ip6_src, response_hdr, absl::string_view(payload.get(), payload_size), [this](absl::string_view packet) { - WritePacketToNetwork(packet.data(), packet.size()); + WritePacketToNetwork(absl::MakeSpan( + reinterpret_cast<const std::byte*>(packet.data()), + packet.size())); }); return L2ValidationResult::kValidLinkLocal; }
diff --git a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h index 7ba8748..13d1ed5 100644 --- a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h +++ b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger.h
@@ -18,7 +18,6 @@ #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" -#include "quiche/quic/qbone/qbone_client_interface.h" namespace quic { @@ -38,9 +37,8 @@ // QboneClientPacketExchanger: void Start(int read_fd, int write_fd) override; void Stop() override; - int OnReadFromNetworkReady(int max_packets_to_read, - QboneClientInterface* qbone_client) override; - void WritePacketToNetwork(const char* packet, size_t size) override; + int OnReadFromNetworkReady(int max_packets_to_read) override; + void WritePacketToNetwork(absl::Span<const std::byte> packet) override; private: enum class L2ValidationResult { @@ -57,8 +55,7 @@ }; // Returns true if more packets may be available to read. - bool ReadAndExchangeSinglePacket(QboneClientInterface* qbone_client, - bool exchange_blocked_error); + bool ReadAndExchangeSinglePacket(bool exchange_blocked_error); void InitializeEthHdr(); L2ValidationResult ValidateL2Headers(const ethhdr& eth_header,
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 442af30..38e7e37 100644 --- a/quiche/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc +++ b/quiche/quic/qbone/bonnet/tun_device_packet_exchanger_test.cc
@@ -17,6 +17,7 @@ #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "quiche/quic/platform/api/quic_test.h" #include "quiche/quic/qbone/bonnet/mock_qbone_client_packet_exchanger.h" #include "quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h" @@ -56,7 +57,6 @@ MockKernel mock_kernel_; StrictMock<MockQboneClientPacketExchanger::MockVisitor> mock_visitor_; - StrictMock<MockQboneClient> mock_client_; TunDevicePacketExchanger exchanger_; }; @@ -76,7 +76,8 @@ }); EXPECT_CALL(mock_visitor_, OnWrite(StatusIs(Ne(absl::StatusCode::kOk)))); - exchanger_.WritePacketToNetwork(packet.data(), packet.size()); + exchanger_.WritePacketToNetwork(absl::MakeConstSpan( + reinterpret_cast<std::byte*>(packet.data()), packet.size())); exchanger_.Stop(); } @@ -106,7 +107,8 @@ ElementsAreArray(reinterpret_cast<const std::byte*>(packet.data()), packet.size())))))) .Times(1); - exchanger_.WritePacketToNetwork(packet.data(), packet.size()); + exchanger_.WritePacketToNetwork(absl::MakeConstSpan( + reinterpret_cast<std::byte*>(packet.data()), packet.size())); exchanger_.Stop(); } @@ -127,7 +129,8 @@ }); EXPECT_CALL(mock_visitor_, OnWrite(StatusIs(Ne(absl::StatusCode::kOk)))); - exchanger_.WritePacketToNetwork(packet.data(), packet.size()); + exchanger_.WritePacketToNetwork(absl::MakeConstSpan( + reinterpret_cast<std::byte*>(packet.data()), packet.size())); exchanger_.Stop(); } @@ -154,7 +157,8 @@ ElementsAreArray(reinterpret_cast<const std::byte*>(packet.data()), packet.size())))))) .Times(1); - exchanger_.WritePacketToNetwork(packet.data(), packet.size()); + exchanger_.WritePacketToNetwork(absl::MakeConstSpan( + reinterpret_cast<std::byte*>(packet.data()), packet.size())); exchanger_.Stop(); } @@ -210,7 +214,8 @@ ElementsAreArray(reinterpret_cast<const std::byte*>(packet.data()), packet.size())))))); - tap_exchanger.WritePacketToNetwork(packet.data(), packet.size()); + tap_exchanger.WritePacketToNetwork(absl::MakeConstSpan( + reinterpret_cast<std::byte*>(packet.data()), packet.size())); tap_exchanger.Stop(); } @@ -224,9 +229,7 @@ return -1; }); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1, - &mock_client_), - 0); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1), 0); exchanger_.Stop(); } @@ -240,9 +243,7 @@ return -1; }); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1, - &mock_client_), - 0); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1), 0); exchanger_.Stop(); } @@ -258,16 +259,13 @@ memcpy(iov[1].iov_base, packet.data(), packet.size()); return packet.size(); }); - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(packet))); EXPECT_CALL( mock_visitor_, OnRead(IsOkAndHolds(ElementsAre(Field( &QboneClientPacketExchanger::ReadResult::packet, ElementsAreArray(reinterpret_cast<const std::byte*>(packet.data()), packet.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1, - &mock_client_), - 1); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1), 1); exchanger_.Stop(); } @@ -288,9 +286,6 @@ return packet2.size(); }); - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(packet1))); - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(packet2))); - EXPECT_CALL( mock_visitor_, OnRead(IsOkAndHolds(ElementsAre(Field( @@ -304,9 +299,7 @@ ElementsAreArray(reinterpret_cast<const std::byte*>(packet2.data()), packet2.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2, - &mock_client_), - 2); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2), 2); exchanger_.Stop(); } @@ -326,8 +319,6 @@ return -1; }); - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(packet1))); - // Expect no error callbacks from the blocked read. In this scenario, the // blocked socket is just a signal that there are no more packets to be read, // rather than an actual error. @@ -339,9 +330,7 @@ ElementsAreArray(reinterpret_cast<const std::byte*>(packet1.data()), packet1.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/5, - &mock_client_), - 1); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/5), 1); exchanger_.Stop(); } @@ -364,7 +353,6 @@ // Expect subsequent packet to still be read and processed after the invalid // packet. - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(valid_packet))); EXPECT_CALL(mock_visitor_, OnRead(IsOkAndHolds(ElementsAre(Field( &QboneClientPacketExchanger::ReadResult::packet, @@ -372,9 +360,7 @@ reinterpret_cast<const std::byte*>(valid_packet.data()), valid_packet.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2, - &mock_client_), - 2); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2), 2); exchanger_.Stop(); } @@ -389,9 +375,7 @@ return -1; }); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2, - &mock_client_), - 0); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2), 0); exchanger_.Stop(); } @@ -407,7 +391,6 @@ MockKernel mock_kernel_; StrictMock<MockNetlink> mock_netlink_; StrictMock<MockQboneClientPacketExchanger::MockVisitor> mock_visitor_; - StrictMock<MockQboneClient> mock_client_; TunDevicePacketExchanger exchanger_; }; @@ -436,16 +419,13 @@ return ETH_HLEN + l3_packet.size(); }); - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(l3_packet))); EXPECT_CALL( mock_visitor_, OnRead(IsOkAndHolds(ElementsAre(Field( &QboneClientPacketExchanger::ReadResult::packet, ElementsAreArray(reinterpret_cast<const std::byte*>(l3_packet.data()), l3_packet.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1, - &mock_client_), - 1); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1), 1); exchanger_.Stop(); } @@ -463,9 +443,7 @@ }); EXPECT_CALL(mock_visitor_, OnRead(StatusIs(Ne(absl::StatusCode::kOk)))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1, - &mock_client_), - 1); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1), 1); exchanger_.Stop(); } @@ -518,9 +496,7 @@ // OnReadFromNetworkReady should return 1 because packet was handled // internally (Neighbor Discovery) but still read from network. - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1, - &mock_client_), - 1); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/1), 1); exchanger_.Stop(); } @@ -555,7 +531,6 @@ // Expect subsequent packet to still be read and processed after the invalid // packet. - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(valid_l3_packet))); EXPECT_CALL(mock_visitor_, OnRead(IsOkAndHolds(ElementsAre( Field(&QboneClientPacketExchanger::ReadResult::packet, @@ -563,9 +538,7 @@ valid_l3_packet.data()), valid_l3_packet.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2, - &mock_client_), - 2); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2), 2); exchanger_.Stop(); } @@ -605,7 +578,6 @@ // Expect subsequent packet to still be read and processed after the invalid // packet. - EXPECT_CALL(mock_client_, ProcessPacketFromNetwork(StrEq(valid_l3_packet))); EXPECT_CALL(mock_visitor_, OnRead(IsOkAndHolds(ElementsAre( Field(&QboneClientPacketExchanger::ReadResult::packet, @@ -613,9 +585,7 @@ valid_l3_packet.data()), valid_l3_packet.size())))))); - EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2, - &mock_client_), - 2); + EXPECT_EQ(exchanger_.OnReadFromNetworkReady(/*max_packets_to_read=*/2), 2); exchanger_.Stop(); }
diff --git a/quiche/quic/qbone/qbone_client_session.cc b/quiche/quic/qbone/qbone_client_session.cc index 6e31e61..90307e5 100644 --- a/quiche/quic/qbone/qbone_client_session.cc +++ b/quiche/quic/qbone/qbone_client_session.cc
@@ -4,6 +4,7 @@ #include "quiche/quic/qbone/qbone_client_session.h" +#include <cstddef> #include <memory> #include <string> #include <utility> @@ -11,6 +12,7 @@ #include "absl/base/nullability.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/qbone/bonnet/qbone_client_packet_exchanger.h" #include "quiche/quic/qbone/qbone_constants.h" @@ -42,8 +44,8 @@ } void QboneClientSession::SendErrorPacketToNetwork(absl::string_view packet) { - local_network_packet_exchanger_.WritePacketToNetwork(packet.data(), - packet.size()); + local_network_packet_exchanger_.WritePacketToNetwork(absl::MakeSpan( + reinterpret_cast<const std::byte*>(packet.data()), packet.size())); } void QboneClientSession::CreateControlStream() { @@ -110,8 +112,8 @@ } void QboneClientSession::ProcessPacketFromPeer(absl::string_view packet) { - local_network_packet_exchanger_.WritePacketToNetwork(packet.data(), - packet.size()); + local_network_packet_exchanger_.WritePacketToNetwork(absl::MakeSpan( + reinterpret_cast<const std::byte*>(packet.data()), packet.size())); } void QboneClientSession::OnProofValid(
diff --git a/quiche/quic/qbone/qbone_client_test.cc b/quiche/quic/qbone/qbone_client_test.cc index 830628a..80c21b6 100644 --- a/quiche/quic/qbone/qbone_client_test.cc +++ b/quiche/quic/qbone/qbone_client_test.cc
@@ -14,6 +14,7 @@ #include "absl/strings/string_view.h" #include "absl/synchronization/mutex.h" +#include "absl/types/span.h" #include "quiche/quic/core/io/quic_default_event_loop.h" #include "quiche/quic/core/io/quic_event_loop.h" #include "quiche/quic/core/quic_alarm_factory.h" @@ -165,9 +166,10 @@ : QboneClient(server_address, server_id, supported_versions, /*session_owner=*/nullptr, QuicConfig(), event_loop, std::move(proof_verifier), &packet_exchanger_, nullptr) { - ON_CALL(packet_exchanger_, WritePacketToNetwork(_, _)) - .WillByDefault([this](const char* packet, size_t size) { - data_.push_back(std::string(packet, size)); + ON_CALL(packet_exchanger_, WritePacketToNetwork(_)) + .WillByDefault([this](absl::Span<const std::byte> packet) { + data_.emplace_back(reinterpret_cast<const char*>(packet.data()), + packet.size()); }); }
diff --git a/quiche/quic/qbone/qbone_session_test.cc b/quiche/quic/qbone/qbone_session_test.cc index 81ab855..14e814f 100644 --- a/quiche/quic/qbone/qbone_session_test.cc +++ b/quiche/quic/qbone/qbone_session_test.cc
@@ -11,6 +11,7 @@ #include <vector> #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "quiche/quic/core/crypto/quic_crypto_server_config.h" #include "quiche/quic/core/io/quic_default_event_loop.h" #include "quiche/quic/core/io/quic_event_loop.h" @@ -313,9 +314,10 @@ } { - EXPECT_CALL(client_packet_exchanger_, WritePacketToNetwork(_, _)) - .WillRepeatedly([this](const char* packet, size_t size) { - client_packets_to_network_.push_back(std::string(packet, size)); + EXPECT_CALL(client_packet_exchanger_, WritePacketToNetwork(_)) + .WillRepeatedly([this](absl::Span<const std::byte> packet) { + client_packets_to_network_.emplace_back( + reinterpret_cast<const char*>(packet.data()), packet.size()); }); client_connection_ = new QuicConnection(