[#2227] fixed unittests

src/bin/dhcp4/tests/dhcp4_client.cc

@@ -237,6 +237,8 @@ Dhcp4Client::appendExtraOptions() {
     if (!extra_options_.empty()) {
         for (OptionCollection::iterator opt = extra_options_.begin();
              opt != extra_options_.end(); ++opt) {
+            // Call base class function so that unittests can add multiple
+            // options with the same code.
             context_.query_->Pkt::addOption(opt->second);
         }
     }
@@ -508,7 +510,7 @@ Dhcp4Client::receiveOneMsg() {
     msg->pack();
     Pkt4Ptr msg_copy(new Pkt4(static_cast<const uint8_t*>
                               (msg->getBuffer().getData()),
-                              msg->getBuffer().getLength()));
+                               msg->getBuffer().getLength()));
     msg_copy->setRemoteAddr(msg->getLocalAddr());
     msg_copy->setLocalAddr(msg->getRemoteAddr());
     msg_copy->setRemotePort(msg->getLocalPort());
@@ -525,18 +527,23 @@ void
 Dhcp4Client::sendMsg(const Pkt4Ptr& msg) {
     srv_->shutdown_ = false;
     if (use_relay_) {
-        msg->setHops(1);
+        try {
-        msg->setGiaddr(relay_addr_);
+            msg->setHops(1);
-        msg->setLocalAddr(server_facing_relay_addr_);
+            msg->setGiaddr(relay_addr_);
-        // Insert RAI
+            msg->setLocalAddr(server_facing_relay_addr_);
-        OptionPtr rai(new Option(Option::V4, DHO_DHCP_AGENT_OPTIONS));
+            // Insert RAI
-        // Insert circuit id, if specified.
+            OptionPtr rai(new Option(Option::V4, DHO_DHCP_AGENT_OPTIONS));
-        if (!circuit_id_.empty()) {
+            // Insert circuit id, if specified.
-            rai->addOption(OptionPtr(new Option(Option::V4, RAI_OPTION_AGENT_CIRCUIT_ID,
+            if (!circuit_id_.empty()) {
-                                                OptionBuffer(circuit_id_.begin(),
+                rai->addOption(OptionPtr(new Option(Option::V4, RAI_OPTION_AGENT_CIRCUIT_ID,
-                                                             circuit_id_.end()))));
+                                                    OptionBuffer(circuit_id_.begin(),
+                                                                 circuit_id_.end()))));
+            }
+            msg->addOption(rai);
+        } catch (...) {
+            // If relay options have already been added in the unittest, ignore
+            // exception on add.
         }
-        msg->addOption(rai);
     }
     // Repack the message to simulate wire-data parsing.
     msg->pack();

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -580,15 +580,7 @@ Dhcpv4SrvTest::createPacketFromBuffer(const Pkt4Ptr& src_pkt,
                                             << ex.what()));
     }
 
-    try {
+    // The dst_pkt unpack is performed on processPacket by the server.
-        // Parse the new packet and return to the caller.
-        dst_pkt->unpack();
-    } catch (const Exception& ex) {
-        return (::testing::AssertionFailure(::testing::Message()
-                                            << "Failed to parse a"
-                                            << " destination packet: "
-                                            << ex.what()));
-    }
 
     return (::testing::AssertionSuccess());
 }
@@ -674,6 +666,7 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     // which was parsed from its wire format.
     Pkt4Ptr received;
     ASSERT_TRUE(createPacketFromBuffer(req, received));
+    received->unpack();
     // Set interface. It is required for the server to generate server id.
     received->setIface("eth0");
     received->setIndex(ETH0_INDEX);
@@ -706,6 +699,7 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     addPrlOption(req);
 
     ASSERT_TRUE(createPacketFromBuffer(req, received));
+    received->unpack();
     ASSERT_TRUE(received->getOption(DHO_DHCP_PARAMETER_REQUEST_LIST));
 
     // Set interface. It is required for the server to generate server id.
@@ -743,6 +737,7 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     // in the packet so as the existing lease is not returned.
     req->setHWAddr(1, 6, std::vector<uint8_t>(2, 6));
     ASSERT_TRUE(createPacketFromBuffer(req, received));
+    received->unpack();
     ASSERT_TRUE(received->getOption(DHO_DHCP_PARAMETER_REQUEST_LIST));
 
     // Set interface. It is required for the server to generate server id.
@@ -953,6 +948,8 @@ Dhcpv4SrvTest::pretendReceivingPkt(NakedDhcpv4Srv& srv, const std::string& confi
     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());
