[2320] Pkt4::setType() is now implemented properly.

src/bin/dhcp4/dhcp4_messages.mes

@@ -79,7 +79,8 @@ incicates successful operation.
 % DHCP4_LEASE_ALLOC_FAIL failed to grant a lease for client-id %1, hwaddr %2
 This message indicates that the server failed to grant a lease to the
 specified client after receiving a REQUEST message from it.  There are many
-possible reasons for such a failure
+possible reasons for such a failure. Additional messages will indicate the
+reason.
 
 % DHCP4_NOT_RUNNING IPv4 DHCP server is not running
 A warning message is issued when an attempt is made to shut down the

src/bin/dhcp4/dhcp4_srv.cc

@@ -223,10 +223,7 @@ void Dhcpv4Srv::appendDefaultOptions(Pkt4Ptr& msg, uint8_t msg_type) {
     OptionPtr opt;
 
     // add Message Type Option (type 53)
-    std::vector<uint8_t> tmp;
-    tmp.push_back(static_cast<uint8_t>(msg_type));
-    opt = OptionPtr(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE, tmp));
-    msg->addOption(opt);
+    msg->setType(msg_type);
 
     // DHCP Server Identifier (type 54)
     msg->addOption(getServerID());
@@ -267,7 +264,7 @@ void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
         LOG_ERROR(dhcp4_logger, DHCP4_SUBNET_SELECTION_FAILED)
             .arg(question->getRemoteAddr().toText())
             .arg(serverReceivedPacketName(question->getType()));
-        setMsgType(answer, DHCPNAK);
+        answer->setType(DHCPNAK);
         answer->setYiaddr(IOAddress("0.0.0.0"));
         return;
     } else {
@@ -343,7 +340,7 @@ void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
             .arg(hwaddr?hwaddr->toText():"(no hwaddr info)")
             .arg(hint.toText());
 
-        setMsgType(answer, DHCPNAK);
+        answer->setType(DHCPNAK);
         answer->setYiaddr(IOAddress("0.0.0.0"));
     }
 }
@@ -357,19 +354,6 @@ OptionPtr Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {
     return (opt);
 }
 
-void Dhcpv4Srv::setMsgType(Pkt4Ptr& pkt, uint8_t dhcp_type) {
-    OptionPtr opt = pkt->getOption(DHO_DHCP_MESSAGE_TYPE);
-    if (opt) {
-        // There is message type option already, update it
-        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));
-        pkt->addOption(opt);
-    }
-}
-
 Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
     Pkt4Ptr offer = Pkt4Ptr
         (new Pkt4(DHCPOFFER, discover->getTransid()));
@@ -484,9 +468,18 @@ Dhcpv4Srv::serverReceivedPacketName(uint8_t type) {
 }
 
 Subnet4Ptr Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
-    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(question->getRemoteAddr());
 
