From: Marcin Siodelski Date: Tue, 22 Jan 2013 17:40:55 +0000 (+0100) Subject: [2591] Changes as a result of the code review. X-Git-Tag: bind10-1.0.0-rc-release~25^2~10^2~16^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e0ffe686d622e43dfbf3e286252044d1fd20a6ed;p=thirdparty%2Fkea.git [2591] Changes as a result of the code review. --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 72ae017daf..a866a5f70d 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -39,9 +39,6 @@ using namespace std; // These are hardcoded parameters. Currently this is a skeleton server that only // grants those options and a single, fixed, hardcoded lease. -const std::string HARDCODED_GATEWAY = "192.0.2.1"; -const std::string HARDCODED_DNS_SERVER = "192.0.2.2"; -const std::string HARDCODED_DOMAIN_NAME = "isc.example.com"; const std::string HARDCODED_SERVER_ID = "192.0.2.1"; Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig) { @@ -271,6 +268,40 @@ void Dhcpv4Srv::appendRequestedOptions(const Pkt4Ptr& question, Pkt4Ptr& msg) { } } +void +Dhcpv4Srv::appendBasicOptions(const Pkt4Ptr& question, Pkt4Ptr& msg) { + // Identify options that we always want to send to the + // client (if they are configured). + static const uint16_t required_options[] = { + DHO_SUBNET_MASK, + DHO_ROUTERS, + DHO_DOMAIN_NAME_SERVERS, + DHO_DOMAIN_NAME }; + + static size_t required_options_size = + sizeof(required_options) / sizeof(required_options[0]); + + // Get the subnet. + Subnet4Ptr subnet = selectSubnet(question); + if (!subnet) { + return; + } + + // Try to find all 'required' options in the outgoing + // message. Those that are not present will be added. + for (int i = 0; i < required_options_size; ++i) { + OptionPtr opt = msg->getOption(required_options[i]); + if (!opt) { + // Check whether option has been configured. + Subnet::OptionDescriptor desc = + subnet->getOptionDescriptor("dhcp4", required_options[i]); + if (desc.option) { + msg->addOption(desc.option); + } + } + } +} + void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) { // We need to select a subnet the client is connected in. @@ -340,10 +371,12 @@ void Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) { opt->setUint32(lease->valid_lft_); answer->addOption(opt); - // @todo: include real router information here // Router (type 3) - opt = OptionPtr(new Option4AddrLst(DHO_ROUTERS, IOAddress(HARDCODED_GATEWAY))); - answer->addOption(opt); + Subnet::OptionDescriptor opt_routers = + subnet->getOptionDescriptor("dhcp4", DHO_ROUTERS); + if (opt_routers.option) { + answer->addOption(opt_routers.option); + } // Subnet mask (type 1) answer->addOption(getNetmaskOption(subnet)); @@ -386,6 +419,11 @@ Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { assignLease(discover, offer); + // There are a few basic options that we always want to + // include in the response. If client did not request + // them we append them for him. + appendBasicOptions(discover, offer); + return (offer); } @@ -399,6 +437,11 @@ Pkt4Ptr Dhcpv4Srv::processRequest(Pkt4Ptr& request) { assignLease(request, ack); + // There are a few basic options that we always want to + // include in the response. If client did not request + // them we append them for him. + appendBasicOptions(request, ack); + return (ack); } diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index ee5ec462d1..a1389207c0 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -187,6 +187,19 @@ protected: /// @param answer OFFER or ACK/NAK message (lease options will be added here) void assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer); + /// @brief Append basic options if they are not present. + /// + /// This function adds the following basic options if they + /// are not yet added to the message: + /// - Subnet Mask, + /// - Router, + /// - Name Server, + /// - Domain Name. + /// + /// @param question DISCOVER or REQUEST message from a client. + /// @param msg the message to add options to. + void appendBasicOptions(const Pkt4Ptr& question, Pkt4Ptr& msg); + /// @brief Attempts to renew received addresses /// /// Attempts to renew existing lease. This typically includes finding a lease that diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 2fe4c334c2..37723069e8 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -70,12 +70,16 @@ public: CfgMgr::instance().deleteSubnets4(); CfgMgr::instance().addSubnet4(subnet_); + + // Add Router option. + Option4AddrLstPtr opt_routers(new Option4AddrLst(DHO_ROUTERS)); + opt_routers->setAddress(IOAddress("192.0.2.2")); + subnet_->addOption(opt_routers, false, "dhcp4"); } - /// @brief Add 'Paramter Request List' option to the packet. + /// @brief Add 'Parameter Request List' option to the packet. /// - /// This function PRL option comprising the following - /// option codes: + /// This function PRL option comprising the following option codes: /// - 5 - Name Server /// - 15 - Domain Name /// - 7 - Log Server @@ -91,15 +95,13 @@ public: std::vector opts; // Let's request options that have been configured for the subnet. - opts.push_back(DHO_DOMAIN_NAME_SERVERS); - opts.push_back(DHO_DOMAIN_NAME); - opts.push_back(DHO_LOG_SERVERS); - opts.push_back(DHO_COOKIE_SERVERS); + option_prl->addValue(DHO_DOMAIN_NAME_SERVERS); + option_prl->addValue(DHO_DOMAIN_NAME); + option_prl->addValue(DHO_LOG_SERVERS); + option_prl->addValue(DHO_COOKIE_SERVERS); // Let's also request the option that hasn't been configured. In such // case server should ignore request for this particular option. - opts.push_back(DHO_LPR_SERVERS); - // Put the requested option codes into the 'Parameter Request List'. - option_prl->setValues(opts); + option_prl->addValue(DHO_LPR_SERVERS); // And add 'Parameter Request List' option into the DISCOVER packet. pkt->addOption(option_prl); } @@ -160,7 +162,6 @@ public: EXPECT_TRUE(a->getOption(DHO_DHCP_SERVER_IDENTIFIER)); EXPECT_TRUE(a->getOption(DHO_DHCP_LEASE_TIME)); EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK)); - EXPECT_TRUE(a->getOption(DHO_ROUTERS)); // Check that something is offered EXPECT_TRUE(a->getYiaddr().toText() != "0.0.0.0"); @@ -424,10 +425,13 @@ TEST_F(Dhcpv4SrvTest, processDiscover) { messageCheck(pkt, offer); + // There are some options that are always present in the + // message, even if not requested. + EXPECT_TRUE(offer->getOption(DHO_DOMAIN_NAME)); + EXPECT_TRUE(offer->getOption(DHO_DOMAIN_NAME_SERVERS)); + // We did not request any options so they should not be present // in the OFFER. - EXPECT_FALSE(offer->getOption(DHO_DOMAIN_NAME)); - EXPECT_FALSE(offer->getOption(DHO_DOMAIN_NAME_SERVERS)); EXPECT_FALSE(offer->getOption(DHO_LOG_SERVERS)); EXPECT_FALSE(offer->getOption(DHO_COOKIE_SERVERS)); EXPECT_FALSE(offer->getOption(DHO_LPR_SERVERS)); @@ -523,10 +527,13 @@ TEST_F(Dhcpv4SrvTest, processRequest) { messageCheck(req, ack); - // We did not request any options so they should not be present + // There are some options that are always present in the + // message, even if not requested. + EXPECT_TRUE(ack->getOption(DHO_DOMAIN_NAME)); + EXPECT_TRUE(ack->getOption(DHO_DOMAIN_NAME_SERVERS)); + + // We did not request any options so these should not be present // in the ACK. - EXPECT_FALSE(ack->getOption(DHO_DOMAIN_NAME)); - EXPECT_FALSE(ack->getOption(DHO_DOMAIN_NAME_SERVERS)); EXPECT_FALSE(ack->getOption(DHO_LOG_SERVERS)); EXPECT_FALSE(ack->getOption(DHO_COOKIE_SERVERS)); EXPECT_FALSE(ack->getOption(DHO_LPR_SERVERS)); diff --git a/src/lib/dhcp/option_int_array.h b/src/lib/dhcp/option_int_array.h index d40c472f86..1babee56d9 100644 --- a/src/lib/dhcp/option_int_array.h +++ b/src/lib/dhcp/option_int_array.h @@ -124,6 +124,13 @@ public: unpack(begin, end); } + /// @brief Adds a new value to the array. + /// + /// @param value a value being added. + void addValue(const T value) { + values_.push_back(value); + } + /// Writes option in wire-format to buf, returns pointer to first unused /// byte after stored option. /// diff --git a/src/lib/dhcp/tests/option_int_array_unittest.cc b/src/lib/dhcp/tests/option_int_array_unittest.cc index 1aeb5841c4..cd868d8b29 100644 --- a/src/lib/dhcp/tests/option_int_array_unittest.cc +++ b/src/lib/dhcp/tests/option_int_array_unittest.cc @@ -294,6 +294,52 @@ public: EXPECT_TRUE(std::equal(buf_.begin(), buf_.begin() + opt_len, out_data.begin()));; } + /// @brief Test ability to set all values. + /// + /// @tparam T numeric type to perform the test for. + template + void setValuesTest() { + const uint16_t opt_code = 100; + // Create option with empty vector of values. + boost::shared_ptr > + opt(new OptionIntArray(Option::V6, opt_code)); + // Initialize vector with some data and pass to the option. + std::vector values; + for (int i = 0; i < 10; ++i) { + values.push_back(numeric_limits::max() - i); + } + opt->setValues(values); + + // Check if universe, option type and data was set correctly. + EXPECT_EQ(Option::V6, opt->getUniverse()); + EXPECT_EQ(opt_code, opt->getType()); + std::vector returned_values = opt->getValues(); + EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + } + + /// @brief Test ability to add values one by one. + /// + /// @tparam T numeric type to perform the test for. + template + void addValuesTest() { + const uint16_t opt_code = 100; + // Create option with empty vector of values. + boost::shared_ptr > + opt(new OptionIntArray(Option::V6, opt_code)); + // Initialize vector with some data and add the same data + // to the option. + std::vector values; + for (int i = 0; i < 10; ++i) { + values.push_back(numeric_limits::max() - i); + opt->addValue(numeric_limits::max() - i); + } + + // Check if universe, option type and data was set correctly. + EXPECT_EQ(Option::V6, opt->getUniverse()); + EXPECT_EQ(opt_code, opt->getType()); + std::vector returned_values = opt->getValues(); + EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + } OptionBuffer buf_; ///< Option buffer OutputBuffer out_buf_; ///< Output buffer @@ -371,118 +417,51 @@ TEST_F(OptionIntArrayTest, bufferToInt32V6) { } TEST_F(OptionIntArrayTest, setValuesUint8) { - const uint16_t opt_code = 100; - // Create option with empty vector of values. - boost::shared_ptr > - opt(new OptionIntArray(Option::V6, opt_code)); - // Initialize vector with some data and pass to the option. - std::vector values; - for (int i = 0; i < 10; ++i) { - values.push_back(numeric_limits::max() - i); - } - opt->setValues(values); - - // Check if universe, option type and data was set correctly. - EXPECT_EQ(Option::V6, opt->getUniverse()); - EXPECT_EQ(opt_code, opt->getType()); - std::vector returned_values = opt->getValues(); - EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + setValuesTest(); } TEST_F(OptionIntArrayTest, setValuesInt8) { - const uint16_t opt_code = 100; - // Create option with empty vector of values. - boost::shared_ptr > - opt(new OptionIntArray(Option::V6, opt_code)); - // Initialize vector with some data and pass to the option. - std::vector values; - for (int i = 0; i < 10; ++i) { - values.push_back(numeric_limits::min() + i); - } - opt->setValues(values); - - // Check if universe, option type and data was set correctly. - EXPECT_EQ(Option::V6, opt->getUniverse()); - EXPECT_EQ(opt_code, opt->getType()); - std::vector returned_values = opt->getValues(); - EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + setValuesTest(); } TEST_F(OptionIntArrayTest, setValuesUint16) { - const uint16_t opt_code = 101; - // Create option with empty vector of values. - boost::shared_ptr > - opt(new OptionIntArray(Option::V6, opt_code)); - // Initialize vector with some data and pass to the option. - std::vector values; - for (int i = 0; i < 10; ++i) { - values.push_back(numeric_limits::max() - i); - } - opt->setValues(values); - - // Check if universe, option type and data was set correctly. - EXPECT_EQ(Option::V6, opt->getUniverse()); - EXPECT_EQ(opt_code, opt->getType()); - std::vector returned_values = opt->getValues(); - EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + setValuesTest(); } TEST_F(OptionIntArrayTest, setValuesInt16) { - const uint16_t opt_code = 101; - // Create option with empty vector of values. - boost::shared_ptr > - opt(new OptionIntArray(Option::V6, opt_code)); - // Initialize vector with some data and pass to the option. - std::vector values; - for (int i = 0; i < 10; ++i) { - values.push_back(numeric_limits::min() + i); - } - opt->setValues(values); - - // Check if universe, option type and data was set correctly. - EXPECT_EQ(Option::V6, opt->getUniverse()); - EXPECT_EQ(opt_code, opt->getType()); - std::vector returned_values = opt->getValues(); - EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + setValuesTest(); } TEST_F(OptionIntArrayTest, setValuesUint32) { - const uint32_t opt_code = 101; - // Create option with empty vector of values. - boost::shared_ptr > - opt(new OptionIntArray(Option::V6, opt_code)); - // Initialize vector with some data and pass to the option. - std::vector values; - for (int i = 0; i < 10; ++i) { - values.push_back(numeric_limits::max() - i); - } - opt->setValues(values); - - // Check if universe, option type and data was set correctly. - EXPECT_EQ(Option::V6, opt->getUniverse()); - EXPECT_EQ(opt_code, opt->getType()); - std::vector returned_values = opt->getValues(); - EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); + setValuesTest(); } TEST_F(OptionIntArrayTest, setValuesInt32) { - const uint32_t opt_code = 101; - // Create option with empty vector of values. - boost::shared_ptr > - opt(new OptionIntArray(Option::V6, opt_code)); - // Initialize vector with some data and pass to the option. - std::vector values; - for (int i = 0; i < 10; ++i) { - values.push_back(numeric_limits::min() + i); - } - opt->setValues(values); + setValuesTest(); +} - // Check if universe, option type and data was set correctly. - EXPECT_EQ(Option::V6, opt->getUniverse()); - EXPECT_EQ(opt_code, opt->getType()); - std::vector returned_values = opt->getValues(); - EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin())); +TEST_F(OptionIntArrayTest, addValuesUint8) { + addValuesTest(); } +TEST_F(OptionIntArrayTest, addValuesInt8) { + addValuesTest(); +} + +TEST_F(OptionIntArrayTest, addValuesUint16) { + addValuesTest(); +} + +TEST_F(OptionIntArrayTest, addValuesInt16) { + addValuesTest(); +} + +TEST_F(OptionIntArrayTest, addValuesUint32) { + addValuesTest(); +} + +TEST_F(OptionIntArrayTest, addValuesInt32) { + addValuesTest(); +} } // anonymous namespace diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index 436fd490e3..c33efda335 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -46,12 +47,10 @@ bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const { void Subnet::addOption(const OptionPtr& option, bool persistent, const std::string& option_space) { - // @todo Once the #2313 is merged we need to use the OptionSpace object to - // validate the option space name here. For now, let's check that the name - // is not empty as the empty namespace has a special meaning here - it is - // returned when desired namespace is not found when getOptions is called. - if (option_space.empty()) { - isc_throw(isc::BadValue, "option space name must not be empty"); + // Check that the option space name is valid. + if (!OptionSpace::validateName(option_space)) { + isc_throw(isc::BadValue, "invalid option space name: '" + << option_space << "'"); } validateOption(option);