diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc index c96db32550..2391dff085 100644 --- a/src/bin/dhcp4/config_parser.cc +++ b/src/bin/dhcp4/config_parser.cc @@ -13,13 +13,13 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include #include #include #include #include -#include #include -//#include +#include #include #include #include @@ -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"); } diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index ee60ab2be9..41fc9ac6ec 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/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_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) { diff --git a/src/lib/dhcpsrv/dhcp_config_parser.h b/src/lib/dhcpsrv/dhcp_config_parser.h index 7419809a01..8abdfc88a3 100644 --- a/src/lib/dhcpsrv/dhcp_config_parser.h +++ b/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 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 (value); + return (param->second); } /// @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); } diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index 6a05b5650c..5776888d03 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/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); }