[3432] Addressed review comments

Added missing commentary, corrected typos.

doc/guide/bind10-guide.xml

@@ -5224,8 +5224,8 @@ Dhcp6/renew-timer	1000	integer	(default)
 DhcpDdns/ip_address "127.0.0.1" string  (default)
 DhcpDdns/port   53001   integer (default)
 DhcpDdns/dns_server_timeout   100   integer (default)
-DhcpDdns/ncr-protocol	"UDP"	string (default)
-DhcpDdns/ncr-format	"JSON"	string (default)
+DhcpDdns/ncr_protocol	"UDP"	string (default)
+DhcpDdns/ncr_format	"JSON"	string (default)
 DhcpDdns/tsig_keys  []  list    (default)
 DhcpDdns/forward_ddns/ddns_domains  []  list    (default)
 DhcpDdns/reverse_ddns/ddns_domains  []  list    (default)
@@ -5278,17 +5278,18 @@ DhcpDdns/reverse_ddns/ddns_domains  []  list    (default)
       is 53001.
       </para></listitem>
       <listitem><para>
-      ncr-format - Socket protocol use when sending requests to D2.  Currently
-      only UDP is supported.  TCP may be available in an upcoming release.
+      ncr_format - Socket protocol to use when sending requests to D2.
+      Currently only UDP is supported.  TCP may be available in an upcoming
+      release.
       </para></listitem>
       <listitem><para>
-      ncr-protocol - Packet format to use when sending requests to D2.
+      ncr_protocol - Packet format to use when sending requests to D2.
       Currently only JSON format is supported.  Other formats may be available
       in future releases.
       </para></listitem>
       <listitem><para>
       dns_server_timeout - The maximum amount of time in milliseconds, that
-      D2 will wait for a response from a DNS server to a single DDNS update
+      D2 will wait for a response from a DNS server to a single DNS update
       message.
       </para></listitem>
       </orderedlist>
@@ -5302,8 +5303,6 @@ DhcpDdns/reverse_ddns/ddns_domains  []  list    (default)
 &gt; <userinput>config set DhcpDdns/port 900</userinput>
 &gt; <userinput>config commit</userinput>
 </screen>
-        The server may be configured to listen over IPv4 or IPv6, therefore
-        ip-address may an IPv4 or IPv6 address.
         </para>
         <warning>
           <simpara>

src/bin/d2/d2_cfg_mgr.h

@@ -270,6 +270,15 @@ protected:
     virtual isc::dhcp::ParserPtr
     createConfigParser(const std::string& element_id);
 
+    /// @brief Creates an new, blank D2CfgContext context
+    ///
+    /// This method is used at the beginning of configuration process to
+    /// create a fresh, empty copy of a D2CfgContext. This new context will
+    /// be populated during the configuration process and will replace the
+    /// existing context provided the configuration process completes without
+    /// error.
+    ///
+    /// @return Returns a DCfgContextBasePtr to the new context instance.
     virtual DCfgContextBasePtr createNewContext();
 };
 

src/bin/d2/d2_config.cc

