[2591] Changes as a result of the code review.

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);
 }
 

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

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<uint8_t> 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));

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.
     ///

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<typename T>
+    void setValuesTest() {
+        const uint16_t opt_code = 100;
+        // Create option with empty vector of values.
+        boost::shared_ptr<OptionIntArray<T> >
+            opt(new OptionIntArray<T>(Option::V6, opt_code));
+        // Initialize vector with some data and pass to the option.
+        std::vector<T> values;
+        for (int i = 0; i < 10; ++i) {
+            values.push_back(numeric_limits<uint8_t>::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<T> 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<typename T>
+    void addValuesTest() {
+        const uint16_t opt_code = 100;
+        // Create option with empty vector of values.
+        boost::shared_ptr<OptionIntArray<T> >
+            opt(new OptionIntArray<T>(Option::V6, opt_code));
+        // Initialize vector with some data and add the same data
+        // to the option.
+        std::vector<T> values;
+        for (int i = 0; i < 10; ++i) {
+            values.push_back(numeric_limits<T>::max() - i);
+            opt->addValue(numeric_limits<T>::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<T> 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<OptionIntArray<uint8_t> >
-        opt(new OptionIntArray<uint8_t>(Option::V6, opt_code));
-    // Initialize vector with some data and pass to the option.
-    std::vector<uint8_t> values;
-    for (int i = 0; i < 10; ++i) {
-        values.push_back(numeric_limits<uint8_t>::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<uint8_t> returned_values = opt->getValues();
-    EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+    setValuesTest<uint8_t>();
 }
 
 TEST_F(OptionIntArrayTest, setValuesInt8) {
-    const uint16_t opt_code = 100;
-    // Create option with empty vector of values.
-    boost::shared_ptr<OptionIntArray<int8_t> >
-        opt(new OptionIntArray<int8_t>(Option::V6, opt_code));
-    // Initialize vector with some data and pass to the option.
-    std::vector<int8_t> values;
-    for (int i = 0; i < 10; ++i) {
-        values.push_back(numeric_limits<int8_t>::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<int8_t> returned_values = opt->getValues();
-    EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+    setValuesTest<int8_t>();
 }
 
 TEST_F(OptionIntArrayTest, setValuesUint16) {
-    const uint16_t opt_code = 101;
-    // Create option with empty vector of values.
-    boost::shared_ptr<OptionIntArray<uint16_t> >
-        opt(new OptionIntArray<uint16_t>(Option::V6, opt_code));
-    // Initialize vector with some data and pass to the option.
-    std::vector<uint16_t> values;
-    for (int i = 0; i < 10; ++i) {
-        values.push_back(numeric_limits<uint16_t>::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<uint16_t> returned_values = opt->getValues();
-    EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+    setValuesTest<uint16_t>();
 }
 
 TEST_F(OptionIntArrayTest, setValuesInt16) {
-    const uint16_t opt_code = 101;
-    // Create option with empty vector of values.
-    boost::shared_ptr<OptionIntArray<int16_t> >
-        opt(new OptionIntArray<int16_t>(Option::V6, opt_code));
-    // Initialize vector with some data and pass to the option.
-    std::vector<int16_t> values;
-    for (int i = 0; i < 10; ++i) {
-        values.push_back(numeric_limits<int16_t>::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<int16_t> returned_values = opt->getValues();
-    EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+    setValuesTest<int16_t>();
 }
 
 TEST_F(OptionIntArrayTest, setValuesUint32) {
-    const uint32_t opt_code = 101;
-    // Create option with empty vector of values.
-    boost::shared_ptr<OptionIntArray<uint32_t> >
-        opt(new OptionIntArray<uint32_t>(Option::V6, opt_code));
-    // Initialize vector with some data and pass to the option.
-    std::vector<uint32_t> values;
-    for (int i = 0; i < 10; ++i) {
-        values.push_back(numeric_limits<uint32_t>::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<uint32_t> returned_values = opt->getValues();
-    EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+    setValuesTest<uint16_t>();
 }
 
 TEST_F(OptionIntArrayTest, setValuesInt32) {
-    const uint32_t opt_code = 101;
-    // Create option with empty vector of values.
-    boost::shared_ptr<OptionIntArray<int32_t> >
-        opt(new OptionIntArray<int32_t>(Option::V6, opt_code));
-    // Initialize vector with some data and pass to the option.
-    std::vector<int32_t> values;
-    for (int i = 0; i < 10; ++i) {
-        values.push_back(numeric_limits<int32_t>::min() + i);
-    }
-    opt->setValues(values);
+    setValuesTest<int16_t>();
+}
 
-    // Check if universe, option type and data was set correctly.
-    EXPECT_EQ(Option::V6, opt->getUniverse());
-    EXPECT_EQ(opt_code, opt->getType());
-    std::vector<int32_t> returned_values = opt->getValues();
-    EXPECT_TRUE(std::equal(values.begin(), values.end(), returned_values.begin()));
+TEST_F(OptionIntArrayTest, addValuesUint8) {
+    addValuesTest<uint8_t>();
 }
 
+TEST_F(OptionIntArrayTest, addValuesInt8) {
+    addValuesTest<int8_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesUint16) {
+    addValuesTest<uint16_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesInt16) {
+    addValuesTest<int16_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesUint32) {
+    addValuesTest<uint16_t>();
+}
+
+TEST_F(OptionIntArrayTest, addValuesInt32) {
+    addValuesTest<int16_t>();
+}
 
 } // anonymous namespace

src/lib/dhcpsrv/subnet.cc

@@ -14,6 +14,7 @@
 
 #include <asiolink/io_address.h>
 #include <dhcpsrv/addr_utilities.h>
+#include <dhcpsrv/option_space.h>
 #include <dhcpsrv/subnet.h>
 
 #include <sstream>
@@ -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);