+    // Clear options so that they can be recreated on unpack.
+    pkt->options_.clear();
 
     // Simulate that we have received that traffic
     srv.fakeReceive(pkt);

src/bin/dhcp4/tests/inform_unittest.cc

@@ -8,6 +8,7 @@
 #include <asiolink/io_address.h>
 #include <cc/data.h>
 #include <dhcp/dhcp4.h>
+#include <dhcp/libdhcp++.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
 #include <dhcp4/tests/dhcp4_client.h>
@@ -498,48 +499,40 @@ TEST_F(InformTest, messageFieldsLongOptions) {
     // Client requests big option.
     client.requestOption(240);
     // Client also sends multiple options with the same code.
-    OptionDefinition opt_def("option-foo", 231, "my-space", "binary",
+    OptionDefinitionPtr rai_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
-                             "option-foo-space");
+                                                        DHO_DHCP_AGENT_OPTIONS);
-    for (uint32_t i = 0; i < 16; i++) {
+    ASSERT_TRUE(rai_def);
+    // Create RAI options which should be fused by the server.
+    OptionCustomPtr rai(new OptionCustom(*rai_def, Option::V4));
+    for (uint8_t i = 0; i < 4; ++i) {
         // Create a buffer holding some binary data. This data will be
         // used as reference when we read back the data from a created
         // option.
-        OptionBuffer buf_in(64);
+        OptionBuffer buf_in(16);
-        for (unsigned i = 0; i < 64; ++i) {
+        for (uint8_t j = 0; j < 16; ++j) {
-            buf_in[i] = i;
+            buf_in[j] = i * 16 + j;
         }
 
-        // Use scoped pointer because it allows to declare the option
+        OptionPtr circuit_id_opt(new Option(Option::V4,
-        // in the function scope and initialize it under ASSERT.
+                                            RAI_OPTION_AGENT_CIRCUIT_ID, buf_in));
-        boost::shared_ptr<OptionCustom> option;
+        ASSERT_TRUE(circuit_id_opt);
-        // Custom option may throw exception if the provided buffer is
+        rai->addOption(circuit_id_opt);
-        // malformed.
-        ASSERT_NO_THROW(
-            option.reset(new OptionCustom(opt_def, Option::V4, buf_in));
-        );
-        ASSERT_TRUE(option);
-        client.addExtraOption(option);
     }
+    client.addExtraOption(rai);
 
-    // Client also sends multiple options with the same code.
+    // Client sends large options which should be split by the client.
-    OptionDefinition opt_def_bar("option-bar", 232, "my-space", "binary",
+    OptionDefinition opt_def_bar("option-foo", 231, "my-space", "binary",
-                                 "option-bar-space");
+                                 "option-foo-space");
     // Create a buffer holding some binary data. This data will be
     // used as reference when we read back the data from a created
     // option.
     OptionBuffer buf_in(2560);
-    for (unsigned i = 0; i < 2560; ++i) {
+    for (uint32_t i = 0; i < 2560; ++i) {
         buf_in[i] = i;
     }
 
-    // Use scoped pointer because it allows to declare the option
-    // in the function scope and initialize it under ASSERT.
     boost::shared_ptr<OptionCustom> option;
-    // Custom option may throw exception if the provided buffer is
+    ASSERT_NO_THROW(option.reset(new OptionCustom(opt_def_bar, Option::V4, buf_in)));
-    // malformed.
-    ASSERT_NO_THROW(
-        option.reset(new OptionCustom(opt_def_bar, Option::V4, buf_in));
-    );
     ASSERT_TRUE(option);
     client.addExtraOption(option);
     // Client sends DHCPINFORM and should receive reserved fields.
@@ -550,10 +543,15 @@ TEST_F(InformTest, messageFieldsLongOptions) {
     // Make sure that the server has responded with DHCPACK.
     ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
 
-    // Long option should have been split by the client.
+    // Long option should have been split by the client on pack.
     uint32_t count = 0;
+    uint8_t index = 0;
     for (auto const& option : client.getContext().query_->options_) {
-        if (option.first == 232) {
+        if (option.first == 231) {
+            for (auto const& value : option.second->getData()) {
+                ASSERT_EQ(value, index);
+                index++;
+            }
             count++;
         }
     }
@@ -561,12 +559,21 @@ TEST_F(InformTest, messageFieldsLongOptions) {
 
     count = 0;
     for (auto const& option : resp->options_) {
-        if (option.second->getType() == 231) {
+        if (option.first == DHO_DHCP_AGENT_OPTIONS) {
-            count++;
+            for (auto const& suboption: option.second->getOptions()) {
+                if (suboption.first == RAI_OPTION_AGENT_CIRCUIT_ID) {
+                    uint8_t index = 0;
+                    for (auto const& value : suboption.second->getData()) {
+                        ASSERT_EQ(value, index);
+                        index++;
+                    }
+                    count++;
+                }
+            }
         }
     }
-    // Multiple options should have been fused by the server.
+    // Multiple options should have been fused by the server on unpack.
-    EXPECT_EQ(count, 1);
+    ASSERT_EQ(count, 1);
 
     // Check that the reserved and requested values have been assigned.
     string expected =
@@ -583,9 +590,9 @@ TEST_F(InformTest, messageFieldsLongOptions) {
             count++;
         }
     }
-    // Multiple options should have been fused by the server.
+    // Multiple options should have been fused by the server on unpack.
-    EXPECT_EQ(count, 1);
+    ASSERT_EQ(count, 1);
-    EXPECT_EQ(value, string("data") + expected + string("-data"));
+    ASSERT_EQ(value, string("data") + expected + string("-data"));
 }
 
 /// This test verifies that after a client completes its INFORM exchange,

src/lib/dhcp/libdhcp++.cc

@@ -562,7 +562,7 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf,
         // Check if option unpacking must be deferred
         if (shouldDeferOptionUnpack(option_space, opt_type)) {
             num_defs = 0;
-            // Only store deferred options once.
+            // Store deferred option only once.
             bool found = false;
             for (auto const& existing : deferred) {
                 if (existing == opt_type) {
@@ -629,8 +629,8 @@ LibDHCP::fuseOptions4(OptionCollection& options) {
             // Fuse suboptions recursively, if any.
             if (sub_options.size()) {
                 // Fusing suboptions might result in new options with multiple
-                // options having the same code , so we need to iterate again
+                // options having the same code, so we need to iterate again
-                // until no options needs fusing.
+                // until no option needs fusing.
                 found_suboptions = LibDHCP::fuseOptions4(sub_options);
                 if (found_suboptions) {
                     result = true;
@@ -950,7 +950,7 @@ LibDHCP::splitOptions4(OptionCollection& options) {
             // Current option size after split is the sum of the data, the
             // suboptions and the header size. The header is duplicated in all
             // new options, but the rest of the data must be serialized.
-            uint32_t size = candidate->len() - header_len;
+            uint32_t size = candidate->getData().size();
             // Only split if data does not fit in the current option.
             if (size > len) {
                 // Make a copy of the options so we can safely iterate over the

src/lib/dhcp/libdhcp++.h

@@ -258,7 +258,7 @@ public:
     ///
     /// @param options The option container which needs to be updated with fused
     /// options.
-    /// @return True any options have been fused, false otherwise.
+    /// @return True if any option has been fused, false otherwise.
     static bool fuseOptions4(isc::dhcp::OptionCollection& options);
 
     /// @brief Parses provided buffer as DHCPv4 options and creates

src/lib/dhcp/option.h

@@ -230,7 +230,9 @@ public:
     /// @brief returns option universe (V4 or V6)
     ///
     /// @return universe type
-    Universe getUniverse() const { return universe_; };
+    Universe getUniverse() const {
+        return (universe_);
+    }
 
     /// @brief Writes option in wire-format to a buffer.
     ///
@@ -286,7 +288,9 @@ public:
     /// Returns option type (0-255 for DHCPv4, 0-65535 for DHCPv6)
     ///
     /// @return option type
-    uint16_t getType() const { return (type_); }
+    uint16_t getType() const {
+        return (type_);
+    }
 
     /// Returns length of the complete option (data length + DHCPv4/DHCPv6
     /// option header)
@@ -308,7 +312,9 @@ public:
     ///
     /// @return pointer to actual data (or reference to an empty vector
     ///         if there is no data)
-    virtual const OptionBuffer& getData() const { return (data_); }
+    virtual const OptionBuffer& getData() const {
+        return (data_);
+    }
 
     /// Adds a sub-option.
     ///

src/lib/dhcp/option_definition.cc

@@ -195,10 +195,10 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
             return (option);
         }
 
-        switch(type_) {
+        switch (type_) {
         case OPT_EMPTY_TYPE:
             if (getEncapsulatedSpace().empty()) {
-                    return (factoryEmpty(u, type));
+                return (factoryEmpty(u, type));
             } else {
                 return (OptionPtr(new OptionCustom(*this, u, begin, end)));
             }

src/lib/dhcp/pkt4.cc

@@ -83,7 +83,7 @@ Pkt4::pack() {
         buffer_out_.writeUint8(op_);
         buffer_out_.writeUint8(hwaddr_->htype_);
         buffer_out_.writeUint8(hw_len < MAX_CHADDR_LEN ?
-                              hw_len : MAX_CHADDR_LEN);
+                               hw_len : MAX_CHADDR_LEN);
         buffer_out_.writeUint8(hops_);
         buffer_out_.writeUint32(transid_);
         buffer_out_.writeUint16(secs_);

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -678,18 +678,12 @@ TEST_F(LibDhcpTest, splitLongOption) {
     // used as reference when we read back the data from a created
     // option.
     OptionBuffer buf_in(2560);
-    for (unsigned i = 0; i < 2560; ++i) {
+    for (uint32_t i = 0; i < 2560; ++i) {
         buf_in[i] = i;
     }
 
-    // Use scoped pointer because it allows to declare the option
-    // in the function scope and initialize it under ASSERT.
     boost::shared_ptr<OptionCustom> option;
-    // Custom option may throw exception if the provided buffer is
+    ASSERT_NO_THROW(option.reset(new OptionCustom(opt_def, Option::V4, buf_in)));
-    // malformed.
-    ASSERT_NO_THROW(
-        option.reset(new OptionCustom(opt_def, Option::V4, buf_in));
-    );
     ASSERT_TRUE(option);
     isc::util::OutputBuffer buf(0);
     OptionCollection col;
@@ -697,7 +691,14 @@ TEST_F(LibDhcpTest, splitLongOption) {
     LibDHCP::splitOptions4(col);
     LibDHCP::packOptions4(buf, col, true);
 
-    EXPECT_EQ(11, col.size());
+    ASSERT_EQ(11, col.size());
+    uint8_t index = 0;
+    for (auto const& option : col) {
+        for (auto const& value : option.second->getData()) {
+            ASSERT_EQ(value, index);
+            index++;
+        }
+    }
 }
 
 // This test verifies that fuse options for v4 is working correctly.
@@ -706,30 +707,31 @@ TEST_F(LibDhcpTest, fuseLongOption) {
                              "option-foo-space");
 
     OptionCollection col;
-    for (uint32_t i = 0; i < 16; i++) {
+    for (uint8_t i = 0; i < 16; ++i) {
         // Create a buffer holding some binary data. This data will be
         // used as reference when we read back the data from a created
         // option.
         OptionBuffer buf_in(64);
-        for (unsigned i = 0; i < 64; ++i) {
+        for (uint8_t j = 0; j < 64; ++j) {
-            buf_in[i] = i;
+            buf_in[j] = i * 64 + j;
         }
 
-        // Use scoped pointer because it allows to declare the option
-        // in the function scope and initialize it under ASSERT.
         boost::shared_ptr<OptionCustom> option;
-        // Custom option may throw exception if the provided buffer is
+        ASSERT_NO_THROW(option.reset(new OptionCustom(opt_def, Option::V4, buf_in)));
-        // malformed.
-        ASSERT_NO_THROW(
-            option.reset(new OptionCustom(opt_def, Option::V4, buf_in));
-        );
         ASSERT_TRUE(option);
         col.insert(std::make_pair(213, option));
     }
     ASSERT_EQ(16, col.size());
     LibDHCP::fuseOptions4(col);
 
-    EXPECT_EQ(1, col.size());
+    ASSERT_EQ(1, col.size());
+    uint8_t index = 0;
+    for (auto const& option : col) {
+        for (auto const& value: option.second->getData()) {
+            ASSERT_EQ(index, value);
+            index++;
+        }
+    }
 }
 
 // This test verifies that pack options for v4 is working correctly.

src/lib/dhcpsrv/cfg_option.cc

@@ -270,7 +270,10 @@ CfgOption::encapsulateInternal(const OptionPtr& option) {
         // Retrieve all options from the encapsulated option space.
         OptionContainerPtr encap_options = getAll(encap_space);
         for (auto const& encap_opt : *encap_options) {
-            option->addOption(encap_opt.option_);
+            // Add sub-option if there isn't one added already.
+            if (!option->getOption(encap_opt.option_->getType())) {
+                option->addOption(encap_opt.option_);
+            }
             // This is a workaround for preventing infinite recursion when
             // trying to encapsulate options created with default global option
             // spaces.