[2417] Added changes suggested in the code review.

src/bin/dhcp6/config_parser.cc

@@ -617,7 +617,11 @@ private:
                       << " spaces");
         }
 
+        // Get option data from the configuration database ('data' field).
+        // Option data is specified by the user as case insensitive string
+        // of hexadecimal digits for each option.
         std::string option_data = getStringParam("data");
+        // Transform string of hexadecimal digits into binary format.
         std::vector<uint8_t> binary;
         try {
             util::encode::decodeHex(option_data, binary);
@@ -625,29 +629,40 @@ private:
             isc_throw(Dhcp6ConfigError, "Parser error: option data is not a valid"
                       << " string of hexadecimal digits: " << option_data);
         }
-
+        // Get all existing DHCPv6 option definitions. The one that matches
+        // our option will be picked and used to create it.
         OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V6);
+        // Get search index #1. It allows searching for options definitions
+        // using option type value.
         const OptionDefContainerTypeIndex& idx = option_defs.get<1>();
-
+        // Get all option definitions matching option code we want to create.
         const OptionDefContainerTypeRange& range = idx.equal_range(option_code);
         size_t num_defs = std::distance(range.first, range.second);
         OptionPtr option;
+        // Currently we do not allow duplicated definitions and if there are
+        // any duplicates we issue internal server error.
         if (num_defs > 1) {
             isc_throw(Dhcp6ConfigError, "Internal error: currently it is not"
                       << " supported to initialize multiple option definitions"
                       << " for the same option code. This will be supported once"
                       << " there option spaces are implemented.");
         } else if (num_defs == 0) {
-            // Create the actual option.
+            // @todo We have a limited set of option definitions intiialized at the moment.
+            // In the future we want to initialize option definitions for all options.
+            // Consequently error will be issued if option definition does not exist
+            // for a particular option code. For now it is ok to create generic option
+            // if definition does not exist.
             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);
         } else {
+            // We have exactly one option definition for the particular option code.
+            // use it to create option instance.
             const OptionDefinitionPtr& def = *(range.first);
-            // getFactory should never return NULL pointer so we skip
+            // getFactory should never return NULL pointer.
-            // sanity check here.
             Option::Factory* factory = def->getFactory();
+            assert(factory != NULL);
             try {
                 OptionPtr option = factory(Option::V6, option_code, binary);
                 options_->push_back(option);

src/bin/dhcp6/dhcp6_messages.mes

@@ -110,9 +110,15 @@ This is a debug message issued during the IPv6 DHCP server startup.
 It lists some information about the parameters with which the server
 is running.
 
-% DHCP6_NO_SUBNET_FOR_ADDRESS fail to find subnet for address: %1
+% DHCP6_NO_SUBNET_DEF_OPT failed to find subnet for address %1 when adding default options
-This warning message indicates that server does not support subnet
+This warning message indicates that when attempting to add default options to a response,
-that received DHCPv6 packet comes from.
+the server found that it was not configured to support the subnet from which the DHCPv6
+request was received.  The packet has been ignored.
+
+% DHCP6_NO_SUBNET_REQ_OPT failed to find subnet for address %1 when adding requested options
+This warning message indicates that when attempting to add requested options to a response,
+the server found that it was not configured to support the subnet from which the DHCPv6
+request was received.  The packet has been ignored.
 
 % DHCP6_CONFIG_LOAD_FAIL failed to load configuration: %1
 This critical error message indicates that the initial DHCPv6

src/bin/dhcp6/dhcp6_srv.cc

@@ -303,7 +303,7 @@ void Dhcpv6Srv::appendDefaultOptions(const Pkt6Ptr& question, Pkt6Ptr& answer) {
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr());
     // Warn if subnet is not supported and quit.
     if (!subnet) {
-        LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_FOR_ADDRESS)
+        LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_DEF_OPT)
             .arg(question->getRemoteAddr().toText());
         return;
     }
