[2544] Changes as a result of a code review.

src/bin/dhcp4/config_parser.cc

@@ -466,12 +466,12 @@ private:
 ///
 /// This parser parses configuration entries that specify value of
 /// a single option. These entries include option name, option code
-/// and data carried by the option. If parsing is successful than an
+/// and data carried by the option. If parsing is successful then an
 /// instance of an option is created and added to the storage provided
 /// by the calling class.
 ///
 /// @todo This class parses and validates the option name. However it is
-/// not used anywhere util support for option spaces is implemented
+/// not used anywhere until support for option spaces is implemented
 /// (see tickets #2319, #2314). When option spaces are implemented
 /// there will be a way to reference the particular option using
 /// its type (code) or option name.
@@ -827,26 +827,21 @@ public:
             // a setStorage and build methods are invoked.
 
             // Try uint32 type parser.
-            if (buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
+            if (!buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
-                                                          param.second)) {
+                                                          param.second) &&
-                // Storage set, build invoked on the parser, proceed with
+                // Try string type parser.
-                // next configuration element.
+                !buildParser<StringParser, StringStorage >(parser, string_values_,
-                continue;
+                                                           param.second) &&
-            }
+                // Try pool parser.
-            // Try string type parser.
+                !buildParser<PoolParser, PoolStorage >(parser, pools_,
-            if (buildParser<StringParser, StringStorage >(parser, string_values_,
+                                                       param.second) &&
-                                                          param.second)) {
+                // Try option data parser.
-                continue;
+                !buildParser<OptionDataListParser, OptionStorage >(parser, options_,
-            }
+                                                                   param.second)) {
-            // Try pools parser.
+                // Appropriate parsers are created in the createSubnet6ConfigParser
-            if (buildParser<PoolParser, PoolStorage >(parser, pools_,
+                // and they should be limited to those that we check here for. Thus,
-                                                      param.second)) {
+                // if we fail to find a matching parser here it is a programming error.
-                continue;
+                isc_throw(Dhcp4ConfigError, "failed to find suitable parser");
-            }
-            // Try option data parser.
-            if (buildParser<OptionDataListParser, OptionStorage >(parser, options_,
-                                                                  param.second)) {
-                continue;
             }
         }
         // Ok, we now have subnet parsed

src/bin/dhcp4/dhcp4_messages.mes

@@ -45,9 +45,9 @@ This is an informational message reporting that the configuration has
 been extended to include the specified IPv4 subnet.
 
 % DHCP4_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
+This warning message is issued on an attempt to configure multiple options with the
 same option code for the particular subnet. Adding multiple options is uncommon
-for DHCPv4, yet it is not prohibited.
+for DHCPv4, but it is not prohibited.
 
 % DHCP4_CONFIG_COMPLETE DHCPv4 server has completed configuration: %1
 This is an informational message announcing the successful processing of a

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -1,4 +1,3 @@
-
 // Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
@@ -205,9 +204,10 @@ public:
             ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(),
                       expected_data_len);
         }
-        // Verify that the data is correct. However do not verify suboptions.
+        // Verify that the data is correct. Do not verify suboptions and a header.
         const uint8_t* data = static_cast<const uint8_t*>(buf.getData());
-        EXPECT_TRUE(memcmp(expected_data, data, expected_data_len));
+        EXPECT_EQ(0, memcmp(expected_data, data + option_desc.option->getHeaderLen(),
+                            expected_data_len));
     }
 
     /// @brief Reset configuration database.
@@ -239,14 +239,15 @@ public:
                    << ex.what() << std::endl;
         }
 
-
+        // status object must not be NULL
-        // returned value should be 0 (configuration success)
         if (!status) {
             FAIL() << "Fatal error: unable to reset configuration database"
                    << " after the test. Configuration function returned"
                    << " NULL pointer" << std::endl;
         }
+
         comment_ = parseAnswer(rcode_, status);
