[master] Merge branch 'trac2591'

Conflicts:
	src/bin/dhcp4/dhcp4_srv.cc
	src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

src/bin/dhcp4/dhcp4_srv.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -17,6 +17,7 @@
 #include <dhcp/iface_mgr.h>
 #include <dhcp/option4_addrlst.h>
 #include <dhcp/option_int.h>
+#include <dhcp/option_int_array.h>
 #include <dhcp/pkt4.h>
 #include <dhcp/duid.h>
 #include <dhcp/hwaddr.h>
@@ -43,9 +44,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";
 
 Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig) {
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_OPEN_SOCKET).arg(port);
@@ -341,18 +339,75 @@ void Dhcpv4Srv::appendDefaultOptions(Pkt4Ptr& msg, uint8_t msg_type) {
 }
 
 
-void Dhcpv4Srv::appendRequestedOptions(Pkt4Ptr& msg) {
-    OptionPtr opt;
+void Dhcpv4Srv::appendRequestedOptions(const Pkt4Ptr& question, Pkt4Ptr& msg) {
+
+    // Get the subnet relevant for the client. We will need it
+    // to get the options associated with it.
+    Subnet4Ptr subnet = selectSubnet(question);
+    // If we can't find the subnet for the client there is no way
+    // to get the options to be sent to a client. We don't log an
+    // error because it will be logged by the assignLease method
+    // anyway.
+    if (!subnet) {
+        return;
+    }
+
+    // try to get the 'Parameter Request List' option which holds the
+    // codes of requested options.
+    OptionUint8ArrayPtr option_prl = boost::dynamic_pointer_cast<
+        OptionUint8Array>(question->getOption(DHO_DHCP_PARAMETER_REQUEST_LIST));
+    // If there is no PRL option in the message from the client then
+    // there is nothing to do.
+    if (!option_prl) {
+        return;
+    }
+
+    // Get the codes of requested options.
+    const std::vector<uint8_t>& requested_opts = option_prl->getValues();
+    // For each requested option code get the instance of the option
+    // to be returned to the client.
+    for (std::vector<uint8_t>::const_iterator opt = requested_opts.begin();
+         opt != requested_opts.end(); ++opt) {
+        Subnet::OptionDescriptor desc =
+            subnet->getOptionDescriptor("dhcp4", *opt);
+        if (desc.option) {
+            msg->addOption(desc.option);
+        }
+    }
+}
 
-    // Domain name (type 15)
-    vector<uint8_t> domain(HARDCODED_DOMAIN_NAME.begin(), HARDCODED_DOMAIN_NAME.end());
-    opt = OptionPtr(new Option(Option::V4, DHO_DOMAIN_NAME, domain));
-    msg->addOption(opt);
-    // TODO: Add Option_String class
+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 };
 
-    // DNS servers (type 6)
-    opt = OptionPtr(new Option4AddrLst(DHO_DOMAIN_NAME_SERVERS, IOAddress(HARDCODED_DNS_SERVER)));
-    msg->addOption(opt);
+    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) {
@@ -424,10 +479,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));
@@ -466,10 +523,15 @@ Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
 
     copyDefaultFields(discover, offer);
     appendDefaultOptions(offer, DHCPOFFER);
-    appendRequestedOptions(offer);
+    appendRequestedOptions(discover, offer);
 
     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);
 }
 
@@ -479,10 +541,15 @@ Pkt4Ptr Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
 
     copyDefaultFields(request, ack);
     appendDefaultOptions(ack, DHCPACK);
-    appendRequestedOptions(ack);
+    appendRequestedOptions(request, ack);
 
     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

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -182,8 +182,9 @@ protected:
     /// This method assigns options that were requested by client
     /// (sent in PRL) or are enforced by server.
     ///
+    /// @param question DISCOVER or REQUEST message from a client.
     /// @param msg outgoing message (options will be added here)
-    void appendRequestedOptions(Pkt4Ptr& msg);
+    void appendRequestedOptions(const Pkt4Ptr& question, Pkt4Ptr& msg);
 
     /// @brief Assigns a lease and appends corresponding options
     ///
@@ -195,6 +196,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

