[5097] Addressed not unit test comments

src/bin/dhcp4/json_config_parser.cc

@@ -54,14 +54,6 @@ namespace {
 ///
 /// It is useful for parsing Dhcp4/subnet4[X]/pool parameters.
 class Pool4Parser : public PoolParser {
-public:
-
-    /// @brief Constructor.
-    ///
-    /// @param pools storage container in which to store the parsed pool.
-    Pool4Parser(PoolStoragePtr pools) : PoolParser(pools) {
-    }
-
 protected:
     /// @brief Creates a Pool4 object given a IPv4 prefix and the prefix length.
     ///
@@ -89,22 +81,19 @@ protected:
 /// @brief Specialization of the pool list parser for DHCPv4
 class Pools4ListParser : PoolsListParser {
 public:
-    /// @brief Constructor.
-    ///
-    /// @param pools storage container in which to store the parsed pool.
-    Pools4ListParser(PoolStoragePtr pools) : PoolsListParser(pools) {
-    }
 
     /// @brief parses the actual structure
     ///
     /// This method parses the actual list of pools.
     ///
+    /// @param pools storage container in which to store the parsed pool.
     /// @param pools_list a list of pool structures
     /// @throw isc::dhcp::DhcpConfigError when pool parsing fails
-    void parse(isc::data::ConstElementPtr pools_list) {
+    void parse(PoolStoragePtr pools,
+               isc::data::ConstElementPtr pools_list) {
         BOOST_FOREACH(ConstElementPtr pool, pools_list->listValue()) {
-            Pool4Parser parser(pools_);
-            parser.parse(pool, AF_INET);
+            Pool4Parser parser;
+            parser.parse(pools, pool, AF_INET);
         }
     }
 };
@@ -133,8 +122,8 @@ public:
         /// Parse Pools first.
         ConstElementPtr pools = subnet->get("pools");
         if (pools) {
-            Pools4ListParser parser(pools_);
-            parser.parse(pools);
+            Pools4ListParser parser;
+            parser.parse(pools_, pools);
         }
 
         SubnetConfigParser::build(subnet);

src/bin/dhcp6/json_config_parser.cc

@@ -71,14 +71,6 @@ typedef boost::shared_ptr<Uint32Parser> Uint32ParserPtr;
 ///
 /// It is useful for parsing Dhcp6/subnet6[X]/pool parameters.
 class Pool6Parser : public PoolParser {
-public:
-
-    /// @brief Constructor.
-    ///
-    /// @param pools storage container in which to store the parsed pool.
-    Pool6Parser(PoolStoragePtr pools) : PoolParser(pools) {
-    }
-
 protected:
     /// @brief Creates a Pool6 object given a IPv6 prefix and the prefix length.
     ///
@@ -112,22 +104,19 @@ protected:
 /// @brief Specialization of the pool list parser for DHCPv6
 class Pools6ListParser : PoolsListParser {
 public:
-    /// @brief Constructor.
-    ///
-    /// @param pools storage container in which to store the parsed pool.
-    Pools6ListParser(PoolStoragePtr pools) : PoolsListParser(pools) {
-    }
 
     /// @brief parses the actual structure
     ///
     /// This method parses the actual list of pools.
     ///
+    /// @param pools storage container in which to store the parsed pool.
     /// @param pools_list a list of pool structures
     /// @throw isc::dhcp::DhcpConfigError when pool parsing fails
-    void parse(isc::data::ConstElementPtr pools_list) {
+    void parse(PoolStoragePtr pools,
+               isc::data::ConstElementPtr pools_list) {
         BOOST_FOREACH(ConstElementPtr pool, pools_list->listValue()) {
-            Pool6Parser parser(pools_);
-            parser.parse(pool, AF_INET6);
+            Pool6Parser parser;
+            parser.parse(pools, pool, AF_INET6);
         }
     }
 };
@@ -154,9 +143,7 @@ public:
 
     /// @brief Constructor.
     ///
-    /// @param pools storage container in which to store the parsed pool.
-    PdPoolParser(PoolStoragePtr pools)
-        : pools_(pools), options_(new CfgOption()) {
+    PdPoolParser() : options_(new CfgOption()) {
     }
 
     /// @brief Builds a prefix delegation pool from the given configuration
@@ -164,11 +151,13 @@ public:
     /// This function parses configuration entries and creates an instance
     /// of a dhcp::Pool6 configured for prefix delegation.
     ///
+    /// @param pools storage container in which to store the parsed pool.
     /// @param pd_pool_ pointer to an element that holds configuration entries
     /// that define a prefix delegation pool.
     ///
     /// @throw DhcpConfigError if configuration parsing fails.
-    void parse(ConstElementPtr pd_pool_) {
+  void parse(PoolStoragePtr pools,
+             ConstElementPtr pd_pool_) {
         std::string addr_str;
         std::string excluded_prefix_str = "::";
         uint8_t prefix_len = 0;
@@ -235,7 +224,7 @@ public:
         }
 
         // Add the local pool to the external storage ptr.
-        pools_->push_back(pool_);
+        pools->push_back(pool_);
     }
 
 private:
@@ -261,23 +250,12 @@ private:
     /// @throw isc::data::TypeError when it is not an integer
     /// isc::dhcp::DhcpConfigError when it does not fit in an uint8_t
     uint8_t getUint8(const std::string& name, ConstElementPtr value) const {
-        int64_t val_int = value->intValue();
-        if ((val_int < std::numeric_limits<uint8_t>::min()) ||
-            (val_int > std::numeric_limits<uint8_t>::max())) {
-            isc_throw(isc::dhcp::DhcpConfigError,
-                      "out of range value (" << val_int
-                      << ") specified for parameter '"
-                      << name << "' (" << value->getPosition() << ")");
-        }
-        return (static_cast<uint8_t>(val_int));
+        return (extractInt<uint8_t, DhcpConfigError>(name, value));
     }
 
     /// Pointer to the created pool object.
     isc::dhcp::Pool6Ptr pool_;
 
-    /// Pointer to storage to which the local pool is written upon commit.
-    isc::dhcp::PoolStoragePtr pools_;
-
     /// A storage for pool specific option values.
     CfgOptionPtr options_;
 
@@ -289,29 +267,25 @@ private:
 /// This parser iterates over a list of prefix delegation pool entries and
 /// creates pool instances for each one. If the parsing is successful, the
 /// collection of pools is committed to the provided storage.
-class PdPoolListParser : public PoolsListParser {
+class PdPoolsListParser : public PoolsListParser {
 public:
-    /// @brief Constructor.
-    ///
-    /// @param storage is the pool storage in which to store the parsed
-    /// pools in this list
-    PdPoolListParser(PoolStoragePtr pools) : PoolsListParser(pools) {
-    }
 
     /// @brief Parse configuration entries.
     ///
     /// This function parses configuration entries and creates instances
     /// of prefix delegation pools .
     ///
+    /// @param storage is the pool storage in which to store the parsed
     /// @param pd_pool_list pointer to an element that holds entries
     /// that define a prefix delegation pool.
     ///
     /// @throw DhcpConfigError if configuration parsing fails.
-    void parse(isc::data::ConstElementPtr pd_pool_list) {
+    void parse(PoolStoragePtr pools,
+               isc::data::ConstElementPtr pd_pool_list) {
         // Loop through the list of pd pools.
         BOOST_FOREACH(ConstElementPtr pd_pool, pd_pool_list->listValue()) {
-            PdPoolParser parser(pools_);
-            parser.parse(pd_pool);
+            PdPoolParser parser;
+            parser.parse(pools, pd_pool);
         }
     }
 };
@@ -341,13 +315,13 @@ public:
         /// Parse all pools first.
         ConstElementPtr pools = subnet->get("pools");
         if (pools) {
-            Pools6ListParser parser(pools_);
-            parser.parse(pools);
+            Pools6ListParser parser;
+            parser.parse(pools_, pools);
         }
         ConstElementPtr pd_pools = subnet->get("pd-pools");
         if (pd_pools) {
-            PdPoolListParser parser(pools_);
-            parser.parse(pd_pools);
+            PdPoolsListParser parser;
+            parser.parse(pools_, pd_pools);
         }
 
         SubnetConfigParser::build(subnet);

src/lib/cc/simple_parser.h

@@ -147,6 +147,28 @@ class SimpleParser {
     /// @return a boolean value of the parameter
     static bool getBoolean(isc::data::ConstElementPtr scope,
                            const std::string& name);
+
+    /// @brief Returns an integer value with range checking
+    ///
+    /// This template should be instantied in parsers when useful
+    ///
+    /// @tparam int_type the integer type e.g. uint32_t
+    /// @tparam out_of_range always @c isc::dhcp::DhcpConfigError
+    /// @param name name of the parameter for error report
+    /// @param value value of the parameter
+    /// @throw isc::data::TypeError when the value is not an integer
+    /// @throw out_of_range when the value does not fit in int_type
+    template <typename int_type, class out_of_range> int_type
+    extractInt(const std::string& name, ConstElementPtr value) const {
+        int64_t val_int = value->intValue();
+        if ((val_int < std::numeric_limits<int_type>::min()) ||
+            (val_int > std::numeric_limits<int_type>::max())) {
+            isc_throw(out_of_range, "out of range value (" << val_int
+                  << ") specified for parameter '" << name
+                      << "' (" << value->getPosition() << ")");
+        }
+        return (static_cast<int_type>(val_int));
+    }
 };
 
 };

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -853,14 +853,10 @@ RelayInfoParser::parse(const isc::dhcp::Subnet::RelayInfoPtr& cfg,
 }
 
 //****************************** PoolParser ********************************
-PoolParser::PoolParser(PoolStoragePtr pools) : pools_(pools) {
-}
-
-PoolParser::~PoolParser() {
-}
 
 void
-PoolParser::parse(ConstElementPtr pool_structure,
+PoolParser::parse(PoolStoragePtr pools,
+                  ConstElementPtr pool_structure,
                   const uint16_t address_family) {
 
     ConstElementPtr text_pool = pool_structure->get("pool");
@@ -899,10 +895,13 @@ PoolParser::parse(ConstElementPtr pool_structure,
             // digits (because there are extra characters left over).
 
             // No checks for values over 128. Range correctness will
-            // be checked in Pool4 constructor.
+            // be checked in Pool4 constructor, here we only check
+            // the representation fits in an uint8_t as this can't
+            // be done by a direct lexical cast as explained...
             int val_len = boost::lexical_cast<int>(prefix_len);
             if ((val_len < std::numeric_limits<uint8_t>::min()) ||
                 (val_len > std::numeric_limits<uint8_t>::max())) {
+                // This exception will be handled 4 line later!
                 isc_throw(OutOfRange, "");
             }
             len = static_cast<uint8_t>(val_len);
@@ -914,7 +913,7 @@ PoolParser::parse(ConstElementPtr pool_structure,
 
         try {
             pool = poolMaker(addr, len);
-            pools_->push_back(pool);
+            pools->push_back(pool);
         } catch (const std::exception& ex) {
             isc_throw(DhcpConfigError, "Failed to create pool defined by: "
                       << txt << " (" << text_pool->getPosition() << ")");
@@ -939,7 +938,7 @@ PoolParser::parse(ConstElementPtr pool_structure,
 
             try {
                 pool = poolMaker(min, max);
-                pools_->push_back(pool);
+                pools->push_back(pool);
             } catch (const std::exception& ex) {
                 isc_throw(DhcpConfigError, "Failed to create pool defined by: "
                           << txt << " (" << text_pool->getPosition() << ")");

src/lib/dhcpsrv/parsers/dhcp_parsers.h

@@ -706,23 +706,21 @@ typedef boost::shared_ptr<PoolStorage> PoolStoragePtr;
 class PoolParser : public isc::data::SimpleParser {
 public:
 
-    /// @brief constructor.
-    ///
-    /// @param pools is the storage in which to store the parsed pool
-    PoolParser(PoolStoragePtr pools);
-
     /// @brief destructor.
-    virtual ~PoolParser();
+    virtual ~PoolParser() {
+    }
 
     /// @brief parses the actual structure
     ///
     /// This method parses the actual list of interfaces.
     /// No validation is done at this stage, everything is interpreted as
     /// interface name.
+    /// @param pools is the storage in which to store the parsed pool
     /// @param pool_structure a single entry on a list of pools
     /// @param address_family AF_INET (for DHCPv4) or AF_INET6 (for DHCPv6).
     /// @throw isc::dhcp::DhcpConfigError when pool parsing fails
-    virtual void parse(isc::data::ConstElementPtr pool_structure,
+    virtual void parse(PoolStoragePtr pools,
+                       isc::data::ConstElementPtr pool_structure,
                        const uint16_t address_family);
 
 protected:
@@ -744,12 +742,6 @@ protected:
     virtual PoolPtr poolMaker(isc::asiolink::IOAddress &min,
                               isc::asiolink::IOAddress &max,
                               int32_t ptype = 0) = 0;
-
-    /// @brief pointer to the actual Pools storage
-    ///
-    /// That is typically a storage somewhere in Subnet parser
-    /// (an upper level parser).
-    PoolStoragePtr pools_;
 };
 
 /// @brief Parser for a list of pools
@@ -760,12 +752,6 @@ protected:
 class PoolsListParser : public isc::data::SimpleParser {
 public:
 
-    /// @brief constructor.
-    ///
-    /// @param pools is the storage in which to store the parsed pools.
-    PoolsListParser(PoolStoragePtr pools) : pools_(pools) {
-    }
-
     /// @brief destructor.
     virtual ~PoolsListParser() {
     }
@@ -774,17 +760,11 @@ public:
     ///
     /// This method parses the actual list of pools.
     ///
+    /// @param pools is the storage in which to store the parsed pools.
     /// @param pools_list a list of pool structures
     /// @throw isc::dhcp::DhcpConfigError when pool parsing fails
-    virtual void parse(isc::data::ConstElementPtr pools_list) = 0;
-
-protected:
-
-    /// @brief pointer to the actual Pools storage
-    ///
-    /// That is typically a storage somewhere in Subnet parser
-    /// (an upper level parser).
-    PoolStoragePtr pools_;
+    virtual void parse(PoolStoragePtr pools,
+                       isc::data::ConstElementPtr pools_list) = 0;
 };
 
 /// @brief parser for additional relay information