[2315] Changes as the result of the review.

src/lib/dhcp/libdhcp++.cc

@@ -74,6 +74,34 @@ LibDHCP::getOptionDef(const Option::Universe u, const uint16_t code) {
     return (OptionDefinitionPtr());
 }
 
+bool
+LibDHCP::isStandardOption(const Option::Universe u, const uint16_t code) {
+    if (u == Option::V6) {
+        if (code < 79 &&
+            code != 10 &&
+            code != 35) {
+            return (true);
+        }
+
+    } else if (u == Option::V4) {
+        if (!(code == 84 ||
+              code == 96 ||
+              (code > 101 && code < 112) ||
+              code == 115 ||
+              code == 126 ||
+              code == 127 ||
+              (code > 146 && code < 150) ||
+              (code > 177  && code < 208) ||
+              (code > 213 && code <  220) ||
+              (code > 221 && code < 224))) {
+                return (true);
+            }
+
+    }
+
+    return (false);
+}
+
 OptionPtr
 LibDHCP::optionFactory(Option::Universe u,
                        uint16_t type,

src/lib/dhcp/libdhcp++.h

@@ -55,6 +55,21 @@ public:
     static OptionDefinitionPtr getOptionDef(const Option::Universe u,
                                             const uint16_t code);
 
+    /// @brief Check if the specified option is a standard option.
+    ///
+    /// @param u universe (V4 or V6)
+    /// @param code option code.
+    ///
+    /// @return true if the specified option is a standard option.
+    /// @todo We arleady create option definitions for the subset if
+    /// standard options. We are aiming that this function checks
+    /// the presence of the standard option definition and if it finds
+    /// it, then the true value is returned. However, at this point
+    /// this is not doable because some of the definitions (for less
+    /// important options) are not created yet.
+    static bool isStandardOption(const Option::Universe u,
+                                 const uint16_t code);
+
     /// @brief Factory function to create instance of option.
     ///
     /// Factory method creates instance of specified option. The option

src/lib/dhcp/option_definition.h

@@ -500,6 +500,9 @@ typedef boost::multi_index_container<
     >
 > OptionDefContainer;
 
+/// Pointer to an option definition container.
+typedef boost::shared_ptr<OptionDefContainer> OptionDefContainerPtr;
+
 /// Type of the index #1 - option type.
 typedef OptionDefContainer::nth_index<1>::type OptionDefContainerTypeIndex;
 /// Pair of iterators to represent the range of options definitions

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

@@ -453,6 +453,66 @@ TEST_F(LibDhcpTest, unpackOptions4) {
     EXPECT_TRUE(x == options.end()); // option 2 not found
 }
 
+TEST_F(LibDhcpTest, isStandardOption4) {
+    // Get all option codes that are not occupied by standard options.
+    const uint16_t unassigned_codes[] = { 84, 96, 102, 103, 104, 105, 106, 107, 108,
+                                          109, 110, 111, 115, 126, 127, 147, 148, 149,
+                                          178, 179, 180, 181, 182, 183, 184, 185, 186,
+                                          187, 188, 189, 190, 191, 192, 193, 194, 195,
+                                          196, 197, 198, 199, 200, 201, 202, 203, 204,
+                                          205, 206, 207, 214, 215, 216, 217, 218, 219,
+                                          222, 223 };
+    const size_t unassigned_num = sizeof(unassigned_codes) / sizeof(unassigned_codes[0]);
+
+    // Try all possible option codes.
+    for (size_t i = 0; i < 256; ++i) {
+        // Some ranges of option codes are unassigned and thus the isStandardOption
+        // should return false for them.
+        bool check_unassigned = false;
+        // Check the array of unassigned options to find out whether option code
+        // is assigned to standard option or unassigned.
+        for (size_t j = 0; j < unassigned_num; ++j) {
+            // If option code is found within the array of unassigned options
+            // we the isStandardOption function should return false.
+            if (unassigned_codes[j] == i) {
+                check_unassigned = true;
+                EXPECT_FALSE(LibDHCP::isStandardOption(Option::V4,
+                                                       unassigned_codes[j]))
+                    << "Test failed for option code " << unassigned_codes[j];
+                break;
+            }
+        }
+        // If the option code belongs to the standard option then the
+        // isStandardOption should return true.
+        if (!check_unassigned) {
+            EXPECT_TRUE(LibDHCP::isStandardOption(Option::V4, i))
+                << "Test failed for the option code " << i;
+        }
+    }
+}
+
+TEST_F(LibDhcpTest, isStandardOption6) {
+    // All option codes in the range from 0 to 78 (except 10 and 35)
+    // identify the standard options.
+    for (uint16_t code = 0; code < 79; ++code) {
+        if (code != 10 && code != 35) {
+            EXPECT_TRUE(LibDHCP::isStandardOption(Option::V6, code))
+                << "Test failed for option code " << code;
+        }
+    }
+
+    // Check the option codes 10 and 35. They are unassigned.
+    EXPECT_FALSE(LibDHCP::isStandardOption(Option::V6, 10));
+    EXPECT_FALSE(LibDHCP::isStandardOption(Option::V6, 35));
+
+    // Check a range of option codes above 78. Those are option codes
+    // identifying non-standard options.
+    for (uint16_t code = 79; code < 512; ++code) {
+        EXPECT_FALSE(LibDHCP::isStandardOption(Option::V6, code))
+            << "Test failed for option code " << code;
+    }
+}
+
 TEST_F(LibDhcpTest, stdOptionDefs4) {
 
     // Create a buffer that holds dummy option data.

src/lib/dhcpsrv/cfgmgr.cc

@@ -13,6 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <asiolink/io_address.h>
+#include <dhcp/libdhcp++.h>
 #include <dhcpsrv/cfgmgr.h>
 
 using namespace isc::asiolink;
@@ -34,23 +35,36 @@ CfgMgr::addOptionDef(const OptionDefinitionPtr& def,
     // This will be implemented when #2313 is merged.
     if (option_space.empty()) {
         isc_throw(BadValue, "option space name must not be empty");
-    } else if (option_space == "dhcp4" || option_space == "dhcp6") {
-        isc_throw(BadValue, "unable to override definition of option"
-                  << " in standard option space '" << option_space
-                  << "'.");
     } else if (!def) {
+        // Option definition must point to a valid object.
         isc_throw(MalformedOptionDefinition, "option definition must not be NULL");
+
     } else if (getOptionDef(option_space, def->getCode())) {
+        // Option definition must not be overriden.
         isc_throw(DuplicateOptionDefinition, "option definition already added"
                   << " to option space " << option_space);
+
+    } else if ((option_space == "dhcp4" &&
+                LibDHCP::isStandardOption(Option::V4, def->getCode())) ||
+               (option_space == "dhcp6" &&
+                LibDHCP::isStandardOption(Option::V6, def->getCode()))) {
+        // We must not override standard (assigned) option. The standard options
+        // belong to dhcp4 or dhcp6 option space.
+        isc_throw(BadValue, "unable to override definition of option '"
+                  << def->getCode() << "' in standard option space '"
+                  << option_space << "'.");
+
     }
+    // Actually add the definition to the option space.
     option_def_spaces_[option_space].push_back(def);
 }
 
 const OptionDefContainer&
 CfgMgr::getOptionDefs(const std::string& option_space) const {
+    // @todo Validate the option space once the #2313 is implemented.
+
     // Get all option definitions for the particular option space.
-    const std::map<std::string, OptionDefContainer>::const_iterator& defs =
+    const OptionDefsMap::const_iterator& defs =
         option_def_spaces_.find(option_space);
     // If there are no option definitions for the particular option space
     // then return empty container.
@@ -65,6 +79,8 @@ CfgMgr::getOptionDefs(const std::string& option_space) const {
 OptionDefinitionPtr
 CfgMgr::getOptionDef(const std::string& option_space,
                      const uint16_t option_code) const {
+    // @todo Validate the option space once the #2313 is implemented.
+
     // Get a reference to option definitions for a particular option space.
     const OptionDefContainer& defs = getOptionDefs(option_space);
     // If there are no matching option definitions then return the empty pointer.

src/lib/dhcpsrv/cfgmgr.h

@@ -88,7 +88,8 @@ public:
     /// @throw isc::dhcp::MalformedOptionDefinition when the pointer to
     /// an option definition is NULL.
     /// @throw isc::BadValue when the option space name is empty or
-    /// option space name is one of reserved names: dhcp4 or dhcp6.
+    /// when trying to override the standard option (in dhcp4 or dhcp6
+    /// option space).
     void addOptionDef(const OptionDefinitionPtr& def,
                       const std::string& option_space);
 
@@ -186,6 +187,7 @@ public:
     /// 192.0.2.0/23 and 192.0.2.0/24 the same subnet or is it something
     /// completely new?
     void deleteSubnets4();
+
 protected:
 
     /// @brief Protected constructor.
@@ -217,9 +219,16 @@ protected:
 
 private:
 
+    /// A map containing option definitions for various option spaces.
+    /// They key of this map is the name of the option space. The
+    /// value is the the option container holding option definitions
+    /// for the particular option space.
+    typedef std::map<std::string, OptionDefContainer> OptionDefsMap;
+
     /// A map containing option definitions for different option spaces.
     /// The map key holds an option space name.
-    std::map<std::string, OptionDefContainer> option_def_spaces_;
+    OptionDefsMap option_def_spaces_;
+
 };
 
 } // namespace isc::dhcp

src/lib/dhcpsrv/subnet.cc

@@ -82,8 +82,8 @@ Subnet::getOptions(const std::string& option_space) const {
 }
 
 Subnet::OptionDescriptor
-Subnet::getOptionSingle(const std::string& option_space,
-                        const uint16_t option_code) {
+Subnet::getOptionDescriptor(const std::string& option_space,
+                            const uint16_t option_code) {
     const OptionContainer& options = getOptions(option_space);
     if (options.empty()) {
         return (OptionDescriptor(false));

src/lib/dhcpsrv/subnet.h

@@ -270,8 +270,8 @@ public:
     ///
     /// @return option descriptor found for the specified option space
     /// and option code.
-    OptionDescriptor getOptionSingle(const std::string& option_space,
-                                     const uint16_t option_code);
+    OptionDescriptor getOptionDescriptor(const std::string& option_space,
+                                         const uint16_t option_code);
 
     /// @brief returns the last address that was tried from this pool
     ///

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -129,7 +129,7 @@ TEST_F(CfgMgrTest, getOptionDef) {
     // add them to the different option space.
     for (uint16_t code = 105; code < 115; ++code) {
         std::ostringstream option_name;
-        option_name << "option-" << code;
+        option_name << "option-other-" << code;
         OptionDefinitionPtr def(new OptionDefinition(option_name.str(), code,
                                                      "uint16"));
         ASSERT_NO_THROW(cfg_mgr.addOptionDef(def, "abcde"));
@@ -140,6 +140,12 @@ TEST_F(CfgMgrTest, getOptionDef) {
     for (uint16_t code = 100; code < 110; ++code) {
         OptionDefinitionPtr def = cfg_mgr.getOptionDef("isc", code);
         ASSERT_TRUE(def);
+        // Check that the option name is in the format of 'option-[code]'.
+        // That way we make sure that the options that have the same codes
+        // within different option spaces are different.
+        std::ostringstream option_name;
+        option_name << "option-" << code;
+        EXPECT_EQ(option_name.str(), def->getName());
         EXPECT_EQ(code, def->getCode());
     }
 
@@ -147,20 +153,44 @@ TEST_F(CfgMgrTest, getOptionDef) {
     for (uint16_t code = 105; code < 115; ++code) {
         OptionDefinitionPtr def = cfg_mgr.getOptionDef("abcde", code);
         ASSERT_TRUE(def);
+        // Check that the option name is in the format of 'option-other-[code]'.
+        // That way we make sure that the options that have the same codes
+        // within different option spaces are different.
+        std::ostringstream option_name;
+        option_name << "option-other-" << code;
+        EXPECT_EQ(option_name.str(), def->getName());
+
         EXPECT_EQ(code, def->getCode());
     }
 
+    // Check that an option definition can be added to the standard
+    // (dhcp4 and dhcp6) option spaces when the option code is not
+    // reserved by the standard option.
+    OptionDefinitionPtr def6(new OptionDefinition("option-foo", 79, "uint16"));
+    EXPECT_NO_THROW(cfg_mgr.addOptionDef(def6, "dhcp6"));
+
+    OptionDefinitionPtr def4(new OptionDefinition("option-foo", 222, "uint16"));
+    EXPECT_NO_THROW(cfg_mgr.addOptionDef(def4, "dhcp4"));
+
     // Try to query the option definition from an non-existing
     // option space and expect NULL pointer.
     OptionDefinitionPtr def = cfg_mgr.getOptionDef("non-existing", 56);
-    ASSERT_FALSE(def);
+    EXPECT_FALSE(def);
+
+    // Try to get the non-existing option definition from an
+    // existing option space.
+    EXPECT_FALSE(cfg_mgr.getOptionDef("isc", 56));
+
 }
 
 // This test verifies that the function that adds new option definition
 // throws exceptions when arguments are invalid.
 TEST_F(CfgMgrTest, addOptionDefNegative) {
     CfgMgr& cfg_mgr = CfgMgr::instance();
-    OptionDefinitionPtr def(new OptionDefinition("option-foo", 100, "uint16"));
+    // The option code 65 is reserved for standard options either in
+    // DHCPv4 or DHCPv6. Thus we expect that adding an option to this
+    // option space fails.
+    OptionDefinitionPtr def(new OptionDefinition("option-foo", 65, "uint16"));
 
     // Try reserved option space names.
     ASSERT_THROW(cfg_mgr.addOptionDef(def, "dhcp4"), isc::BadValue);
@@ -170,6 +200,10 @@ TEST_F(CfgMgrTest, addOptionDefNegative) {
     // Try NULL option definition.
     ASSERT_THROW(cfg_mgr.addOptionDef(OptionDefinitionPtr(), "isc"),
                  isc::dhcp::MalformedOptionDefinition);
+    // Try adding option definition twice and make sure that it
+    // fails on the second attempt.
+    ASSERT_NO_THROW(cfg_mgr.addOptionDef(def, "isc"));
+    EXPECT_THROW(cfg_mgr.addOptionDef(def, "isc"), DuplicateOptionDefinition);
 }
 
 // This test verifies if the configuration manager is able to hold and return

src/lib/dhcpsrv/tests/subnet_unittest.cc

@@ -441,11 +441,11 @@ TEST(Subnet6Test, getOptionSingle) {
     for (uint16_t code = 100; code < 110; ++code) {
         std::ostringstream stream;
         // First, try the invalid option space name.
-        Subnet::OptionDescriptor desc = subnet->getOptionSingle("isc", code);
+        Subnet::OptionDescriptor desc = subnet->getOptionDescriptor("isc", code);
         // Returned descriptor should contain NULL option ptr.
         EXPECT_FALSE(desc.option);
         // Now, try the valid option space.
-        desc = subnet->getOptionSingle("dhcp6", code);
+        desc = subnet->getOptionDescriptor("dhcp6", code);
         // Test that the option code matches the expected code.
         ASSERT_TRUE(desc.option);
         EXPECT_EQ(code, desc.option->getType());