From: Thomas Markwalder Date: Fri, 16 Feb 2024 13:10:38 +0000 (-0500) Subject: [#3230] Addressed review comments X-Git-Tag: Kea-2.5.6~98 X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=ff951201b431696382fbf3780802b2e6a9e00f51;p=thirdparty%2Fkea.git [#3230] Addressed review comments src/lib/dhcp/iface_mgr.* IfaceMgr::isSocketReceviedTimeSupported() - new function src/lib/dhcp/pkt_filter.* PktFilter::isSocketReceviedTimeSupported() - new pure virtual function src/lib/dhcp/pkt_filter6.cc src/lib/dhcp/pkt_filter_bpf.h src/lib/dhcp/pkt_filter_inet.h src/lib/dhcp/pkt_filter_inet6.h src/lib/dhcp/pkt_filter_lpf.h Added implementation of isSocketReceivedTimeSupported() src/lib/dhcp/pkt_filter_lpf.cc Removed WITH_CMSG and refactored receive() to be a single function with conditional codde based on SO_TIMESTAMP Updated UTs accordingly --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index e4b7798f47..01a1d63da5 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -320,6 +320,11 @@ IfaceMgr::isDirectResponseSupported() const { return (packet_filter_->isDirectResponseSupported()); } +bool +IfaceMgr::isSocketReceivedTimeSupported() const { + return (packet_filter_->isSocketReceivedTimeSupported()); +} + void IfaceMgr::addExternalSocket(int socketfd, SocketCallback callback) { if (socketfd < 0) { diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index c56b15ab1f..3966e4bea2 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -752,6 +752,14 @@ public: /// @return true if direct response is supported. bool isDirectResponseSupported() const; + /// @brief Check if the socket received time is supported. + /// + /// If true, then received packets will include a SOCKET_RECEIVED + /// event in their event stack. + /// + /// @return True if it is supported. + virtual bool isSocketReceivedTimeSupported() const; + /// @brief Returns interface specified interface index /// /// @param ifindex index of searched interface diff --git a/src/lib/dhcp/pkt_filter.cc b/src/lib/dhcp/pkt_filter.cc index 770d71d265..688931a5cb 100644 --- a/src/lib/dhcp/pkt_filter.cc +++ b/src/lib/dhcp/pkt_filter.cc @@ -67,6 +67,5 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr, return (sock); } - } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcp/pkt_filter.h b/src/lib/dhcp/pkt_filter.h index e175bef543..73bf9d383c 100644 --- a/src/lib/dhcp/pkt_filter.h +++ b/src/lib/dhcp/pkt_filter.h @@ -59,6 +59,16 @@ public: /// @return true of the direct response is supported. virtual bool isDirectResponseSupported() const = 0; + /// @brief Check if the socket received time is supported. + /// + /// If true, then packets received through this filter will include + /// a SOCKET_RECEIVED event in its event stack. Other than direct + /// clients using BPF for which this is always true, this function + /// is true provided SO_TIMESTAMP is defined. + /// + /// @return True if it is supported. + virtual bool isSocketReceivedTimeSupported() const = 0; + /// @brief Open primary and fallback socket. /// /// A method implementation in the derived class may open one or two diff --git a/src/lib/dhcp/pkt_filter6.cc b/src/lib/dhcp/pkt_filter6.cc index b72de5c2d6..bdf8025075 100644 --- a/src/lib/dhcp/pkt_filter6.cc +++ b/src/lib/dhcp/pkt_filter6.cc @@ -33,6 +33,5 @@ PktFilter6::joinMulticast(int sock, const std::string& ifname, return (true); } - } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcp/pkt_filter_bpf.h b/src/lib/dhcp/pkt_filter_bpf.h index 9413848d9c..1cf5deca23 100644 --- a/src/lib/dhcp/pkt_filter_bpf.h +++ b/src/lib/dhcp/pkt_filter_bpf.h @@ -65,6 +65,13 @@ public: return (true); } + /// @brief Check if the socket received time is supported. + /// + /// @return true always. + virtual bool isSocketReceivedTimeSupported() const { + return (true); + } + /// @brief Open primary and fallback socket. /// /// This method opens the BPF device and applies the following diff --git a/src/lib/dhcp/pkt_filter_inet.h b/src/lib/dhcp/pkt_filter_inet.h index 12f764e8fd..0af2b132bb 100644 --- a/src/lib/dhcp/pkt_filter_inet.h +++ b/src/lib/dhcp/pkt_filter_inet.h @@ -31,6 +31,20 @@ public: return (false); } + /// @brief Check if the socket received time is supported. + /// + /// If true, then packets received through this filter will include + /// a SOCKET_RECEIVED event in its event stack. + /// + /// @return True if SO_TIMESTAMP is defined. + virtual bool isSocketReceivedTimeSupported() const { +#ifdef SO_TIMESTAMP + return (true); +#else + return (false); +#endif + } + /// @brief Open primary and fallback socket. /// /// @param iface Interface descriptor. diff --git a/src/lib/dhcp/pkt_filter_inet6.h b/src/lib/dhcp/pkt_filter_inet6.h index 8d40a4483a..96fb072661 100644 --- a/src/lib/dhcp/pkt_filter_inet6.h +++ b/src/lib/dhcp/pkt_filter_inet6.h @@ -20,6 +20,19 @@ namespace dhcp { /// class to be used by @c IfaceMgr to access IPv6 sockets. class PktFilterInet6 : public PktFilter6 { public: + /// @brief Check if the socket received time is supported. + /// + /// If true, then packets received through this filter will include + /// a SOCKET_RECEIVED event in its event stack. + /// + /// @return True if SO_TIMESTAMP is defined. + virtual bool isSocketReceivedTimeSupported() const { +#ifdef SO_TIMESTAMP + return (true); +#else + return (false); +#endif + } /// @brief Opens a socket. /// diff --git a/src/lib/dhcp/pkt_filter_lpf.cc b/src/lib/dhcp/pkt_filter_lpf.cc index c58d8290d9..a7acc12571 100644 --- a/src/lib/dhcp/pkt_filter_lpf.cc +++ b/src/lib/dhcp/pkt_filter_lpf.cc @@ -17,8 +17,6 @@ #include #include -#define WITH_CMSG - namespace { using namespace isc::dhcp; @@ -223,7 +221,6 @@ PktFilterLPF::openSocket(Iface& iface, } -#ifndef WITH_CMSG Pkt4Ptr PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { uint8_t raw_buf[IfaceMgr::RCVBUFSIZE]; @@ -246,6 +243,7 @@ PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { datalen = recv(socket_info.fallbackfd_, raw_buf, sizeof(raw_buf), 0); } while (datalen > 0); +#ifndef SO_TIMESTAMP // Now that we finished getting data from the fallback socket, we // have to get the data from the raw socket too. int data_len = read(socket_info.sockfd_, raw_buf, sizeof(raw_buf)); @@ -259,73 +257,8 @@ PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { } InputBuffer buf(raw_buf, data_len); - - // @todo: This is awkward way to solve the chicken and egg problem - // whereby we don't know the offset where DHCP data start in the - // received buffer when we create the packet object. In general case, - // the IP header has variable length. The information about its length - // is stored in one of its fields. Therefore, we have to decode the - // packet to get the offset of the DHCP data. The dummy object is - // created so as we can pass it to the functions which decode IP stack - // and find actual offset of the DHCP data. - // Once we find the offset we can create another Pkt4 object from - // the reminder of the input buffer and set the IP addresses and - // ports from the dummy packet. We should consider doing it - // in some more elegant way. - Pkt4Ptr dummy_pkt = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 0)); - - // Decode ethernet, ip and udp headers. - decodeEthernetHeader(buf, dummy_pkt); - decodeIpUdpHeader(buf, dummy_pkt); - - // Read the DHCP data. - std::vector dhcp_buf; - buf.readVector(dhcp_buf, buf.getLength() - buf.getPosition()); - - // Decode DHCP data into the Pkt4 object. - Pkt4Ptr pkt = Pkt4Ptr(new Pkt4(&dhcp_buf[0], dhcp_buf.size())); - - // Set the appropriate packet members using data collected from - // the decoded headers. - pkt->setIndex(iface.getIndex()); - pkt->setIface(iface.getName()); - pkt->setLocalAddr(dummy_pkt->getLocalAddr()); - pkt->setRemoteAddr(dummy_pkt->getRemoteAddr()); - pkt->setLocalPort(dummy_pkt->getLocalPort()); - pkt->setRemotePort(dummy_pkt->getRemotePort()); - pkt->setLocalHWAddr(dummy_pkt->getLocalHWAddr()); - pkt->setRemoteHWAddr(dummy_pkt->getRemoteHWAddr()); - - // Set time packet was read from the buffer. - pkt->addPktEvent(PktEvent::BUFFER_READ); - - return (pkt); -} #else -Pkt4Ptr -PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { - uint8_t raw_buf[IfaceMgr::RCVBUFSIZE]; - // First let's get some data from the fallback socket. The data will be - // discarded but we don't want the socket buffer to bloat. We get the - // packets from the socket in loop but most of the time the loop will - // end after receiving one packet. The call to recv returns immediately - // when there is no data left on the socket because the socket is - // non-blocking. - // @todo In the normal conditions, both the primary socket and the fallback - // socket are in sync as they are set to receive packets on the same - // address and port. The reception of packets on the fallback socket - // shouldn't cause significant lags in packet reception. If we find in the - // future that it does, the sort of threshold could be set for the maximum - // bytes received on the fallback socket in a single round. Further - // optimizations would include an asynchronous read from the fallback socket - // when the DHCP server is idle. - int datalen; - do { - datalen = recv(socket_info.fallbackfd_, raw_buf, sizeof(raw_buf), 0); - } while (datalen > 0); - const size_t CONTROL_BUF_LEN = 512; - uint8_t msg_buf[IfaceMgr::RCVBUFSIZE]; uint8_t control_buf[CONTROL_BUF_LEN]; @@ -356,6 +289,7 @@ PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { } InputBuffer buf(msg_buf, result); +#endif // @todo: This is awkward way to solve the chicken and egg problem // whereby we don't know the offset where DHCP data start in the @@ -393,6 +327,7 @@ PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { pkt->setLocalHWAddr(dummy_pkt->getLocalHWAddr()); pkt->setRemoteHWAddr(dummy_pkt->getRemoteHWAddr()); +#ifdef SO_TIMESTAMP struct cmsghdr* cmsg = CMSG_FIRSTHDR(&m); while (cmsg != NULL) { if ((cmsg->cmsg_level == SOL_SOCKET) && @@ -406,13 +341,13 @@ PktFilterLPF::receive(Iface& iface, const SocketInfo& socket_info) { cmsg = CMSG_NXTHDR(&m, cmsg); } +#endif // Set time packet was read from the buffer. pkt->addPktEvent(PktEvent::BUFFER_READ); return (pkt); } -#endif int PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) { @@ -462,6 +397,5 @@ PktFilterLPF::send(const Iface& iface, uint16_t sockfd, const Pkt4Ptr& pkt) { } - } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcp/pkt_filter_lpf.h b/src/lib/dhcp/pkt_filter_lpf.h index 04c2181f0a..5cb9822bcc 100644 --- a/src/lib/dhcp/pkt_filter_lpf.h +++ b/src/lib/dhcp/pkt_filter_lpf.h @@ -32,6 +32,20 @@ public: return (true); } + /// @brief Check if the socket received time is supported. + /// + /// If true, then packets received through this filter will include + /// a SOCKET_RECEIVED event in its event stack. + /// + /// @return True if SO_TIMESTAMP is defined. + virtual bool isSocketReceivedTimeSupported() const { +#ifdef SO_TIMESTAMP + return (true); +#else + return (false); +#endif + } + /// @brief Open primary and fallback socket. /// /// @param iface Interface descriptor. diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 7814e06c62..0ec0c47f8a 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -144,6 +144,10 @@ public: return (false); } + virtual bool isSocketReceivedTimeSupported() const { + return (false); + } + /// @brief Pretend to open a socket. /// /// This function doesn't open a real socket. It always returns the diff --git a/src/lib/dhcp/tests/pkt_filter6_test_utils.cc b/src/lib/dhcp/tests/pkt_filter6_test_utils.cc index 44ee44de52..9302b8b0d4 100644 --- a/src/lib/dhcp/tests/pkt_filter6_test_utils.cc +++ b/src/lib/dhcp/tests/pkt_filter6_test_utils.cc @@ -172,6 +172,18 @@ PktFilter6Test::testRcvdMessage(const Pkt6Ptr& rcvd_msg) const { EXPECT_EQ(test_message_->getTransid(), rcvd_msg->getTransid()); } +void +PktFilter6Test::testReceivedPktEvents(const PktPtr& msg, + bool so_time_supported) const { + std::list expected_events; + if (so_time_supported) { + expected_events.push_back(PktEvent::SOCKET_RECEIVED); + } + + expected_events.push_back(PktEvent::BUFFER_READ); + testPktEvents(msg, start_time_, expected_events); +} + void PktFilter6Test::testPktEvents(const PktPtr& msg, ptime start_time, std::list expected_events) const { @@ -183,6 +195,7 @@ PktFilter6Test::testPktEvents(const PktPtr& msg, ptime start_time, for (auto const& event : events) { ASSERT_EQ(event.label_, *expected_event); EXPECT_GE(event.timestamp_, prev_time); + prev_time = event.timestamp_; ++expected_event; } } diff --git a/src/lib/dhcp/tests/pkt_filter6_test_utils.h b/src/lib/dhcp/tests/pkt_filter6_test_utils.h index ba4ecc68a1..a2e06205e8 100644 --- a/src/lib/dhcp/tests/pkt_filter6_test_utils.h +++ b/src/lib/dhcp/tests/pkt_filter6_test_utils.h @@ -73,6 +73,15 @@ public: /// @param rcvd_msg An instance of the message to be tested. void testRcvdMessage(const Pkt6Ptr& rcvd_msg) const; + /// @brief Checks that a received message has the appropriate events + /// in it's event stack. + /// + /// @param rcvd_msg An instance of the message to be tested. + /// @param so_time_supported If true the event stack should have with + /// a SOCKET_RECEIVED event followed by a BUFFER_READ event, if false + /// it should have only the latter. + void testReceivedPktEvents(const PktPtr& msg, bool so_time_supported) const; + /// @brief Checks the contents of a packet's event stack agains a list /// of expected events. /// diff --git a/src/lib/dhcp/tests/pkt_filter_bpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_bpf_unittest.cc index 0ecf70b567..123976055e 100644 --- a/src/lib/dhcp/tests/pkt_filter_bpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_bpf_unittest.cc @@ -196,10 +196,8 @@ TEST_F(PktFilterBPFTest, receive) { testRcvdMessage(rcvd_pkt); testRcvdMessageAddressPort(rcvd_pkt); - // Verify that the packet event stack has SOCKET_RECEIVED and BUFFER_READ events. - testPktEvents(rcvd_pkt, start_time_, - std::list{PktEvent::SOCKET_RECEIVED, - PktEvent::BUFFER_READ}); + // Verify that the packet event stack is as expected. + testReceivedPktEvents (rcvd_pkt, pkt_filter.isSocketReceivedTimeSupported()); } // This test verifies that if the packet is received over the raw diff --git a/src/lib/dhcp/tests/pkt_filter_inet6_unittest.cc b/src/lib/dhcp/tests/pkt_filter_inet6_unittest.cc index 17730ee6b9..d3fea68792 100644 --- a/src/lib/dhcp/tests/pkt_filter_inet6_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_inet6_unittest.cc @@ -132,10 +132,8 @@ TEST_F(PktFilterInet6Test, receive) { // Check if the received message is correct. testRcvdMessage(rcvd_pkt); - // Verify that the packet event stack has SOCKET_RECEIVED and BUFFER_READ events. - testPktEvents(rcvd_pkt, start_time_, - std::list{PktEvent::SOCKET_RECEIVED, - PktEvent::BUFFER_READ}); + // Verify that the packet event stack is as expected. + testReceivedPktEvents (rcvd_pkt, pkt_filter.isSocketReceivedTimeSupported()); } } // anonymous namespace diff --git a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc index 9bd1c9b0e1..b0b75344d4 100644 --- a/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_inet_unittest.cc @@ -147,10 +147,8 @@ TEST_F(PktFilterInetTest, receive) { testRcvdMessage(rcvd_pkt); testRcvdMessageAddressPort(rcvd_pkt); - // Verify that the packet event stack has SOCKET_RECEIVED and BUFFER_READ events. - testPktEvents(rcvd_pkt, start_time_, - std::list{PktEvent::SOCKET_RECEIVED, - PktEvent::BUFFER_READ}); + // Verify that the packet event stack is as expected. + testReceivedPktEvents (rcvd_pkt, pkt_filter.isSocketReceivedTimeSupported()); } } // anonymous namespace diff --git a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc index 39fd0a1ef0..e2e0b4ff56 100644 --- a/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc +++ b/src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc @@ -183,10 +183,8 @@ TEST_F(PktFilterLPFTest, receive) { testRcvdMessage(rcvd_pkt); testRcvdMessageAddressPort(rcvd_pkt); - // Verify that the packet event stack has SOCKET_RECEIVED and BUFFER_READ events. - testPktEvents(rcvd_pkt, start_time_, - std::list{PktEvent::SOCKET_RECEIVED, - PktEvent::BUFFER_READ}); + // Verify that the packet event stack is as expected. + testReceivedPktEvents (rcvd_pkt, pkt_filter.isSocketReceivedTimeSupported()); } // This test verifies that if the packet is received over the raw diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.cc b/src/lib/dhcp/tests/pkt_filter_test_utils.cc index 6be7505bd5..950f904d9f 100644 --- a/src/lib/dhcp/tests/pkt_filter_test_utils.cc +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.cc @@ -170,6 +170,18 @@ PktFilterTest::testRcvdMessageAddressPort(const Pkt4Ptr& rcvd_msg) const { EXPECT_EQ(test_message_->getLocalPort(), rcvd_msg->getRemotePort()); } +void +PktFilterTest::testReceivedPktEvents(const PktPtr& msg, + bool so_time_supported) const { + std::list expected_events; + if (so_time_supported) { + expected_events.push_back(PktEvent::SOCKET_RECEIVED); + } + + expected_events.push_back(PktEvent::BUFFER_READ); + testPktEvents(msg, start_time_, expected_events); +} + void PktFilterTest::testPktEvents(const PktPtr& msg, ptime start_time, std::list expected_events) const { @@ -181,6 +193,7 @@ PktFilterTest::testPktEvents(const PktPtr& msg, ptime start_time, for (auto const& event : events) { ASSERT_EQ(event.label_, *expected_event); EXPECT_GE(event.timestamp_, prev_time); + prev_time = event.timestamp_; ++expected_event; } } @@ -190,6 +203,11 @@ PktFilterStub::isDirectResponseSupported() const { return (true); } +bool +PktFilterStub::isSocketReceivedTimeSupported() const { + return (true); +} + SocketInfo PktFilterStub::openSocket(Iface&, const isc::asiolink::IOAddress& addr, diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.h b/src/lib/dhcp/tests/pkt_filter_test_utils.h index a5d8838bb9..ef7574e6b4 100644 --- a/src/lib/dhcp/tests/pkt_filter_test_utils.h +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.h @@ -82,6 +82,15 @@ public: /// @param rcvd_msg An instance of the message to be tested. void testRcvdMessageAddressPort(const Pkt4Ptr& rcvd_msg) const; + /// @brief Checks that a received message has the appropriate events + /// in it's event stack. + /// + /// @param rcvd_msg An instance of the message to be tested. + /// @param so_time_supported If true the event stack should have with + /// a SOCKET_RECEIVED event followed by a BUFFER_READ event, if false + /// it should have only the latter. + void testReceivedPktEvents(const PktPtr& msg, bool so_time_supported) const; + /// @brief Checks the contents of a packet's event stack agains a list /// of expected events. /// @@ -130,6 +139,14 @@ public: /// @return always true. virtual bool isDirectResponseSupported() const; + /// @brief Check if the socket received time is supported. + /// + /// If true, then packets received through this filter will include + /// a SOCKET_RECEIVED event in its event stack. + /// + /// @return always true. + virtual bool isSocketReceivedTimeSupported() const; + /// @brief Simulate opening of the socket. /// /// This function simulates opening a primary socket. In reality, it doesn't diff --git a/src/lib/dhcp/testutils/pkt_filter_test_stub.cc b/src/lib/dhcp/testutils/pkt_filter_test_stub.cc index 769085a64d..2cc511e240 100644 --- a/src/lib/dhcp/testutils/pkt_filter_test_stub.cc +++ b/src/lib/dhcp/testutils/pkt_filter_test_stub.cc @@ -24,6 +24,15 @@ PktFilterTestStub::isDirectResponseSupported() const { return (direct_response_supported_); } +bool +PktFilterTestStub::isSocketReceivedTimeSupported() const { +#ifdef SO_TIMESTAMP + return(true); +#else + return(false); +#endif +} + SocketInfo PktFilterTestStub::openSocket(Iface&, const isc::asiolink::IOAddress& addr, diff --git a/src/lib/dhcp/testutils/pkt_filter_test_stub.h b/src/lib/dhcp/testutils/pkt_filter_test_stub.h index 084c86b0a0..3607b22a74 100644 --- a/src/lib/dhcp/testutils/pkt_filter_test_stub.h +++ b/src/lib/dhcp/testutils/pkt_filter_test_stub.h @@ -42,6 +42,16 @@ public: /// @return always true. virtual bool isDirectResponseSupported() const; + /// @brief Check if the socket received time is supported. + /// + /// If true, then packets received through this filter will include + /// a SOCKET_RECEIVED event in its event stack. Other than direct + /// clients using BPF for which this is always true, this function + /// is true provided SO_TIMESTAMP is defined. + /// + /// @return True if it is supported. + virtual bool isSocketReceivedTimeSupported() const; + /// @brief Simulate opening of the socket. /// /// This function simulates opening a primary socket. Rather than open