@@ -325,7 +325,7 @@ void Dhcpv6Srv::appendRequestedOptions(const Pkt6Ptr& question, Pkt6Ptr& answer)
     // Get the subnet for a particular address.
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr());
     if (!subnet) {
-        LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_FOR_ADDRESS)
+        LOG_WARN(dhcp6_logger, DHCP6_NO_SUBNET_REQ_OPT)
             .arg(question->getRemoteAddr().toText());
         return;
     }

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -76,7 +76,7 @@ TEST_F(Dhcpv6SrvTest, basic) {
     // fe80::1234 link-local address on eth0 interface. Obviously
     // an attempt to bind this socket will fail.
     Dhcpv6Srv* srv = NULL;
-    ASSERT_NO_THROW( {
+    ASSERT_NO_THROW({
         // open an unpriviledged port
         srv = new Dhcpv6Srv(DHCP6_SERVER_PORT + 10000);
     });
@@ -88,7 +88,7 @@ TEST_F(Dhcpv6SrvTest, DUID) {
     // tests that DUID is generated properly
 
     boost::scoped_ptr<Dhcpv6Srv> srv;
-    ASSERT_NO_THROW( {
+    ASSERT_NO_THROW({
         srv.reset(new Dhcpv6Srv(DHCP6_SERVER_PORT + 10000));
     });
 
@@ -186,7 +186,7 @@ TEST_F(Dhcpv6SrvTest, solicitBasic) {
     ElementPtr json = Element::fromJSON(config);
 
     boost::scoped_ptr<NakedDhcpv6Srv> srv;
-    ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv()) );
+    ASSERT_NO_THROW(srv.reset(new NakedDhcpv6Srv()));
 
     EXPECT_NO_THROW(x = configureDhcp6Server(*srv, json));
     ASSERT_TRUE(x);
@@ -233,8 +233,8 @@ TEST_F(Dhcpv6SrvTest, solicitBasic) {
     // check if we get response at all
     ASSERT_TRUE(reply);
 
-    EXPECT_EQ( DHCPV6_ADVERTISE, reply->getType() );
+    EXPECT_EQ(DHCPV6_ADVERTISE, reply->getType());
-    EXPECT_EQ( 1234, reply->getTransid() );
+    EXPECT_EQ(1234, reply->getTransid());
 
     // We have not requested option with code 1000 so it should not
     // be included in the response.
@@ -258,28 +258,28 @@ TEST_F(Dhcpv6SrvTest, solicitBasic) {
     EXPECT_EQ(DHCPV6_ADVERTISE, reply->getType());
 
     OptionPtr tmp = reply->getOption(D6O_IA_NA);
-    ASSERT_TRUE( tmp );
+    ASSERT_TRUE(tmp);
 
     boost::shared_ptr<Option6IA> reply_ia =
         boost::dynamic_pointer_cast<Option6IA>(tmp);
     ASSERT_TRUE(reply_ia);
-    EXPECT_EQ( 234, reply_ia->getIAID() );
+    EXPECT_EQ(234, reply_ia->getIAID());
 
     // check that there's an address included
-    EXPECT_TRUE( reply_ia->getOption(D6O_IAADDR));
+    EXPECT_TRUE(reply_ia->getOption(D6O_IAADDR));
 
     // check that server included our own client-id
     tmp = reply->getOption(D6O_CLIENTID);
-    ASSERT_TRUE( tmp );
+    ASSERT_TRUE(tmp);
-    EXPECT_EQ(clientid->getType(), tmp->getType() );
+    EXPECT_EQ(clientid->getType(), tmp->getType());
-    ASSERT_EQ(clientid->len(), tmp->len() );
+    ASSERT_EQ(clientid->len(), tmp->len());
 
-    EXPECT_TRUE( clientid->getData() == tmp->getData() );
+    EXPECT_TRUE(clientid->getData() == tmp->getData());
 
     // check that server included its server-id
     tmp = reply->getOption(D6O_SERVERID);
-    EXPECT_EQ(tmp->getType(), srv->getServerID()->getType() );
+    EXPECT_EQ(tmp->getType(), srv->getServerID()->getType());
-    ASSERT_EQ(tmp->len(),  srv->getServerID()->len() );
+    ASSERT_EQ(tmp->len(),  srv->getServerID()->len());
 
     EXPECT_TRUE(tmp->getData() == srv->getServerID()->getData());
  

src/lib/dhcp/libdhcp++.cc

@@ -287,8 +287,11 @@ LibDHCP::initStdOptionDefs6() {
         case D6O_STATUS_CODE:
             definition->addRecordField(OptionDefinition::UINT16_TYPE);
             definition->addRecordField(OptionDefinition::STRING_TYPE);
-        default:
             break;
+        default:
+            // The default case is intentionally left empty
+            // as it does not need any processing.
+            ;
         }
         try {
             definition->validate();

src/lib/dhcp/option_definition.h

@@ -413,10 +413,13 @@ private:
 ///
 /// This container allows to search for DHCP option definition
 /// using two indexes:
-/// - sequenced: used to access elements in the oreder they have
+/// - sequenced: used to access elements in the order they have
 /// been added to the container
 /// - option code: used to search defintions of options
 /// with a specified option code (aka option type).
+/// Note that this container can hold multiple options with the
+/// same code. For this reason, the latter index can be used to
+/// obtain a <b>range of options for a particular option code.
 /// 
 /// @todo: need an index to search options using option space name
 /// once option spaces are implemented.
@@ -433,7 +436,9 @@ typedef boost::multi_index_container<
             // Use option type as the index key. The type is held
             // in OptionDefinition object so we have to call
             // OptionDefinition::getCode to retrieve this key
-            // for each element.
+            // for each element. The option code is non-unique so
+            // multiple elements with the same option code can
+            // be returned by this index.
             boost::multi_index::const_mem_fun<
                 OptionDefinition,
                 uint16_t,

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

@@ -64,19 +64,42 @@ public:
     static void testInitOptionDefs6(const uint16_t code,
                              const OptionBuffer& buf,
                              const std::type_info& expected_type) {
+        // Initialize stdandard options definitions. They are held
+        // in the static container throughout the program.
         LibDHCP::initStdOptionDefs(Option::V6);
+        // Get all option definitions, we will use them to extract
+        // the definition for a particular option code.
         OptionDefContainer options = LibDHCP::getOptionDefs(Option::V6);
+        // Get the container index #1. This one allows for searching
+        // option definitions using option code.
         const OptionDefContainerTypeIndex& idx = options.get<1>();
+        // Get 'all' option definitions for a particular option code.
+        // For standard options we expect that the range returned
+        // will contain single option as their codes are unique.
         OptionDefContainerTypeRange range = idx.equal_range(code);
         ASSERT_EQ(1, std::distance(range.first, range.second));
+        // If we have single option definition returned, the
+        // first iterator holds it.
         OptionDefinitionPtr def = *(range.first);
+        // It should not happen that option definition is NULL but
+        // let's make sure (test should take things like that into
+        // account).
         ASSERT_TRUE(def);
+        // Check that option definition is valid.
         ASSERT_NO_THROW(def->validate());
+        // Get the factory function for the particular option
+        // definition. We will use this factory function to
+        // create option instance.
         Option::Factory* factory = NULL;
         ASSERT_NO_THROW(factory = def->getFactory());
         OptionPtr option;
+        // Create the option.
         ASSERT_NO_THROW(option = factory(Option::V6, code, buf));
+        // Make sure it is not NULL.
         ASSERT_TRUE(option);
+        // And the actual object type is the one that we expect.
+        // Note that for many options there are dedicated classes
+        // derived from Option class to represent them.
         EXPECT_TRUE(typeid(*option) == expected_type);
     }
 };

src/lib/dhcp/tests/option_definition_unittest.cc

@@ -282,25 +282,38 @@ TEST_F(OptionDefinitionTest, factoryEmpty) {
 }
 
 TEST_F(OptionDefinitionTest, factoryBinary) {
+    // Binary option is the one that is represented by the generic
+    // Option class. In fact all options can be represented by this
+    // class but for some of them it is just natural. The SERVERID
+    // option consists of the option code, length and binary data so
+    // this one was picked for this test.
     OptionDefinition opt_def("OPTION_SERVERID", D6O_SERVERID, "binary");
     Option::Factory* factory(NULL);
     EXPECT_NO_THROW(factory = opt_def.getFactory());
     ASSERT_TRUE(factory != NULL);
 
+    // Prepare some dummy data (serverid): 0, 1, 2 etc.
     OptionBuffer buf(14);
     for (int i = 0; i < 14; ++i) {
         buf[i] = i;
     }
+    // Create option instance with the factory function.
+    // If the OptionDefinition code works properly than
+    // object of the type Option should be returned.
     OptionPtr option_v6;
     ASSERT_NO_THROW(
         option_v6 = factory(Option::V6, D6O_SERVERID, buf);
     );
     // Expect base option type returned.
     ASSERT_TRUE(typeid(*option_v6) == typeid(Option));
+    // Sanity check on universe, length and size. These are
+    // the basic parameters identifying any option.
     EXPECT_EQ(Option::V6, option_v6->getUniverse());
     EXPECT_EQ(4, option_v6->getHeaderLen());
     ASSERT_EQ(buf.size(), option_v6->getData().size());
 
+    // Get the server id data from the option and compare
+    // against reference buffer. They are expected to match.
     EXPECT_TRUE(std::equal(option_v6->getData().begin(),
                            option_v6->getData().end(),
                            buf.begin()));