[3589] Addressed review comments.

src/lib/dhcp/option.h

@@ -352,7 +352,17 @@ public:
     /// just to force that every option has virtual dtor
     virtual ~Option();
 
-    /// @brief Checks if two options are equal
+    /// @brief Checks if options are equal.
+    ///
+    /// This method calls a virtual @c equals function to compare objects.
+    /// This method is not meant to be overriden in the derived classes.
+    /// Instead, the other @c equals function must be overriden.
+    ///
+    /// @param other Pointer to the option to compare this option to.
+    /// @return true if both options are equal, false otherwise.
+    bool equals(const OptionPtr& other) const;
+
+    /// @brief Checks if two options are equal.
     ///
     /// Equality verifies option type and option content. Care should
     /// be taken when using this method. Implementation for derived
@@ -361,10 +371,9 @@ public:
     /// will detect differences between base Option and derived
     /// objects.
     ///
-    /// @param other the other option
-    /// @return true if both options are equal
-    bool equals(const OptionPtr& other) const;
-
+    /// @param other Instance of the option to compare to.
+    ///
+    /// @return true if options are equal, false otherwise.
     virtual bool equals(const Option& other) const;
 
 protected:

src/lib/dhcp/tests/option_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 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
@@ -547,8 +547,9 @@ TEST_F(OptionTest, setData) {
                             buf_.size()));
 }
 
