Commit 9976c775 authored by Tomek Mrugalski's avatar Tomek Mrugalski 🛰

[3794] Changes after review:

 - Pkt4::check() removed
 - commented recent change in Pkt4::setType()
 - Pkt4::setType() now used OptionInt<uint8> rather than
   Option::setUint8()
parent 055a5b32
......@@ -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);
......
......@@ -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);
}
}
......
......@@ -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
......
......@@ -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)",
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment