[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

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);
         }

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

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);

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

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.

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;

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:

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);
+}
+
 };