@@ -61,12 +61,9 @@ D2Params::~D2Params(){};
 
 void
 D2Params::validateContents() {
-    if (ip_address_.toText() == "0.0.0.0") {
-        isc_throw(D2CfgError, "D2Params: IP address cannot be \"0.0.0.0\"");
-    }
-
-    if (ip_address_.toText() == "::") {
-        isc_throw(D2CfgError, "D2Params: IP address cannot be \"::\"");
+    if ((ip_address_.toText() == "0.0.0.0") || (ip_address_.toText() == "::")) {
+        isc_throw(D2CfgError,
+                  "D2Params: IP address cannot be \"" << ip_address_ << "\"");
     }
 
     if (port_ == 0) {

src/bin/d2/d2_config.h

@@ -147,6 +147,7 @@ public:
 class D2Params {
 public:
     /// @brief Default configuration constants.
+    //@{
     /// @todo For now these are hard-coded as configuration layer cannot
     /// readily provide them (see Trac #3358).
     static const char *DFT_IP_ADDRESS;
@@ -154,13 +155,26 @@ public:
     static const size_t DFT_DNS_SERVER_TIMEOUT;
     static const char *DFT_NCR_PROTOCOL;
     static const char *DFT_NCR_FORMAT;
+    //@}
 
     /// @brief Constructor
     ///
-    /// @throw D2CfgError if given an invalid protocol or format.
+    /// @param ip_address IP address at which D2 should listen for NCRs
+    /// @param port port on which D2 should listen NCRs
+    /// @param dns_server_timeout  maximum amount of time in milliseconds to
+    /// wait for a response to a single DNS update request.
+    /// @param ncr_protocol socket protocol D2 should use to receive NCRS
+    /// @param ncr_format packet format of the inbound NCRs
+    ///
+    /// @throw D2CfgError if:
+    /// -# ip_address is 0.0.0.0 or ::
+    /// -# port is 0
+    /// -# dns_server_timeout is < 1
+    /// -# ncr_protocol is invalid, currently only NCR_UDP is supported
+    /// -# ncr_format is invalid, currently only FMT_JSON is supported
     D2Params(const isc::asiolink::IOAddress& ip_address,
                    const size_t port,
-                   const size_t DFT_DNS_SERVER_TIMEOUT,
+                   const size_t dns_server_timeout,
                    const dhcp_ddns::NameChangeProtocol& ncr_protocol,
                    const dhcp_ddns::NameChangeFormat& ncr_format);
 
@@ -171,12 +185,12 @@ public:
     /// @brief Destructor
     virtual ~D2Params();
 
-    /// @brief Return the IP D2 listens on.
+    /// @brief Return the IP address D2 listens on.
     const isc::asiolink::IOAddress& getIpAddress() const {
         return(ip_address_);
     }
 
-    /// @brief Return the IP port D2 listens on.
+    /// @brief Return the TCP/UPD port D2 listens on.
     size_t getPort() const {
         return(port_);
     }
@@ -238,6 +252,10 @@ private:
     dhcp_ddns::NameChangeFormat ncr_format_;
 };
 
+/// @brief Dumps the contents of a D2Params as text to an output stream
+///
+/// @param os output stream to which text should be sent
+/// @param config D2Param instnace to dump
 std::ostream&
 operator<<(std::ostream& os, const D2Params& config);
 

src/bin/d2/d_cfg_mgr.cc

@@ -135,7 +135,6 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
     // inconsistency if the parsing operation fails after the context has been
     // modified. We need to preserve the original context here
     // so as we can rollback changes when an error occurs.
-//    DCfgContextBasePtr original_context = context_->clone();
     DCfgContextBasePtr original_context = context_;
     resetContext();
 
@@ -178,7 +177,6 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
         buildParams(params_config);
 
         // Now parse the configuration objects.
-        const ElementMap& values_map = objects_map;
 
         // Use a pre-ordered list of element ids to parse the elements in a
         // specific order if the list (parser_order_) is not empty; otherwise
@@ -194,8 +192,8 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
             int parsed_count = 0;
             std::map<std::string, ConstElementPtr>::const_iterator it;
             BOOST_FOREACH(element_id, parse_order_) {
-                it = values_map.find(element_id);
-                if (it != values_map.end()) {
+                it = objects_map.find(element_id);
+                if (it != objects_map.end()) {
                     ++parsed_count;
                     buildAndCommit(element_id, it->second);
                 }
@@ -218,7 +216,7 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
             // or we parsed fewer than are in the map; then either the parse i
             // order is incomplete OR the map has unsupported values.
             if (!parsed_count ||
-                (parsed_count && ((parsed_count + 1) < values_map.size()))) {
+                (parsed_count && ((parsed_count + 1) < objects_map.size()))) {
                 LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR);
                 isc_throw(DCfgMgrBaseError,
                         "Configuration contains elements not in parse order");
@@ -227,7 +225,7 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
             // Order doesn't matter so iterate over the value map directly.
             // Pass each element and it's associated data in to be parsed.
             ConfigPair config_pair;
-            BOOST_FOREACH(config_pair, values_map) {
+            BOOST_FOREACH(config_pair, objects_map) {
                 element_id = config_pair.first;
                 buildAndCommit(element_id, config_pair.second);
             }

src/bin/d2/d_cfg_mgr.h

@@ -116,7 +116,7 @@ public:
     /// into parsers.
     ///
     /// @return returns a pointer to the Boolean Storage.
-    isc::dhcp::BooleanStoragePtr& getBooleanStorage() {
+    isc::dhcp::BooleanStoragePtr getBooleanStorage() {
         return (boolean_values_);
     }
 
@@ -124,7 +124,7 @@ public:
     /// into parsers.
     ///
     /// @return returns a pointer to the uint32 Storage.
-    isc::dhcp::Uint32StoragePtr& getUint32Storage() {
+    isc::dhcp::Uint32StoragePtr getUint32Storage() {
         return (uint32_values_);
     }
 
@@ -132,7 +132,7 @@ public:
     /// into parsers.
     ///
     /// @return returns a pointer to the string Storage.
-    isc::dhcp::StringStoragePtr& getStringStorage() {
+    isc::dhcp::StringStoragePtr getStringStorage() {
         return (string_values_);
     }
 
@@ -184,10 +184,7 @@ private:
     isc::dhcp::StringStoragePtr string_values_;
 };
 
-/// @brief Defines an unsorted, list of string Element IDs.
-typedef std::vector<std::string> ElementIdList;
-
-/// @brief Defines an unsorted, list of string Element IDs.
+/// @brief Defines a sequence of Element IDs used to specify a parsing order.
 typedef std::vector<std::string> ElementIdList;
 
 /// @brief Configuration Manager
@@ -327,6 +324,12 @@ protected:
 
     /// @brief Abstract factory which creates a context instance.
     ///
+    /// This method is used at the beginning of configuration process to
+    /// create a fresh, empty copy of the derivation-specific context. This
+    /// new context will be populated during the configuration process
+    /// and will replace the existing context provided the configuration
+    /// process completes without error.
+    ///
     /// @return Returns a DCfgContextBasePtr to the new context instance.
     virtual DCfgContextBasePtr createNewContext() = 0;
 

src/bin/d2/nc_trans.h

@@ -168,6 +168,7 @@ public:
     /// @param ncr is the NameChangeRequest to fulfill
     /// @param forward_domain is the domain to use for forward DNS updates
     /// @param reverse_domain is the domain to use for reverse DNS updates
+    /// @param cfg_mgr reference to the current configuration manager
     ///
     /// @throw NameChangeTransactionError if given an null request,
     /// if forward change is enabled but forward domain is null, if

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -82,6 +82,18 @@ public:
         return (config.str());
     }
 
+    /// @brief Parses a configuration string and tests against a given outcome
+    ///
+    /// Convenience method which accepts JSON text and an expected pass or fail
+    /// outcome. It converts the text into an ElementPtr and passes that to
+    /// configuration manager's parseConfig method.  It then tests the
+    /// parse result against the expected outcome  If they do not match it
+    /// the method asserts a failure.   If they do match, it refreshes the
+    /// the D2Params pointer with the newly parsed instance.
+    ///
+    /// @param config_str the JSON configuration text to parse
+    /// @param should_fail boolean indicator if the parsing should fail or not.
+    /// It defaults to false.
     void runConfig(std::string config_str, bool should_fail=false) {
         // We assume the config string is valid JSON.
         ASSERT_TRUE(fromJSON(config_str));
@@ -99,6 +111,7 @@ public:
         ASSERT_TRUE(d2_params_);
     }
 
+    /// @brief Pointer the D2Params most recently parsed.
     D2ParamsPtr d2_params_;
 };
 
@@ -1060,9 +1073,6 @@ TEST(D2CfgMgr, construction) {
     EXPECT_NO_THROW(delete cfg_mgr);
 }
 
-TEST_F(D2CfgMgrTest, paramsConfig) {
-}
-
 /// @brief Tests the parsing of a complete, valid DHCP-DDNS configuration.
 /// This tests passes the configuration into an instance of D2CfgMgr just
 /// as it would be done by d2_process in response to a configuration update

src/bin/d2/tests/d_cfg_mgr_unittests.cc

@@ -205,9 +205,6 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) {
 
     // Verify that the parsed order matches what we expected.
     EXPECT_TRUE(cfg_mgr_->parsed_order_ ==  order_expected);
-    for (int i = 0; i < cfg_mgr_->parsed_order_.size(); ++i) {
-        std::cout << i << ":" << cfg_mgr_->parsed_order_[i] << std::endl;
-    }
 
     // Clear the manager's parse order "memory".
     cfg_mgr_->parsed_order_.clear();