[master] Merge branch 'trac2545'

Conflicts:
	src/lib/dhcpsrv/Makefile.am

src/bin/dhcp4/config_parser.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -18,7 +18,9 @@
 #include <dhcp/libdhcp++.h>
 #include <dhcp/option_definition.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/dhcp_config_parser.h>
 #include <util/encode/hex.h>
+#include <util/strutil.h>
 #include <boost/foreach.hpp>
 #include <boost/lexical_cast.hpp>
 #include <boost/algorithm/string.hpp>
@@ -28,21 +30,31 @@
 #include <map>
 
 using namespace std;
+using namespace isc;
+using namespace isc::dhcp;
 using namespace isc::data;
 using namespace isc::asiolink;
 
-namespace isc {
+namespace {
-namespace dhcp {
 
 /// @brief auxiliary type used for storing element name and its parser
 typedef pair<string, ConstElementPtr> ConfigPair;
 
 /// @brief a factory method that will create a parser for a given element name
-typedef Dhcp4ConfigParser* ParserFactory(const std::string& config_id);
+typedef isc::dhcp::DhcpConfigParser* ParserFactory(const std::string& config_id);
 
 /// @brief a collection of factories that creates parsers for specified element names
 typedef std::map<std::string, ParserFactory*> FactoryMap;
 
+/// @brief a collection of elements that store uint32 values (e.g. renew-timer = 900)
+typedef std::map<std::string, uint32_t> Uint32Storage;
+
+/// @brief a collection of elements that store string values
+typedef std::map<std::string, std::string> StringStorage;
+
+/// @brief Storage for parsed boolean values.
+typedef std::map<string, bool> BooleanStorage;
+
 /// @brief a collection of pools
 ///
 /// That type is used as intermediate storage, when pools are parsed, but there is
@@ -69,12 +81,12 @@ OptionStorage option_defaults;
 /// will accept any configuration and will just print it out
 /// on commit. Useful for debugging existing configurations and
 /// adding new ones.
-class DebugParser : public Dhcp4ConfigParser {
+class DebugParser : public DhcpConfigParser {
 public:
 
     /// @brief Constructor
     ///
-    /// See \ref Dhcp4ConfigParser class for details.
+    /// See @ref DhcpConfigParser class for details.
     ///
     /// @param param_name name of the parsed parameter
     DebugParser(const std::string& param_name)
@@ -83,7 +95,7 @@ public:
 
     /// @brief builds parameter value
     ///
-    /// See \ref Dhcp4ConfigParser class for details.
+    /// See @ref DhcpConfigParser class for details.
     ///
     /// @param new_config pointer to the new configuration
     virtual void build(ConstElementPtr new_config) {
@@ -97,7 +109,7 @@ public:
     /// This is a method required by base class. It pretends to apply the
     /// configuration, but in fact it only prints the parameter out.
     ///
-    /// See \ref Dhcp4ConfigParser class for details.
+    /// See @ref DhcpConfigParser class for details.
     virtual void commit() {
         // Debug message. The whole DebugParser class is used only for parser
         // debugging, and is not used in production code. It is very convenient
@@ -109,7 +121,7 @@ public:
     /// @brief factory that constructs DebugParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* Factory(const std::string& param_name) {
         return (new DebugParser(param_name));
     }
 
@@ -121,6 +133,85 @@ private:
     ConstElementPtr value_;
 };
 
+/// @brief A boolean value parser.
+///
+/// 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.
+class BooleanParser : public DhcpConfigParser {
+public:
+    /// @brief Constructor.
+    ///
+    /// @param param_name name of the parameter.
+    BooleanParser(const std::string& param_name)
+        : storage_(NULL),
+          param_name_(param_name),
+          value_(false) {
+        // Empty parameter name is invalid.
+        if (param_name_.empty()) {
+            isc_throw(isc::dhcp::Dhcp4ConfigError, "parser logic error:"
+                      << "empty parameter name provided");
+        }
+    }
+
+    /// @brief Parse a boolean value.
+    ///
+    /// @param value a value to be parsed.
+    ///
+    /// @throw isc::InvalidOperation if a storage has not been set
+    ///        prior to calling this function
+    /// @throw isc::dhcp::Dhcp4ConfigError if a provided parameter's
+    ///        name is empty.
+    virtual void build(ConstElementPtr value) {
+        if (storage_ == NULL) {
+            isc_throw(isc::InvalidOperation, "parser logic error:"
+                      << " storage for the " << param_name_
+                      << " value has not been set");
+        } else if (param_name_.empty()) {
+            isc_throw(isc::dhcp::Dhcp4ConfigError, "parser logic error:"
+                      << "empty parameter name provided");
+        }
+        // The Config Manager checks if user specified a
+        // valid value for a boolean parameter: True or False.
+        // It is then ok to assume that if str() does not return
+        // 'true' the value is 'false'.
+        value_ = (value->str() == "true") ? true : false;
+    }
+
+    /// @brief Put a parsed value to the storage.
+    virtual void commit() {
+        if (storage_ != NULL && !param_name_.empty()) {
+            (*storage_)[param_name_] = value_;
+        }
+    }
+
+    /// @brief Create an instance of the boolean parser.
+    ///
+    /// @param param_name name of the parameter for which the
+    ///        parser is created.
+    static DhcpConfigParser* factory(const std::string& param_name) {
+        return (new BooleanParser(param_name));
+    }
+
+    /// @brief Set the storage for parsed value.
+    ///
+    /// This function must be called prior to calling build.
+    ///
+    /// @param storage a pointer to the storage where parsed data
+    ///        is to be stored.
+    void setStorage(BooleanStorage* storage) {
+        storage_ = storage;
+    }
+
+private:
+    /// Pointer to the storage where parsed value is stored.
+    BooleanStorage* storage_;
+    /// Name of the parameter which value is parsed with this parser.
+    std::string param_name_;
+    /// Parsed value.
+    bool value_;
+};
+
 /// @brief Configuration parser for uint32 parameters
 ///
 /// This class is a generic parser that is able to handle any uint32 integer
@@ -128,27 +219,36 @@ private:
 /// (uint32_defaults). If used in smaller scopes (e.g. to parse parameters
 /// in subnet config), it can be pointed to a different storage, using
 /// setStorage() method. This class follows the parser interface, laid out
-/// in its base class, \ref Dhcp4ConfigParser.
+/// in its base class, @ref DhcpConfigParser.
 ///
 /// For overview of usability of this generic purpose parser, see
-/// \ref dhcpv4ConfigInherit page.
+/// @ref dhcpv4ConfigInherit page.
-class Uint32Parser : public Dhcp4ConfigParser {
+class Uint32Parser : public DhcpConfigParser {
 public:
 
     /// @brief constructor for Uint32Parser
     /// @param param_name name of the configuration parameter being parsed
     Uint32Parser(const std::string& param_name)
-        :storage_(&uint32_defaults), param_name_(param_name) {
+        : storage_(&uint32_defaults),
+          param_name_(param_name) {
+        // Empty parameter name is invalid.
+        if (param_name_.empty()) {
+            isc_throw(Dhcp4ConfigError, "parser logic error:"
+                      << "empty parameter name provided");
+        }
     }
 
-    /// @brief builds parameter value
+    /// @brief Parses configuration configuration parameter as uint32_t.
-    ///
-    /// Parses configuration entry and stores it in a storage. See
-    /// \ref setStorage() for details.
     ///
     /// @param value pointer to the content of parsed values
     /// @throw BadValue if supplied value could not be base to uint32_t
+    ///        or the parameter name is empty.
     virtual void build(ConstElementPtr value) {
+        if (param_name_.empty()) {
+            isc_throw(Dhcp4ConfigError, "parser logic error:"
+                      << "empty parameter name provided");
+        }
+
         int64_t check;
         string x = value->str();
         try {
@@ -168,31 +268,27 @@ public:
 
         // value is small enough to fit
         value_ = static_cast<uint32_t>(check);
-
-        (*storage_)[param_name_] = value_;
     }
 
-    /// @brief does nothing
+    /// @brief Stores the parsed uint32_t value in a storage.
-    ///
-    /// This method is required for all parsers. The value itself
-    /// is not commited anywhere. Higher level parsers are expected to
-    /// use values stored in the storage, e.g. renew-timer for a given
-    /// subnet is stored in subnet-specific storage. It is not commited
-    /// here, but is rather used by \ref Subnet4ConfigParser when constructing
-    /// the subnet.
     virtual void commit() {
+        if (storage_ != NULL && !param_name_.empty()) {
+            // If a given parameter already exists in the storage we override
+            // its value. If it doesn't we insert a new element.
+            (*storage_)[param_name_] = value_;
+        }
     }
 
     /// @brief factory that constructs Uint32Parser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* factory(const std::string& param_name) {
         return (new Uint32Parser(param_name));
     }
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv4ConfigInherit for details.
+    /// See @ref dhcpv4ConfigInherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(Uint32Storage* storage) {
@@ -217,47 +313,48 @@ private:
 /// (string_defaults). If used in smaller scopes (e.g. to parse parameters
 /// in subnet config), it can be pointed to a different storage, using
 /// setStorage() method. This class follows the parser interface, laid out
-/// in its base class, \ref Dhcp4ConfigParser.
+/// in its base class, @ref DhcpConfigParser.
 ///
 /// For overview of usability of this generic purpose parser, see
-/// \ref dhcpv4ConfigInherit page.
+/// @ref dhcpv4ConfigInherit page.
-class StringParser : public Dhcp4ConfigParser {
+class StringParser : public DhcpConfigParser {
 public:
 
     /// @brief constructor for StringParser
     /// @param param_name name of the configuration parameter being parsed
     StringParser(const std::string& param_name)
         :storage_(&string_defaults), param_name_(param_name) {
+        // Empty parameter name is invalid.
+        if (param_name_.empty()) {
+            isc_throw(Dhcp4ConfigError, "parser logic error:"
+                      << "empty parameter name provided");
+        }
     }
 
     /// @brief parses parameter value
     ///
     /// Parses configuration entry and stores it in storage. See
-    /// \ref setStorage() for details.
+    /// @ref setStorage() for details.
     ///
     /// @param value pointer to the content of parsed values
     virtual void build(ConstElementPtr value) {
         value_ = value->str();
         boost::erase_all(value_, "\"");
-
-        (*storage_)[param_name_] = value_;
     }
 
-    /// @brief does nothing
+    /// @brief Stores the parsed value in a storage.
-    ///
-    /// This method is required for all parser. The value itself
-    /// is not commited anywhere. Higher level parsers are expected to
-    /// use values stored in the storage, e.g. renew-timer for a given
-    /// subnet is stored in subnet-specific storage. It is not commited
-    /// here, but is rather used by its parent parser when constructing
-    /// an object, e.g. the subnet.
     virtual void commit() {
+        if (storage_ != NULL && !param_name_.empty()) {
+            // If a given parameter already exists in the storage we override
+            // its value. If it doesn't we insert a new element.
+            (*storage_)[param_name_] = value_;
+        }
     }
 
     /// @brief factory that constructs StringParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* factory(const std::string& param_name) {
         return (new StringParser(param_name));
     }
 
@@ -290,7 +387,7 @@ private:
 /// designates all interfaces.
 ///
 /// It is useful for parsing Dhcp4/interface parameter.
-class InterfaceListConfigParser : public Dhcp4ConfigParser {
+class InterfaceListConfigParser : public DhcpConfigParser {
 public:
 
     /// @brief constructor
@@ -328,7 +425,7 @@ public:
     /// @brief factory that constructs InterfaceListConfigParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* factory(const std::string& param_name) {
         return (new InterfaceListConfigParser(param_name));
     }
 
@@ -347,7 +444,7 @@ private:
 /// before build(). Otherwise exception will be thrown.
 ///
 /// It is useful for parsing Dhcp4/subnet4[X]/pool parameters.
-class PoolParser : public Dhcp4ConfigParser {
+class PoolParser : public DhcpConfigParser {
 public:
 
     /// @brief constructor.
@@ -408,7 +505,7 @@ public:
                 }
 
                 Pool4Ptr pool(new Pool4(addr, len));
-                pools_->push_back(pool);
+                local_pools_.push_back(pool);
                 continue;
             }
 
@@ -421,7 +518,7 @@ public:
 
                 Pool4Ptr pool(new Pool4(min, max));
 
-                pools_->push_back(pool);
+                local_pools_.push_back(pool);
                 continue;
             }
 
@@ -440,17 +537,22 @@ public:
         pools_ = storage;
     }
 
-    /// @brief does nothing.
+    /// @brief Stores the parsed values in a storage provided
-    ///
+    ///        by an upper level parser.
-    /// This method is required for all parsers. The value itself
+    virtual void commit() {
-    /// is not commited anywhere. Higher level parsers (for subnet) are expected
+        if (pools_) {
-    /// to use values stored in the storage.
+            // local_pools_ holds the values produced by the build function.
-    virtual void commit() {}
+            // At this point parsing should have completed successfuly so
+            // we can append new data to the supplied storage.
+            pools_->insert(pools_->end(), local_pools_.begin(),
+                           local_pools_.end());
+        }
+    }
 
     /// @brief factory that constructs PoolParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* factory(const std::string& param_name) {
         return (new PoolParser(param_name));
     }
 
@@ -460,6 +562,9 @@ private:
     /// That is typically a storage somewhere in Subnet parser
     /// (an upper level parser).
     PoolStorage* pools_;
+    /// A temporary storage for pools configuration. It is a
+    /// storage where pools are stored by build function.
+    PoolStorage local_pools_;
 };
 
 /// @brief Parser for option data value.
@@ -475,7 +580,7 @@ private:
 /// (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.
-class OptionDataParser : public Dhcp4ConfigParser {
+class OptionDataParser : public DhcpConfigParser {
 public:
 
     /// @brief Constructor.
@@ -512,31 +617,45 @@ public:
             ParserPtr parser;
             if (param.first == "name") {
                 boost::shared_ptr<StringParser>
-                    name_parser(dynamic_cast<StringParser*>(StringParser::Factory(param.first)));
+                    name_parser(dynamic_cast<StringParser*>(StringParser::factory(param.first)));
                 if (name_parser) {
                     name_parser->setStorage(&string_values_);
                     parser = name_parser;
                 }
             } else if (param.first == "code") {
                 boost::shared_ptr<Uint32Parser>
-                    code_parser(dynamic_cast<Uint32Parser*>(Uint32Parser::Factory(param.first)));
+                    code_parser(dynamic_cast<Uint32Parser*>(Uint32Parser::factory(param.first)));
                 if (code_parser) {
                     code_parser->setStorage(&uint32_values_);
                     parser = code_parser;
                 }
             } else if (param.first == "data") {
                 boost::shared_ptr<StringParser>
-                    value_parser(dynamic_cast<StringParser*>(StringParser::Factory(param.first)));
+                    value_parser(dynamic_cast<StringParser*>(StringParser::factory(param.first)));
                 if (value_parser) {
                     value_parser->setStorage(&string_values_);
                     parser = value_parser;
                 }
+            } else if (param.first == "csv-format") {
+                boost::shared_ptr<BooleanParser>
+                    value_parser(dynamic_cast<BooleanParser*>(BooleanParser::factory(param.first)));
+                if (value_parser) {
+                    value_parser->setStorage(&boolean_values_);
+                    parser = value_parser;
+                }
             } else {
                 isc_throw(Dhcp4ConfigError,
                           "Parser error: option-data parameter not supported: "
                           << param.first);
             }
             parser->build(param.second);
+            // Before we can create an option we need to get the data from
+            // the child parsers. The only way to do it is to invoke commit
+            // on them so as they store the values in appropriate storages
+            // that this class provided to them. Note that this will not
+            // modify values stored in the global storages so the configuration
+            // will remain consistent even parsing fails somewhere further on.
+            parser->commit();
         }
         // Try to create the option instance.
         createOption();
@@ -627,16 +746,27 @@ private:
         }
 
         // Get option data from the configuration database ('data' field).
-        // Option data is specified by the user as case insensitive string
+        const std::string option_data = getStringParam("data");
-        // of hexadecimal digits for each option.
+        const bool csv_format = getBooleanParam("csv-format");
-        std::string option_data = getStringParam("data");
         // Transform string of hexadecimal digits into binary format.
         std::vector<uint8_t> binary;
-        try {
+        std::vector<std::string> data_tokens;
-            util::encode::decodeHex(option_data, binary);
+
-        } catch (...) {
+        if (csv_format) {
-            isc_throw(Dhcp4ConfigError, "Parser error: option data is not a valid"
+            // If the option data is specified as a string of comma
-                      << " string of hexadecimal digits: " << option_data);
+            // separated values then we need to split this string into
+            // individual values - each value will be used to initialize
+            // one data field of an option.
+            data_tokens = isc::util::str::tokens(option_data, ",");
+        } else {
+            // Otherwise, the option data is specified as a string of
+            // hexadecimal digits that we have to turn into binary format.
+            try {
+                util::encode::decodeHex(option_data, binary);
+            } catch (...) {
+                isc_throw(Dhcp4ConfigError, "Parser error: option data is not a valid"
+                          << " string of hexadecimal digits: " << option_data);
+            }
         }
         // Get all existing DHCPv4 option definitions. The one that matches
         // our option will be picked and used to create it.
@@ -656,6 +786,13 @@ private:
                       << " for the same option code. This will be supported once"
                       << " there option spaces are implemented.");
         } else if (num_defs == 0) {
+            if (csv_format) {
+                isc_throw(Dhcp4ConfigError, "the CSV option data format can be"
+                          " used to specify values for an option that has a"
+                          " definition. The option with code " << option_code
+                          << " does not have a definition.");
+            }
+
             // @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 an error will be issued if an option definition does not exist
@@ -673,7 +810,9 @@ private:
             // use it to create the option instance.
             const OptionDefinitionPtr& def = *(range.first);
             try {
-                OptionPtr option = def->optionFactory(Option::V4, option_code, binary);
+                OptionPtr option = csv_format ?
+                    def->optionFactory(Option::V4, option_code, data_tokens) :
+                    def->optionFactory(Option::V4, option_code, binary);
                 Subnet::OptionDescriptor desc(option, false);
                 option_descriptor_.option = option;
                 option_descriptor_.persistent = false;
@@ -711,10 +850,27 @@ private:
         return (param->second);
     }
 
+    /// @brief Get a parameter from the boolean values storage.
+    ///
+    /// @param param_id parameter identifier.
+    ///
+    /// @throw isc::dhcp::Dhcp6ConfigError if a parameter has not been found.
+    /// @return a value of the boolean parameter.
+    bool getBooleanParam(const std::string& param_id) const {
+        BooleanStorage::const_iterator param = boolean_values_.find(param_id);
+        if (param == boolean_values_.end()) {
+            isc_throw(isc::dhcp::Dhcp4ConfigError, "parser error: option-data parameter"
+                      << " '" << param_id << "' not specified");
+        }
+        return (param->second);
+    }
+
     /// Storage for uint32 values (e.g. option code).
     Uint32Storage uint32_values_;
     /// Storage for string values (e.g. option name or data).
     StringStorage string_values_;
+    /// Storage for boolean values.
+    BooleanStorage boolean_values_;
     /// Pointer to options storage. This storage is provided by
     /// the calling class and is shared by all OptionDataParser objects.
     OptionStorage* options_;
@@ -728,7 +884,7 @@ private:
 /// data for a particular subnet and creates a collection of options.
 /// If parsing is successful, all these options are added to the Subnet
 /// object.
-class OptionDataListParser : public Dhcp4ConfigParser {
+class OptionDataListParser : public DhcpConfigParser {
 public:
 
     /// @brief Constructor.
@@ -785,7 +941,7 @@ public:
     /// @param param_name param name.
     ///
     /// @return DhcpConfigParser object.
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* factory(const std::string& param_name) {
         return (new OptionDataListParser(param_name));
     }
 
@@ -804,7 +960,7 @@ public:
 ///
 /// This class parses the whole subnet definition. It creates parsers
 /// for received configuration parameters as needed.
-class Subnet4ConfigParser : public Dhcp4ConfigParser {
+class Subnet4ConfigParser : public DhcpConfigParser {
 public:
 
     /// @brief constructor
@@ -844,7 +1000,20 @@ public:
                 isc_throw(Dhcp4ConfigError, "failed to find suitable parser");
             }
         }
-        // Ok, we now have subnet parsed
+        // In order to create new subnet we need to get the data out
+        // of the child parsers first. The only way to do it is to
+        // invoke commit on them because it will make them write
+        // parsed data into storages we have supplied.
+        // Note that triggering commits on child parsers does not
+        // affect global data because we supplied pointers to storages
+        // local to this object. Thus, even if this method fails
+        // later on, the configuration remains consistent.
+        BOOST_FOREACH(ParserPtr parser, parsers_) {
+            parser->commit();
+        }
+
+        // Create a subnet.
+        createSubnet();
     }
 
     /// @brief commits received configuration.
@@ -855,80 +1024,9 @@ public:
     /// objects. Subnet4 are then added to DHCP CfgMgr.
     /// @throw Dhcp4ConfigError if there are any issues encountered during commit
     void commit() {
-        // Invoke commit on all sub-data parsers.
+        if (subnet_) {
-        BOOST_FOREACH(ParserPtr parser, parsers_) {
+            CfgMgr::instance().addSubnet4(subnet_);
-            parser->commit();
         }
-
-        StringStorage::const_iterator it = string_values_.find("subnet");
-        if (it == string_values_.end()) {
-            isc_throw(Dhcp4ConfigError,
-                      "Mandatory subnet definition in subnet missing");
-        }
-        string subnet_txt = it->second;
-        boost::erase_all(subnet_txt, " ");
-        boost::erase_all(subnet_txt, "\t");
-
-        size_t pos = subnet_txt.find("/");
-        if (pos == string::npos) {
-            isc_throw(Dhcp4ConfigError,
-                      "Invalid subnet syntax (prefix/len expected):" << it->second);
-        }
-        IOAddress addr(subnet_txt.substr(0, pos));
-        uint8_t len = boost::lexical_cast<unsigned int>(subnet_txt.substr(pos + 1));
-
-        Triplet<uint32_t> t1 = getParam("renew-timer");
-        Triplet<uint32_t> t2 = getParam("rebind-timer");
-        Triplet<uint32_t> valid = getParam("valid-lifetime");
-
-        /// @todo: Convert this to logger once the parser is working reliably
-        stringstream tmp;
-        tmp << addr.toText() << "/" << (int)len
-            << " with params t1=" << t1 << ", t2=" << t2 << ", valid=" << valid;
-
-        LOG_INFO(dhcp4_logger, DHCP4_CONFIG_NEW_SUBNET).arg(tmp.str());
-
-        Subnet4Ptr subnet(new Subnet4(addr, len, t1, t2, valid));
-
-        for (PoolStorage::iterator it = pools_.begin(); it != pools_.end(); ++it) {
-            subnet->addPool4(*it);
-        }
-
-        const Subnet::OptionContainer& options = subnet->getOptions();
-        const Subnet::OptionContainerTypeIndex& idx = options.get<1>();
-
-        // Add subnet specific options.
-        BOOST_FOREACH(Subnet::OptionDescriptor desc, options_) {
-            Subnet::OptionContainerTypeRange range = idx.equal_range(desc.option->getType());
-            if (std::distance(range.first, range.second) > 0) {
-                LOG_WARN(dhcp4_logger, DHCP4_CONFIG_OPTION_DUPLICATE)
-                    .arg(desc.option->getType()).arg(addr.toText());
-            }
-            subnet->addOption(desc.option);
-        }
-
-        // Check all global options and add them to the subnet object if
-        // they have been configured in the global scope. If they have been
-        // configured in the subnet scope we don't add global option because
-        // the one configured in the subnet scope always takes precedence.
-        BOOST_FOREACH(Subnet::OptionDescriptor desc, option_defaults) {
-            // Get all options specified locally in the subnet and having
-            // code equal to global option's code.
-            Subnet::OptionContainerTypeRange range = idx.equal_range(desc.option->getType());
-            // @todo: In the future we will be searching for options using either
-            // an option code or namespace. Currently we have only the option
-            // code available so if there is at least one option found with the
-            // specific code we don't add the globally configured option.
-            // @todo with this code the first globally configured option
-            // with the given code will be added to a subnet. We may
-            // want to issue a warning about dropping the configuration of
-            // a global option if one already exsists.
-            if (std::distance(range.first, range.second) == 0) {
-                subnet->addOption(desc.option);
-            }
-        }
-
-        CfgMgr::instance().addSubnet4(subnet);
     }
 
 private:
@@ -967,6 +1065,93 @@ private:
         return (false);
     }
 
+    /// @brief Create a new subnet using a data from child parsers.
+    ///
+    /// @throw isc::dhcp::Dhcp4ConfigError if subnet configuration parsing failed.
+    void createSubnet() {
+        StringStorage::const_iterator it = string_values_.find("subnet");
+        if (it == string_values_.end()) {
+            isc_throw(Dhcp4ConfigError,
+                      "Mandatory subnet definition in subnet missing");
+        }
+        // Remove any spaces or tabs.
+        string subnet_txt = it->second;
+        boost::erase_all(subnet_txt, " ");
+        boost::erase_all(subnet_txt, "\t");
+
+        // The subnet format is prefix/len. We are going to extract
+        // the prefix portion of a subnet string to create IOAddress
+        // object from it. IOAddress will be passed to the Subnet's
+        // constructor later on. In order to extract the prefix we
+        // need to get all characters preceding "/".
+        size_t pos = subnet_txt.find("/");
+        if (pos == string::npos) {
+            isc_throw(Dhcp4ConfigError,
+                      "Invalid subnet syntax (prefix/len expected):" << it->second);
+        }
+
+        // Try to create the address object. It also validates that
+        // the address syntax is ok.
+        IOAddress addr(subnet_txt.substr(0, pos));
+        uint8_t len = boost::lexical_cast<unsigned int>(subnet_txt.substr(pos + 1));
+
+        // Get all 'time' parameters using inheritance.
+        // If the subnet-specific value is defined then use it, else
+        // use the global value. The global value must always be
+        // present. If it is not, it is an internal error and exception
+        // is thrown.
+        Triplet<uint32_t> t1 = getParam("renew-timer");
+        Triplet<uint32_t> t2 = getParam("rebind-timer");
+        Triplet<uint32_t> valid = getParam("valid-lifetime");
+
+        /// @todo: Convert this to logger once the parser is working reliably
+        stringstream tmp;
+        tmp << addr.toText() << "/" << (int)len
+            << " with params t1=" << t1 << ", t2=" << t2 << ", valid=" << valid;
+
+        LOG_INFO(dhcp4_logger, DHCP4_CONFIG_NEW_SUBNET).arg(tmp.str());
+
+        subnet_.reset(new Subnet4(addr, len, t1, t2, valid));
+
+        for (PoolStorage::iterator it = pools_.begin(); it != pools_.end(); ++it) {
+            subnet_->addPool4(*it);
+        }
+
+        const Subnet::OptionContainer& options = subnet_->getOptions();
+        const Subnet::OptionContainerTypeIndex& idx = options.get<1>();
+
+        // Add subnet specific options.
+        BOOST_FOREACH(Subnet::OptionDescriptor desc, options_) {
+            Subnet::OptionContainerTypeRange range = idx.equal_range(desc.option->getType());
+            if (std::distance(range.first, range.second) > 0) {
+                LOG_WARN(dhcp4_logger, DHCP4_CONFIG_OPTION_DUPLICATE)
+                    .arg(desc.option->getType()).arg(addr.toText());
+            }
+            subnet_->addOption(desc.option);
+        }
+
+        // Check all global options and add them to the subnet object if
+        // they have been configured in the global scope. If they have been
+        // configured in the subnet scope we don't add global option because
+        // the one configured in the subnet scope always takes precedence.
+        BOOST_FOREACH(Subnet::OptionDescriptor desc, option_defaults) {
+            // Get all options specified locally in the subnet and having
+            // code equal to global option's code.
+            Subnet::OptionContainerTypeRange range = idx.equal_range(desc.option->getType());
+            // @todo: In the future we will be searching for options using either
+            // an option code or namespace. Currently we have only the option
+            // code available so if there is at least one option found with the
+            // specific code we don't add the globally configured option.
+            // @todo with this code the first globally configured option
+            // with the given code will be added to a subnet. We may
+            // want to issue a warning about dropping the configuration of
+            // a global option if one already exsists.
+            if (std::distance(range.first, range.second) == 0) {
+                subnet_->addOption(desc.option);
+            }
+        }
+    }
+
     /// @brief creates parsers for entries in subnet definition
     ///
     /// @todo Add subnet-specific things here (e.g. subnet-specific options)
@@ -974,15 +1159,15 @@ private:
     /// @param config_id name od the entry
     /// @return parser object for specified entry name
     /// @throw NotImplemented if trying to create a parser for unknown config element
-    Dhcp4ConfigParser* createSubnet4ConfigParser(const std::string& config_id) {
+    DhcpConfigParser* createSubnet4ConfigParser(const std::string& config_id) {
         FactoryMap factories;
 
-        factories["valid-lifetime"] = Uint32Parser::Factory;
+        factories["valid-lifetime"] = Uint32Parser::factory;
-        factories["renew-timer"] = Uint32Parser::Factory;
+        factories["renew-timer"] = Uint32Parser::factory;
-        factories["rebind-timer"] = Uint32Parser::Factory;
+        factories["rebind-timer"] = Uint32Parser::factory;
-        factories["subnet"] = StringParser::Factory;
+        factories["subnet"] = StringParser::factory;
-        factories["pool"] = PoolParser::Factory;
+        factories["pool"] = PoolParser::factory;
-        factories["option-data"] = OptionDataListParser::Factory;
+        factories["option-data"] = OptionDataListParser::factory;
 
         FactoryMap::iterator f = factories.find(config_id);
         if (f == factories.end()) {
@@ -1043,6 +1228,9 @@ private:
 
     /// parsers are stored here
     ParserCollection parsers_;
+
+    /// @brief Pointer to the created subnet object.
+    isc::dhcp::Subnet4Ptr subnet_;
 };
 
 /// @brief this class parses list of subnets
@@ -1050,7 +1238,7 @@ private:
 /// This is a wrapper parser that handles the whole list of Subnet4
 /// definitions. It iterates over all entries and creates Subnet4ConfigParser
 /// for each entry.
-class Subnets4ListConfigParser : public Dhcp4ConfigParser {
+class Subnets4ListConfigParser : public DhcpConfigParser {
 public:
 
     /// @brief constructor
@@ -1098,14 +1286,20 @@ public:
     /// @brief Returns Subnet4ListConfigParser object
     /// @param param_name name of the parameter
     /// @return Subnets4ListConfigParser object
-    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
+    static DhcpConfigParser* factory(const std::string& param_name) {
         return (new Subnets4ListConfigParser(param_name));
     }
 
     /// @brief collection of subnet parsers.
     ParserCollection subnets_;
+
 };
 
+} // anonymous namespace
+
+namespace isc {
+namespace dhcp {
+
 /// @brief creates global parsers
 ///
 /// This method creates global parsers that parse global parameters, i.e.
@@ -1114,16 +1308,16 @@ public:
 /// @param config_id pointer to received global configuration entry
 /// @return parser for specified global DHCPv4 parameter
 /// @throw NotImplemented if trying to create a parser for unknown config element
-Dhcp4ConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
+DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
     FactoryMap factories;
 
-    factories["valid-lifetime"] = Uint32Parser::Factory;
+    factories["valid-lifetime"] = Uint32Parser::factory;
-    factories["renew-timer"] = Uint32Parser::Factory;
+    factories["renew-timer"] = Uint32Parser::factory;
-    factories["rebind-timer"] = Uint32Parser::Factory;
+    factories["rebind-timer"] = Uint32Parser::factory;
-    factories["interface"] = InterfaceListConfigParser::Factory;
+    factories["interface"] = InterfaceListConfigParser::factory;
-    factories["subnet4"] = Subnets4ListConfigParser::Factory;
+    factories["subnet4"] = Subnets4ListConfigParser::factory;
-    factories["option-data"] = OptionDataListParser::Factory;
+    factories["option-data"] = OptionDataListParser::factory;
-    factories["version"] = StringParser::Factory;
+    factories["version"] = StringParser::factory;
 
     FactoryMap::iterator f = factories.find(config_id);
     if (f == factories.end()) {
@@ -1150,44 +1344,117 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
 
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_COMMAND, DHCP4_CONFIG_START).arg(config_set->str());
 
-    ParserCollection parsers;
+    // Some of the values specified in the configuration depend on
+    // other values. Typically, the values in the subnet6 structure
+    // depend on the global values. Thus we need to make sure that
+    // the global values are processed first and that they can be
+    // accessed by the subnet6 parsers. We separate parsers that
+    // should process data first (independent_parsers) from those
+    // that must process data when the independent data is already
+    // processed (dependent_parsers).
+    ParserCollection independent_parsers;
+    ParserCollection dependent_parsers;
+
+    // The subnet parsers implement data inheritance by directly
+    // accessing global storage. For this reason the global data
+    // parsers must store the parsed data into global storages
+    // immediately. This may cause data inconsistency if the
+    // 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.
+    Uint32Storage uint32_local(uint32_defaults);
+    StringStorage string_local(string_defaults);
+    OptionStorage option_local(option_defaults);
+
+    // answer will hold the result.
+    ConstElementPtr answer;
+    // rollback informs whether error occured and original data
+    // have to be restored to global storages.
+    bool rollback = false;
+
     try {
+
+        // Iterate over all independent parsers first (all but subnet4)
+        // and try to parse the data.
         BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
-
+            if (config_pair.first != "subnet4") {
-            ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+                ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
-            parser->build(config_pair.second);
+                independent_parsers.push_back(parser);
-            parsers.push_back(parser);
+                parser->build(config_pair.second);
+                // The commit operation here may modify the global storage
+                // but we need it so as the subnet6 parser can access the
+                // parsed data.
+                parser->commit();
+            }
         }
+
+        // Process dependent configuration data.
+        BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
+            if (config_pair.first == "subnet4") {
+                ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+                dependent_parsers.push_back(parser);
+                parser->build(config_pair.second);
+            }
+        }
+
     } catch (const isc::Exception& ex) {
-        ConstElementPtr answer = isc::config::createAnswer(1,
+        answer =
-                                 string("Configuration parsing failed: ") + ex.what());
+            isc::config::createAnswer(1, string("Configuration parsing failed: ") + ex.what());
-        return (answer);
+
+        // An error occured, so make sure that we restore original data.
+        rollback = true;
+
     } catch (...) {
         // for things like bad_cast in boost::lexical_cast
-        ConstElementPtr answer = isc::config::createAnswer(1,
+        answer =
-                                 string("Configuration parsing failed"));
+            isc::config::createAnswer(1, string("Configuration parsing failed"));
+
+        // An error occured, so make sure that we restore original data.
+        rollback = true;
     }
 
-    try {
+    // So far so good, there was no parsing error so let's commit the
-        BOOST_FOREACH(ParserPtr parser, parsers) {
+    // configuration. This will add created subnets and option values into
-            parser->commit();
+    // the server's configuration.
+    // This operation should be exception safe but let's make sure.
+    if (!rollback) {
+        try {
+            BOOST_FOREACH(ParserPtr parser, dependent_parsers) {
+                parser->commit();
+            }
+        }
+        catch (const isc::Exception& ex) {
+            answer =
+                isc::config::createAnswer(2, string("Configuration commit failed: ") + ex.what());
+            rollback = true;
+
+        } catch (...) {
+            // for things like bad_cast in boost::lexical_cast
+            answer =
+                isc::config::createAnswer(2, string("Configuration commit failed"));
+            rollback = true;
+
         }
     }
-    catch (const isc::Exception& ex) {
+
-        ConstElementPtr answer = isc::config::createAnswer(2,
+    // Rollback changes as the configuration parsing failed.
-                                 string("Configuration commit failed: ") + ex.what());
+    if (rollback) {
+        std::swap(uint32_defaults, uint32_local);
+        std::swap(string_defaults, string_local);
+        std::swap(option_defaults, option_local);
         return (answer);
-    } catch (...) {
-        // for things like bad_cast in boost::lexical_cast
-        ConstElementPtr answer = isc::config::createAnswer(2,
-                                 string("Configuration commit failed"));
     }
 
     LOG_INFO(dhcp4_logger, DHCP4_CONFIG_COMPLETE).arg(config_details);
 
-    ConstElementPtr answer = isc::config::createAnswer(0, "Configuration commited.");
+    // Everything was fine. Configuration is successful.
+    answer = isc::config::createAnswer(0, "Configuration commited.");
     return (answer);
 }
 
+const std::map<std::string, uint32_t>& getUint32Defaults() {
+    return (uint32_defaults);
+}
+
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

src/bin/dhcp4/config_parser.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -28,14 +28,8 @@ namespace dhcp {
 
 class Dhcpv4Srv;
 
-/// @brief a collection of elements that store uint32 values (e.g. renew-timer = 900)
-typedef std::map<std::string, uint32_t> Uint32Storage;
-
-/// @brief a collection of elements that store string values
-typedef std::map<std::string, std::string> StringStorage;
-
 /// An exception that is thrown if an error occurs while configuring an
-/// \c Dhcpv4Srv object.
+/// @c Dhcpv4Srv object.
 class Dhcp4ConfigError : public isc::Exception {
 public:
 
@@ -48,97 +42,12 @@ public:
         : isc::Exception(file, line, what) {}
 };
 
-/// @brief Base abstract class for all DHCPv4 parsers
+/// @brief Configure DHCPv4 server (@c Dhcpv4Srv) with a set of configuration values.
 ///
-/// Each instance of a class derived from this class parses one specific config
+/// This function parses configuration information stored in @c config_set
-/// element. Sometimes elements are simple (e.g. a string) and sometimes quite
+/// and configures the @c server by applying the configuration to it.
-/// complex (e.g. a subnet). In such case, it is likely that a parser will
-/// spawn child parsers to parse child elements in the configuration.
-/// @todo: Merge this class with DhcpConfigParser in src/bin/dhcp6
-class Dhcp4ConfigParser {
-    ///
-    /// \name Constructors and Destructor
-    ///
-    /// Note: The copy constructor and the assignment operator are
-    /// intentionally defined as private to make it explicit that this is a
-    /// pure base class.
-    //@{
-private:
-
-    // Private construtor and assignment operator assures that nobody
-    // will be able to copy or assign a parser. There are no defined
-    // bodies for them.
-    Dhcp4ConfigParser(const Dhcp4ConfigParser& source);
-    Dhcp4ConfigParser& operator=(const Dhcp4ConfigParser& source);
-protected:
-    /// \brief The default constructor.
-    ///
-    /// This is intentionally defined as \c protected as this base class should
-    /// never be instantiated (except as part of a derived class).
-    Dhcp4ConfigParser() {}
-public:
-    /// The destructor.
-    virtual ~Dhcp4ConfigParser() {}
-    //@}
-
-    /// \brief Prepare configuration value.
-    ///
-    /// This method parses the "value part" of the configuration identifier
-    /// that corresponds to this derived class and prepares a new value to
-    /// apply to the server.
-    ///
-    /// This method must validate the given value both in terms of syntax
-    /// and semantics of the configuration, so that the server will be
-    /// validly configured at the time of \c commit().  Note: the given
-    /// configuration value is normally syntactically validated, but the
-    /// \c build() implementation must also expect invalid input.  If it
-    /// detects an error it may throw an exception of a derived class
-    /// of \c isc::Exception.
-    ///
-    /// Preparing a configuration value will often require resource
-    /// allocation.  If it fails, it may throw a corresponding standard
-    /// exception.
-    ///
-    /// This method is not expected to be called more than once in the
-    /// life of the object. Although multiple calls are not prohibited
-    /// by the interface, the behavior is undefined.
-    ///
-    /// \param config_value The configuration value for the identifier
-    /// corresponding to the derived class.
-    virtual void build(isc::data::ConstElementPtr config_value) = 0;
-
-    /// \brief Apply the prepared configuration value to the server.
-    ///
-    /// This method is expected to be exception free, and, as a consequence,
-    /// it should normally not involve resource allocation.
-    /// Typically it would simply perform exception free assignment or swap
-    /// operation on the value prepared in \c build().
-    /// In some cases, however, it may be very difficult to meet this
-    /// condition in a realistic way, while the failure case should really
-    /// be very rare.  In such a case it may throw, and, if the parser is
-    /// called via \c configureDhcp4Server(), the caller will convert the
-    /// exception as a fatal error.
-    ///
-    /// This method is expected to be called after \c build(), and only once.
-    /// The result is undefined otherwise.
-    virtual void commit() = 0;
-};
-
-/// @brief a pointer to configuration parser
-typedef boost::shared_ptr<Dhcp4ConfigParser> ParserPtr;
-
-/// @brief a collection of parsers
-///
-/// This container is used to store pointer to parsers for a given scope.
-typedef std::vector<ParserPtr> ParserCollection;
-
-
-/// \brief Configure DHCPv4 server (\c Dhcpv4Srv) with a set of configuration values.
-///
-/// This function parses configuration information stored in \c config_set
-/// and configures the \c server by applying the configuration to it.
 /// It provides the strong exception guarantee as long as the underlying
-/// derived class implementations of \c DhcpConfigParser meet the assumption,
+/// derived class implementations of @c DhcpConfigParser meet the assumption,
 /// that is, it ensures that either configuration is fully applied or the
 /// state of the server is intact.
 ///
@@ -154,7 +63,8 @@ typedef std::vector<ParserPtr> ParserCollection;
 /// reconfiguration statuses. It may return the following response codes:
 /// 0 - configuration successful
 /// 1 - malformed configuration (parsing failed)
-/// 2 - logical error (parsing was successful, but the values are invalid)
+/// 2 - commit failed (parsing was successful, but failed to store the
+/// values in to server's configuration)
 ///
 /// @param config_set a new configuration (JSON) for DHCPv4 server
 /// @return answer that contains result of reconfiguration
@@ -162,6 +72,16 @@ isc::data::ConstElementPtr
 configureDhcp4Server(Dhcpv4Srv&,
                      isc::data::ConstElementPtr config_set);
 
+
+/// @brief Returns the global uint32_t values storage.
+///
+/// This function must be only used by unit tests that need
+/// to access uint32_t global storage to verify that the
+/// Uint32Parser works as expected.
+///
+/// @return a reference to a global uint32 values storage.
+const std::map<std::string, uint32_t>& getUint32Defaults();
+
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 

src/bin/dhcp4/dhcp4.spec

@@ -61,6 +61,11 @@
             "item_type": "string",
             "item_optional": false,
             "item_default": ""
+          },
+          { "item_name": "csv-format",
+            "item_type": "boolean",
+            "item_optional": false,
+            "item_default": False
           } ]
         }
       },
@@ -141,6 +146,11 @@
                       "item_type": "string",
                       "item_optional": false,
                       "item_default": ""
+                    },
+                    { "item_name": "csv-format",
+                      "item_type": "boolean",
+                      "item_optional": false,
+                      "item_default": False
                     } ]
                   }
                 } ]

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -17,9 +17,10 @@
 #include <arpa/inet.h>
 #include <gtest/gtest.h>
 
+#include <config/ccsession.h>
 #include <dhcp4/dhcp4_srv.h>
 #include <dhcp4/config_parser.h>
-#include <config/ccsession.h>
+#include <dhcp/option4_addrlst.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <boost/foreach.hpp>
@@ -35,12 +36,6 @@ using namespace isc::asiolink;
 using namespace isc::data;
 using namespace isc::config;
 
-namespace isc {
-namespace dhcp {
-extern Uint32Storage uint32_defaults;
-}
-}
-
 namespace {
 
 class Dhcp4ParserTest : public ::testing::Test {
@@ -55,7 +50,9 @@ public:
 
     // Checks if global parameter of name have expected_value
     void checkGlobalUint32(string name, uint32_t expected_value) {
-        Uint32Storage::const_iterator it = uint32_defaults.find(name);
+        const std::map<std::string, uint32_t>& uint32_defaults = getUint32Defaults();
+        std::map<std::string, uint32_t>::const_iterator it =
+            uint32_defaults.find(name);
         if (it == uint32_defaults.end()) {
             ADD_FAILURE() << "Expected uint32 with name " << name
                           << " not found";
@@ -81,7 +78,8 @@ public:
     /// @brief Create the simple configuration with single option.
     ///
     /// This function allows to set one of the parameters that configure
-    /// option value. These parameters are: "name", "code" and "data".
+    /// option value. These parameters are: "name", "code", "data" and
+    /// "csv-format".
     ///
     /// @param param_value string holiding option parameter value to be
     /// injected into the configuration string.
@@ -96,14 +94,22 @@ public:
             params["name"] = param_value;
             params["code"] = "56";
             params["data"] = "AB CDEF0105";
+            params["csv-format"] = "False";
         } else if (parameter == "code") {
             params["name"] = "option_foo";
             params["code"] = param_value;
             params["data"] = "AB CDEF0105";
+            params["csv-format"] = "False";
         } else if (parameter == "data") {
             params["name"] = "option_foo";
             params["code"] = "56";
             params["data"] = param_value;
+            params["csv-format"] = "False";
+        } else if (parameter == "csv-format") {
+            params["name"] = "option_foo";
+            params["code"] = "56";
+            params["data"] = "AB CDEF0105";
+            params["csv-format"] = param_value;
         }
         return (createConfigWithOption(params));
     }
@@ -140,6 +146,8 @@ public:
                 stream << "\"code\": " << param.second << "";
             } else if (param.first == "data") {
                 stream << "\"data\": \"" << param.second << "\"";
+            } else if (param.first == "csv-format") {
+                stream << "\"csv-format\": " << param.second;
             }
         }
         stream <<
@@ -393,9 +401,9 @@ TEST_F(Dhcp4ParserTest, poolOutOfSubnet) {
 
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
 
-    // returned value must be 2 (values error)
+    // returned value must be 1 (values error)
     // as the pool does not belong to that subnet
-    checkResult(status, 2);
+    checkResult(status, 1);
 }
 
 // Goal of this test is to verify if pools can be defined
@@ -439,12 +447,14 @@ TEST_F(Dhcp4ParserTest, optionDataDefaults) {
         "\"option-data\": [ {"
         "    \"name\": \"option_foo\","
         "    \"code\": 56,"
-        "    \"data\": \"AB CDEF0105\""
+        "    \"data\": \"AB CDEF0105\","
+        "    \"csv-format\": False"
         " },"
         " {"
         "    \"name\": \"option_foo2\","
         "    \"code\": 23,"
-        "    \"data\": \"01\""
+        "    \"data\": \"01\","
+        "    \"csv-format\": False"
         " } ],"
         "\"subnet4\": [ { "
         "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
@@ -502,7 +512,8 @@ TEST_F(Dhcp4ParserTest, optionDataInSingleSubnet) {
         "\"option-data\": [ {"
         "      \"name\": \"option_foo\","
         "      \"code\": 56,"
-        "      \"data\": \"AB\""
+        "      \"data\": \"AB\","
+        "      \"csv-format\": False"
         " } ],"
         "\"subnet4\": [ { "
         "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
@@ -510,12 +521,14 @@ TEST_F(Dhcp4ParserTest, optionDataInSingleSubnet) {
         "    \"option-data\": [ {"
         "          \"name\": \"option_foo\","
         "          \"code\": 56,"
-        "          \"data\": \"AB CDEF0105\""
+        "          \"data\": \"AB CDEF0105\","
+        "          \"csv-format\": False"
         "        },"
         "        {"
         "          \"name\": \"option_foo2\","
         "          \"code\": 23,"
-        "          \"data\": \"01\""
+        "          \"data\": \"01\","
+        "          \"csv-format\": False"
         "        } ]"
         " } ],"
         "\"valid-lifetime\": 4000 }";
@@ -571,7 +584,8 @@ TEST_F(Dhcp4ParserTest, optionDataInMultipleSubnets) {
         "    \"option-data\": [ {"
         "          \"name\": \"option_foo\","
         "          \"code\": 56,"
-        "          \"data\": \"0102030405060708090A\""
+        "          \"data\": \"0102030405060708090A\","
+        "          \"csv-format\": False"
         "        } ]"
         " },"
         " {"
@@ -580,7 +594,8 @@ TEST_F(Dhcp4ParserTest, optionDataInMultipleSubnets) {
         "    \"option-data\": [ {"
         "          \"name\": \"option_foo2\","
         "          \"code\": 23,"
-        "          \"data\": \"FF\""
+        "          \"data\": \"FF\","
+        "          \"csv-format\": False"
         "        } ]"
         " } ],"
         "\"valid-lifetime\": 4000 }";
@@ -724,10 +739,70 @@ TEST_F(Dhcp4ParserTest, optionDataLowerCase) {
     testOption(*range.first, 56, foo_expected, sizeof(foo_expected));
 }
 
+// Verify that specific option object is returned for standard
+// option which has dedicated option class derived from Option.
+TEST_F(Dhcp4ParserTest, stdOptionData) {
+    ConstElementPtr x;
+    std::map<std::string, std::string> params;
+    params["name"] = "nis-servers";
+    // Option code 41 means nis-servers.
+    params["code"] = "41";
+    // Specify option values in a CSV (user friendly) format.
+    params["data"] = "192.0.2.10, 192.0.2.1, 192.0.2.3";
+    params["csv-format"] = "True";
+
+    std::string config = createConfigWithOption(params);
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
+    ASSERT_TRUE(x);
+    comment_ = parseAnswer(rcode_, x);
+    ASSERT_EQ(0, rcode_);
+
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.5"));
+    ASSERT_TRUE(subnet);
+    const Subnet::OptionContainer& options = subnet->getOptions();
+    ASSERT_EQ(1, options.size());
+
+    // Get the search index. Index #1 is to search using option code.
+    const Subnet::OptionContainerTypeIndex& idx = options.get<1>();
+
+    // Get the options for specified index. Expecting one option to be
+    // returned but in theory we may have multiple options with the same
+    // code so we get the range.
+    std::pair<Subnet::OptionContainerTypeIndex::const_iterator,
+              Subnet::OptionContainerTypeIndex::const_iterator> range =
+        idx.equal_range(DHO_NIS_SERVERS);
+    // Expect single option with the code equal to NIS_SERVERS option code.
+    ASSERT_EQ(1, std::distance(range.first, range.second));
+    // The actual pointer to the option is held in the option field
+    // in the structure returned.
+    OptionPtr option = range.first->option;
+    ASSERT_TRUE(option);
+    // Option object returned for here is expected to be Option6IA
+    // which is derived from Option. This class is dedicated to
+    // represent standard option IA_NA.
+    boost::shared_ptr<Option4AddrLst> option_addrs =
+        boost::dynamic_pointer_cast<Option4AddrLst>(option);
+    // If cast is unsuccessful than option returned was of a
+    // differnt type than Option6IA. This is wrong.
+    ASSERT_TRUE(option_addrs);
+
+    // Get addresses from the option.
+    Option4AddrLst::AddressContainer addrs = option_addrs->getAddresses();
+    // Verify that the addresses have been configured correctly.
+    ASSERT_EQ(3, addrs.size());
+    EXPECT_EQ("192.0.2.10", addrs[0].toText());
+    EXPECT_EQ("192.0.2.1", addrs[1].toText());
+    EXPECT_EQ("192.0.2.3", addrs[2].toText());
+}
+
 /// This test checks if Uint32Parser can really parse the whole range
 /// and properly err of out of range values. As we can't call Uint32Parser
 /// directly, we are exploiting the fact that it is used to parse global
 /// parameter renew-timer and the results are stored in uint32_defaults.
+/// We get the uint32_defaults using a getUint32Defaults functions which
+/// is defined only to access the values from this test.
 TEST_F(Dhcp4ParserTest, DISABLED_Uint32Parser) {
 
     ConstElementPtr status;

src/bin/dhcp6/config_parser.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -28,7 +28,7 @@ namespace dhcp {
 class Dhcpv6Srv;
 
 /// An exception that is thrown if an error occurs while configuring an
-/// \c Dhcpv6Srv object.
+/// @c Dhcpv6Srv object.
 class Dhcp6ConfigError : public isc::Exception {
 public:
 
@@ -41,115 +41,25 @@ public:
         : isc::Exception(file, line, what) {}
 };
 
-/// @brief Base abstract class for all DHCPv6 parsers
+/// @brief Configures DHCPv6 server
 ///
-/// Each instance of a class derived from this class parses one specific config
+/// This function is called every time a new configuration is received. The extra
-/// element. Sometimes elements are simple (e.g. a string) and sometimes quite
+/// parameter is a reference to DHCPv6 server component. It is currently not used
-/// complex (e.g. a subnet). In such case, it is likely that a parser will
+/// and CfgMgr::instance() is accessed instead.
-/// spawn child parsers to parse child elements in the configuration.
-/// @todo: Merge this class with Dhcp4ConfigParser in src/bin/dhcp4
-class DhcpConfigParser {
-    ///
-    /// \name Constructors and Destructor
-    ///
-    /// Note: The copy constructor and the assignment operator are
-    /// intentionally defined as private to make it explicit that this is a
-    /// pure base class.
-    //@{
-private:
-    DhcpConfigParser(const DhcpConfigParser& source);
-    DhcpConfigParser& operator=(const DhcpConfigParser& source);
-protected:
-    /// \brief The default constructor.
-    ///
-    /// This is intentionally defined as \c protected as this base class should
-    /// never be instantiated (except as part of a derived class).
-    DhcpConfigParser() {}
-public:
-    /// The destructor.
-    virtual ~DhcpConfigParser() {}
-    //@}
-
-    /// \brief Prepare configuration value.
-    ///
-    /// This method parses the "value part" of the configuration identifier
-    /// that corresponds to this derived class and prepares a new value to
-    /// apply to the server.
-    ///
-    /// This method must validate the given value both in terms of syntax
-    /// and semantics of the configuration, so that the server will be
-    /// validly configured at the time of \c commit().  Note: the given
-    /// configuration value is normally syntactically validated, but the
-    /// \c build() implementation must also expect invalid input.  If it
-    /// detects an error it may throw an exception of a derived class
-    /// of \c isc::Exception.
-    ///
-    /// Preparing a configuration value will often require resource
-    /// allocation.  If it fails, it may throw a corresponding standard
-    /// exception.
-    ///
-    /// This method is not expected to be called more than once in the
-    /// life of the object. Although multiple calls are not prohibited
-    /// by the interface, the behavior is undefined.
-    ///
-    /// \param config_value The configuration value for the identifier
-    /// corresponding to the derived class.
-    virtual void build(isc::data::ConstElementPtr config_value) = 0;
-
-    /// \brief Apply the prepared configuration value to the server.
-    ///
-    /// This method is expected to be exception free, and, as a consequence,
-    /// it should normally not involve resource allocation.
-    /// Typically it would simply perform exception free assignment or swap
-    /// operation on the value prepared in \c build().
-    /// In some cases, however, it may be very difficult to meet this
-    /// condition in a realistic way, while the failure case should really
-    /// be very rare.  In such a case it may throw, and, if the parser is
-    /// called via \c configureDhcp6Server(), the caller will convert the
-    /// exception as a fatal error.
-    ///
-    /// This method is expected to be called after \c build(), and only once.
-    /// The result is undefined otherwise.
-    virtual void commit() = 0;
-};
-
-/// @brief a pointer to configuration parser
-typedef boost::shared_ptr<DhcpConfigParser> ParserPtr;
-
-/// @brief a collection of parsers
 ///
-/// This container is used to store pointer to parsers for a given scope.
+/// This method does not throw. It catches all exceptions and returns them as
-typedef std::vector<ParserPtr> ParserCollection;
+/// reconfiguration statuses. It may return the following response codes:
-
+/// 0 - configuration successful
-
+/// 1 - malformed configuration (parsing failed)
-/// \brief Configure an \c Dhcpv6Srv object with a set of configuration values.
+/// 2 - commit failed (parsing was successful, but the values could not be
+/// stored in the configuration).
 ///
-/// This function parses configuration information stored in \c config_set
+/// @param server DHCPv6 server object.
-/// and configures the \c server by applying the configuration to it.
+/// @param config_set a new configuration for DHCPv6 server.
-/// It provides the strong exception guarantee as long as the underlying
+/// @return answer that contains result of the reconfiguration.
-/// derived class implementations of \c DhcpConfigParser meet the assumption,
+/// @throw Dhcp6ConfigError if trying to create a parser for NULL config.
-/// that is, it ensures that either configuration is fully applied or the
-/// state of the server is intact.
-///
-/// If a syntax or semantics level error happens during the configuration
-/// (such as malformed configuration or invalid configuration parameter),
-/// this function throws an exception of class \c Dhcp6ConfigError.
-/// If the given configuration requires resource allocation and it fails,
-/// a corresponding standard exception will be thrown.
-/// Other exceptions may also be thrown, depending on the implementation of
-/// the underlying derived class of \c Dhcp6ConfigError.
-/// In any case the strong guarantee is provided as described above except
-/// in the very rare cases where the \c commit() method of a parser throws
-/// an exception.  If that happens this function converts the exception
-/// into a \c FatalError exception and rethrows it.  This exception is
-/// expected to be caught at the highest level of the application to terminate
-/// the program gracefully.
-///
-/// \param server The \c Dhcpv6Srv object to be configured.
-/// \param config_set A JSON style configuration to apply to \c server.
 isc::data::ConstElementPtr
-configureDhcp6Server(Dhcpv6Srv& server,
+configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set);
-                     isc::data::ConstElementPtr config_set);
 
 }; // end of isc::dhcp namespace
 }; // end of isc namespace

src/bin/dhcp6/dhcp6.dox

@@ -35,19 +35,19 @@
 
  This method iterates over list of received configuration elements and creates a
  list of parsers for each received entry. Parser is an object that is derived
- from a \ref isc::dhcp::DhcpConfigParser class. Once a parser is created
+ from a DhcpConfigParser class. Once a parser is created
  (constructor), its value is set (using build() method). Once all parsers are
  build, the configuration is then applied ("commited") and commit() method is
  called.
 
  All parsers are defined in src/bin/dhcp6/config_parser.cc file. Some of them
- are generic (e.g. \ref isc::dhcp::Uint32Parser that is able to handle any
+ are generic (e.g. Uint32Parser that is able to handle any
- unsigned 32 bit integer), but some are very specialized (e.g. \ref
+ unsigned 32 bit integer), but some are very specialized (e.g.
- isc::dhcp::Subnets6ListConfigParser parses definitions of Subnet6 lists). In
+ Subnets6ListConfigParser parses definitions of Subnet6 lists). In some cases,
- some cases, e.g. subnet6 definitions, the configuration entry is not a simple
+ e.g. subnet6 definitions, the configuration entry is not a simple value, but
- value, but a map or a list itself. In such case, the parser iterates over all
+ a map or a list itself. In such case, the parser iterates over all elements
- elements and creates parsers for a given scope. This process may be repeated
+ and creates parsers for a given scope. This process may be repeated (sort of)
- (sort of) recursively.
+ recursively.
 
  @section dhcpv6ConfigInherit DHCPv6 Configuration Inheritance
 
@@ -55,16 +55,16 @@
  For example, renew-timer value may be specified at a global scope and it then
  applies to all subnets. However, some subnets may have it overwritten with more
  specific values that takes precedence over global values that are considered
- defaults. Some parsers (e.g. \ref isc::dhcp::Uint32Parser and \ref
+ defaults. Some parsers (e.g. Uint32Parser and StringParser) implement that
- isc::dhcp::StringParser) implement that inheritance. By default, they store
+ inheritance. By default, they store values in global uint32_defaults and
- values in global uint32_defaults and string_defaults storages. However, it is
+ string_defaults storages. However, it is possible to instruct them to store
- possible to instruct them to store parsed values in more specific
+ parsed values in more specific storages. That capability is used, e.g. in
- storages. That capability is used, e.g. in \ref isc::dhcp::Subnet6ConfigParser
+ Subnet6ConfigParser that has its own storage that is unique for each subnet.
- that has its own storage that is unique for each subnet. Finally, during commit
+ Finally, during commit phase (commit() method), appropriate parsers can use
- phase (commit() method), appropriate parsers can use apply parameter inheritance.
+ apply parameter inheritance.
 
  Debugging configuration parser may be confusing. Therefore there is a special
- class called \ref isc::dhcp::DebugParser. It does not configure anything, but just
+ class called DebugParser. It does not configure anything, but just
  accepts any parameter of any type. If requested to commit configuration, it will
  print out received parameter name and its value. This class is not currently used,
  but it is convenient to have it every time a new parameter is added to DHCP

src/bin/dhcp6/dhcp6.spec

@@ -67,6 +67,11 @@
             "item_type": "string",
             "item_optional": false,
             "item_default": ""
+          },
+          { "item_name": "csv-format",
+            "item_type": "boolean",
+            "item_optional": false,
+            "item_default": False
           } ]
         }
       },
@@ -152,6 +157,11 @@
                       "item_type": "string",
                       "item_optional": false,
                       "item_default": ""
+                    },
+                    { "item_name": "csv-format",
+                      "item_type": "boolean",
+                      "item_optional": false,
+                      "item_default": False
                     } ]
                   }
                 } ]

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -56,7 +56,8 @@ public:
     /// @brief Create the simple configuration with single option.
     ///
     /// This function allows to set one of the parameters that configure
