[2355] Minor changes in response to review comments.

src/bin/dhcp4/config_parser.cc

@@ -41,10 +41,6 @@ using namespace isc::asiolink;
 
 namespace {
 
-/// @brief Create the global parser context which stores global
-/// parameters, options, and option definitions.
-ParserContextPtr global_context_ptr(new ParserContext(Option::V4));
-
 /// @brief Parser for DHCP4 option data value.
 ///
 /// This parser parses configuration entries that specify value of
@@ -66,7 +62,7 @@ public:
 
     /// @brief static factory method for instantiating Dhcp4OptionDataParsers
     ///
-    /// @param param_name name fo the parameter to be parsed.
+    /// @param param_name name of the parameter to be parsed.
     /// @param options storage where the parameter value is to be stored.
     /// @param global_context is a pointer to the global context which 
     /// stores global scope parameters, options, option defintions.
@@ -130,10 +126,10 @@ protected:
     ///
     /// @param addr is the IPv4 prefix of the pool.
     /// @param len is the prefix length.
-    /// @param ignored dummy parameter to provide symmetry between 
+    /// @param ignored dummy parameter to provide symmetry between the 
+    /// PoolParser derivations. The V6 derivation requires a third value.
     /// @return returns a PoolPtr to the new Pool4 object. 
-    PoolPtr poolMaker (IOAddress &addr, uint32_t len, int32_t)
-    {
+    PoolPtr poolMaker (IOAddress &addr, uint32_t len, int32_t) {
         return (PoolPtr(new Pool4(addr, len)));
     }
 
@@ -144,8 +140,7 @@ protected:
     /// @param ignored dummy parameter to provide symmetry between the 
     /// PoolParser derivations. The V6 derivation requires a third value.
     /// @return returns a PoolPtr to the new Pool4 object. 
-    PoolPtr poolMaker (IOAddress &min, IOAddress &max, int32_t)
-    {
+    PoolPtr poolMaker (IOAddress &min, IOAddress &max, int32_t) {
         return (PoolPtr(new Pool4(min, max)));
     }
 };
@@ -162,15 +157,22 @@ public:
     /// @param ignored first parameter
     /// @param global_context is a pointer to the global context which 
     /// stores global scope parameters, options, option defintions.
-    Subnet4ConfigParser(const std::string&, ParserContextPtr global_context)
-        :SubnetConfigParser("", global_context) {
+    Subnet4ConfigParser(const std::string&)
+        :SubnetConfigParser("", globalContext()) {
     } 
 
     /// @brief Adds the created subnet to a server's configuration.
+    /// @throw throws Unexpected if dynamic cast fails.
     void commit() {
         if (subnet_) {
-            Subnet4Ptr bs = boost::dynamic_pointer_cast<Subnet4>(subnet_);
-            isc::dhcp::CfgMgr::instance().addSubnet4(bs);
+            Subnet4Ptr sub4ptr = boost::dynamic_pointer_cast<Subnet4>(subnet_);
+            if (!sub4ptr) {
+                // If we hit this, it is a programming error.
+                isc_throw(Unexpected, 
+                          "Invalid cast in Subnet4ConfigParser::commit");
+            }
+
+            isc::dhcp::CfgMgr::instance().addSubnet4(sub4ptr);
         }
     }
 
@@ -191,14 +193,14 @@ protected:
             (config_id.compare("rebind-timer") == 0))  {
             parser = new Uint32Parser(config_id, uint32_values_);
         } else if ((config_id.compare("subnet") == 0) || 
-                 (config_id.compare("interface") == 0)) {
+                   (config_id.compare("interface") == 0)) {
             parser = new StringParser(config_id, string_values_);
         } else if (config_id.compare("pool") == 0) {
             parser = new Pool4Parser(config_id, pools_);
         } else if (config_id.compare("option-data") == 0) {
            parser = new OptionDataListParser(config_id, options_, 
-                                            global_context_,
-                                            Dhcp4OptionDataParser::factory);
+                                             global_context_,
+                                             Dhcp4OptionDataParser::factory);
         } else {
             isc_throw(NotImplemented,
                 "parser error: Subnet4 parameter not supported: " << config_id);
@@ -277,7 +279,7 @@ public:
 
     /// @brief constructor
     ///
-    /// @param dummy first argument, always ingored. All parsers accept a
+    /// @param dummy first argument, always ignored. All parsers accept a
     /// string parameter "name" as their first argument.
     Subnets4ListConfigParser(const std::string&) {
     }
@@ -290,8 +292,7 @@ public:
     /// @param subnets_list pointer to a list of IPv4 subnets
     void build(ConstElementPtr subnets_list) {
         BOOST_FOREACH(ConstElementPtr subnet, subnets_list->listValue()) {
-            ParserPtr parser(new Subnet4ConfigParser("subnet", 
-                                                    global_context_ptr));
+            ParserPtr parser(new Subnet4ConfigParser("subnet"));
             parser->build(subnet);
             subnets_.push_back(parser);
         }
@@ -346,22 +347,22 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
         (config_id.compare("renew-timer") == 0)  ||
         (config_id.compare("rebind-timer") == 0))  {
         parser = new Uint32Parser(config_id, 
-                                 global_context_ptr->uint32_values_);
+                                 globalContext()->uint32_values_);
     } else if (config_id.compare("interface") == 0) {
         parser = new InterfaceListConfigParser(config_id);
     } else if (config_id.compare("subnet4") == 0) {
         parser = new Subnets4ListConfigParser(config_id);
     } else if (config_id.compare("option-data") == 0) {
         parser = new OptionDataListParser(config_id, 
-                                          global_context_ptr->options_, 
-                                          global_context_ptr,
+                                          globalContext()->options_, 
+                                          globalContext(),
                                           Dhcp4OptionDataParser::factory);
     } else if (config_id.compare("option-def") == 0) {
         parser  = new OptionDefListParser(config_id, 
-                                          global_context_ptr->option_defs_);
+                                          globalContext()->option_defs_);
     } else if (config_id.compare("version") == 0) {
         parser  = new StringParser(config_id, 
-                                    global_context_ptr->string_values_);
+                                    globalContext()->string_values_);
     } else if (config_id.compare("lease-database") == 0) {
         parser = new DbAccessParser(config_id); 
     } else {
@@ -405,7 +406,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     // parsing operation fails after the global storage has been
     // modified. We need to preserve the original global data here
     // so as we can rollback changes when an error occurs.
-    ParserContext original_context(*global_context_ptr);
+    ParserContext original_context(*globalContext());
 
     // Answer will hold the result.
     ConstElementPtr answer;
@@ -500,7 +501,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
 
     // Rollback changes as the configuration parsing failed.
     if (rollback) {
-        global_context_ptr.reset(new ParserContext(original_context));
+        globalContext().reset(new ParserContext(original_context));
         return (answer);
     }
 
@@ -511,11 +512,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     return (answer);
 }
 