@@ -18,6 +18,9 @@
 #include <asiolink/io_address.h>
 #include <dhcp/dhcp4.h>
 #include <dhcp/option.h>
+#include <dhcp/option4_addrlst.h>
+#include <dhcp/option_custom.h>
+#include <dhcp/option_int_array.h>
 #include <dhcp4/dhcp4_srv.h>
 #include <dhcp4/dhcp4_log.h>
 #include <dhcpsrv/cfgmgr.h>
@@ -74,14 +77,82 @@ 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");
+
         // it's ok if that fails. There should not be such a file anyway
         unlink(SRVID_FILE);
     }
 
+    /// @brief Add 'Parameter Request List' option to the packet.
+    ///
+    /// This function PRL option comprising the following option codes:
+    /// - 5 - Name Server
+    /// - 15 - Domain Name
+    /// - 7 - Log Server
+    /// - 8 - Quotes Server
+    /// - 9 - LPR Server
+    ///
+    /// @param pkt packet to add PRL option to.
+    void addPrlOption(Pkt4Ptr& pkt) {
+
+        OptionUint8ArrayPtr option_prl =
+            OptionUint8ArrayPtr(new OptionUint8Array(Option::V4,
+                                                     DHO_DHCP_PARAMETER_REQUEST_LIST));
+
+        // Let's request options that have been configured for the subnet.
+        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.
+        option_prl->addValue(DHO_LPR_SERVERS);
+        // And add 'Parameter Request List' option into the DISCOVER packet.
+        pkt->addOption(option_prl);
+    }
+
+    /// @brief Configures options being requested in the PRL option.
+    ///
+    /// The lpr-servers option is NOT configured here altough it is
+    /// added to the 'Parameter Request List' option in the
+    /// \ref addPrlOption. When requested option is not configured
+    /// the server should not return it in its rensponse. The goal
+    /// of not configuring the requested option is to verify that
+    /// the server will not return it.
+    void configureRequestedOptions() {
+        // dns-servers
+        Option4AddrLstPtr
+            option_dns_servers(new Option4AddrLst(DHO_DOMAIN_NAME_SERVERS));
+        option_dns_servers->addAddress(IOAddress("192.0.2.1"));
+        option_dns_servers->addAddress(IOAddress("192.0.2.100"));
+        ASSERT_NO_THROW(subnet_->addOption(option_dns_servers, false, "dhcp4"));
+
+        // domain-name
+        OptionDefinition def("domain-name", DHO_DOMAIN_NAME, OPT_FQDN_TYPE);
+        boost::shared_ptr<OptionCustom>
+            option_domain_name(new OptionCustom(def, Option::V4));
+        option_domain_name->writeFqdn("example.com");
+        subnet_->addOption(option_domain_name, false, "dhcp4");
+
+        // log-servers
+        Option4AddrLstPtr option_log_servers(new Option4AddrLst(DHO_LOG_SERVERS));
+        option_log_servers->addAddress(IOAddress("192.0.2.2"));
+        option_log_servers->addAddress(IOAddress("192.0.2.10"));
+        ASSERT_NO_THROW(subnet_->addOption(option_log_servers, false, "dhcp4"));
+
+        // cookie-servers
+        Option4AddrLstPtr option_cookie_servers(new Option4AddrLst(DHO_COOKIE_SERVERS));
+        option_cookie_servers->addAddress(IOAddress("192.0.2.1"));
+        ASSERT_NO_THROW(subnet_->addOption(option_cookie_servers, false, "dhcp4"));
+    }
+
     /// @brief checks that the response matches request
     /// @param q query (client's message)
     /// @param a answer (server's message)
-    void MessageCheck(const boost::shared_ptr<Pkt4>& q,
+    void messageCheck(const boost::shared_ptr<Pkt4>& q,
                       const boost::shared_ptr<Pkt4>& a) {
         ASSERT_TRUE(q);
         ASSERT_TRUE(a);
@@ -91,20 +162,40 @@ public:
         EXPECT_EQ(q->getIndex(),  a->getIndex());
         EXPECT_EQ(q->getGiaddr(), a->getGiaddr());
 
-        // Check that bare minimum of required options are there
+        // Check that bare minimum of required options are there.
+        // We don't check options requested by a client. Those
+        // are checked elsewhere.
         EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK));
         EXPECT_TRUE(a->getOption(DHO_ROUTERS));
         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));