-    return (subnet);
+    // Is this relayed message?
+    IOAddress relay = question->getGiaddr();
+    if (relay.toText() == "0.0.0.0") {
+
+        // Yes: Use relay address to select subnet
+        return (CfgMgr::instance().getSubnet4(relay));
+    } else {
+
+        // No: Use client's address to select subnet
+        return (CfgMgr::instance().getSubnet4(question->getRemoteAddr()));
+    }
 }
 
 void Dhcpv4Srv::sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid) {

src/bin/dhcp4/dhcp4_srv.h

@@ -207,14 +207,6 @@ protected:
     /// @return server-id option
     OptionPtr getServerID() { return serverid_; }
 
-
-    /// @brief Sets DHCPv4 message type
-    ///
-    /// It tries to find existing DHCP TYPE (53) option and update
-    /// it to specified value. If there is no such option, it is
-    /// added.
-    static void setMsgType(Pkt4Ptr& pkt, uint8_t dhcp_type);
-
     /// @brief Sets server-identifier.
     ///
     /// This method attempts to set server-identifier DUID. It tries to

src/lib/dhcp/pkt4.cc

@@ -49,11 +49,12 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
       yiaddr_(DEFAULT_ADDRESS),
       siaddr_(DEFAULT_ADDRESS),
       giaddr_(DEFAULT_ADDRESS),
-      bufferOut_(DHCPV4_PKT_HDR_LEN),
-      msg_type_(msg_type)
+      bufferOut_(DHCPV4_PKT_HDR_LEN)
 {
     memset(sname_, 0, MAX_SNAME_LEN);
     memset(file_, 0, MAX_FILE_LEN);
+
+    setType(msg_type);
 }
 
 Pkt4::Pkt4(const uint8_t* data, size_t len)
@@ -72,8 +73,7 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
       yiaddr_(DEFAULT_ADDRESS),
       siaddr_(DEFAULT_ADDRESS),
       giaddr_(DEFAULT_ADDRESS),
-      bufferOut_(0), // not used, this is RX packet
-      msg_type_(DHCPDISCOVER)
+      bufferOut_(0) // not used, this is RX packet
 {
     if (len < DHCPV4_PKT_HDR_LEN) {
         isc_throw(OutOfRange, "Truncated DHCPv4 packet (len=" << len
@@ -215,21 +215,44 @@ Pkt4::unpack() {
 }
 
 void Pkt4::check() {
+    uint8_t msg_type = getType();
+    if (msg_type > DHCPLEASEACTIVE) {
+        isc_throw(BadValue, "Invalid DHCP message type received: "
+                  << msg_type);
+    }
+}
+
+uint8_t Pkt4::getType() const {
+    OptionPtr generic = getOption(DHO_DHCP_MESSAGE_TYPE);
+    if (!generic) {
+        isc_throw(Unexpected, "Missing DHCP Message Type option");
+    }
+
+    // Check if Message Type is specified as OptionInt<uint8_t>
     boost::shared_ptr<OptionInt<uint8_t> > typeOpt =
-        boost::dynamic_pointer_cast<OptionInt<uint8_t> >(getOption(DHO_DHCP_MESSAGE_TYPE));
+        boost::dynamic_pointer_cast<OptionInt<uint8_t> >(generic);
     if (typeOpt) {
-        uint8_t msg_type = typeOpt->getValue();
-        if (msg_type > DHCPLEASEACTIVE) {
-            isc_throw(BadValue, "Invalid DHCP message type received: "
-                      << msg_type);
-        }
-        msg_type_ = msg_type;
+        return (typeOpt->getValue());
+    }
+
+    // Try to use it as generic option
+    return (generic->getUint8());
+}
 
+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);
     } else {
-        isc_throw(Unexpected, "Missing DHCP Message Type option");
+        // 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));
+        addOption(opt);
     }
 }
 
+
 void Pkt4::repack() {
     bufferOut_.writeData(&data_[0], data_.size());
 }
@@ -239,7 +262,7 @@ Pkt4::toText() {
     stringstream tmp;
     tmp << "localAddr=" << local_addr_.toText() << ":" << local_port_
         << " remoteAddr=" << remote_addr_.toText()
-        << ":" << remote_port_ << ", msgtype=" << int(msg_type_)
+        << ":" << remote_port_ << ", msgtype=" << getType()
         << ", transid=0x" << hex << transid_ << dec << endl;
 
     for (isc::dhcp::Option::OptionCollection::iterator opt=options_.begin();
@@ -361,7 +384,7 @@ Pkt4::addOption(boost::shared_ptr<Option> opt) {
 }
 
 boost::shared_ptr<isc::dhcp::Option>
-Pkt4::getOption(uint8_t type) {
+Pkt4::getOption(uint8_t type) const {
     Option::OptionCollection::const_iterator x = options_.find(type);
     if (x != options_.end()) {
         return (*x).second;

src/lib/dhcp/pkt4.h

@@ -218,16 +218,15 @@ public:
     /// @return transaction-id
     uint32_t getTransid() const { return (transid_); };
 
-    /// @brief Returns message type (e.g. 1 = DHCPDISCOVER).
+    /// @brief Returns DHCP message type (e.g. 1 = DHCPDISCOVER).
     ///
     /// @return message type
-    uint8_t
-    getType() const { return (msg_type_); }
+    uint8_t getType() const;
 
-    /// @brief Sets message type (e.g. 1 = DHCPDISCOVER).
+    /// @brief Sets DHCP message type (e.g. 1 = DHCPDISCOVER).
     ///
     /// @param type message type to be set
-    void setType(uint8_t type) { msg_type_=type; };
+    void setType(uint8_t type);
 
     /// @brief Returns sname field
     ///
@@ -323,7 +322,7 @@ public:
     /// @return returns option of requested type (or NULL)
     ///         if no such option is present
     boost::shared_ptr<Option>
-    getOption(uint8_t opt_type);
+    getOption(uint8_t opt_type) const;
 
     /// @brief Deletes specified option
     /// @param type option type to be deleted
@@ -522,11 +521,6 @@ protected:
     /// data format change etc.
     std::vector<uint8_t> data_;
 
-    /// message type (e.g. 1=DHCPDISCOVER)
-    /// @todo this will eventually be replaced with DHCP Message Type
-    /// option (option 53)
-    uint8_t msg_type_;
-
     /// collection of options present in this message
     ///
     /// @warning This protected member is accessed by derived

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -702,11 +702,9 @@ TEST_F(IfaceMgrTest, sendReceive4) {
     sendPkt->setYiaddr(IOAddress("192.0.2.3"));
     sendPkt->setGiaddr(IOAddress("192.0.2.4"));
 
-    // unpack() now checks if mandatory DHCP_MESSAGE_TYPE is present
-    boost::shared_ptr<Option> msgType(new Option(Option::V4,
-           static_cast<uint16_t>(DHO_DHCP_MESSAGE_TYPE)));
-    msgType->setUint8(static_cast<uint8_t>(DHCPDISCOVER));
-    sendPkt->addOption(msgType);
+    // Unpack() now checks if mandatory DHCP_MESSAGE_TYPE is present.
+    // Workarounds (creating DHCP Message Type Option by hand) are no longer
+    // needed as setDhcpType() is called in constructor.
 
     uint8_t sname[] = "That's just a string that will act as SNAME";
     sendPkt->setSname(sname, strlen((const char*)sname));
@@ -744,7 +742,6 @@ TEST_F(IfaceMgrTest, sendReceive4) {
     EXPECT_EQ(sendPkt->getYiaddr(), rcvPkt->getYiaddr());
     EXPECT_EQ(sendPkt->getGiaddr(), rcvPkt->getGiaddr());
     EXPECT_EQ(sendPkt->getTransid(), rcvPkt->getTransid());
-    EXPECT_EQ(sendPkt->getType(), rcvPkt->getType());
     EXPECT_TRUE(sendPkt->getSname() == rcvPkt->getSname());
     EXPECT_TRUE(sendPkt->getFile() == rcvPkt->getFile());
     EXPECT_EQ(sendPkt->getHtype(), rcvPkt->getHtype());

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -69,8 +69,9 @@ TEST(Pkt4Test, constructor) {
         pkt = new Pkt4(DHCPDISCOVER, 0xffffffff);
     );
 
-    // DHCPv4 packet must be at least 236 bytes long
-    EXPECT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN), pkt->len());
+    // DHCPv4 packet must be at least 236 bytes long, with Message Type
+    // Option taking extra 3 bytes it is 239
+    EXPECT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + 3, pkt->len());
     EXPECT_EQ(DHCPDISCOVER, pkt->getType());
     EXPECT_EQ(0xffffffff, pkt->getTransid());
     EXPECT_NO_THROW(
@@ -219,8 +220,9 @@ TEST(Pkt4Test, fixedFields) {
     EXPECT_EQ(dummySiaddr.toText(), pkt->getSiaddr().toText());
     EXPECT_EQ(dummyGiaddr.toText(), pkt->getGiaddr().toText());
 
-    // chaddr is always 16 bytes long and contains link-layer addr (MAC)
-    EXPECT_EQ(0, memcmp(dummyChaddr, &pkt->getHWAddr()->hwaddr_[0], 16));
+    // Chaddr contains link-layer addr (MAC). It is no longer always 16 bytes
+    // long and its length depends on hlen value (it is up to 16 bytes now).
+    EXPECT_EQ(0, memcmp(dummyChaddr, &pkt->getHWAddr()->hwaddr_[0], dummyHlen));
 
     EXPECT_EQ(0, memcmp(dummySname, &pkt->getSname()[0], 64));
 
@@ -237,7 +239,9 @@ TEST(Pkt4Test, fixedFieldsPack) {
         pkt->pack();
     );
 
-    ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN), pkt->len());
+    // Minimum packet size is 236 bytes + 3 bytes of mandatory
+    // DHCP Message Type Option
+    ASSERT_EQ(static_cast<size_t>(Pkt4::DHCPV4_PKT_HDR_LEN) + 3, pkt->len());
 
     // redundant but MUCH easier for debug in gdb
     const uint8_t* exp = &expectedFormat[0];
@@ -469,7 +473,7 @@ TEST(Pkt4Test, file) {
 static uint8_t v4Opts[] = {
     12,  3, 0,   1,  2, // Hostname
     14,  3, 10, 11, 12, // Merit Dump File
-    53, 1, 1, // Message Type (required to not throw exception during unpack)
+    53, 1, 2, // Message Type (required to not throw exception during unpack)
     60,  3, 20, 21, 22, // Class Id
     128, 3, 30, 31, 32, // Vendor specific
     254, 3, 40, 41, 42, // Reserved
@@ -487,18 +491,15 @@ TEST(Pkt4Test, options) {
 
     boost::shared_ptr<Option> opt1(new Option(Option::V4, 12, payload[0]));
     boost::shared_ptr<Option> opt3(new Option(Option::V4, 14, payload[1]));
-    boost::shared_ptr<Option> optMsgType(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE));
     boost::shared_ptr<Option> opt2(new Option(Option::V4, 60, payload[2]));
     boost::shared_ptr<Option> opt5(new Option(Option::V4,128, payload[3]));
     boost::shared_ptr<Option> opt4(new Option(Option::V4,254, payload[4]));
-    optMsgType->setUint8(static_cast<uint8_t>(DHCPDISCOVER));
 
     pkt->addOption(opt1);
     pkt->addOption(opt2);
     pkt->addOption(opt3);
     pkt->addOption(opt4);
     pkt->addOption(opt5);
-    pkt->addOption(optMsgType);
 
     EXPECT_TRUE(pkt->getOption(12));
     EXPECT_TRUE(pkt->getOption(60));