-// Makes global context accessible for unit tests.
-const ParserContext& getGlobalParserContext() {
-    return (*global_context_ptr);
+ParserContextPtr globalContext() {
+    static ParserContextPtr global_context_ptr(new ParserContext(Option::V4));
+    return (global_context_ptr);
 }
 
+
+
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 

src/bin/dhcp4/config_parser.h

@@ -63,11 +63,8 @@ configureDhcp4Server(Dhcpv4Srv&,
 
 /// @brief Returns the global context
 ///
-/// This function must be only used by unit tests that need
-/// to access global context.
-///
-/// @returns a const reference to the global context
-const ParserContext& getGlobalParserContext();
+/// @return a const reference to the global context
+ParserContextPtr globalContext();
 
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -53,7 +53,11 @@ public:
     // Checks if global parameter of name have expected_value
     void checkGlobalUint32(string name, uint32_t expected_value) {
         const Uint32StoragePtr uint32_defaults = 
+#if 0
                                         getGlobalParserContext().uint32_values_;
+#else
+                                        globalContext()->uint32_values_;
+#endif
         try {
             uint32_t actual_value = uint32_defaults->getParam(name);
             EXPECT_EQ(expected_value, actual_value);

src/bin/dhcp6/config_parser.cc

@@ -55,9 +55,6 @@ typedef boost::shared_ptr<BooleanParser> BooleanParserPtr;
 typedef boost::shared_ptr<StringParser> StringParserPtr;
 typedef boost::shared_ptr<Uint32Parser> Uint32ParserPtr;
 
-// TKM - declare a global parser context
-ParserContextPtr global_context_ptr(new ParserContext(Option::V6));
-
 /// @brief Parser for DHCP6 option data value.
 ///
 /// This parser parses configuration entries that specify value of
@@ -182,15 +179,21 @@ public:
     /// @param ignored first parameter
     /// @param global_context is a pointer to the global context which 
     /// stores global scope parameters, options, option defintions.
-    Subnet6ConfigParser(const std::string&, ParserContextPtr global_context) 
-        :SubnetConfigParser("", global_context) {
+    Subnet6ConfigParser(const std::string&) 
+        :SubnetConfigParser("", globalContext()) {
     }
 
     /// @brief Adds the created subnet to a server's configuration.
+    /// @throw throws Unexpected if dynamic cast fails.
     void commit() {
         if (subnet_) {
-            Subnet6Ptr bs = boost::dynamic_pointer_cast<Subnet6>(subnet_);
-            isc::dhcp::CfgMgr::instance().addSubnet6(bs);
+            Subnet6Ptr sub6ptr = boost::dynamic_pointer_cast<Subnet6>(subnet_);
+            if (!sub6ptr) {
+                // If we hit this, it is a programming error.
+                isc_throw(Unexpected,
+                          "Invalid cast in Subnet4ConfigParser::commit");
+            }
+            isc::dhcp::CfgMgr::instance().addSubnet6(sub6ptr);
         }
     }
 
@@ -212,7 +215,7 @@ protected:
             (config_id.compare("rebind-timer") == 0))  {
             parser = new Uint32Parser(config_id, uint32_values_);
         } else if ((config_id.compare("subnet") == 0) ||
-                 (config_id.compare("interface") == 0)) {
+                   (config_id.compare("interface") == 0)) {
             parser = new StringParser(config_id, string_values_);
         } else if (config_id.compare("pool") == 0) {
             parser = new Pool6Parser(config_id, pools_);
@@ -303,7 +306,7 @@ public:
 
     /// @brief constructor
     ///
-    /// @param dummy first argument, always ingored. All parsers accept a
+    /// @param dummy first argument, always ignored. All parsers accept a
     /// string parameter "name" as their first argument.
     Subnets6ListConfigParser(const std::string&) {
     }
@@ -316,8 +319,7 @@ public:
     /// @param subnets_list pointer to a list of IPv6 subnets
     void build(ConstElementPtr subnets_list) {
         BOOST_FOREACH(ConstElementPtr subnet, subnets_list->listValue()) {
-            ParserPtr parser(new Subnet6ConfigParser("subnet", 
-                                                    global_context_ptr));
+            ParserPtr parser(new Subnet6ConfigParser("subnet" ));
             parser->build(subnet);
             subnets_.push_back(parser);
         }
@@ -373,22 +375,22 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id) {
         (config_id.compare("renew-timer") == 0)  ||
         (config_id.compare("rebind-timer") == 0))  {
         parser = new Uint32Parser(config_id, 
-                                 global_context_ptr->uint32_values_);
+                                 globalContext()->uint32_values_);
     } else if (config_id.compare("interface") == 0) {
         parser = new InterfaceListConfigParser(config_id);
     } else if (config_id.compare("subnet6") == 0) {
         parser = new Subnets6ListConfigParser(config_id);
     } else if (config_id.compare("option-data") == 0) {
         parser = new OptionDataListParser(config_id, 
-                                          global_context_ptr->options_, 
-                                          global_context_ptr,
+                                          globalContext()->options_, 
+                                          globalContext(),
                                           Dhcp6OptionDataParser::factory);
     } else if (config_id.compare("option-def") == 0) {
         parser  = new OptionDefListParser(config_id, 
-                                          global_context_ptr->option_defs_);
+                                          globalContext()->option_defs_);
     } else if (config_id.compare("version") == 0) {
         parser  = new StringParser(config_id, 
-                                   global_context_ptr->string_values_);
+                                   globalContext()->string_values_);
     } else if (config_id.compare("lease-database") == 0) {
         parser = new DbAccessParser(config_id);
     } else {
@@ -432,7 +434,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
     // parsing operation fails after the global storage has been
     // modified. We need to preserve the original global data here
     // so as we can rollback changes when an error occurs.
-    ParserContext original_context(*global_context_ptr);
+    ParserContext original_context(*globalContext());
 
     // answer will hold the result.
     ConstElementPtr answer;
@@ -528,7 +530,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
 
     // Rollback changes as the configuration parsing failed.
     if (rollback) {
-        global_context_ptr.reset(new ParserContext(original_context));
+        globalContext().reset(new ParserContext(original_context));
         return (answer);
     }
 
@@ -539,9 +541,9 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
     return (answer);
 }
 
-// Makes global context accessible for unit tests.
-const ParserContext& getGlobalParserContext() {
-    return (*global_context_ptr);
+ParserContextPtr globalContext() {
+    static ParserContextPtr global_context_ptr(new ParserContext(Option::V6));
+    return (global_context_ptr);
 }
 
 }; // end of isc::dhcp namespace

src/bin/dhcp6/config_parser.h

@@ -51,11 +51,8 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set);
 
 /// @brief Returns the global context
 ///
-/// This function must be only used by unit tests that need
-/// to access global context.
-///
 /// @returns a const reference to the global context
-const ParserContext& getGlobalParserContext();
+ParserContextPtr globalContext();
  
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

src/lib/dhcpsrv/dhcp_config_parser.h

@@ -130,82 +130,6 @@ public:
     virtual void commit() = 0;
 };
 
