[3794] Many pkt4-receive-* statistics implemented

src/bin/dhcp4/dhcp4_srv.cc

@@ -480,6 +480,7 @@ Dhcpv4Srv::run() {
         // Check whether the message should be further processed or discarded.
         // There is no need to log anything here. This function logs by itself.
         if (!accept(query)) {
+            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop", 1ul);
             continue;
         }
 

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -3510,37 +3510,61 @@ TEST_F(Dhcpv4SrvTest, acceptMessageType) {
     }
 }
 
+// Test checks whether statistic is bumped up appropriately when Decline
+// message is received.
 TEST_F(Dhcpv4SrvTest, statisticsDecline) {
-    IfaceMgrTestConfig test_config(true);
-    IfaceMgr::instance().openSockets4();
+    NakedDhcpv4Srv srv(0);
 
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPDECLINE, "pkt4-decline-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Offer
+// message is received (this should never happen in a sane metwork).
+TEST_F(Dhcpv4SrvTest, statisticsOfferRcvd) {
     NakedDhcpv4Srv srv(0);
 
-    // Use of the captured DHCPDISCOVER packet requires that
-    // subnet 10.254.226.0/24 is in use, because this packet
-    // contains the giaddr which belongs to this subnet and
-    // this giaddr is used to select the subnet
-    configure(CONFIGS[0]);
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPOFFER, "pkt4-offer-received");
+}
 
-    Pkt4Ptr decline = PktCaptures::captureRelayedDiscover();
-    decline->setType(DHCPDECLINE);
+// Test checks whether statistic is bumped up appropriately when Ack
+// message is received (this should never happen in a sane metwork).
+TEST_F(Dhcpv4SrvTest, statisticsAckRcvd) {
+    NakedDhcpv4Srv srv(0);
 
-    // Simulate that we have received that traffic
-    srv.fakeReceive(decline);
-    srv.run();
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPACK, "pkt4-ack-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Nak
+// message is received (this should never happen in a sane metwork).
+TEST_F(Dhcpv4SrvTest, statisticsNakRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPNAK, "pkt4-nak-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Release
+// message is received.
+TEST_F(Dhcpv4SrvTest, statisticsReleaseRcvd) {
+    NakedDhcpv4Srv srv(0);
 
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPRELEASE, "pkt4-release-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when unknown
+// message is received.
+TEST_F(Dhcpv4SrvTest, statisticsUnknownRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], 200, "pkt4-unknown-received");
+
+    // There should also be pkt4-receive-drop stat bumped up
     using namespace isc::stats;
     StatsMgr& mgr = StatsMgr::instance();
-    ObservationPtr pkt4_rcvd = mgr.getObservation("pkt4-received");
-    ObservationPtr pkt4_decline_rcvd = mgr.getObservation("pkt4-decline-received");
-
-    // All expected statstics must be present.
-    ASSERT_TRUE(pkt4_rcvd);
-    ASSERT_TRUE(pkt4_decline_rcvd);
+    ObservationPtr drop_stat = mgr.getObservation("pkt4-receive-drop");
 
-    // They also must have expected values.
-    EXPECT_EQ(1, pkt4_rcvd->getInteger().first);
-    EXPECT_EQ(1, pkt4_decline_rcvd->getInteger().first);
+    // This statistic must be present and must be set to 1.
+    ASSERT_TRUE(drop_stat);
+    EXPECT_EQ(1, drop_stat->getInteger().first);
 }
 
 }; // end of anonymous namespace

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -25,6 +25,7 @@
 #include <dhcp/option_custom.h>
 #include <dhcp/iface_mgr.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
+#include <dhcp/tests/pkt_captures.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease.h>
 #include <dhcpsrv/lease_mgr.h>
@@ -601,6 +602,50 @@ Dhcpv4SrvTest::createExchange(const Pkt4Ptr& query) {
     return (Dhcpv4Exchange(srv_.alloc_engine_, query, srv_.selectSubnet(query)));
 }
 