-    /// option value. These parameters are: "name", "code" and "data".
+    /// option value. These parameters are: "name", "code", "data" and
+    /// "csv-format".
     ///
     /// @param param_value string holiding option parameter value to be
     /// injected into the configuration string.
@@ -69,14 +70,22 @@ public:
             params["name"] = param_value;
             params["code"] = "80";
             params["data"] = "AB CDEF0105";
+            params["csv-format"] = "False";
         } else if (parameter == "code") {
             params["name"] = "option_foo";
             params["code"] = param_value;
             params["data"] = "AB CDEF0105";
+            params["csv-format"] = "False";
         } else if (parameter == "data") {
             params["name"] = "option_foo";
             params["code"] = "80";
             params["data"] = param_value;
+            params["csv-format"] = "False";
+        } else if (parameter == "csv-format") {
+            params["name"] = "option_foo";
+            params["code"] = "80";
+            params["data"] = "AB CDEF0105";
+            params["csv-format"] = param_value;
         }
         return (createConfigWithOption(params));
     }
@@ -105,9 +114,11 @@ public:
             if (param.first == "name") {
                 stream << "\"name\": \"" << param.second << "\"";
             } else if (param.first == "code") {
-                stream << "\"code\": " << param.second << "";
+                stream << "\"code\": " << param.second;;
             } else if (param.first == "data") {
                 stream << "\"data\": \"" << param.second << "\"";
+            } else if (param.first == "csv-format") {
+                stream << "\"csv-format\": " << param.second;
             }
         }
         stream <<