-// This test verifies that options can be compared using equals() method.
-TEST_F(OptionTest, equals) {
+// This test verifies that options can be compared using equals(OptionPtr)
+// method.
+TEST_F(OptionTest, equalsWithPointers) {
 
     // Five options with varying lengths
     OptionPtr opt1(new Option(Option::V6, 258, buf_.begin(), buf_.begin() + 1));
@@ -570,6 +571,29 @@ TEST_F(OptionTest, equals) {
     EXPECT_TRUE(opt2->equals(opt5));
 }
 
+// This test verifies that options can be compared using equals(Option) method.
+TEST_F(OptionTest, equals) {
+
+    // Five options with varying lengths
+    Option opt1(Option::V6, 258, buf_.begin(), buf_.begin() + 1);
+    Option opt2(Option::V6, 258, buf_.begin(), buf_.begin() + 2);
+    Option opt3(Option::V6, 258, buf_.begin(), buf_.begin() + 3);
+
+    // The same content as opt2, but different type
+    Option opt4(Option::V6, 1, buf_.begin(), buf_.begin() + 2);
+
+    // Another instance with the same type and content as opt2
+    Option opt5(Option::V6, 258, buf_.begin(), buf_.begin() + 2);
+
+    EXPECT_TRUE(opt1.equals(opt1));
+
+    EXPECT_FALSE(opt1.equals(opt2));
+    EXPECT_FALSE(opt1.equals(opt3));
+    EXPECT_FALSE(opt1.equals(opt4));
+
+    EXPECT_TRUE(opt2.equals(opt5));
+}
+
 // This test verifies that the name of the option space being encapsulated by
 // the particular option can be set.
 TEST_F(OptionTest, setEncapsulatedSpace) {

src/lib/dhcpsrv/cfg_option.h

@@ -281,6 +281,7 @@ public:
     ///
     /// This method will not return vendor options, i.e. having option space
     /// name in the format of "vendor-X" where X is 32-bit unsiged integer.
+    /// See @c getAll(uint32_t) for vendor options.
     ///
     /// @param option_space Name of the option space.
     ///
@@ -330,6 +331,8 @@ public:
     /// If the option space name is specified in the following format:
     /// "vendor-X" where X is an uint32_t number, it is assumed to be
     /// a vendor space and the uint32_t number is returned by this function.
+    /// If the option space name is invalid this method will return 0 which
+    /// is not valid vendor-id.
     ///
     /// @todo remove this function once when the conversion is dealt by the
     /// appropriate functions returning options by option space names.
@@ -342,7 +345,14 @@ private:
 
     /// @brief Appends encapsulated options to the options in an option space.
     ///
-    /// @param option_space Name of the option space containing optionn to
+    /// This method appends sub-options to the options belonging to the
+    /// particular option space. For example: if the option space "foo"
+    /// is specified, this function will go over all options belonging to
+    /// "foo" and will check which option spaces they encapsulate. For each
+    /// such option it will retrieve options for these option spaces and append
+    /// as sub-options to options belonging to "foo".
+    ///
+    /// @param option_space Name of the option space containing option to
     /// which encapsulated options are appended.
     void encapsulateInternal(const std::string& option_space);
 

src/lib/dhcpsrv/subnet.h

@@ -107,6 +107,12 @@ public:
         return (cfg_option_);
     }
 
+    /// @brief Returns const pointer to the option data configuration for this
+    /// subnet.
+    ConstCfgOptionPtr getCfgOption() const {
+        return (cfg_option_);
+    }
+
     /// @brief returns the last address that was tried from this pool
     ///
     /// This method returns the last address that was attempted to be allocated

src/lib/dhcpsrv/tests/cfg_option_unittest.cc

@@ -19,6 +19,8 @@
 #include <dhcpsrv/cfg_option.h>
 #include <boost/pointer_cast.hpp>
 #include <gtest/gtest.h>
+#include <limits>
+#include <sstream>
 
 using namespace isc;
 using namespace isc::dhcp;
@@ -246,7 +248,7 @@ TEST(CfgOptionTest, copy) {
 // to the top level options on request.
 TEST(CfgOptionTest, encapsulate) {
     CfgOption cfg;
-    // Create top-level options. These options encapsulate "foo" option space.
+    // Create top-level options encapsulating "foo" option space.
     for (uint16_t code = 1000; code < 1020; ++code) {
         OptionUint16Ptr option = OptionUint16Ptr(new OptionUint16(Option::V6,
                                                                   code, 1234));
@@ -254,6 +256,14 @@ TEST(CfgOptionTest, encapsulate) {
         ASSERT_NO_THROW(cfg.add(option, false, DHCP6_OPTION_SPACE));
     }
 
+    // Create top level options encapsulating "bar" option space.
+    for (uint16_t code = 1020; code < 1040; ++code) {
+        OptionUint16Ptr option = OptionUint16Ptr(new OptionUint16(Option::V6,
+                                                                  code, 2345));
+        option->setEncapsulatedSpace("bar");
+        ASSERT_NO_THROW(cfg.add(option, false, DHCP6_OPTION_SPACE));
+    }
+
     // Create sub-options belonging to "foo" option space.
     for (uint16_t code = 1; code < 20; ++code) {
         OptionUint8Ptr option = OptionUint8Ptr(new OptionUint8(Option::V6, code,
@@ -261,25 +271,36 @@ TEST(CfgOptionTest, encapsulate) {
         ASSERT_NO_THROW(cfg.add(option, false, "foo"));
     }
 
-    // Append options from "foo" space as sub-options.
+    // Create sub-options belonging to "bar" option space.
+    for (uint16_t code = 100;  code < 130; ++code) {
+        OptionUint8Ptr option = OptionUint8Ptr(new OptionUint8(Option::V6,
+                                                               code, 0x02));
+        ASSERT_NO_THROW(cfg.add(option, false, "bar"));
+    }
+
+    // Append options from "foo" and "bar" space as sub-options.
     ASSERT_NO_THROW(cfg.encapsulate());
 
-    // Verify that we have 20 top-level options.
+    // Verify that we have 40 top-level options.
     OptionContainerPtr options = cfg.getAll(DHCP6_OPTION_SPACE);
-    ASSERT_EQ(20, options->size());
+    ASSERT_EQ(40, options->size());
 
-    // Verify that each of them contains expected sub-options.
-    for (uint16_t code = 1000; code < 1020; ++code) {
+    for (uint16_t code = 1000; code < 1040; ++code) {
         OptionUint16Ptr option = boost::dynamic_pointer_cast<
             OptionUint16>(cfg.get(DHCP6_OPTION_SPACE, code).option);
         ASSERT_TRUE(option) << "option with code " << code << " not found";
-        EXPECT_EQ(1234, option->getValue());
-        for (uint16_t subcode = 1; subcode < 20; ++subcode) {
-            OptionUint8Ptr suboption = boost::dynamic_pointer_cast<
-                OptionUint8>(option->getOption(subcode));
-            ASSERT_TRUE(suboption) << "suboption with code " << subcode
-                                   << " not found";
-            EXPECT_EQ(0x01, suboption->getValue());
+        const OptionCollection& suboptions = option->getOptions();
+        for (OptionCollection::const_iterator suboption =
+                 suboptions.begin(); suboption != suboptions.end();
+             ++suboption) {
+            OptionUint8Ptr opt = boost::dynamic_pointer_cast<
+                OptionUint8>(suboption->second);
+            ASSERT_TRUE(opt);
+            if (code < 1020) {
+                EXPECT_EQ(0x01, opt->getValue());
+            } else {
+                EXPECT_EQ(0x02, opt->getValue());
+            }
         }
     }
 }
@@ -410,11 +431,16 @@ TEST(CfgOptionTest, addVendorOptions) {
         ASSERT_NO_THROW(cfg.add(option, false, "vendor-12345678"));
     }
 
+    // Second option space uses corner case value for vendor id = max uint8.
+    uint32_t vendor_id = std::numeric_limits<uint32_t>::max();
+    std::ostringstream option_space;
+    option_space << "vendor-" << vendor_id;
+
     // Add 7 options to another option space. The option codes partially overlap
     // with option codes that we have added to dhcp6 option space.
     for (uint16_t code = 105; code < 112; ++code) {
         OptionPtr option(new Option(Option::V6, code, OptionBuffer(10, 0xFF)));
-        ASSERT_NO_THROW(cfg.add(option, false, "vendor-87654321"));
+        ASSERT_NO_THROW(cfg.add(option, false, option_space.str()));
     }
 
     // Get options from the Subnet and check if all 10 are there.
@@ -431,7 +457,7 @@ TEST(CfgOptionTest, addVendorOptions) {
         ++expected_code;
     }
 
-    options = cfg.getAll(87654321);
+    options = cfg.getAll(vendor_id);
     ASSERT_TRUE(options);
     ASSERT_EQ(7, options->size());