BIND 10 trac2270, updated. 0589469f4a57ee95f16ea7e316365ae0641c4c3c [2270] Dhcp4ConfigParser members are now private.

BIND 10 source code commits bind10-changes at lists.isc.org
Wed Dec 5 14:48:55 UTC 2012


The branch, trac2270 has been updated
       via  0589469f4a57ee95f16ea7e316365ae0641c4c3c (commit)
       via  c84efb2586d129061f0fc0e1bcf291e0582b0f9d (commit)
       via  bdeb3c67d12483d7ed6945e7d190195764e24fcc (commit)
       via  65a83c79b14885d4a4106e1a707ef431b72053e4 (commit)
      from  d0af0ecaadd8950008ef65ee2b40b56fa21a1e82 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit 0589469f4a57ee95f16ea7e316365ae0641c4c3c
Author: Tomek Mrugalski <tomasz at isc.org>
Date:   Wed Dec 5 15:38:14 2012 +0100

    [2270] Dhcp4ConfigParser members are now private.

commit c84efb2586d129061f0fc0e1bcf291e0582b0f9d
Author: Tomek Mrugalski <tomasz at isc.org>
Date:   Wed Dec 5 15:27:30 2012 +0100

    [2270] Changes after review
    
    - added boundary checks for Uint32Parser
    - fixed #include order
    - Class renamed to Dhcp4ConfigParser
    - added extra test for Uint32Parser
    - Many Doxygen fixes and clean-ups

commit bdeb3c67d12483d7ed6945e7d190195764e24fcc
Author: Tomek Mrugalski <tomasz at isc.org>
Date:   Wed Dec 5 15:23:34 2012 +0100

    [2270] Fix for repeated reconf in dhcp6 (old defaults are now removed)

commit 65a83c79b14885d4a4106e1a707ef431b72053e4
Author: Tomek Mrugalski <tomasz at isc.org>
Date:   Wed Dec 5 15:22:05 2012 +0100

    [2270] Fixes in Doxygen comments in src/bin/dhcp6

-----------------------------------------------------------------------

Summary of changes:
 src/bin/dhcp4/config_parser.cc                |  162 +++++++++++++------------
 src/bin/dhcp4/config_parser.h                 |   75 +++++++-----
 src/bin/dhcp4/ctrl_dhcp4_srv.cc               |    7 +-
 src/bin/dhcp4/ctrl_dhcp4_srv.h                |    4 +-
 src/bin/dhcp4/dhcp4.dox                       |    2 -
 src/bin/dhcp4/dhcp4_srv.cc                    |    2 +-
 src/bin/dhcp4/dhcp4_srv.h                     |    6 +-
 src/bin/dhcp4/tests/config_parser_unittest.cc |  122 ++++++++++++++-----
 src/bin/dhcp6/config_parser.cc                |   42 ++++---
 src/bin/dhcp6/config_parser.h                 |   21 ++--
 10 files changed, 273 insertions(+), 170 deletions(-)

-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc
index ac4b3c6..6f8cf86 100644
--- a/src/bin/dhcp4/config_parser.cc
+++ b/src/bin/dhcp4/config_parser.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012 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
@@ -12,26 +12,22 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <stdint.h>
-#include <iostream>
-#include <vector>
-#include <map>
+// We want UINT32_MAX macro to be defined in stdint.h
+#define __STDC_LIMIT_MACROS
+
 #include <boost/foreach.hpp>
-#include <boost/shared_ptr.hpp>
-#include <boost/scoped_ptr.hpp>
 #include <boost/lexical_cast.hpp>
 #include <boost/algorithm/string.hpp>
-#include <asiolink/io_address.h>
-#include <cc/data.h>
 #include <config/ccsession.h>
-#include <log/logger_support.h>
-#include <dhcp/triplet.h>
-#include <dhcp/pool.h>
-#include <dhcp/subnet.h>
 #include <dhcp/cfgmgr.h>
 #include <dhcp4/config_parser.h>
 #include <dhcp4/dhcp4_log.h>
 
+#include <stdint.h>
+#include <iostream>
+#include <vector>
+#include <map>
+
 using namespace std;
 using namespace isc::data;
 using namespace isc::asiolink;
