]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[#3230] Addressed review comments
authorThomas Markwalder <tmark@isc.org>
Fri, 16 Feb 2024 13:10:38 +0000 (08:10 -0500)
committerThomas Markwalder <tmark@isc.org>
Fri, 16 Feb 2024 13:10:38 +0000 (08:10 -0500)
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

21 files changed:
src/lib/dhcp/iface_mgr.cc
src/lib/dhcp/iface_mgr.h
src/lib/dhcp/pkt_filter.cc
src/lib/dhcp/pkt_filter.h
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.cc
src/lib/dhcp/pkt_filter_lpf.h
src/lib/dhcp/tests/iface_mgr_unittest.cc
src/lib/dhcp/tests/pkt_filter6_test_utils.cc
src/lib/dhcp/tests/pkt_filter6_test_utils.h
src/lib/dhcp/tests/pkt_filter_bpf_unittest.cc
src/lib/dhcp/tests/pkt_filter_inet6_unittest.cc
src/lib/dhcp/tests/pkt_filter_inet_unittest.cc
src/lib/dhcp/tests/pkt_filter_lpf_unittest.cc
src/lib/dhcp/tests/pkt_filter_test_utils.cc
src/lib/dhcp/tests/pkt_filter_test_utils.h
src/lib/dhcp/testutils/pkt_filter_test_stub.cc
src/lib/dhcp/testutils/pkt_filter_test_stub.h

index e4b7798f4731f593e497b505cb0ef4ccf97c7f57..01a1d63da5d17550bd77dfce938d535516a84b3a 100644 (file)
@@ -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) {
index c56b15ab1f4428b4ffeda67d6e0e4af84fe9a495..3966e4bea2cdb7148dc49b2de572e72046f0ec07 100644 (file)
@@ -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
index 770d71d2654c24222dbce9025b8900f341acac64..688931a5cb2a924c97d8b042b2216c6ca0326d74 100644 (file)
@@ -67,6 +67,5 @@ PktFilter::openFallbackSocket(const isc::asiolink::IOAddress& addr,
     return (sock);
 }
 
-
 } // end of isc::dhcp namespace
 } // end of isc namespace
index e175bef54354218dc8baaed04abf508f6f5ad1f3..73bf9d383c1981beeaf0573b954a531d5af63c77 100644 (file)
@@ -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
index b72de5c2d6a8418a15c5a677b15bca37a06a7f4e..bdf80250752d39bf5571cce0ffe852ac0556eb42 100644 (file)
@@ -33,6 +33,5 @@ PktFilter6::joinMulticast(int sock, const std::string& ifname,
     return (true);
 }
 
-
 } // end of isc::dhcp namespace
 } // end of isc namespace
index 9413848d9cdd720d490924c1d03ddc0233d3d76e..1cf5deca23321d67a45725ea8e0b37d49f2f2d99 100644 (file)
@@ -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
index 12f764e8fdcdded30df276640aa355a1baa9aaee..0af2b132bbb323b3f9bdba52e4c83931db03ffc6 100644 (file)
@@ -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.
index 8d40a4483aa53c36b34cdab7815578ee68b358a1..96fb0726616065ce90181ad81e92c48e5742ea2a 100644 (file)
@@ -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.
     ///
index c58d8290d9a9b95b7c6b4a5530b598053816cded..a7acc12571dd1ac7c13ff645abc5de328ea7b2d5 100644 (file)
@@ -17,8 +17,6 @@
 #include <linux/if_ether.h>
 #include <linux/if_packet.h>
 
-#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<uint8_t> 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
index 04c2181f0a500a671f0a3aad21d4e131025e21a5..5cb9822bccb4ffe2a828d26d1c3c4e2df1bf1284 100644 (file)
@@ -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.
index 7814e06c62014cb70f0bf17823db22f4b50f0f0c..0ec0c47f8a1d914bbad8c3992c13d6d59c0ea161 100644 (file)
@@ -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
index 44ee44de52afb84b98694e0d42c2bee6021a8d1a..9302b8b0d4d5dc1990806b1b52f06a360ea10d35 100644 (file)
@@ -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<std::string> 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<std::string> 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;
     }
 }
index ba4ecc68a14e873178812ebf89f6b81772a8f12c..a2e06205e8f09a36e62f59398c72c8fbe4083804 100644 (file)
@@ -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.
     ///
index 0ecf70b5673b267d98331e17e79810a2ba6c5eb7..123976055ee184dec9b1ebc9127901882d716f7a 100644 (file)
@@ -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<std::string>{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
index 17730ee6b99fa26365d3f81e019d98bc98f6b707..d3fea68792f68f3df3c9b79df55e82cd5651d3d9 100644 (file)
@@ -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<std::string>{PktEvent::SOCKET_RECEIVED,
-                                         PktEvent::BUFFER_READ});
+    // Verify that the packet event stack is as expected.
+    testReceivedPktEvents (rcvd_pkt, pkt_filter.isSocketReceivedTimeSupported());
 }
 
 } // anonymous namespace
index 9bd1c9b0e1a74c9e24844bfcd4789d6d08528167..b0b75344d42047c3f88b73cd93837fabe76a8c96 100644 (file)
@@ -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<std::string>{PktEvent::SOCKET_RECEIVED,
-                                         PktEvent::BUFFER_READ});
+    // Verify that the packet event stack is as expected.
+    testReceivedPktEvents (rcvd_pkt, pkt_filter.isSocketReceivedTimeSupported());
 }
 
 } // anonymous namespace
index 39fd0a1ef031de21b323826d8ff26d07d0373285..e2e0b4ff56b248e937ea4bff93b936ca3d3d5efc 100644 (file)
@@ -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<std::string>{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
index 6be7505bd52914e4fc8cd8bfab38c9cc89faf0d5..950f904d9f92e1e227fc92ddd0d683264e3ab2d6 100644 (file)
@@ -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<std::string> 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<std::string> 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,
index a5d8838bb977e336683e871c4eb36aa1acdb610c..ef7574e6b4ae1ad86abca59014f2c31e41cbfba2 100644 (file)
@@ -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
index 769085a64d4c46d7c2804ac58560f9ca4a5287c2..2cc511e2403d5856de4d850761dd9f2852a4df30 100644 (file)
@@ -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,
index 084c86b0a07e57b70885fff8b73c6c6665166a04..3607b22a74f02b9503a9f2344c5e97b1d091297e 100644 (file)
@@ -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