-        EXPECT_TRUE(a->getOption(DHO_DOMAIN_NAME));
-        EXPECT_TRUE(a->getOption(DHO_DOMAIN_NAME_SERVERS));
 
         // Check that something is offered
         EXPECT_TRUE(a->getYiaddr().toText() != "0.0.0.0");
     }
 
+    /// @brief Check that requested options are present.
+    ///
+    /// @param pkt packet to be checked.
+    void optionsCheck(const Pkt4Ptr& pkt) {
+        // Check that the requested and configured options are returned
+        // in the ACK message.
+        EXPECT_TRUE(pkt->getOption(DHO_DOMAIN_NAME))
+            << "domain-name not present in the response";
+        EXPECT_TRUE(pkt->getOption(DHO_DOMAIN_NAME_SERVERS))
+            << "dns-servers not present in the response";
+        EXPECT_TRUE(pkt->getOption(DHO_LOG_SERVERS))
+            << "log-servers not present in the response";
+        EXPECT_TRUE(pkt->getOption(DHO_COOKIE_SERVERS))
+            << "cookie-servers not present in the response";
+        // Check that the requested but not configured options are not
+        // returned in the ACK message.
+        EXPECT_FALSE(pkt->getOption(DHO_LPR_SERVERS))
+            << "domain-name present in the response but it is"
+            << " expected not to be present";
+    }
+
     /// @brief generates client-id option
     ///
     /// Generate client-id option of specified length
@@ -324,6 +415,12 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
     pkt->setHops(3);
     pkt->setRemotePort(DHCP4_SERVER_PORT);
 
+    // We are going to test that certain options are returned
+    // (or not returned) in the OFFER message when requested
+    // using 'Parameter Request List' option. Let's configure
+    // those options that are returned when requested.
+    configureRequestedOptions();
+
     // Should not throw
     EXPECT_NO_THROW(
         offer = srv->processDiscover(pkt);
@@ -337,7 +434,39 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
     // This is relayed message. It should be sent back to relay address.
     EXPECT_EQ(pkt->getGiaddr(), offer->getRemoteAddr());
 
-    MessageCheck(pkt, offer);
+    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_LOG_SERVERS));
+    EXPECT_FALSE(offer->getOption(DHO_COOKIE_SERVERS));
+    EXPECT_FALSE(offer->getOption(DHO_LPR_SERVERS));
+
+    // Add 'Parameter Request List' option.
+    addPrlOption(pkt);
+
+    // Now repeat the test but request some options.
+    EXPECT_NO_THROW(
+        offer = srv->processDiscover(pkt);
+    );
+
+    // Should return something
+    ASSERT_TRUE(offer);
+
+    EXPECT_EQ(DHCPOFFER, offer->getType());
+
+    // This is relayed message. It should be sent back to relay address.
+    EXPECT_EQ(pkt->getGiaddr(), offer->getRemoteAddr());
+
+    messageCheck(pkt, offer);
+
+    // Check that the requested options are returned.
+    optionsCheck(offer);
 
     // Now repeat the test for directly sent message
     pkt->setHops(0);
@@ -357,7 +486,10 @@ TEST_F(Dhcpv4SrvTest, processDiscover) {
     // to relay.
     EXPECT_EQ(pkt->getRemoteAddr(), offer->getRemoteAddr());
 
-    MessageCheck(pkt, offer);
+    messageCheck(pkt, offer);
+
+    // Check that the requested options are returned.
+    optionsCheck(offer);
 
     delete srv;
 }
@@ -385,6 +517,12 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
     req->setRemoteAddr(IOAddress("192.0.2.56"));
     req->setGiaddr(IOAddress("192.0.2.67"));
 
