[3794] Changes after review:

 - Pkt4::check() removed
 - commented recent change in Pkt4::setType()
 - Pkt4::setType() now used OptionInt<uint8> rather than
   Option::setUint8()

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index c398fe1fc61a507b1a303a956dfb9672dbfc627f..1acb14f3ce4b4bf523f3a1a124958607bbbb97f4 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -405,7 +405,8 @@ Dhcpv4Srv::run() {
         // failures in unpacking will cause the packet to be dropped. We
         // will increase type specific packets further down the road.
         // See processStatsReceived().
-        isc::stats::StatsMgr::instance().addValue("pkt4-received", 1ul);
+        isc::stats::StatsMgr::instance().addValue("pkt4-received",
+                                                  static_cast<uint64_t>(1));
 
         // In order to parse the DHCP options, the server needs to use some
         // configuration information such as: existing option spaces, option
@@ -467,8 +468,10 @@ Dhcpv4Srv::run() {
                     .arg(e.what());
 
                 // Increase the statistics of parse failues and dropped packets.
-                isc::stats::StatsMgr::instance().addValue("pkt4-parse-failed", 1ul);
-                isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop", 1ul);
+                isc::stats::StatsMgr::instance().addValue("pkt4-parse-failed",
+                                                          static_cast<uint64_t>(1));
+                isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                          static_cast<uint64_t>(1));
                 continue;
             }
         }
@@ -485,7 +488,8 @@ Dhcpv4Srv::run() {
         // There is no need to log anything here. This function logs by itself.
         if (!accept(query)) {
             // Increase the statistic of dropped packets.
-            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop", 1ul);
+            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                      static_cast<uint64_t>(1));
             continue;
         }
 
@@ -574,7 +578,8 @@ Dhcpv4Srv::run() {
                 .arg(e.what());
 
             // Increase the statistic of dropped packets.
-            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop", 1ul);
+            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                      static_cast<uint64_t>(1));
         }
 
         if (!rsp) {
@@ -1056,7 +1061,7 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) {
         // clients do not handle the hostnames with the trailing dot.
         opt_hostname_resp->setValue(d2_mgr.qualifyName(hostname, false));
     }
-    
+
     LOG_DEBUG(ddns_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_RESPONSE_HOSTNAME_DATA)
         .arg(ex.getQuery()->getLabel())
         .arg(opt_hostname_resp->getValue());
@@ -2289,7 +2294,8 @@ void Dhcpv4Srv::processStatsReceived(const Pkt4Ptr& query) {
         // name of pkt4-unknown-received.
     }
 
-    isc::stats::StatsMgr::instance().addValue(stat_name, 1ul);
+    isc::stats::StatsMgr::instance().addValue(stat_name,
+                                              static_cast<uint64_t>(1));
 }
 
 void Dhcpv4Srv::processStatsSent(const Pkt4Ptr& response) {
@@ -2311,7 +2317,7 @@ void Dhcpv4Srv::processStatsSent(const Pkt4Ptr& response) {
         break;
     default:
         // That should never happen
-        stat_name = "pkt4-unknown-received";
+        return;
     }
 
     isc::stats::StatsMgr::instance().addValue(stat_name, 1ul);

diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index a9565b788d3b9b323ab851e84eca8d29762c0cf3..5c7fa31af243d21357d6049518a49069fe1913fb 100644 (file)
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -244,14 +244,6 @@ Pkt4::unpack() {
     // so we'll be able to log more detailed drop reason.
 }
 
-void Pkt4::check() {
-    uint8_t msg_type = getType();
-    if (msg_type > DHCPLEASEACTIVE) {
-        isc_throw(BadValue, "Invalid DHCP message type received: "
-                  << static_cast<int>(msg_type));
-    }
-}
-
 uint8_t Pkt4::getType() const {
     OptionPtr generic = getOption(DHO_DHCP_MESSAGE_TYPE);
     if (!generic) {
@@ -273,18 +265,29 @@ void Pkt4::setType(uint8_t dhcp_type) {
     OptionPtr opt = getOption(DHO_DHCP_MESSAGE_TYPE);
     if (opt) {
 
-        // There is message type option already, update it
+        // There is message type option already, update it. It seems that
+        // we do have two types of objects representing message-type option.
+        // It would be more preferable to use only one type, but there's no
+        // easy way to enforce it.
+        //
+        // One is an instance of the Option class. It stores type in
+        // Option::data_, so Option::setUint8() and Option::getUint8() can be
+        // used. The other one is an instance of OptionInt<uint8_t> and
+        // it stores message type as integer, hence
+        // OptionInt<uint8_t>::getValue() and OptionInt<uint8_t>::setValue()
+        // should be used.
         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));
+            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);
-        opt = OptionPtr(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE, tmp));
+        opt = OptionPtr(new OptionInt<uint8_t>(Option::V4, DHO_DHCP_MESSAGE_TYPE,
+                                               tmp.begin(), tmp.end()));
         addOption(opt);
     }
 }

diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h
index 8dfa312d76017db9e8bef94e770be51cda6da9d5..efcd749a06770f53cff670f5f080b98d39f9654d 100644 (file)
--- a/src/lib/dhcp/pkt4.h
+++ b/src/lib/dhcp/pkt4.h
@@ -96,19 +96,6 @@ public:
     /// Method with throw exception if packet parsing fails.
     virtual void unpack();
 
-    /// @brief performs sanity check on a packet.
-    ///
-    /// This is usually performed after unpack(). It checks if packet is sane:
-    /// required options are present, fields have sane content etc.
-    /// For example verifies that DHCP_MESSAGE_TYPE is present and have
-    /// reasonable value. This method is expected to grow significantly.
-    /// It makes sense to separate unpack() and check() for testing purposes.
-    ///
-    /// @todo It is called from unpack() directly. It should be separated.
-    ///
-    /// Method will throw exception if anomaly is found.
-    void check();
-
     /// @brief Returns text representation of the primary packet identifiers
     ///
     /// This method is intended to be used to provide a consistent way to

diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index b954d6bc4bf91a4dcfbd4ec3bdeba16fb2a5b246..f7d9ed1ef3d62204dc2ad32d66b5af8e58b11b58 100644 (file)
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -978,7 +978,7 @@ TEST_F(Pkt4Test, toText) {
     EXPECT_EQ("local_address=192.0.2.34:67, remote_adress=192.10.33.4:68, "
               "msg_type=DHCPDISCOVER (1), transid=0x9ef,\n"
               "options:\n"
-              "  type=053, len=001: 01\n"
+              "  type=053, len=001: 1 (uint8)\n"
               "  type=087, len=011: \"lorem ipsum\" (string)\n"
               "  type=123, len=004: 192.0.2.3\n"
               "  type=156, len=004: 123456 (uint32)",