-/// @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 parameter name and as instances 
-/// of type "ValueType". 
-///
-/// @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 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) {
-            values_[name] = value;
-        }
-
-        /// @brief Returns the data value for the given parameter.
-        ///
-        /// Finds and returns the data value for the given parameter.
-        /// @param name is the name of the parameter for which the data
-        /// value is desired.
-        ///
-        /// @return The paramater's data value of type <ValueType>.
-        /// @throw DhcpConfigError if the parameter is not found.
-        ValueType getParam(const std::string& name) const {
-            typename std::map<std::string, ValueType>::const_iterator param 
-                = values_.find(name);
-
-            if (param == values_.end()) {
-                isc_throw(DhcpConfigError, "Missing parameter '"
-                       << name << "'");
-            }
-
-            return (param->second);
-        }
-
-        /// @brief  Remove the parameter from the store.
-        ///
-        /// Deletes the entry for the given parameter from the store if it 
-        /// exists. 
-        ///
-        /// @param name is the name of the paramater to delete.
-        void delParam(const std::string& name) {
-            values_.erase(name);
-        }
-
-        /// @brief Deletes all of the entries from the store.
-        ///
-        void clear() {
-            values_.clear();
-        }
-
-
-    private:
-        /// @brief An std::map of the data values, keyed by parameter names.
-        std::map<std::string, ValueType> values_;
-};
-
-
-/// @brief a collection of elements that store uint32 values (e.g. renew-timer = 900)
-typedef ValueStorage<uint32_t> Uint32Storage;
-typedef boost::shared_ptr<Uint32Storage> Uint32StoragePtr;
-
-/// @brief a collection of elements that store string values
-typedef ValueStorage<std::string> StringStorage;
-typedef boost::shared_ptr<StringStorage> StringStoragePtr;
-
-/// @brief Storage for parsed boolean values.
-typedef ValueStorage<bool> BooleanStorage;
-typedef boost::shared_ptr<BooleanStorage> BooleanStoragePtr;
-
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -44,7 +44,7 @@ ParserContext::ParserContext(Option::Universe universe):
         universe_(universe) {
     }
 
