[2634] Addressed review comments.

src/bin/dhcp4/config_parser.cc

@@ -13,13 +13,13 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <config/ccsession.h>
+#include <dhcp4/config_parser.h>
 #include <dhcp4/dhcp4_log.h>
 #include <dhcp/libdhcp++.h>
 #include <dhcp/option_definition.h>
 #include <dhcpsrv/cfgmgr.h>
-#include <dhcp4/config_parser.h>
 #include <dhcpsrv/dbaccess_parser.h>
-//#include <dhcpsrv/dhcp_config_parser.h>
+#include <dhcpsrv/dhcp_config_parser.h>
 #include <dhcpsrv/option_space_container.h>
 #include <util/encode/hex.h>
 #include <util/strutil.h>
@@ -1418,7 +1418,7 @@ private:
         try {
             subnet_txt = string_values_.getParam("subnet"); 
         } catch (DhcpConfigError) {
-            // rethrow with precise error
+            // Rethrow with precise error.
             isc_throw(DhcpConfigError,
                       "Mandatory subnet definition in subnet missing");
         }

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -52,11 +52,8 @@ public:
 
     // Checks if global parameter of name have expected_value
     void checkGlobalUint32(string name, uint32_t expected_value) {
-        //const Uint32Storage& uint32_defaults = getUint32Defaults();
-        //const ValueStorage<uint32_t>& uint32_defaults = getUint32Defaults();
         const Uint32Storage& uint32_defaults = getUint32Defaults();
         try {
-            //uint32_defaults.addParam("boo", name);
             uint32_t actual_value = uint32_defaults.getParam(name);
             EXPECT_EQ(expected_value, actual_value);
         } catch (DhcpConfigError) {

src/lib/dhcpsrv/dhcp_config_parser.h

@@ -133,23 +133,24 @@ public:
 /// @brief A template class that stores named elements of a given data type.
 ///
 /// This template class is provides data value storage for configuration parameters
-/// of a given data type.  The values are stored by parmater name and as instances 
+/// of a given data type.  The values are stored by parameter name and as instances 
 /// of type "ValueType". 
 ///
-/// @tparam ValueType is the data type of the elements to store.
+/// @param ValueType is the data type of the elements to store.
 template<typename ValueType>
 class ValueStorage {
     public:
         /// @brief  Stores the the parameter and its value in the store.
         ///
-        /// If the parmater does not exist in the store, then it will be added,
+        /// If the parameter does not exist in the store, then it will be added,
         /// otherwise its data value will be updated with the given value. 
         ///
         /// @param name is the name of the paramater to store.
         /// @param value is the data value to store.
-        void setParam(const std::string name, const ValueType value) {
+        void setParam(const std::string name, const ValueType& value) {
             values_[name] = value;
         }
+
         /// @brief Returns the data value for the given parameter.
         ///
         /// Finds and returns the data value for the given parameter.
@@ -163,12 +164,11 @@ class ValueStorage {
                 = values_.find(name);
 
             if (param == values_.end()) {
-                isc_throw(DhcpConfigError, "missing parameter '"
+                isc_throw(DhcpConfigError, "Missing parameter '"
                        << name << "'");
             }
 
-            ValueType value = param->second;
+            return (param->second);
-            return (value);
         }
 
         /// @brief  Remove the parameter from the store.
@@ -177,7 +177,7 @@ class ValueStorage {
         /// exists. 
         ///
         /// @param name is the name of the paramater to delete.
-        void delParam(const std::string name) {
+        void delParam(const std::string& name) {
             values_.erase(name);
         }
 

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -41,61 +41,74 @@ namespace {
 TEST(ValueStorageTest, BooleanTesting) {
     BooleanStorage testStore;
 
-    // verify that we can add and retrieve them
+    // Verify that we can add and retrieve parameters.
     testStore.setParam("firstBool", false);
     testStore.setParam("secondBool", true);
 
     EXPECT_FALSE(testStore.getParam("firstBool"));
     EXPECT_TRUE(testStore.getParam("secondBool"));
 
-    // verify that we can update them
+    // Verify that we can update paramaters. 
     testStore.setParam("firstBool", true);
     testStore.setParam("secondBool", false);
 
     EXPECT_TRUE(testStore.getParam("firstBool"));
     EXPECT_FALSE(testStore.getParam("secondBool"));
 
-    // verify that we can delete one
+    // Verify that we can delete a parameter and it will no longer be found.
     testStore.delParam("firstBool");
     ASSERT_THROW(testStore.getParam("firstBool"), isc::dhcp::DhcpConfigError);
 
-    // verify that the delete was safe
+    // Verify that the delete was safe and the store still operates.
     EXPECT_FALSE(testStore.getParam("secondBool"));
 
-    // verify that we can empty the list
+    // Verify that looking for a parameter that never existed throws.
+    ASSERT_THROW(testStore.getParam("bogusBool"), isc::dhcp::DhcpConfigError);
+
+    // Verify that attempting to delete a parameter that never existed does not throw. 
+    ASSERT_NO_THROW(testStore.delParam("bogusBool"));
+
+    // Verify that we can empty the list.
     testStore.clear();
     ASSERT_THROW(testStore.getParam("secondBool"), isc::dhcp::DhcpConfigError);
+
 }
 
 // This test verifies that Uint32Storage functions properly. 
 TEST(ValueStorageTest, Uint32Testing) {
     Uint32Storage testStore;
 
-    uint32_t intOne = -77;
+    uint32_t intOne = 77;
     uint32_t intTwo = 33;
 
-    // verify that we can add and retrieve them
+    // Verify that we can add and retrieve parameters. 
     testStore.setParam("firstInt", intOne);
     testStore.setParam("secondInt", intTwo);
 
     EXPECT_EQ(testStore.getParam("firstInt"), intOne);
     EXPECT_EQ(testStore.getParam("secondInt"), intTwo);
 
-    // verify that we can update them
+    // Verify that we can update parameters. 
     testStore.setParam("firstInt", --intOne);
     testStore.setParam("secondInt", ++intTwo);
 
     EXPECT_EQ(testStore.getParam("firstInt"), intOne);
     EXPECT_EQ(testStore.getParam("secondInt"), intTwo);
 
-    // verify that we can delete one
+    // Verify that we can delete a parameter and it will no longer be found.
     testStore.delParam("firstInt");
     ASSERT_THROW(testStore.getParam("firstInt"), isc::dhcp::DhcpConfigError);
 
-    // verify that the delete was safe
+    // Verify that the delete was safe and the store still operates.
     EXPECT_EQ(testStore.getParam("secondInt"), intTwo);
 
-    // verify that we can empty the list
+    // Verify that looking for a parameter that never existed throws.
+    ASSERT_THROW(testStore.getParam("bogusInt"), isc::dhcp::DhcpConfigError);
+
+    // Verify that attempting to delete a parameter that never existed does not throw. 
+    ASSERT_NO_THROW(testStore.delParam("bogusInt"));
+
+    // Verify that we can empty the list.
     testStore.clear();
     ASSERT_THROW(testStore.getParam("secondInt"), isc::dhcp::DhcpConfigError);
 }
@@ -107,14 +120,14 @@ TEST(ValueStorageTest, StringTesting) {
     std::string stringOne = "seventy-seven";
     std::string stringTwo = "thirty-three";
 
-    // verify that we can add and retrieve them
+    // Verify that we can add and retrieve parameters.
     testStore.setParam("firstString", stringOne);
     testStore.setParam("secondString", stringTwo);
 
     EXPECT_EQ(testStore.getParam("firstString"), stringOne);
     EXPECT_EQ(testStore.getParam("secondString"), stringTwo);
 
-    // verify that we can update them
+    // Verify that we can update parameters. 
     stringOne.append("-boo");
     stringTwo.append("-boo");
 
@@ -124,14 +137,20 @@ TEST(ValueStorageTest, StringTesting) {
     EXPECT_EQ(testStore.getParam("firstString"), stringOne);
     EXPECT_EQ(testStore.getParam("secondString"), stringTwo);
 
-    // verify that we can delete one
+    // Verify that we can delete a parameter and it will no longer be found.
     testStore.delParam("firstString");
     ASSERT_THROW(testStore.getParam("firstString"), isc::dhcp::DhcpConfigError);
 
-    // verify that the delete was safe
+    // Verify that the delete was safe and the store still operates.
     EXPECT_EQ(testStore.getParam("secondString"), stringTwo);
 
-    // verify that we can empty the list
+    // Verify that looking for a parameter that never existed throws.
+    ASSERT_THROW(testStore.getParam("bogusString"), isc::dhcp::DhcpConfigError);
+
+    // Verify that attempting to delete a parameter that never existed does not throw. 
+    ASSERT_NO_THROW(testStore.delParam("bogusString"));
+
+    // Verify that we can empty the list.
     testStore.clear();
     ASSERT_THROW(testStore.getParam("secondString"), isc::dhcp::DhcpConfigError);
 }