From: Tomek Mrugalski Date: Tue, 9 Jun 2015 14:48:51 +0000 (+0200) Subject: [3794] Changes after review: X-Git-Tag: throw_with_base~3^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9976c77569831e182c1fe3ef56305185c9bf0ddb;p=thirdparty%2Fkea.git [3794] Changes after review: - Pkt4::check() removed - commented recent change in Pkt4::setType() - Pkt4::setType() now used OptionInt rather than Option::setUint8() --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index c398fe1fc6..1acb14f3ce 100644 --- 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(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(1)); + isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop", + static_cast(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(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(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(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 a9565b788d..5c7fa31af2 100644 --- 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(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 and + // it stores message type as integer, hence + // OptionInt::getValue() and OptionInt::setValue() + // should be used. boost::shared_ptr > type_opt = boost::dynamic_pointer_cast >(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 tmp(1, dhcp_type); - opt = OptionPtr(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE, tmp)); + opt = OptionPtr(new OptionInt(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 8dfa312d76..efcd749a06 100644 --- 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 b954d6bc4b..f7d9ed1ef3 100644 --- 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)",