-ParserContext::ParserContext(ParserContext& rhs):
+ParserContext::ParserContext(const ParserContext& rhs):
         boolean_values_(new BooleanStorage(*(rhs.boolean_values_))),
         uint32_values_(new Uint32Storage(*(rhs.uint32_values_))),
         string_values_(new StringStorage(*(rhs.string_values_))),
@@ -53,19 +53,38 @@ ParserContext::ParserContext(ParserContext& rhs):
         universe_(rhs.universe_) {
     }
 
+ParserContext& 
+ParserContext::operator=(const ParserContext& rhs) {
+        ParserContext tmp(rhs);
+        boolean_values_ = 
+            BooleanStoragePtr(new BooleanStorage(*(rhs.boolean_values_)));
+        uint32_values_ = 
+            Uint32StoragePtr(new Uint32Storage(*(tmp.uint32_values_)));
+        string_values_ = 
+            StringStoragePtr(new StringStorage(*(tmp.string_values_)));
+        options_ = OptionStoragePtr(new OptionStorage(*(tmp.options_)));
+        option_defs_ = 
+            OptionDefStoragePtr(new OptionDefStorage(*(tmp.option_defs_)));
+        universe_ = rhs.universe_;
+        return (*this);
+    }
+
+
 // **************************** DebugParser *************************
 
 DebugParser::DebugParser(const std::string& param_name)
     :param_name_(param_name) {
 }
 