+    // We are going to test that certain options are returned
+    // in the ACK message when requested using 'Parameter
+    // Request List' option. Let's configure those options that
+    // are returned when requested.
+    configureRequestedOptions();
+
     // Should not throw
     ASSERT_NO_THROW(
         ack = srv->processRequest(req);
@@ -398,7 +536,37 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
     // This is relayed message. It should be sent back to relay address.
     EXPECT_EQ(req->getGiaddr(), ack->getRemoteAddr());
 
-    MessageCheck(req, ack);
+    messageCheck(req, ack);
+
+    // 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_LOG_SERVERS));
+    EXPECT_FALSE(ack->getOption(DHO_COOKIE_SERVERS));
+    EXPECT_FALSE(ack->getOption(DHO_LPR_SERVERS));
+
+    // Add 'Parameter Request List' option.
+    addPrlOption(req);
+
+    // Repeat the test but request some options.
+    ASSERT_NO_THROW(
+        ack = srv->processRequest(req);
+    );
+
+    // Should return something
+    ASSERT_TRUE(ack);
+
+    EXPECT_EQ(DHCPACK, ack->getType());
+
+    // This is relayed message. It should be sent back to relay address.
+    EXPECT_EQ(req->getGiaddr(), ack->getRemoteAddr());
+
+    // Check that the requested options are returned.
+    optionsCheck(ack);
 
     // Now repeat the test for directly sent message
     req->setHops(0);
@@ -418,7 +586,10 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
     // to relay.
     EXPECT_EQ(ack->getRemoteAddr(), req->getRemoteAddr());
 
-    MessageCheck(req, ack);
+    messageCheck(req, ack);
+
+    // Check that the requested options are returned.
+    optionsCheck(ack);
 
     delete srv;
 }

src/lib/dhcp/option4_addrlst.h

@@ -29,6 +29,11 @@
 namespace isc {
 namespace dhcp {
 
+/// Forward declaration to Option4AddrLst class.
+class Option4AddrLst;
+
+/// A pointer to the Option4AddrLst object.
+typedef boost::shared_ptr<Option4AddrLst> Option4AddrLstPtr;
 
 /// @brief DHCPv4 Option class for handling list of IPv4 addresses.
 ///

src/lib/dhcp/option_int_array.h

@@ -25,6 +25,23 @@
 namespace isc {
 namespace dhcp {
 
+/// Forward declaration of OptionIntArray.
+template<typename T>
+class OptionIntArray;
+
+/// @defgroup option_int_array_defs Typedefs for OptionIntArray class.
+///
+/// @brief Classes that represent options comprising array of integers.
+///
+/// @{
+typedef OptionIntArray<uint8_t> OptionUint8Array;
+typedef boost::shared_ptr<OptionUint8Array> OptionUint8ArrayPtr;
+typedef OptionIntArray<uint16_t> OptionUint16Array;
+typedef boost::shared_ptr<OptionUint16Array> OptionUint16ArrayPtr;
+typedef OptionIntArray<uint32_t> OptionUint32Array;
+typedef boost::shared_ptr<OptionUint32Array> OptionUint32ArrayPtr;
+/// @}
+
 /// This template class represents DHCP (v4 or v6) option with an
 /// array of integer values. The type of the elements in the array
 /// can be any of the following:
@@ -107,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

@@ -13,6 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <asiolink/io_address.h>
+#include <dhcp/option_space.h>
 #include <dhcpsrv/addr_utilities.h>
 #include <dhcpsrv/subnet.h>
 
@@ -44,14 +45,12 @@ bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const {
 }
 
 void
-Subnet::addOption(OptionPtr& option, bool persistent,
+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);
 

src/lib/dhcpsrv/subnet.h

@@ -69,7 +69,7 @@ public:
         ///
         /// @param opt option
         /// @param persist if true option is always sent.
-        OptionDescriptor(OptionPtr& opt, bool persist)
+        OptionDescriptor(const OptionPtr& opt, bool persist)
             : option(opt), persistent(persist) {};
 
         /// @brief Constructor
@@ -225,7 +225,7 @@ public:
     /// @param option_space name of the option space to add an option to.
     ///
     /// @throw isc::BadValue if invalid option provided.
-    void addOption(OptionPtr& option, bool persistent,
+    void addOption(const OptionPtr& option, bool persistent,
                    const std::string& option_space);
 
     /// @brief Delete all options configured for the subnet.