diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc index f4f7412c2a..581d3eeee0 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.cc +++ b/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 (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); - msg->setGiaddr(relay_addr_); - msg->setLocalAddr(server_facing_relay_addr_); - // Insert RAI - OptionPtr rai(new Option(Option::V4, DHO_DHCP_AGENT_OPTIONS)); - // Insert circuit id, if specified. - if (!circuit_id_.empty()) { - rai->addOption(OptionPtr(new Option(Option::V4, RAI_OPTION_AGENT_CIRCUIT_ID, - OptionBuffer(circuit_id_.begin(), - circuit_id_.end())))); + try { + msg->setHops(1); + msg->setGiaddr(relay_addr_); + msg->setLocalAddr(server_facing_relay_addr_); + // Insert RAI + OptionPtr rai(new Option(Option::V4, DHO_DHCP_AGENT_OPTIONS)); + // Insert circuit id, if specified. + if (!circuit_id_.empty()) { + rai->addOption(OptionPtr(new Option(Option::V4, RAI_OPTION_AGENT_CIRCUIT_ID, + 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(); diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index e987b5872d..1f0a0b5714 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -580,15 +580,7 @@ Dhcpv4SrvTest::createPacketFromBuffer(const Pkt4Ptr& src_pkt, << ex.what())); } - try { - // 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())); - } + // The dst_pkt unpack is performed on processPacket by the server. 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(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); diff --git a/src/bin/dhcp4/tests/inform_unittest.cc b/src/bin/dhcp4/tests/inform_unittest.cc index 407e2db322..f1c4edc3d9 100644 --- a/src/bin/dhcp4/tests/inform_unittest.cc +++ b/src/bin/dhcp4/tests/inform_unittest.cc @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -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", - "option-foo-space"); - for (uint32_t i = 0; i < 16; i++) { + OptionDefinitionPtr rai_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_AGENT_OPTIONS); + 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); - for (unsigned i = 0; i < 64; ++i) { - buf_in[i] = i; + OptionBuffer buf_in(16); + for (uint8_t j = 0; j < 16; ++j) { + buf_in[j] = i * 16 + j; } - // Use scoped pointer because it allows to declare the option - // in the function scope and initialize it under ASSERT. - boost::shared_ptr option; - // Custom option may throw exception if the provided buffer is - // malformed. - ASSERT_NO_THROW( - option.reset(new OptionCustom(opt_def, Option::V4, buf_in)); - ); - ASSERT_TRUE(option); - client.addExtraOption(option); + OptionPtr circuit_id_opt(new Option(Option::V4, + RAI_OPTION_AGENT_CIRCUIT_ID, buf_in)); + ASSERT_TRUE(circuit_id_opt); + rai->addOption(circuit_id_opt); } + client.addExtraOption(rai); - // Client also sends multiple options with the same code. - OptionDefinition opt_def_bar("option-bar", 232, "my-space", "binary", - "option-bar-space"); + // Client sends large options which should be split by the client. + OptionDefinition opt_def_bar("option-foo", 231, "my-space", "binary", + "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 option; - // Custom option may throw exception if the provided buffer is - // malformed. - ASSERT_NO_THROW( - option.reset(new OptionCustom(opt_def_bar, Option::V4, buf_in)); - ); + 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(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) { - count++; + if (option.first == DHO_DHCP_AGENT_OPTIONS) { + 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. - EXPECT_EQ(count, 1); + // Multiple options should have been fused by the server on unpack. + 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. - EXPECT_EQ(count, 1); - EXPECT_EQ(value, string("data") + expected + string("-data")); + // Multiple options should have been fused by the server on unpack. + ASSERT_EQ(count, 1); + ASSERT_EQ(value, string("data") + expected + string("-data")); } /// This test verifies that after a client completes its INFORM exchange, diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index 9e52b52c49..8bb889da0c 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/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 - // until no options needs fusing. + // options having the same code, so we need to iterate again + // 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 diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index 68eeb3d635..3a21a4227e 100644 --- a/src/lib/dhcp/libdhcp++.h +++ b/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 diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h index 453c00c554..1a67b9a3c1 100644 --- a/src/lib/dhcp/option.h +++ b/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. /// diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index aa8ce8a0a1..aceff85eab 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/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))); } diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc index 062719445f..53f4ed67e1 100644 --- a/src/lib/dhcp/pkt4.cc +++ b/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_); diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index f3e266d6cb..a4c85b72a7 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/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 option; - // Custom option may throw exception if the provided buffer is - // malformed. - ASSERT_NO_THROW( - option.reset(new OptionCustom(opt_def, Option::V4, buf_in)); - ); + 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) { - buf_in[i] = i; + for (uint8_t j = 0; j < 64; ++j) { + 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 option; - // Custom option may throw exception if the provided buffer is - // malformed. - ASSERT_NO_THROW( - option.reset(new OptionCustom(opt_def, Option::V4, buf_in)); - ); + 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. diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index eb5cadc8b2..416d185d35 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/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.