-void DebugParser::build(ConstElementPtr new_config) {
+void 
+DebugParser::build(ConstElementPtr new_config) {
     std::cout << "Build for token: [" << param_name_ << "] = ["
         << value_->str() << "]" << std::endl; 
     value_ = new_config;
 }
 
-void DebugParser::commit() {
+void 
+DebugParser::commit() {
     // Debug message. The whole DebugParser class is used only for parser
     // debugging, and is not used in production code. It is very convenient
     // to keep it around. Please do not turn this cout into logger calls.
@@ -128,13 +147,15 @@ InterfaceListConfigParser::InterfaceListConfigParser(const std::string&
     }
 }
 
-void InterfaceListConfigParser::build(ConstElementPtr value) {
+void 
+InterfaceListConfigParser::build(ConstElementPtr value) {
     BOOST_FOREACH(ConstElementPtr iface, value->listValue()) {
         interfaces_.push_back(iface->str());
     }
 }
 
-void InterfaceListConfigParser::commit() {
+void 
+InterfaceListConfigParser::commit() {
     /// @todo: Implement per interface listening. Currently always listening
     /// on all interfaces.
 }
@@ -157,7 +178,8 @@ OptionDataParser::OptionDataParser(const std::string&, OptionStoragePtr options,
     }
 }
 
-void OptionDataParser::build(ConstElementPtr option_data_entries) {
+void 
+OptionDataParser::build(ConstElementPtr option_data_entries) {
     BOOST_FOREACH(ConfigPair param, option_data_entries->mapValue()) {
         ParserPtr parser;
         if (param.first == "name" || param.first == "data" ||
@@ -193,7 +215,8 @@ void OptionDataParser::build(ConstElementPtr option_data_entries) {
     createOption();
 }
 
-void OptionDataParser::commit() {
+void 
+OptionDataParser::commit() {
     if (!option_descriptor_.option) {
         // Before we can commit the new option should be configured. If it is 
         // not than somebody must have called commit() before build().
@@ -221,7 +244,8 @@ void OptionDataParser::commit() {
     options_->addItem(option_descriptor_, option_space_);
 }
 
-void OptionDataParser::createOption() {
+void 
+OptionDataParser::createOption() {
     // Option code is held in the uint32_t storage but is supposed to
     // be uint16_t value. We need to check that value in the configuration
     // does not exceed range of uint8_t and is not zero.
@@ -389,7 +413,8 @@ OptionDataListParser::OptionDataListParser(const std::string&,
     }
 }
 
-void OptionDataListParser::build(ConstElementPtr option_data_list) {
+void 
+OptionDataListParser::build(ConstElementPtr option_data_list) {
     BOOST_FOREACH(ConstElementPtr option_value, option_data_list->listValue()) {
         boost::shared_ptr<OptionDataParser> 
             parser((*optionDataParserFactory_)("option-data", 
@@ -404,7 +429,8 @@ void OptionDataListParser::build(ConstElementPtr option_data_list) {
     }
 }
 
-void OptionDataListParser::commit() {
+void 
+OptionDataListParser::commit() {
     BOOST_FOREACH(ParserPtr parser, parsers_) {
         parser->commit();
     }
@@ -426,7 +452,8 @@ OptionDefParser::OptionDefParser(const std::string&,
     }
 }
 
-void OptionDefParser::build(ConstElementPtr option_def) {
+void 
+OptionDefParser::build(ConstElementPtr option_def) {
     // Parse the elements that make up the option definition.
      BOOST_FOREACH(ConfigPair param, option_def->mapValue()) {
         std::string entry(param.first);
@@ -473,14 +500,16 @@ void OptionDefParser::build(ConstElementPtr option_def) {
     }
 }
 
-void OptionDefParser::commit() {
+void 
+OptionDefParser::commit() {
     if (storage_ && option_definition_ &&
         OptionSpace::validateName(option_space_name_)) {
             storage_->addItem(option_definition_, option_space_name_);
     }
 }
 
-void OptionDefParser::createOptionDef() {
+void 
+OptionDefParser::createOptionDef() {
     // Get the option space name and validate it.
     std::string space = string_values_->getParam("space");
     if (!OptionSpace::validateName(space)) {
@@ -567,7 +596,8 @@ OptionDefListParser::OptionDefListParser(const std::string&,
     }
 }
 
-void OptionDefListParser::build(ConstElementPtr option_def_list) {
+void 
+OptionDefListParser::build(ConstElementPtr option_def_list) {
     // Clear existing items in the storage.
     // We are going to replace all of them.
     storage_->clearItems();
@@ -585,7 +615,8 @@ void OptionDefListParser::build(ConstElementPtr option_def_list) {
     }
 }
 
-void OptionDefListParser::commit() {
+void 
+OptionDefListParser::commit() {
     CfgMgr& cfg_mgr = CfgMgr::instance();
     cfg_mgr.deleteOptionDefs();
 
@@ -616,7 +647,8 @@ PoolParser::PoolParser(const std::string&,  PoolStoragePtr pools)
     }
 }
 
-void PoolParser::build(ConstElementPtr pools_list) {
+void 
+PoolParser::build(ConstElementPtr pools_list) {
     BOOST_FOREACH(ConstElementPtr text_pool, pools_list->listValue()) {
         // That should be a single pool representation. It should contain
         // text is form prefix/len or first - last. Note that spaces
@@ -673,9 +705,10 @@ void PoolParser::build(ConstElementPtr pools_list) {
                   << text_pool->stringValue() <<
                   ". Does not contain - (for min-max) nor / (prefix/len)");
         }
-    }
+}
 
-void PoolParser::commit() {
+void 
+PoolParser::commit() {
     if (pools_) {
         // local_pools_ holds the values produced by the build function.
         // At this point parsing should have completed successfuly so
@@ -699,7 +732,8 @@ SubnetConfigParser::SubnetConfigParser(const std::string&,
     }
 }
 
-void SubnetConfigParser::build(ConstElementPtr subnet) {
+void 
+SubnetConfigParser::build(ConstElementPtr subnet) {
     BOOST_FOREACH(ConfigPair param, subnet->mapValue()) {
         ParserPtr parser(createSubnetConfigParser(param.first));
         parser->build(param.second);
@@ -722,8 +756,9 @@ void SubnetConfigParser::build(ConstElementPtr subnet) {
     createSubnet();
 }
 
-void SubnetConfigParser::appendSubOptions(const std::string& option_space, 
-                                         OptionPtr& option) {
+void 
+SubnetConfigParser::appendSubOptions(const std::string& option_space, 
+                                     OptionPtr& option) {
     // Only non-NULL options are stored in option container.
     // If this option pointer is NULL this is a serious error.
     assert(option);
@@ -776,7 +811,8 @@ void SubnetConfigParser::appendSubOptions(const std::string& option_space,
     }
 }
 
-void SubnetConfigParser::createSubnet() {
+void 
+SubnetConfigParser::createSubnet() {
     std::string subnet_txt;
     try {
         subnet_txt = string_values_->getParam("subnet");
@@ -896,8 +932,8 @@ void SubnetConfigParser::createSubnet() {
     }
 }
 
-isc::dhcp::Triplet<uint32_t> SubnetConfigParser::getParam(const 
-                                                          std::string& name) {
+isc::dhcp::Triplet<uint32_t> 
+SubnetConfigParser::getParam(const std::string& name) {
     uint32_t value = 0;
     try {
         // look for local value 

src/lib/dhcpsrv/dhcp_parsers.h

@@ -34,19 +34,94 @@ namespace dhcp {
 typedef OptionSpaceContainer<OptionDefContainer,
                              OptionDefinitionPtr> OptionDefStorage;
 
-/// @brief Shared pointer to  option definitions storage.
+/// @brief Shared pointer to option definitions storage.
 typedef boost::shared_ptr<OptionDefStorage> OptionDefStoragePtr;
 
 /// Collection of containers holding option spaces. Each container within
 /// a particular option space holds so-called option descriptors.
 typedef OptionSpaceContainer<Subnet::OptionContainer,
                              Subnet::OptionDescriptor> OptionStorage;
-/// @brief Shared pointer to  option storage.
+/// @brief Shared pointer to option storage.
 typedef boost::shared_ptr<OptionStorage> OptionStoragePtr;
 
+/// @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 parameter name and as instances 
+/// of type "ValueType". 
+///
+/// @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 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) {
+            values_[name] = value;
+        }
+
+        /// @brief Returns the data value for the given parameter.
+        ///
+        /// Finds and returns the data value for the given parameter.
+        /// @param name is the name of the parameter for which the data
+        /// value is desired.
+        ///
+        /// @return The paramater's data value of type <ValueType>.
+        /// @throw DhcpConfigError if the parameter is not found.
+        ValueType getParam(const std::string& name) const {
+            typename std::map<std::string, ValueType>::const_iterator param 
+                = values_.find(name);
+
+            if (param == values_.end()) {
+                isc_throw(DhcpConfigError, "Missing parameter '"
+                       << name << "'");
+            }
+
+            return (param->second);
+        }
+
+        /// @brief  Remove the parameter from the store.
+        ///
+        /// Deletes the entry for the given parameter from the store if it 
+        /// exists. 
+        ///
+        /// @param name is the name of the paramater to delete.
+        void delParam(const std::string& name) {
+            values_.erase(name);
+        }
+
+        /// @brief Deletes all of the entries from the store.
+        ///
+        void clear() {
+            values_.clear();
+        }
+
+
+    private:
+        /// @brief An std::map of the data values, keyed by parameter names.
+        std::map<std::string, ValueType> values_;
+};
+
+
+/// @brief a collection of elements that store uint32 values 
+typedef ValueStorage<uint32_t> Uint32Storage;
+typedef boost::shared_ptr<Uint32Storage> Uint32StoragePtr;
+
+/// @brief a collection of elements that store string values
+typedef ValueStorage<std::string> StringStorage;
+typedef boost::shared_ptr<StringStorage> StringStoragePtr;
+
+/// @brief Storage for parsed boolean values.
+typedef ValueStorage<bool> BooleanStorage;
+typedef boost::shared_ptr<BooleanStorage> BooleanStoragePtr;
 
 /// @brief Container for the current parsing context. It provides a
-/// single enclosure for the storage of configuration paramaters,
+/// single enclosure for the storage of configuration parameters,
 /// options, option definitions, and other context specific information
 /// that needs to be accessible throughout the parsing and parsing
 /// constructs.
@@ -59,7 +134,7 @@ public:
     ParserContext(Option::Universe universe);
 
     /// @brief Copy constructor
-    ParserContext(ParserContext& rhs);
+    ParserContext(const ParserContext& rhs);
 
     /// @brief Storage for boolean parameters.
     BooleanStoragePtr boolean_values_;
@@ -78,16 +153,23 @@ public:
 
     /// @brief The parsing universe of this context.
     Option::Universe universe_;
+
+    /// @brief Assignment operator
+    ParserContext& operator=(const ParserContext& rhs);
 };
 
 /// @brief Pointer to various parser context.
 typedef boost::shared_ptr<ParserContext> ParserContextPtr;
 
-/// @brief - TKM  - simple type parser template class 
+/// @brief Simple data-type parser template class 
 ///
-/// This parser handles configuration values of the boolean type.
-/// Parsed values are stored in a provided storage. If no storage
-/// is provided then the build function throws an exception.
+/// This is the template class for simple data-type parsers. It supports
+/// parsing a configuration parameter with specific data-type for its 
+/// possible values. It provides a common constructor, commit, and templated 
+/// data storage.  The "build" method implementation must be provided by a 
+/// declaring type.
+/// @param ValueType is the data type of the configuration paramater value
+/// the parser should handle.
 template<typename ValueType>
 class ValueParser : public DhcpConfigParser {
 public:
@@ -117,11 +199,12 @@ public:
     }
 
 
-    /// @brief Parse a boolean value.
+    /// @brief Parse a given element into a value of type <ValueType>
     ///
     /// @param value a value to be parsed.
     ///
-    /// @throw isc::BadValue if value is not a boolean type Element.
+    /// @throw isc::BadValue Typically the implementing type will throw
+    /// a BadValue exception when given an invalid Element to parse. 
     void build(isc::data::ConstElementPtr value);
 
     /// @brief Put a parsed value to the storage.
@@ -132,7 +215,7 @@ public:
     }
     
 private:
-    /// Pointer to the storage where parsed value is stored.
+    /// Pointer to the storage where committed value is stored.
     boost::shared_ptr<ValueStorage<ValueType> > storage_;
 
     /// Name of the parameter which value is parsed with this parser.
@@ -499,9 +582,6 @@ typedef boost::shared_ptr<PoolStorage> PoolStoragePtr;
 /// of two syntaxes: min-max and prefix/len. Pool objects are created
 /// and stored in chosen PoolStorage container.
 ///
-/// As there are no default values for pool, setStorage() must be called
-/// before build(). Otherwise exception will be thrown.
-///
 /// It is useful for parsing Dhcp<4/6>/subnet<4/6>[X]/pool parameters.
 class PoolParser : public DhcpConfigParser {
 public:

src/lib/dhcpsrv/subnet.cc

@@ -33,11 +33,13 @@ Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len,
      last_allocated_(lastAddrInPrefix(prefix, len)) {
     if ((prefix.isV6() && len > 128) ||
         (prefix.isV4() && len > 32)) {
-        isc_throw(BadValue, "Invalid prefix length specified for subnet: " << len);
+        isc_throw(BadValue, 
+                  "Invalid prefix length specified for subnet: " << len);
     }
 }
 
-bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const {
+bool 
+Subnet::inRange(const isc::asiolink::IOAddress& addr) const {
     IOAddress first = firstAddrInPrefix(prefix_, prefix_len_);
     IOAddress last = lastAddrInPrefix(prefix_, prefix_len_);
 
@@ -84,7 +86,8 @@ Subnet::getOptionDescriptor(const std::string& option_space,
     return (*range.first);
 }
 
-std::string Subnet::toText() const {
+std::string 
+Subnet::toText() const {
     std::stringstream tmp;
     tmp << prefix_.toText() << "/" << static_cast<unsigned int>(prefix_len_);
     return (tmp.str());
@@ -101,12 +104,14 @@ Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length,
     }
 }
 
-void Subnet::addPool(const PoolPtr& pool) {
+void 
+Subnet::addPool(const PoolPtr& pool) {
     IOAddress first_addr = pool->getFirstAddress();
     IOAddress last_addr = pool->getLastAddress();
 
     if (!inRange(first_addr) || !inRange(last_addr)) {
-        isc_throw(BadValue, "Pool (" << first_addr.toText() << "-" << last_addr.toText()
+        isc_throw(BadValue, "Pool (" << first_addr.toText() << "-" 
+                  << last_addr.toText()
                   << " does not belong in this (" << prefix_.toText() << "/"
                   << static_cast<int>(prefix_len_) << ") subnet4");
     }
@@ -119,15 +124,16 @@ void Subnet::addPool(const PoolPtr& pool) {
 PoolPtr Subnet::getPool(isc::asiolink::IOAddress hint) {
 
     PoolPtr candidate;
-    for (PoolCollection::iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
+    for (PoolCollection::iterator pool = pools_.begin(); 
+         pool != pools_.end(); ++pool) {
 
-        // if we won't find anything better, then let's just use the first pool
+        // If we won't find anything better, then let's just use the first pool
         if (!candidate) {
             candidate = *pool;
         }
 
-        // if the client provided a pool and there's a pool that hint is valid in,
-        // then let's use that pool
+        // If the client provided a pool and there's a pool that hint is valid 
+        // in, then let's use that pool
         if ((*pool)->inRange(hint)) {
             return (*pool);
         }
@@ -135,11 +141,13 @@ PoolPtr Subnet::getPool(isc::asiolink::IOAddress hint) {
     return (candidate);
 }
 
-void Subnet::setIface(const std::string& iface_name) {
+void 
+Subnet::setIface(const std::string& iface_name) {
     iface_ = iface_name;
 }
 
-std::string Subnet::getIface() const {
+std::string 
+Subnet::getIface() const {
     return (iface_);
 }
 
@@ -148,25 +156,29 @@ std::string Subnet::getIface() const {
 void
 Subnet4::validateOption(const OptionPtr& option) const {
     if (!option) {
-        isc_throw(isc::BadValue, "option configured for subnet must not be NULL");
+        isc_throw(isc::BadValue, 
+                  "option configured for subnet must not be NULL");
     } else if (option->getUniverse() != Option::V4) {
-        isc_throw(isc::BadValue, "expected V4 option to be added to the subnet");
+        isc_throw(isc::BadValue, 
+                  "expected V4 option to be added to the subnet");
     }
 }
 
-bool Subnet::inPool(const isc::asiolink::IOAddress& addr) const {
+bool 
+Subnet::inPool(const isc::asiolink::IOAddress& addr) const {
 
     // Let's start with checking if it even belongs to that subnet.
     if (!inRange(addr)) {
         return (false);
     }
 
-    for (PoolCollection::const_iterator pool = pools_.begin(); pool != pools_.end(); ++pool) {
+    for (PoolCollection::const_iterator pool = pools_.begin(); 
+         pool != pools_.end(); ++pool) {
         if ((*pool)->inRange(addr)) {
             return (true);
         }
     }
-    // there's no pool that address belongs to
+    // There's no pool that address belongs to
     return (false);
 }
 
@@ -186,21 +198,13 @@ Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length,
 void
 Subnet6::validateOption(const OptionPtr& option) const {
     if (!option) {
-        isc_throw(isc::BadValue, "option configured for subnet must not be NULL");
+        isc_throw(isc::BadValue, 
+                  "option configured for subnet must not be NULL");
     } else if (option->getUniverse() != Option::V6) {
-        isc_throw(isc::BadValue, "expected V6 option to be added to the subnet");
+        isc_throw(isc::BadValue, 
+                  "expected V6 option to be added to the subnet");
     }
 }
 
-#if 0
-void Subnet6::setIface(const std::string& iface_name) {
-    iface_ = iface_name;
-}
-
-std::string Subnet6::getIface() const {
-    return (iface_);
-}
-#endif
-
 } // end of isc::dhcp namespace
 } // end of isc namespace

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -15,7 +15,7 @@
 #include <config.h>
 
 #include <dhcpsrv/cfgmgr.h>
-#include <dhcpsrv/dhcp_config_parser.h>
+#include <dhcpsrv/dhcp_parsers.h>
 #include <exceptions/exceptions.h>
 
 #include <gtest/gtest.h>

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -76,7 +76,7 @@ TEST_F(DhcpParserTest, booleanParserTest) {
     ASSERT_NO_THROW(parser.build(element));
 
     // Verify that commit updates storage.
-    bool actual_value = ~test_value;
+    bool actual_value = !test_value;
     parser.commit();
     EXPECT_NO_THROW((actual_value = storage->getParam(name)));
     EXPECT_EQ(test_value, actual_value);
@@ -116,6 +116,12 @@ TEST_F(DhcpParserTest, stringParserTest) {
     ElementPtr element = Element::create(9999);
     EXPECT_NO_THROW(parser.build(element));
 
+    // Verify that commit updates storage.
+    parser.commit();
+    std::string actual_value;
+    EXPECT_NO_THROW((actual_value = storage->getParam(name)));
+    EXPECT_EQ("9999", actual_value);
+
     // Verify that parser will build with a string value.
     const std::string test_value = "test value";
     element = Element::create(test_value);
@@ -123,7 +129,6 @@ TEST_F(DhcpParserTest, stringParserTest) {
 
     // Verify that commit updates storage.
     parser.commit();
-    std::string actual_value;
     EXPECT_NO_THROW((actual_value = storage->getParam(name)));
     EXPECT_EQ(test_value, actual_value);
 }
@@ -168,6 +173,12 @@ TEST_F(DhcpParserTest, uint32ParserTest) {
     int_element->setValue((long)test_value);
     ASSERT_NO_THROW(parser.build(int_element));
 
+    // Verify that commit updates storage.
+    parser.commit();
+    uint32_t actual_value = 0;
+    EXPECT_NO_THROW((actual_value = storage->getParam(name)));
+    EXPECT_EQ(test_value, actual_value);
+
     // Verify that parser will build with a valid positive value.
     test_value = 77;
     int_element->setValue((long)test_value);
@@ -175,7 +186,6 @@ TEST_F(DhcpParserTest, uint32ParserTest) {
 
     // Verify that commit updates storage.
     parser.commit();
-    uint32_t actual_value = 0;
     EXPECT_NO_THROW((actual_value = storage->getParam(name)));
     EXPECT_EQ(test_value, actual_value);
 }