[2318] Changes suggested in the code review.

src/bin/dhcp6/config_parser.cc

@@ -490,7 +490,8 @@ public:
     /// @brief Constructor.
     ///
     /// Class constructor.
-    OptionDataParser(const std::string&) { }
+    OptionDataParser(const std::string&)
+        : options_(NULL) { }
 
     /// @brief Parses the single option data.
     ///
@@ -499,11 +500,21 @@ public:
     /// carried by this option. Eventually it creates the instance of the
     /// option.
     ///
+    /// @warning setStorage must be called with valid storage pointer prior
+    /// to calling this method.
+    ///
     /// @param option_data_entries collection of entries that define value
     /// for a particular option.
-    /// @throw isc::InvalidParameter if invalid parameter specified in
+    /// @throw Dhcp6ConfigError if invalid parameter specified in
     /// the configuration.
+    /// @throw isc::InvalidOperation if failed to set storage prior to
+    /// calling build.
+    /// @throw isc::BadValue if option data storage is invalid.
     virtual void build(ConstElementPtr option_data_entries) {
+        if (options_ == NULL) {
+            isc_throw(isc::InvalidOperation, "Parser logic error: storage must be set before "
+                      "parsing option data.");
+        }
         BOOST_FOREACH(ConfigPair param, option_data_entries->mapValue()) {
             ParserPtr parser;
             if (param.first == "name") {
@@ -565,6 +576,10 @@ private:
     /// options storage. If the option data parsed by \ref build function
     /// are invalid or insufficient it emits exception.
     ///
+    /// @warning this function does not check if options_ storage pointer
+    /// is intitialized but this is not needed here because it is checked in
+    /// \ref build function.
+    ///
     /// @throw Dhcp6ConfigError if parameters provided in the configuration
     /// are invalid.
     void createOption() {
@@ -607,6 +622,7 @@ private:
         // created for all options.
         OptionPtr option(new Option(Option::V6, static_cast<uint16_t>(option_code),
                                     binary));
+
         // If option is created succesfully, add it to the storage.
         options_->push_back(option);
     }
@@ -656,6 +672,10 @@ class OptionDataListParser : public DhcpConfigParser {
 public:
 
     /// @brief Constructor.
+    ///
+    /// Unless otherwise specified, parsed options will be stored in
+    /// a global option containers (option_default). That storage location
+    /// is overriden on a subnet basis.
     OptionDataListParser(const std::string&)
         : options_(&option_defaults) { }
 
@@ -723,39 +743,36 @@ public:
     void build(ConstElementPtr subnet) {
 
         BOOST_FOREACH(ConfigPair param, subnet->mapValue()) {
-
             ParserPtr parser(createSubnet6ConfigParser(param.first));
-
-            // if this is an Uint32 parser, tell it to store the values
-            // in values_, rather than in global storage
-            boost::shared_ptr<Uint32Parser> uintParser =
-                boost::dynamic_pointer_cast<Uint32Parser>(parser);
-            if (uintParser) {
-                uintParser->setStorage(&uint32_values_);
-            } else {
-
-                boost::shared_ptr<StringParser> stringParser =
-                    boost::dynamic_pointer_cast<StringParser>(parser);
-                if (stringParser) {
-                    stringParser->setStorage(&string_values_);
-                } else {
-
-                    boost::shared_ptr<PoolParser> poolParser =
-                        boost::dynamic_pointer_cast<PoolParser>(parser);
-                    if (poolParser) {
-                        poolParser->setStorage(&pools_);
-                    } else {
-                        boost::shared_ptr<OptionDataListParser> option_data_list_parser =
-                            boost::dynamic_pointer_cast<OptionDataListParser>(parser);
-                        option_data_list_parser->setStorage(&options_);
-                    }
-                }
+            // The actual type of the parser is unknown here. We have to discover
+            // parser type here to invoke corresponding setStorage function on it.
+            // We discover parser type by trying to cast the parser to various
+            // parser types and checking which one was successful. For this one
+            // a setStorage and build methods are invoked.
+
+            // Try uint32 type parser.
+            if (buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
+                                                          param.second)) {
+                // Storage set, build invoked on the parser, proceed with
+                // next configuration element.
+                continue;
+            }
+            // Try string type parser.
+            if (buildParser<StringParser, StringStorage >(parser, string_values_,
+                                                          param.second)) {
+                continue;
+            }
+            // Try pools parser.
+            if (buildParser<PoolParser, PoolStorage >(parser, pools_,
+                                                      param.second)) {
+                continue;
+            }
+            // Try option data parser.
+            if (buildParser<OptionDataListParser, OptionStorage >(parser, options_,
+                                                                  param.second)) {
+                continue;
             }
-
-            parser->build(param.second);
-            parsers_.push_back(parser);
         }
-
         // Ok, we now have subnet parsed
     }
 
@@ -805,29 +822,32 @@ public:
 
         // Add subnet specific options.
         BOOST_FOREACH(OptionPtr option, options_) {
+            Subnet::OptionContainerTypeRange range =
+                subnet->getOptions(option->getType());
+            if (std::distance(range.first, range.second) > 0) {
+                LOG_WARN(dhcp6_logger, DHCP6_CONFIG_OPTION_DUPLICATE)
+                    .arg(option->getType()).arg(addr.toText());
+            }
             subnet->addOption(option);
         }
 
-        // Get all options that we have added to subnet so far. We will
-        // use them to check which of the global options must be added to
-        // the subnet.
-        Subnet::OptionContainer options = subnet->getOptions();
-        // Get the search index #1 which is used to search options
-        // by their code (type).
-        Subnet::OptionContainerTypeIndex& idx = options.get<1>();
         // Check all global options and add them to the subnet object if
         // they have been configured in the global scope. If they have been
         // configured in the subnet scope we don't add global option because
         // the one configured in the subnet scope always takes precedense.
         BOOST_FOREACH(OptionPtr option, option_defaults) {
-            // Get local option descriptors using global option code.
-            std::pair<Subnet::OptionContainerTypeIndex::const_iterator,
-                      Subnet::OptionContainerTypeIndex::const_iterator> range =
-                idx.equal_range(option->getType());
+            // Get all options specified locally in the subnet and having
+            // code equal to global option's code.
+            Subnet::OptionContainerTypeRange range =
+                subnet->getOptions(option->getType());
             // @todo: In the future we will be searching for options using either
             // option code or namespace. Currently we have only the option
             // code available so if there is at least one option found with the
             // specific code we don't add globally configured option.
+            // @todo with this code the first globally configured option
+            // with the given code will be added to a subnet. We may
+            // want to issue warning about dropping configuration of
+            // global option if one already exsist.
             if (std::distance(range.first, range.second) == 0) {
                 subnet->addOption(option);
             }
@@ -836,7 +856,41 @@ public:
         CfgMgr::instance().addSubnet6(subnet);
     }
 
-protected:
+private:
+
+    /// @brief Set storage for a parser and invoke build.
+    ///
+    /// This helper method casts the provided parser pointer to specified
+    /// type. If cast is successful it sets the corresponding storage for
+    /// this parser, invokes build on it and save the parser.
+    ///
+    /// @tparam T parser type to which parser argument should be cast.
+    /// @tparam Y storage type for the specified parser type.
+    /// @param parser parser on which build must be invoked.
+    /// @param storage reference to a storage that will be set for a parser.
+    /// @param subnet subnet element read from the configuration and being parsed.
+    /// @return true if parser pointer was successfully cast to specialized
+    /// parser type provided as Y.
+    template<typename T, typename Y>
+    bool buildParser(const ParserPtr& parser, Y& storage, const ConstElementPtr& subnet) {
+        // We need to cast to T in order to set storage for the parser.
+        boost::shared_ptr<T> cast_parser = boost::dynamic_pointer_cast<T>(parser);
+        // It is common that this cast is not successful because we try to cast to all
+        // supported parser types as we don't know the type of a parser in advance.
+        if (cast_parser) {
+            // Cast, successful so we go ahead with setting storage and actual parse.
+            cast_parser->setStorage(&storage);
+            parser->build(subnet);
+            parsers_.push_back(parser);
+            // We indicate that cast was successful so as the calling function
+            // may skip attempts to cast to other parser types and proceed to
+            // next element.
+            return (true);
+        }
+        // It was not successful. Indicate that another parser type
+        // should be tried.
+        return (false);
+    }
 
     /// @brief creates parsers for entries in subnet definition
     ///

src/bin/dhcp6/dhcp6_messages.mes

@@ -129,3 +129,8 @@ This is an informational message announcing the successful processing of a
 new configuration. it is output during server startup, and when an updated
 configuration is committed by the administrator.  Additional information
 may be provided.
+
+% DHCP6_CONFIG_OPTION_DUPLICATE multiple options with the code: %1 added to the subnet: %2
+This warning message is issued on attempt to configure multiple options with the
+same option code for the particular subnet. Adding multiple options is uncommon
+for DHCPv6, yet it is not prohibited.

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -120,9 +120,12 @@ public:
     /// @param expected_code expected code of the option.
     /// @param expected_data expected data in the option.
     /// @param expected_data_len length of the reference data.
+    /// @param extra_data if true extra data is allowed in an option
+    /// after tested data.
     void testOption(const Subnet::OptionDescriptor& option_desc,
                     uint16_t expected_code, const uint8_t* expected_data,
-                    size_t expected_data_len) {
+                    size_t expected_data_len,
+                    bool extra_data = false) {
         // Check if option descriptor contains valid option pointer.
         ASSERT_TRUE(option_desc.option);
         // Verify option type.
@@ -134,12 +137,17 @@ public:
         // way is to call pack() which will prepare on-wire data.
         util::OutputBuffer buf(option_desc.option->getData().size());
         option_desc.option->pack(buf);
-        // The length of the buffer must be at least equal to size of the
-        // reference data but it can sometimes be greater than that. This is
-        // because some options carry suboptions that increase the overall
-        // length.
-        ASSERT_GE(buf.getLength() - option_desc.option->getHeaderLen(),
-                  expected_data_len);
+        if (extra_data) {
+            // The length of the buffer must be at least equal to size of the
+            // reference data but it can sometimes be greater than that. This is
+            // because some options carry suboptions that increase the overall
+            // length.
+            ASSERT_GE(buf.getLength() - option_desc.option->getHeaderLen(),
+                      expected_data_len);
+        } else {
+            ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(),
+                      expected_data_len);
+        }
         // Verify that the data is correct. However do not verify suboptions.
         const uint8_t* data = static_cast<const uint8_t*>(buf.getData());
         EXPECT_TRUE(memcmp(expected_data, data, expected_data_len));
@@ -336,6 +344,9 @@ TEST_F(Dhcp6ParserTest, poolPrefixLen) {
     EXPECT_EQ(4000, subnet->getValid());
 }
 
+// Goal of this test is to verify that global option
+// data is configured for the subnet if the subnet
+// configuration does not include options configuration.
 TEST_F(Dhcp6ParserTest, optionDataDefaults) {
     ConstElementPtr x;
     string config = "{ \"interface\": [ \"all\" ],"
@@ -394,8 +405,18 @@ TEST_F(Dhcp6ParserTest, optionDataDefaults) {
         0x01
     };
     testOption(*range.first, 101, foo2_expected, sizeof(foo2_expected));
+
+    // Check that options with other option codes are not returned.
+    for (uint16_t code = 102; code < 110; ++code) {
+        ASSERT_NO_THROW(range = subnet->getOptions(code));
+        EXPECT_EQ(0, std::distance(range.first, range.second));
+    }
 }
 
+// Goal of this test is to verify options configuration
+// for a single subnet. In particular this test checks
+// that local options configuration overrides global
+// option setting.
 TEST_F(Dhcp6ParserTest, optionDataInSingleSubnet) {
     ConstElementPtr x;
     string config = "{ \"interface\": [ \"all\" ],"
@@ -461,6 +482,8 @@ TEST_F(Dhcp6ParserTest, optionDataInSingleSubnet) {
     testOption(*range.first, 101, foo2_expected, sizeof(foo2_expected));
 }
 
+// Goal of this test is to verify options configuration
+// for multiple subnets.
 TEST_F(Dhcp6ParserTest, optionDataInMultipleSubnets) {
     ConstElementPtr x;
     string config = "{ \"interface\": [ \"all\" ],"
@@ -535,16 +558,21 @@ TEST_F(Dhcp6ParserTest, optionDataInMultipleSubnets) {
     testOption(*range2.first, 101, foo2_expected, sizeof(foo2_expected));
 }
 
+// Verify that empty option name is rejected in the configuration.
 TEST_F(Dhcp6ParserTest, optionNameEmpty) {
     // Empty option names not allowed.
     testInvalidOptionParam("", "name");
 }
 
+// Verify that empty option name with spaces is rejected
+// in the configuration.
 TEST_F(Dhcp6ParserTest, optionNameSpaces) {
     // Spaces in option names not allowed.
     testInvalidOptionParam("option foo", "name");
 }
 
+// Verify that very low negative option code is rejected in
+//  the configuration.
 TEST_F(Dhcp6ParserTest, optionCodeNegativeOverflow) {
     // Using negative code. If range checking is not applied on the
     // value then it may be successfully cast to uint16_t resulting
@@ -554,11 +582,13 @@ TEST_F(Dhcp6ParserTest, optionCodeNegativeOverflow) {
     testInvalidOptionParam("-4294901765", "code");
 }
 
+// Verify that negative option code is rejected in the configuration.
 TEST_F(Dhcp6ParserTest, optionCodeNegative) {
     // Check negative option code -4. This should fail too.
     testInvalidOptionParam("-4", "code");
 }
 
+// Verify that out of bounds option code is rejected in the configuration.
 TEST_F(Dhcp6ParserTest, optionCodeNonUint16) {
     // The valid option codes are uint16_t values so passing
     // uint16_t maximum value incremented by 1 should result
@@ -566,36 +596,46 @@ TEST_F(Dhcp6ParserTest, optionCodeNonUint16) {
     testInvalidOptionParam("65536", "code");
 }
 
+// Verify that out of bounds option code is rejected in the configuration.
 TEST_F(Dhcp6ParserTest, optionCodeHighNonUint16) {
     // Another check for uint16_t overflow but this time
     // let's pass even greater option code value.
     testInvalidOptionParam("70000", "code");
 }
 
+// Verify that zero option code is rejected in the configuration.
 TEST_F(Dhcp6ParserTest, optionCodeZero) {
     // Option code 0 is reserved and should not be accepted
     // by configuration parser.
     testInvalidOptionParam("0", "code");
 }
 
+// Verify that option data which contains non hexadecimal characters
+// is rejected by the configuration.
 TEST_F(Dhcp6ParserTest, optionDataInvalidChar) {
     // Option code 0 is reserved and should not be accepted
     // by configuration parser.
     testInvalidOptionParam("01020R", "data");
 }
 
+// Verify that option data containins '0x' prefix is rejected
+// by the configuration.
 TEST_F(Dhcp6ParserTest, optionDataUnexpectedPrefix) {
     // Option code 0 is reserved and should not be accepted
     // by configuration parser.
     testInvalidOptionParam("0x0102", "data");
 }
 
+// Verify that option data consisting od an odd number of
+// hexadecimal digits is rejected in the configuration.
 TEST_F(Dhcp6ParserTest, optionDataOddLength) {
     // Option code 0 is reserved and should not be accepted
     // by configuration parser.
     testInvalidOptionParam("123", "data");
 }
 
+// Verify that either lower or upper case characters are allowed
+// to specify the option data.
 TEST_F(Dhcp6ParserTest, optionDataLowerCase) {
     ConstElementPtr x;
     std::string config = createConfigWithOption("0a0b0C0D", "data");

src/lib/dhcp/subnet.cc

@@ -52,6 +52,18 @@ Subnet::delOptions() {
     options_.clear();
 }
 
+Subnet::OptionContainerTypeRange
+Subnet::getOptions(uint16_t type) {
+    OptionContainer options = getOptions();
+    // Get the search index #1. This index allows to search
+    // for options using option type.
+    OptionContainerTypeIndex& idx = options.get<1>();
+    OptionContainerTypeRange range = idx.equal_range(type);
+    // We don't perform any check if this range is empty. It is
+    // up to the calling function to check it with std::distance().
+    return (range);
+}
+
 Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length,
                  const Triplet<uint32_t>& t1,
                  const Triplet<uint32_t>& t2,

src/lib/dhcp/subnet.h

@@ -186,6 +186,11 @@ public:
 
     /// Type of the index #1 - option type.
     typedef OptionContainer::nth_index<1>::type OptionContainerTypeIndex;
+    /// Pair of iterators to represent the range of options having the
+    /// same option type value. The first element in this pair represents
+    /// the begining of the range, the second element represents the end.
+    typedef std::pair<OptionContainerTypeIndex::const_iterator,
+                      OptionContainerTypeIndex::const_iterator> OptionContainerTypeRange;
     /// Type of the index #2 - option persistency flag.
     typedef OptionContainer::nth_index<2>::type OptionContainerPersistIndex;
 
@@ -228,6 +233,13 @@ public:
         return (options_);
     }
 
+    /// @brief Return a collection of options of a specified type.
+    ///
+    /// @param type option type.
+    /// @return pair of iterators, first indicating begining of
+    /// options range, second indicating end of the range.
+    OptionContainerTypeRange getOptions(uint16_t type);
+
 protected:
     /// @brief protected constructor
     //

src/lib/dhcp/tests/subnet_unittest.cc

@@ -285,6 +285,43 @@ TEST(Subnet6Test, addNonUniqueOptions) {
     EXPECT_EQ(0, options.size());
 }
 
+TEST(Subnet6Test, getOptionsByType) {
+    Subnet6Ptr subnet(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));
+    Subnet::OptionContainerTypeRange range;
+
+    // First check that subnet does not contain any options with
+    // option codes 100 and 101.
+    ASSERT_NO_THROW(range = subnet->getOptions(100));
+    EXPECT_EQ(0, std::distance(range.first, range.second));
+    ASSERT_NO_THROW(range = subnet->getOptions(101));
+    EXPECT_EQ(0, std::distance(range.first, range.second));
+
+    // Add 3 options with option code 100.
+    for (int i = 0; i < 3; ++i) {
+        OptionPtr option(new Option(Option::V6, 100, OptionBuffer(10, 0xFF)));
+        ASSERT_NO_THROW(subnet->addOption(option));
+    }
+    // Add 7 options with option code 101.
+    for (int i = 0; i < 7; ++i) {
+        OptionPtr option(new Option(Option::V6, 101, OptionBuffer(10, 0xFF)));
+        ASSERT_NO_THROW(subnet->addOption(option));
+    }
+    // Get options by their type and check that 3 options with the
+    // code 100 and 7 options with the code 101 are returned.
+    ASSERT_NO_THROW(range = subnet->getOptions(100));
+    EXPECT_EQ(3, std::distance(range.first, range.second));
+    ASSERT_NO_THROW(range = subnet->getOptions(101));
+    EXPECT_EQ(7, std::distance(range.first, range.second));
+    // Delete all options.
+    EXPECT_NO_THROW(subnet->delOptions());
+    // Verify that after deletion getOptions will not return
+    // any options.
+    ASSERT_NO_THROW(range = subnet->getOptions(100));
+    EXPECT_EQ(0, std::distance(range.first, range.second));
+    ASSERT_NO_THROW(range = subnet->getOptions(101));
+    EXPECT_EQ(0, std::distance(range.first, range.second));
+}
+
 TEST(Subnet6Test, addInvalidOption) {
     // Create as subnet to add options to it.
     Subnet6Ptr subnet(new Subnet6(IOAddress("2001:db8:1::"), 56, 1, 2, 3, 4));