+        // returned value should be 0 (configuration success)
         if (rcode_ != 0) {
             FAIL() << "Fatal error: unable to reset configuration database"
                    << " after the test. Configuration function returned"

src/bin/dhcp6/config_parser.cc

@@ -496,12 +496,12 @@ private:
 ///
 /// This parser parses configuration entries that specify value of
 /// a single option. These entries include option name, option code
-/// and data carried by the option. If parsing is successful than an
+/// and data carried by the option. If parsing is successful then an
 /// instance of an option is created and added to the storage provided
 /// by the calling class.
 ///
 /// @todo This class parses and validates the option name. However it is
-/// not used anywhere util support for option spaces is implemented
+/// not used anywhere until support for option spaces is implemented
 /// (see tickets #2319, #2314). When option spaces are implemented
 /// there will be a way to reference the particular option using
 /// its type (code) or option name.
@@ -857,26 +857,21 @@ public:
             // a setStorage and build methods are invoked.
 
             // Try uint32 type parser.
-            if (buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
+            if (!buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
-                                                          param.second)) {
+                                                           param.second) ||
-                // Storage set, build invoked on the parser, proceed with
+                // Try string type parser.
-                // next configuration element.
+                !buildParser<StringParser, StringStorage >(parser, string_values_,
-                continue;
+                                                           param.second) ||
-            }
+                // Try pool parser.
-            // Try string type parser.
+                !buildParser<PoolParser, PoolStorage >(parser, pools_,
-            if (buildParser<StringParser, StringStorage >(parser, string_values_,
+                                                       param.second) ||
-                                                          param.second)) {
+                // Try option data parser.
-                continue;
+                !buildParser<OptionDataListParser, OptionStorage >(parser, options_,
-            }
+                                                                   param.second)) {
-            // Try pools parser.
+                // Appropriate parsers are created in the createSubnet6ConfigParser
-            if (buildParser<PoolParser, PoolStorage >(parser, pools_,
+                // and they should be limited to those that we check here for. Thus,
-                                                      param.second)) {
+                // if we fail to find a matching parser here it is a programming error.
-                continue;
+                isc_throw(Dhcp6ConfigError, "failed to find suitable parser");
-            }
-            // Try option data parser.
-            if (buildParser<OptionDataListParser, OptionStorage >(parser, options_,
-                                                                  param.second)) {
-                continue;
             }
         }
         // Ok, we now have subnet parsed

src/bin/dhcp6/dhcp6_messages.mes

@@ -47,9 +47,9 @@ This is an informational message reporting that the configuration has
 been extended to include the specified subnet.
 
 % 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
+This warning message is issued on an 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.
+for DHCPv6, but it is not prohibited.
 
 % DHCP6_CONFIG_START DHCPv6 server is processing the following configuration: %1
 This is a debug message that is issued every time the server receives a

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -144,14 +144,14 @@ public:
                    << ex.what() << std::endl;
         }
 
-
+        // status object must not be NULL
-        // returned value should be 0 (configuration success)
         if (!status) {
             FAIL() << "Fatal error: unable to reset configuration database"
                    << " after the test. Configuration function returned"
                    << " NULL pointer" << std::endl;
         }
         comment_ = parseAnswer(rcode_, status);
+        // returned value should be 0 (configuration success)
         if (rcode_ != 0) {
             FAIL() << "Fatal error: unable to reset configuration database"
                    << " after the test. Configuration function returned"
@@ -215,9 +215,10 @@ public:
             ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(),
                       expected_data_len);
         }
-        // Verify that the data is correct. However do not verify suboptions.
+        // Verify that the data is correct. Do not verify suboptions and a header.
         const uint8_t* data = static_cast<const uint8_t*>(buf.getData());
-        EXPECT_TRUE(memcmp(expected_data, data, expected_data_len));
+        EXPECT_EQ(0, memcmp(expected_data, data + option_desc.option->getHeaderLen(),
+                            expected_data_len));
     }
 
     Dhcpv6Srv srv_;