@@ -43,17 +39,11 @@ namespace dhcp {
 typedef pair<string, ConstElementPtr> ConfigPair;
 
 /// @brief a factory method that will create a parser for a given element name
-typedef DhcpConfigParser* ParserFactory(const std::string& config_id);
+typedef Dhcp4ConfigParser* 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<string, uint32_t> Uint32Storage;
-
-/// @brief a collection of elements that store string values
-typedef std::map<string, string> StringStorage;
-
 /// @brief a collection of pools
 ///
 /// That type is used as intermediate storage, when pools are parsed, but there is
@@ -72,12 +62,12 @@ StringStorage string_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 DhcpConfigParser {
+class DebugParser : public Dhcp4ConfigParser {
 public:
 
     /// @brief Constructor
     ///
-    /// See \ref DhcpConfigParser class for details.
+    /// See \ref Dhcp4ConfigParser class for details.
     ///
     /// @param param_name name of the parsed parameter
     DebugParser(const std::string& param_name)
@@ -86,7 +76,7 @@ public:
 
     /// @brief builds parameter value
     ///
-    /// See \ref DhcpConfigParser class for details.
+    /// See \ref Dhcp4ConfigParser class for details.
     ///
     /// @param new_config pointer to the new configuration
     virtual void build(ConstElementPtr new_config) {
@@ -100,7 +90,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 DhcpConfigParser class for details.
+    /// See \ref Dhcp4ConfigParser 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
@@ -112,11 +102,11 @@ public:
     /// @brief factory that constructs DebugParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static DhcpConfigParser* Factory(const std::string& param_name) {
+    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
         return (new DebugParser(param_name));
     }
 
-protected:
+private:
     /// name of the parsed parameter
     std::string param_name_;
 
@@ -131,11 +121,11 @@ protected:
 /// (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 DhcpConfigParser.
+/// in its base class, \ref Dhcp4ConfigParser.
 ///
 /// For overview of usability of this generic purpose parser, see
-/// \ref dhcpv4-config-inherit page.
-class Uint32Parser : public DhcpConfigParser {
+/// \ref dhcp4-config-inherit page.
+class Uint32Parser : public Dhcp4ConfigParser {
 public:
 
     /// @brief constructor for Uint32Parser
@@ -150,13 +140,30 @@ public:
     /// \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
     virtual void build(ConstElementPtr value) {
+        int64_t check;
+        string x = value->str();
         try {
-            value_ = boost::lexical_cast<uint32_t>(value->str());
+            check = boost::lexical_cast<int64_t>(x);
         } catch (const boost::bad_lexical_cast &) {
             isc_throw(BadValue, "Failed to parse value " << value->str()
                       << " as unsigned 32-bit integer.");
         }
+        if (check > UINT32_MAX) {
+            isc_throw(BadValue, "Value " << value->str() << "is too large"
+                      << " for unsigned 32-bit integer.");
+        }
+        if (check < 0) {
+            isc_throw(BadValue, "Value " << value->str() << "is negative."
+                      << " Only 0 or larger are allowed for unsigned 32-bit integer.");
+        }
+
+        // value is small enough to fit
+        value_ = static_cast<uint32_t>(check);
+
+        /// @todo: check if there is no such name in the map. Otherwise
+        /// the insert will fail silently
         storage_->insert(pair<string, uint32_t>(param_name_, value_));
     }
 
@@ -166,7 +173,7 @@ public:
     /// 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 Subnet4Parser when constructing
+    /// here, but is rather used by \ref Subnet4ConfigParser when constructing
     /// the subnet.
     virtual void commit() {
     }
@@ -174,13 +181,13 @@ public:
     /// @brief factory that constructs Uint32Parser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static DhcpConfigParser* Factory(const std::string& param_name) {
+    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
         return (new Uint32Parser(param_name));
     }
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv4-config-inherit for details.
+    /// See \ref dhcp4-config-inherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(Uint32Storage* storage) {
@@ -205,11 +212,11 @@ protected:
 /// (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 DhcpConfigParser.
+/// in its base class, \ref Dhcp4ConfigParser.
 ///
 /// For overview of usability of this generic purpose parser, see
-/// \ref dhcpv4-config-inherit page.
-class StringParser : public DhcpConfigParser {
+/// \ref dhcp4-config-inherit page.
+class StringParser : public Dhcp4ConfigParser {
 public:
 
     /// @brief constructor for StringParser
@@ -227,6 +234,8 @@ public:
     virtual void build(ConstElementPtr value) {
         value_ = value->str();
         boost::erase_all(value_, "\"");
+        /// @todo: check if there is no such name in the map. Otherwise
+        /// the insert will fail silently
         storage_->insert(pair<string, string>(param_name_, value_));
     }
 
@@ -244,13 +253,13 @@ public:
     /// @brief factory that constructs StringParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static DhcpConfigParser* Factory(const std::string& param_name) {
+    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
         return (new StringParser(param_name));
     }
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv4-config-inherit for details.
+    /// See \ref dhcp4-config-inherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(StringStorage* storage) {
@@ -277,7 +286,7 @@ protected:
 /// designates all interfaces.
 ///
 /// It is useful for parsing Dhcp4/interface parameter.
-class InterfaceListConfigParser : public DhcpConfigParser {
+class InterfaceListConfigParser : public Dhcp4ConfigParser {
 public:
 
     /// @brief constructor
@@ -286,17 +295,18 @@ public:
     /// "interface" parameter only. All other types will throw exception.
     ///
     /// @param param_name name of the configuration parameter being parsed
+    /// @throw BadValue if supplied parameter name is not "interface"
     InterfaceListConfigParser(const std::string& param_name) {
         if (param_name != "interface") {
-            isc_throw(NotImplemented, "Internal error. Interface configuration "
+            isc_throw(BadValue, "Internal error. Interface configuration "
                       "parser called for the wrong parameter: " << param_name);
         }
     }
 
     /// @brief parses parameters value
     ///
-    /// Parses configuration entry (list of parameters) and stores it in
-    /// storage. See \ref setStorage() for details.
+    /// Parses configuration entry (list of parameters) and adds each element
+    /// to the interfaces list.
     ///
     /// @param value pointer to the content of parsed values
     virtual void build(ConstElementPtr value) {
@@ -314,7 +324,7 @@ public:
     /// @brief factory that constructs InterfaceListConfigParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static DhcpConfigParser* Factory(const std::string& param_name) {
+    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
         return (new InterfaceListConfigParser(param_name));
     }
 
@@ -333,7 +343,7 @@ protected:
 /// before build(). Otherwise exception will be thrown.
 ///
 /// It is useful for parsing Dhcp4/subnet4[X]/pool parameters.
-class PoolParser : public DhcpConfigParser {
+class PoolParser : public Dhcp4ConfigParser {
 public:
 
     /// @brief constructor.
@@ -347,10 +357,13 @@ public:
     /// This method parses the actual list of interfaces.
     /// No validation is done at this stage, everything is interpreted as
     /// interface name.
+    /// @param pools_list list of pools defined for a subnet
+    /// @throw InvalidOperation if storage was not specified (setStorage() not called)
+    /// @throw Dhcp4ConfigError when pool parsing fails
     void build(ConstElementPtr pools_list) {
         // setStorage() should have been called before build
         if (!pools_) {
-            isc_throw(NotImplemented, "Parser logic error. No pool storage set,"
+            isc_throw(InvalidOperation, "Parser logic error. No pool storage set,"
                       " but pool parser asked to parse pools");
         }
 
@@ -416,7 +429,7 @@ public:
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv4-config-inherit for details.
+    /// See \ref dhcp4-config-inherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(PoolStorage* storage) {
@@ -433,7 +446,7 @@ public:
     /// @brief factory that constructs PoolParser objects
     ///
     /// @param param_name name of the parameter to be parsed
-    static DhcpConfigParser* Factory(const std::string& param_name) {
+    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
         return (new PoolParser(param_name));
     }
 
@@ -449,7 +462,7 @@ protected:
 ///
 /// This class parses the whole subnet definition. It creates parsers
 /// for received configuration parameters as needed.
-class Subnet4ConfigParser : public DhcpConfigParser {
+class Subnet4ConfigParser : public Dhcp4ConfigParser {
 public:
 
     /// @brief constructor
@@ -469,22 +482,22 @@ public:
 
             // if this is an Uint32 parser, tell it to store the values
             // in values_, rather than in global storage
-            boost::shared_ptr<Uint32Parser> uintParser =
+            boost::shared_ptr<Uint32Parser> uint_parser =
                 boost::dynamic_pointer_cast<Uint32Parser>(parser);
-            if (uintParser) {
-                uintParser->setStorage(&uint32_values_);
+            if (uint_parser) {
+                uint_parser->setStorage(&uint32_values_);
             } else {
 
-                boost::shared_ptr<StringParser> stringParser =
+                boost::shared_ptr<StringParser> string_parser =
                     boost::dynamic_pointer_cast<StringParser>(parser);
-                if (stringParser) {
-                    stringParser->setStorage(&string_values_);
+                if (string_parser) {
+                    string_parser->setStorage(&string_values_);
                 } else {
 
-                    boost::shared_ptr<PoolParser> poolParser =
+                    boost::shared_ptr<PoolParser> pool_parser =
                         boost::dynamic_pointer_cast<PoolParser>(parser);
-                    if (poolParser) {
-                        poolParser->setStorage(&pools_);
+                    if (pool_parser) {
+                        pool_parser->setStorage(&pools_);
                     }
                 }
             }
@@ -502,6 +515,7 @@ public:
     /// storing the values that are actually consumed here. Pool definitions
     /// created in other parsers are used here and added to newly created Subnet4
     /// objects. Subnet4 are then added to DHCP CfgMgr.
+    /// @throw Dhcp4ConfigError if there are any issues encountered during commit
     void commit() {
 
         StringStorage::const_iterator it = string_values_.find("subnet");
@@ -549,7 +563,8 @@ protected:
     ///
     /// @param config_id name od the entry
     /// @return parser object for specified entry name
-    DhcpConfigParser* createSubnet4ConfigParser(const std::string& config_id) {
+    /// @throw NotImplemented if trying to create a parser for unknown config element
+    Dhcp4ConfigParser* createSubnet4ConfigParser(const std::string& config_id) {
         FactoryMap factories;
 
         factories.insert(pair<string, ParserFactory*>(
@@ -585,6 +600,7 @@ protected:
     ///
     /// @param name name of the parameter
     /// @return triplet with the parameter name
+    /// @throw Dhcp4ConfigError when requested parameter is not present
     Triplet<uint32_t> getParam(const std::string& name) {
         uint32_t value = 0;
         bool found = false;
@@ -627,7 +643,7 @@ protected:
 /// 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 DhcpConfigParser {
+class Subnets4ListConfigParser : public Dhcp4ConfigParser {
 public:
 
     /// @brief constructor
@@ -676,7 +692,7 @@ public:
     /// @brief Returns Subnet4ListConfigParser object
     /// @param param_name name of the parameter
     /// @return Subnets4ListConfigParser object
-    static DhcpConfigParser* Factory(const std::string& param_name) {
+    static Dhcp4ConfigParser* Factory(const std::string& param_name) {
         return (new Subnets4ListConfigParser(param_name));
     }
 
@@ -691,7 +707,8 @@ public:
 ///
 /// @param config_id pointer to received global configuration entry
 /// @return parser for specified global DHCPv4 parameter
-DhcpConfigParser* createGlobalDhcpConfigParser(const std::string& config_id) {
+/// @throw NotImplemented if trying to create a parser for unknown config element
+Dhcp4ConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
     FactoryMap factories;
 
     factories.insert(pair<string, ParserFactory*>(
@@ -723,25 +740,18 @@ DhcpConfigParser* createGlobalDhcpConfigParser(const std::string& config_id) {
 
 /// @brief configures DHCPv4 server
 ///
-/// This function is called every time a new configuration is received. The extra
-/// parameter is a reference to DHCPv4 server component. It is currently not used
-/// and CfgMgr::instance() is accessed instead.
-///
-/// This method does not throw. It catches all exceptions and returns them as
-/// 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)
-///
-/// @param config_set a new configuration for DHCPv4 server
-/// @return answer that contains result of reconfiguration
 isc::data::ConstElementPtr
 configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
     if (!config_set) {
-        isc_throw(Dhcp4ConfigError,
-                  "Null pointer is passed to configuration parser");
+        ConstElementPtr answer = isc::config::createAnswer(1,
+                                 string("Can't parse NULL config"));
+        return (answer);
     }
 
+    /// Let's wipe previous configuration defaults
+    uint32_defaults.clear();
+    string_defaults.clear();
+
     /// @todo: append most essential info here (like "2 new subnets configured")
     string config_details;
 
@@ -751,7 +761,7 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
     try {
         BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
 
-            ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first));
+            ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
             parser->build(config_pair.second);
             parsers.push_back(parser);
         }
diff --git a/src/bin/dhcp4/config_parser.h b/src/bin/dhcp4/config_parser.h
index efead34..2ee9cf6 100644
--- a/src/bin/dhcp4/config_parser.h
+++ b/src/bin/dhcp4/config_parser.h
@@ -12,9 +12,9 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <string>
 #include <exceptions/exceptions.h>
 #include <cc/data.h>
+#include <string>
 
 #ifndef DHCP4_CONFIG_PARSER_H
 #define DHCP4_CONFIG_PARSER_H
@@ -27,21 +27,34 @@ 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.
 class Dhcp4ConfigError : public isc::Exception {
 public:
 
-/// @brief constructor
-///
-/// @param file name of the file, where exception occurred
-/// @param line line of the file, where exception occurred
-/// @param what text description of the issue that caused exception
-Dhcp4ConfigError(const char* file, size_t line, const char* what) :
-    isc::Exception(file, line, what) {}
+    /// @brief constructor
+    ///
+    /// @param file name of the file, where exception occurred
+    /// @param line line of the file, where exception occurred
+    /// @param what text description of the issue that caused exception
+    Dhcp4ConfigError(const char* file, size_t line, const char* what)
+        : isc::Exception(file, line, what) {}
 };
 
-class DhcpConfigParser {
+/// @brief Base abstract class for all DHCPv4 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.
+/// @todo: Merge this class with DhcpConfigParser in src/bin/dhcp6
+class Dhcp4ConfigParser {
     ///
     /// \name Constructors and Destructor
     ///
@@ -50,17 +63,21 @@ class DhcpConfigParser {
     /// pure base class.
     //@{
 private:
-    DhcpConfigParser(const DhcpConfigParser& source);
-    DhcpConfigParser& operator=(const DhcpConfigParser& source);
+
+    // 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).
-    DhcpConfigParser() {}
+    Dhcp4ConfigParser() {}
 public:
     /// The destructor.
-    virtual ~DhcpConfigParser() {}
+    virtual ~Dhcp4ConfigParser() {}
     //@}
 
     /// \brief Prepare configuration value.
@@ -107,7 +124,7 @@ public:
 };
 
 /// @brief a pointer to configuration parser
-typedef boost::shared_ptr<DhcpConfigParser> ParserPtr;
+typedef boost::shared_ptr<Dhcp4ConfigParser> ParserPtr;
 
 /// @brief a collection of parsers
 ///
@@ -115,7 +132,7 @@ typedef boost::shared_ptr<DhcpConfigParser> ParserPtr;
 typedef std::vector<ParserPtr> ParserCollection;
 
 
-/// \brief Configure an \c Dhcpv4Srv object with a set of configuration values.
+/// \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.
@@ -126,22 +143,22 @@ typedef std::vector<ParserPtr> ParserCollection;
 ///
 /// 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 Dhcp4ConfigError.
-/// 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 Dhcp4ConfigError.
-/// 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.
+/// this function returns appropriate error code.
+///
+/// This function is called every time a new configuration is received. The extra
+/// parameter is a reference to DHCPv4 server component. It is currently not used
+/// and CfgMgr::instance() is accessed instead.
+///
+/// This method does not throw. It catches all exceptions and returns them as
+/// 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)
 ///
-/// \param server The \c Dhcpv4Srv object to be configured.
-/// \param config_set A JSON style configuration to apply to \c server.
+/// @param config_set a new configuration (JSON) for DHCPv4 server
+/// @return answer that contains result of reconfiguration
 isc::data::ConstElementPtr
-configureDhcp4Server(Dhcpv4Srv& server,
+configureDhcp4Server(Dhcpv4Srv&,
                      isc::data::ConstElementPtr config_set);
 
 }; // end of isc::dhcp namespace
diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
index 01067fa..c9366af 100644
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
@@ -14,9 +14,6 @@
 
 #include <config.h>
 
-#include <cassert>
-#include <iostream>
-
 #include <asiolink/asiolink.h>
 #include <cc/data.h>
 #include <cc/session.h>
@@ -29,6 +26,8 @@
 #include <dhcp/iface_mgr.h>
 #include <exceptions/exceptions.h>
 #include <util/buffer.h>
+#include <cassert>
+#include <iostream>
 
 using namespace isc::asiolink;
 using namespace isc::cc;
@@ -126,7 +125,7 @@ void ControlledDhcpv4Srv::establishSession() {
 
     /// Integrate the asynchronous I/O model of BIND 10 configuration
     /// control with the "select" model of the DHCP server.  This is
-    /// fully explained in \ref dhcpv4Session.
+    /// fully explained in \ref dhcp4-session.
     int ctrl_socket = cc_session_->getSocketDesc();
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_CCSESSION_STARTED)
               .arg(ctrl_socket);
diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h
index 688b1db..76c5d6d 100644
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.h
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h
@@ -34,7 +34,7 @@ namespace dhcp {
 /// embedded environments.
 ///
 /// For detailed explanation or relations between main(), ControlledDhcpv4Srv,
-/// Dhcpv4Srv and other classes, see \ref dhcpv4Session.
+/// Dhcpv4Srv and other classes, see \ref dhcp4-session.
 class ControlledDhcpv4Srv : public isc::dhcp::Dhcpv4Srv {
 public:
 
@@ -66,7 +66,7 @@ public:
 
     /// @brief Session callback, processes received commands.
     ///
-    /// @param command_id text represenation of the command (e.g. "shutdown")
+    /// @param command text represenation of the command (e.g. "shutdown")
     /// @param args optional parameters
     ///
     /// @return status of the command
diff --git a/src/bin/dhcp4/dhcp4.dox b/src/bin/dhcp4/dhcp4.dox
index 5dacf63..54f2bd3 100644
--- a/src/bin/dhcp4/dhcp4.dox
+++ b/src/bin/dhcp4/dhcp4.dox
@@ -15,8 +15,6 @@ DHCPv4 server component does not support direct traffic (relayed
 only), as support for transmission to hosts without IPv4 address
 assigned is not implemented in IfaceMgr yet.
 
-DHCPv4 server component does not use BIND10 logging yet.
-
 @section dhcp4-session BIND10 message queue integration
 
 DHCPv4 server component is now integrated with BIND10 message queue.
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 67943c5..aa79ce9 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -17,8 +17,8 @@
 #include <dhcp/iface_mgr.h>
 #include <dhcp4/dhcp4_srv.h>
 #include <dhcp4/dhcp4_log.h>
-#include <asiolink/io_address.h>
 #include <dhcp/option4_addrlst.h>
+#include <asiolink/io_address.h>
 
 using namespace isc;
 using namespace isc::asiolink;
diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index f677259..10470e4 100644
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/src/bin/dhcp4/dhcp4_srv.h
@@ -15,10 +15,10 @@
 #ifndef DHCPV4_SRV_H
 #define DHCPV4_SRV_H
 
-#include <boost/noncopyable.hpp>
 #include <dhcp/dhcp4.h>
 #include <dhcp/pkt4.h>
 #include <dhcp/option.h>
+#include <boost/noncopyable.hpp>
 #include <iostream>
 
 namespace isc {
@@ -34,11 +34,11 @@ namespace dhcp {
 /// appropriate responses.
 ///
 /// This class does not support any controlling mechanisms directly.
-/// See derived \ref ControlledDhcv4Srv class for support for
+/// See derived \ref ControlledDhcpv4Srv class for support for
 /// command and configuration updates over msgq.
 ///
 /// For detailed explanation or relations between main(), ControlledDhcpv4Srv,
-/// Dhcpv4Srv and other classes, see \ref dhcpv4Session.
+/// Dhcpv4Srv and other classes, see \ref dhcp4-session.
 class Dhcpv4Srv : public boost::noncopyable {
 
     public:
diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc
index 8a8873f..ea7048b 100644
--- a/src/bin/dhcp4/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp4/tests/config_parser_unittest.cc
@@ -13,9 +13,6 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <config.h>
-#include <iostream>
-#include <fstream>
-#include <sstream>
 
 #include <arpa/inet.h>
 #include <gtest/gtest.h>
@@ -25,6 +22,10 @@
 #include <config/ccsession.h>
 #include <dhcp/subnet.h>
 #include <dhcp/cfgmgr.h>
+#include <iostream>
+#include <fstream>
+#include <sstream>
+#include <limits.h>
 
 using namespace std;
 using namespace isc;
@@ -33,6 +34,12 @@ 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 {
@@ -45,6 +52,26 @@ public:
         srv_ = new Dhcpv4Srv(0);
     }
 
+    // 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);
+        if (it == uint32_defaults.end()) {
+            ADD_FAILURE() << "Expected uint32 with name " << name
+                          << " not found";
+            return;
+        }
+        EXPECT_EQ(expected_value, it->second);
+    }
+
+    // Checks if config_result (result of DHCP server configuration) has
+    // expected code (0 for success, other for failures).
+    // Also stores result in rcode_ and comment_.
+    void checkResult(ConstElementPtr status, int expected_code) {
+        ASSERT_TRUE(status);
+        comment_ = parseAnswer(rcode_, status);
+        EXPECT_EQ(expected_code, rcode_);
+    }
+
     ~Dhcp4ParserTest() {
         delete srv_;
     };
@@ -66,9 +93,7 @@ TEST_F(Dhcp4ParserTest, version) {
                     Element::fromJSON("{\"version\": 0}")));
 
     // returned value must be 0 (configuration accepted)
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    checkResult(x, 0);
 }
 
 /// The goal of this test is to verify that the code accepts only
@@ -81,15 +106,13 @@ TEST_F(Dhcp4ParserTest, bogus_command) {
                     Element::fromJSON("{\"bogus\": 5}")));
 
     // returned value must be 1 (configuration parse error)
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(1, rcode_);
+    checkResult(x, 1);
 }
 
 /// The goal of this test is to verify if wrongly defined subnet will
 /// be rejected. Properly defined subnet must include at least one
 /// pool definition.
-TEST_F(Dhcp4ParserTest, empty_subnet) {
+TEST_F(Dhcp4ParserTest, emptySubnet) {
 
     ConstElementPtr status;
 
@@ -101,9 +124,11 @@ TEST_F(Dhcp4ParserTest, empty_subnet) {
                                       "\"valid-lifetime\": 4000 }")));
 
     // returned value should be 0 (success)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
+
+    checkGlobalUint32("rebind-timer", 2000);
+    checkGlobalUint32("renew-timer", 1000);
+    checkGlobalUint32("valid-lifetime", 4000);
 }
 
 /// The goal of this test is to verify if defined subnet uses global
@@ -126,9 +151,7 @@ TEST_F(Dhcp4ParserTest, subnet_global_defaults) {
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
 
     // check if returned status is OK
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     // Now check if the configuration was indeed handled and we have
     // expected pool configured.
@@ -141,7 +164,7 @@ TEST_F(Dhcp4ParserTest, subnet_global_defaults) {
 
 // This test checks if it is possible to override global values
 // on a per subnet basis.
-TEST_F(Dhcp4ParserTest, subnet_local) {
+TEST_F(Dhcp4ParserTest, subnetLocal) {
 
     ConstElementPtr status;
 
@@ -162,9 +185,7 @@ TEST_F(Dhcp4ParserTest, subnet_local) {
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
 
     // returned value should be 0 (configuration success)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"));
     ASSERT_TRUE(subnet);
@@ -175,7 +196,7 @@ TEST_F(Dhcp4ParserTest, subnet_local) {
 
 // Test verifies that a subnet with pool values that do not belong to that
 // pool are rejected.
-TEST_F(Dhcp4ParserTest, pool_out_of_subnet) {
+TEST_F(Dhcp4ParserTest, poolOutOfSubnet) {
 
     ConstElementPtr status;
 
@@ -194,17 +215,15 @@ TEST_F(Dhcp4ParserTest, pool_out_of_subnet) {
 
     // returned value must be 2 (values error)
     // as the pool does not belong to that subnet
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(2, rcode_);
+    checkResult(status, 2);
 }
 
 // Goal of this test is to verify if pools can be defined
 // using prefix/length notation. There is no separate test for min-max
 // notation as it was tested in several previous tests.
-TEST_F(Dhcp4ParserTest, pool_prefix_len) {
+TEST_F(Dhcp4ParserTest, poolPrefixLen) {
 
-    ConstElementPtr x;
+    ConstElementPtr status;
 
     string config = "{ \"interface\": [ \"all\" ],"
         "\"rebind-timer\": 2000, "
@@ -217,12 +236,10 @@ TEST_F(Dhcp4ParserTest, pool_prefix_len) {
 
     ElementPtr json = Element::fromJSON(config);
 
-    EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
 
-    // returned value must be 1 (configuration parse error)
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    // returned value must be 0 (configuration accepted)
+    checkResult(status, 0);
 
     Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"));
     ASSERT_TRUE(subnet);
@@ -231,4 +248,47 @@ TEST_F(Dhcp4ParserTest, pool_prefix_len) {
     EXPECT_EQ(4000, subnet->getValid());
 }
 
+/// 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.
+TEST_F(Dhcp4ParserTest, Uint32Parser) {
+
+    ConstElementPtr status;
+
+    // CASE 1: 0 - minimum value, should work
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_,
+                    Element::fromJSON("{\"version\": 0,"
+                                      "\"renew-timer\": 0}")));
+
+    // returned value must be ok (0 is a proper value)
+    checkResult(status, 0);
+    checkGlobalUint32("renew-timer", 0);
+
+    // CASE 2: 4294967295U (UINT_MAX) should work as well
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_,
+                    Element::fromJSON("{\"version\": 0,"
+                                      "\"renew-timer\": 4294967295}")));
+
+    // returned value must be ok (0 is a proper value)
+    checkResult(status, 0);
+    checkGlobalUint32("renew-timer", 4294967295U);
+
+    // CASE 3: 4294967296U (UINT_MAX + 1) should not work
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_,
+                    Element::fromJSON("{\"version\": 0,"
+                                      "\"renew-timer\": 4294967296}")));
+
+    // returned value must be rejected (1 configuration error)
+    checkResult(status, 1);
+
+    // CASE 4: -1 (UINT_MIN -1 ) should not work
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_,
+                    Element::fromJSON("{\"version\": 0,"
+                                      "\"renew-timer\": -1}")));
+
+    // returned value must be rejected (1 configuration error)
+    checkResult(status, 1);
+}
+
 };
diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc
index 91310f0..a8e80e0 100644
--- a/src/bin/dhcp6/config_parser.cc
+++ b/src/bin/dhcp6/config_parser.cc
@@ -116,7 +116,7 @@ public:
         return (new DebugParser(param_name));
     }
 
-protected:
+private:
     /// name of the parsed parameter
     std::string param_name_;
 
@@ -134,7 +134,7 @@ protected:
 /// in its base class, \ref DhcpConfigParser.
 ///
 /// For overview of usability of this generic purpose parser, see
-/// \ref dhcpv6-config-inherit page.
+/// \ref dhcp6-config-inherit page.
 class Uint32Parser : public DhcpConfigParser {
 public:
 
@@ -147,7 +147,7 @@ public:
     /// @brief builds parameter value
     ///
     /// Parses configuration entry and stores it in a storage. See
-    /// \ref setStorage() for details.
+    /// \ref Uint32Parser::setStorage() for details.
     ///
     /// @param value pointer to the content of parsed values
     virtual void build(ConstElementPtr value) {
@@ -180,14 +180,14 @@ public:
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv6-config-inherit for details.
+    /// See \ref dhcp6-config-inherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(Uint32Storage* storage) {
         storage_ = storage;
     }
 
-protected:
+private:
     /// pointer to the storage, where parsed value will be stored
     Uint32Storage* storage_;
 
@@ -208,7 +208,7 @@ protected:
 /// in its base class, \ref DhcpConfigParser.
 ///
 /// For overview of usability of this generic purpose parser, see
-/// \ref dhcpv6-config-inherit page.
+/// \ref dhcp6-config-inherit page.
 class StringParser : public DhcpConfigParser {
 public:
 
@@ -250,14 +250,14 @@ public:
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv6-config-inherit for details.
+    /// See \ref dhcp6-config-inherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(StringStorage* storage) {
         storage_ = storage;
     }
 
-protected:
+private:
     /// pointer to the storage, where parsed value will be stored
     StringStorage* storage_;
 
@@ -286,9 +286,10 @@ public:
     /// "interface" parameter only. All other types will throw exception.
     ///
     /// @param param_name name of the configuration parameter being parsed
+    /// @throw BadValue if supplied parameter name is not "interface"
     InterfaceListConfigParser(const std::string& param_name) {
         if (param_name != "interface") {
-            isc_throw(NotImplemented, "Internal error. Interface configuration "
+            isc_throw(BadValue, "Internal error. Interface configuration "
                       "parser called for the wrong parameter: " << param_name);
         }
     }
@@ -318,7 +319,7 @@ public:
         return (new InterfaceListConfigParser(param_name));
     }
 
-protected:
+private:
     /// contains list of network interfaces
     vector<string> interfaces_;
 };
@@ -347,6 +348,8 @@ public:
     /// This method parses the actual list of interfaces.
     /// No validation is done at this stage, everything is interpreted as
     /// interface name.
+    /// @param pools_list list of pools defined for a subnet
+    /// @throw BadValue if storage was not specified (setStorage() not called)
     void build(ConstElementPtr pools_list) {
         // setStorage() should have been called before build
         if (!pools_) {
@@ -416,7 +419,7 @@ public:
 
     /// @brief sets storage for value of this parameter
     ///
-    /// See \ref dhcpv6-config-inherit for details.
+    /// See \ref dhcp6-config-inherit for details.
     ///
     /// @param storage pointer to the storage container
     void setStorage(PoolStorage* storage) {
@@ -437,7 +440,7 @@ public:
         return (new PoolParser(param_name));
     }
 
-protected:
+private:
     /// @brief pointer to the actual Pools storage
     ///
     /// That is typically a storage somewhere in Subnet parser
@@ -543,7 +546,7 @@ public:
         CfgMgr::instance().addSubnet6(subnet);
     }
 
-protected:
+private:
 
     /// @brief creates parsers for entries in subnet definition
     ///
@@ -551,6 +554,7 @@ protected:
     ///
     /// @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
     DhcpConfigParser* createSubnet6ConfigParser(const std::string& config_id) {
         FactoryMap factories;
 
@@ -589,6 +593,7 @@ protected:
     ///
     /// @param name name of the parameter
     /// @return triplet with the parameter name
+    /// @throw Dhcp6ConfigError when requested parameter is not present
     Triplet<uint32_t> getParam(const std::string& name) {
         uint32_t value = 0;
         bool found = false;
@@ -695,6 +700,7 @@ public:
 ///
 /// @param config_id pointer to received global configuration entry
 /// @return parser for specified global DHCPv6 parameter
+/// @throw NotImplemented if trying to create a parser for unknown config element
 DhcpConfigParser* createGlobalDhcpConfigParser(const std::string& config_id) {
     FactoryMap factories;
 
@@ -742,13 +748,19 @@ DhcpConfigParser* createGlobalDhcpConfigParser(const std::string& config_id) {
 ///
 /// @param config_set a new configuration for DHCPv6 server
 /// @return answer that contains result of reconfiguration
+/// @throw Dhcp6ConfigError if trying to create a parser for NULL config
 ConstElementPtr
 configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
     if (!config_set) {
-        isc_throw(Dhcp6ConfigError,
-                  "Null pointer is passed to configuration parser");
+        ConstElementPtr answer = isc::config::createAnswer(1,
+                                 string("Can't parse NULL config"));
+        return (answer);
     }
 
+    /// Let's wipe previous configuration defaults
+    uint32_defaults.clear();
+    string_defaults.clear();
+
     /// @todo: append most essential info here (like "2 new subnets configured")
     string config_details;
 
diff --git a/src/bin/dhcp6/config_parser.h b/src/bin/dhcp6/config_parser.h
index 654e41f..f8fe76c 100644
--- a/src/bin/dhcp6/config_parser.h
+++ b/src/bin/dhcp6/config_parser.h
@@ -32,15 +32,22 @@ class Dhcpv6Srv;
 class Dhcp6ConfigError : public isc::Exception {
 public:
 
-/// @brief constructor
-///
-/// @param file name of the file, where exception occurred
-/// @param line line of the file, where exception occurred
-/// @param what text description of the issue that caused exception
-Dhcp6ConfigError(const char* file, size_t line, const char* what) :
-    isc::Exception(file, line, what) {}
+    /// @brief constructor
+    ///
+    /// @param file name of the file, where exception occurred
+    /// @param line line of the file, where exception occurred
+    /// @param what text description of the issue that caused exception
+    Dhcp6ConfigError(const char* file, size_t line, const char* what)
+        : isc::Exception(file, line, what) {}
 };
 
+/// @brief Base abstract class for all DHCPv6 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.
+/// @todo: Merge this class with Dhcp4ConfigParser in src/bin/dhcp4
 class DhcpConfigParser {
     ///
     /// \name Constructors and Destructor



More information about the bind10-changes mailing list