@@ -374,11 +385,12 @@ TEST_F(Dhcp6ParserTest, poolOutOfSubnet) {
 
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
-    // returned value must be 2 (values error)
+    // returned value must be 1 (values error)
     // as the pool does not belong to that subnet
     ASSERT_TRUE(status);
     comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(2, rcode_);
+
+    EXPECT_EQ(1, rcode_);
 }
 
 // Goal of this test is to verify if pools can be defined
@@ -427,12 +439,14 @@ TEST_F(Dhcp6ParserTest, optionDataDefaults) {
         "\"option-data\": [ {"
         "    \"name\": \"option_foo\","
         "    \"code\": 100,"
-        "    \"data\": \"AB CDEF0105\""
+        "    \"data\": \"AB CDEF0105\","
+        "    \"csv-format\": False"
         " },"
         " {"
         "    \"name\": \"option_foo2\","
         "    \"code\": 101,"
-        "    \"data\": \"01\""
+        "    \"data\": \"01\","
+        "    \"csv-format\": False"
         " } ],"
         "\"subnet6\": [ { "
         "    \"pool\": [ \"2001:db8:1::/80\" ],"
@@ -497,7 +511,8 @@ TEST_F(Dhcp6ParserTest, optionDataInSingleSubnet) {
         "\"option-data\": [ {"
         "      \"name\": \"option_foo\","
         "      \"code\": 100,"
-        "      \"data\": \"AB\""
+        "      \"data\": \"AB\","
+        "      \"csv-format\": False"
         " } ],"
         "\"subnet6\": [ { "
         "    \"pool\": [ \"2001:db8:1::/80\" ],"
@@ -505,12 +520,14 @@ TEST_F(Dhcp6ParserTest, optionDataInSingleSubnet) {
         "    \"option-data\": [ {"
         "          \"name\": \"option_foo\","
         "          \"code\": 100,"
-        "          \"data\": \"AB CDEF0105\""
+        "          \"data\": \"AB CDEF0105\","
+        "          \"csv-format\": False"
         "        },"
         "        {"
         "          \"name\": \"option_foo2\","
         "          \"code\": 101,"
-        "          \"data\": \"01\""
+        "          \"data\": \"01\","
+        "          \"csv-format\": False"
         "        } ]"
         " } ],"
         "\"valid-lifetime\": 4000 }";
@@ -567,7 +584,8 @@ TEST_F(Dhcp6ParserTest, optionDataInMultipleSubnets) {
         "    \"option-data\": [ {"
         "          \"name\": \"option_foo\","
         "          \"code\": 100,"
-        "          \"data\": \"0102030405060708090A\""
+        "          \"data\": \"0102030405060708090A\","
+        "          \"csv-format\": False"
         "        } ]"
         " },"
         " {"
@@ -576,7 +594,8 @@ TEST_F(Dhcp6ParserTest, optionDataInMultipleSubnets) {
         "    \"option-data\": [ {"
         "          \"name\": \"option_foo2\","
         "          \"code\": 101,"
-        "          \"data\": \"FFFEFDFCFB\""
+        "          \"data\": \"FFFEFDFCFB\","
+        "          \"csv-format\": False"
         "        } ]"
         " } ],"
         "\"valid-lifetime\": 4000 }";
@@ -737,7 +756,8 @@ TEST_F(Dhcp6ParserTest, stdOptionData) {
     params["name"] = "OPTION_IA_NA";
     // Option code 3 means OPTION_IA_NA.
     params["code"] = "3";
-    params["data"] = "ABCDEF01 02030405 06070809";
+    params["data"] = "12345, 6789, 1516";
+    params["csv-format"] = "True";
 
     std::string config = createConfigWithOption(params);
     ElementPtr json = Element::fromJSON(config);
@@ -778,9 +798,9 @@ TEST_F(Dhcp6ParserTest, stdOptionData) {
     // If cast was successful we may use accessors exposed by
     // Option6IA to validate that the content of this option
     // has been set correctly.
-    EXPECT_EQ(0xABCDEF01, optionIA->getIAID());
+    EXPECT_EQ(12345, optionIA->getIAID());
-    EXPECT_EQ(0x02030405, optionIA->getT1());
+    EXPECT_EQ(6789, optionIA->getT1());
-    EXPECT_EQ(0x06070809, optionIA->getT2());
+    EXPECT_EQ(1516, optionIA->getT2());
 }
 
 };

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -370,13 +370,14 @@ TEST_F(Dhcpv6SrvTest, advertiseOptions) {
         "    \"option-data\": [ {"
         "          \"name\": \"OPTION_DNS_SERVERS\","
         "          \"code\": 23,"
-        "          \"data\": \"2001 0DB8 1234 FFFF 0000 0000 0000 0001"
+        "          \"data\": \"2001:db8:1234:FFFF::1, 2001:db8:1234:FFFF::2\","
-        "2001 0DB8 1234 FFFF 0000 0000 0000 0002\""
+        "          \"csv-format\": True"
         "        },"
         "        {"
         "          \"name\": \"OPTION_FOO\","
         "          \"code\": 1000,"
-        "          \"data\": \"1234\""
+        "          \"data\": \"1234\","
+        "          \"csv-format\": False"
         "        } ]"
         " } ],"
         "\"valid-lifetime\": 4000 }";

src/lib/dhcp/option_definition.cc

@@ -22,6 +22,7 @@
 #include <dhcp/option_int.h>
 #include <dhcp/option_int_array.h>
 #include <util/encode/hex.h>
+#include <util/strutil.h>
 
 using namespace std;
 using namespace isc::util;
@@ -176,10 +177,10 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
         if (values.empty()) {
             isc_throw(InvalidOptionValue, "no option value specified");
         }
-        writeToBuffer(values[0], type_, buf);
+        writeToBuffer(util::str::trim(values[0]), type_, buf);
     } else if (array_type_ && type_ != OPT_RECORD_TYPE) {
         for (size_t i = 0; i < values.size(); ++i) {
-            writeToBuffer(values[i], type_, buf);
+            writeToBuffer(util::str::trim(values[i]), type_, buf);
         }
     } else if (type_ == OPT_RECORD_TYPE) {
         const RecordFieldsCollection& records = getRecordFields();
@@ -189,7 +190,8 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
                       << " provided.");
         }
         for (size_t i = 0; i < records.size(); ++i) {
-            writeToBuffer(values[i], records[i], buf);
+            writeToBuffer(util::str::trim(values[i]),
+                          records[i], buf);
         }
     }
     return (optionFactory(u, type, buf.begin(), buf.end()));

src/lib/dhcp/option_definition.h

@@ -361,7 +361,7 @@ public:
 
     /// @brief Factory function to create option with array of integer values.
     ///
-    /// @param universe (V4 or V6).
+    /// @param u universe (V4 or V6).
     /// @param type option type.
     /// @param begin iterator pointing to the beginning of the buffer.
     /// @param end iterator pointing to the end of the buffer.

src/lib/dhcpsrv/Makefile.am

@@ -35,6 +35,7 @@ libb10_dhcpsrv_la_SOURCES += addr_utilities.cc addr_utilities.h
 libb10_dhcpsrv_la_SOURCES += alloc_engine.cc alloc_engine.h
 libb10_dhcpsrv_la_SOURCES += dhcpsrv_log.cc dhcpsrv_log.h
 libb10_dhcpsrv_la_SOURCES += cfgmgr.cc cfgmgr.h
+libb10_dhcpsrv_la_SOURCES += dhcp_config_parser.h
 libb10_dhcpsrv_la_SOURCES += hwaddr.cc hwaddr.h
 libb10_dhcpsrv_la_SOURCES += lease_mgr.cc lease_mgr.h
 libb10_dhcpsrv_la_SOURCES += lease_mgr_factory.cc lease_mgr_factory.h

src/lib/dhcpsrv/dhcp_config_parser.h

@@ -0,0 +1,114 @@
+// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef DHCP_CONFIG_PARSER_H
+#define DHCP_CONFIG_PARSER_H
+
+namespace isc {
+namespace dhcp {
+
+/// @brief Forward declaration to DhcpConfigParser class.
+///
+/// It is only needed here to define types that are
+/// based on this class before the class definition.
+class DhcpConfigParser;
+
+/// @brief a pointer to configuration parser
+typedef boost::shared_ptr<DhcpConfigParser> ParserPtr;
+
+/// @brief Collection of parsers.
+///
+/// This container is used to store pointer to parsers for a given scope.
+typedef std::vector<ParserPtr> ParserCollection;
+
+/// @brief Base abstract class for all DHCP parsers
+///
+/// Each instance of a class derived from this class parses one specific config
+/// element. Sometimes elements are simple (e.g. a string) and sometimes quite
+/// complex (e.g. a subnet). In such case, it is likely that a parser will
+/// spawn child parsers to parse child elements in the configuration.
+class DhcpConfigParser {
+    ///
+    /// @name Constructors and Destructor
+    ///
+    /// Note: The copy constructor and the assignment operator are
+    /// intentionally defined as private to make it explicit that this is a
+    /// pure base class.
+    //@{
+private:
+
+    // Private construtor and assignment operator assures that nobody
+    // will be able to copy or assign a parser. There are no defined
+    // bodies for them.
+    DhcpConfigParser(const DhcpConfigParser& source);
+    DhcpConfigParser& operator=(const DhcpConfigParser& source);
+protected:
+    /// @brief The default constructor.
+    ///
+    /// This is intentionally defined as @c protected as this base class should
+    /// never be instantiated (except as part of a derived class).
+    DhcpConfigParser() {}
+public:
+    /// The destructor.
+    virtual ~DhcpConfigParser() {}
+    //@}
+
+    /// @brief Prepare configuration value.
+    ///
+    /// This method parses the "value part" of the configuration identifier
+    /// that corresponds to this derived class and prepares a new value to
+    /// apply to the server.
+    ///
+    /// This method must validate the given value both in terms of syntax
+    /// and semantics of the configuration, so that the server will be
+    /// validly configured at the time of @c commit().  Note: the given
+    /// configuration value is normally syntactically validated, but the
+    /// @c build() implementation must also expect invalid input.  If it
+    /// detects an error it may throw an exception of a derived class
+    /// of @c isc::Exception.
+    ///
+    /// Preparing a configuration value will often require resource
+    /// allocation.  If it fails, it may throw a corresponding standard
+    /// exception.
+    ///
+    /// This method is not expected to be called more than once in the
+    /// life of the object. Although multiple calls are not prohibited
+    /// by the interface, the behavior is undefined.
+    ///
+    /// @param config_value The configuration value for the identifier
+    /// corresponding to the derived class.
+    virtual void build(isc::data::ConstElementPtr config_value) = 0;
+
+    /// @brief Apply the prepared configuration value to the server.
+    ///
+    /// This method is expected to be exception free, and, as a consequence,
+    /// it should normally not involve resource allocation.
+    /// Typically it would simply perform exception free assignment or swap
+    /// operation on the value prepared in @c build().
+    /// In some cases, however, it may be very difficult to meet this
+    /// condition in a realistic way, while the failure case should really
+    /// be very rare.  In such a case it may throw, and, if the parser is
+    /// called via @c configureDhcp4Server(), the caller will convert the
+    /// exception as a fatal error.
+    ///
+    /// This method is expected to be called after @c build(), and only once.
+    /// The result is undefined otherwise.
+    virtual void commit() = 0;
+};
+
+
+} // end of isc::dhcp namespace
+} // end of isc namespace
+
+#endif // DHCP_CONFIG_PARSER_H

src/lib/dhcpsrv/subnet.cc

@@ -77,8 +77,8 @@ void Subnet4::addPool4(const Pool4Ptr& pool) {
 
     if (!inRange(first_addr) || !inRange(last_addr)) {
         isc_throw(BadValue, "Pool4 (" << first_addr.toText() << "-" << last_addr.toText()
-                  << " does not belong in this (" << prefix_ << "/" << prefix_len_
+                  << " does not belong in this (" << prefix_.toText() << "/"
-                  << ") subnet4");
+                  << static_cast<int>(prefix_len_) << ") subnet4");
     }
 
     /// @todo: Check that pools do not overlap
@@ -148,10 +148,10 @@ void Subnet6::addPool6(const Pool6Ptr& pool) {
 
     if (!inRange(first_addr) || !inRange(last_addr)) {
         isc_throw(BadValue, "Pool6 (" << first_addr.toText() << "-" << last_addr.toText()
-                  << " does not belong in this (" << prefix_ << "/" << prefix_len_
+                  << ") does not belong in this (" << prefix_.toText()
+                  << "/" << static_cast<int>(prefix_len_)
                   << ") subnet6");
     }
-
     /// @todo: Check that pools do not overlap
 
     pools_.push_back(pool);