+void
+Dhcpv4SrvTest::pretendReceivingPkt(NakedDhcpv4Srv& srv, const std::string& config,
+                                   uint8_t pkt_type, const std::string& stat_name) {
+
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Apply the configuration we just received.
+    configure(config);
+
+    // Let's just use one of the actual captured packets that we have.
+    Pkt4Ptr pkt = isc::test::PktCaptures::captureRelayedDiscover();
+
+    // We just need to tweak it a it, to pretend that it's type is as desired.
+    // Note that when receiving a packet, its on-wire form is stored in data_
+    // field. Most methods (including setType()) operates on option objects
+    // (objects stored in options_ after unpack() is called). Finally, outgoing
+    // packets are stored in out_buffer_. So we need to go through the full
+    // unpack/tweak/pack cycle and do repack, i.e. move the output buffer back
+    // to incoming buffer.
+    pkt->unpack();
+    pkt->setType(pkt_type); // Set message type.
+    pkt->pack();
+    pkt->data_.resize(pkt->getBuffer().getLength());
+    // Copy out_buffer_ to data_ to pretend that it's what was just received.
+    memcpy(&pkt->data_[0], pkt->getBuffer().getData(), pkt->getBuffer().getLength());
+
+    // Simulate that we have received that traffic
+    srv.fakeReceive(pkt);
+    srv.run();
+
+    using namespace isc::stats;
+    StatsMgr& mgr = StatsMgr::instance();
+    ObservationPtr pkt4_rcvd = mgr.getObservation("pkt4-received");
+    ObservationPtr tested_stat = mgr.getObservation(stat_name);
+
+    // All expected statstics must be present.
+    ASSERT_TRUE(pkt4_rcvd);
+    ASSERT_TRUE(tested_stat);
+
+    // They also must have expected values.
+    EXPECT_EQ(1, pkt4_rcvd->getInteger().first);
+    EXPECT_EQ(1, tested_stat->getInteger().first);
+}
 
 }; // end of isc::dhcp::test namespace
 }; // end of isc::dhcp namespace

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -391,6 +391,20 @@ public:
     void configure(const std::string& config, NakedDhcpv4Srv& srv,
                    const bool commit = true);
 
+    /// @brief Pretents a packet of specified type was received.
+    ///
+    /// Instantiates fake network interfaces, configures passed Dhcpv4Srv,
+    /// then creates a message of specified type and sends it to the
+    /// server and then checks whether expected statstics were set
+    /// appropriately.
+    ///
+    /// @param srv the DHCPv4 server to be used
+    /// @param config JSON configuration to be used
+    /// @param pkt_type type of the packet to be faked
+    /// @param stat_name name of the expected statistic
+    void pretendReceivingPkt(NakedDhcpv4Srv& srv, const std::string& config,
+                             uint8_t pkt_type, const std::string& stat_name);
+
     /// @brief Create @c Dhcpv4Exchange from client's query.
     Dhcpv4Exchange createExchange(const Pkt4Ptr& query);
 

src/lib/dhcp/pkt4.cc

@@ -239,9 +239,9 @@ Pkt4::unpack() {
     // }
     (void)offset;
 
-    // @todo check will need to be called separately, so hooks can be called
-    // after the packet is parsed, but before its content is verified
-    check();
+    // No need to call check() here. There are thorough tests for this
+    // later (see Dhcp4Srv::accept()). We want to drop the packet later,
+    // so we'll be able to log more detailed drop reason.
 }
 
 void Pkt4::check() {
@@ -272,8 +272,15 @@ uint8_t Pkt4::getType() const {
 void Pkt4::setType(uint8_t dhcp_type) {
     OptionPtr opt = getOption(DHO_DHCP_MESSAGE_TYPE);
     if (opt) {
+
         // There is message type option already, update it
-        opt->setUint8(dhcp_type);
+        boost::shared_ptr<OptionInt<uint8_t> > type_opt =
+            boost::dynamic_pointer_cast<OptionInt<uint8_t> >(opt);
+        if (type_opt) {
+            return (type_opt->setValue(dhcp_type));
+        } else {
+            opt->setUint8(dhcp_type);
+        }
     } else {
         // There is no message type option yet, add it
         std::vector<uint8_t> tmp(1, dhcp_type);