From: Thomas Markwalder Date: Tue, 20 May 2014 11:16:05 +0000 (-0400) Subject: [3432] Addressed review comments X-Git-Tag: trac3434_base~30^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ff3fc5e41f5e45bbc0709e0ad28dd64579d81850;p=thirdparty%2Fkea.git [3432] Addressed review comments Added missing commentary, corrected typos. --- diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml index 7eaef3e44f..0bc4e7ecad 100644 --- a/doc/guide/bind10-guide.xml +++ b/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. - 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. - 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. 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. @@ -5302,8 +5303,6 @@ DhcpDdns/reverse_ddns/ddns_domains [] list (default) > config set DhcpDdns/port 900 > config commit - The server may be configured to listen over IPv4 or IPv6, therefore - ip-address may an IPv4 or IPv6 address. diff --git a/src/bin/d2/d2_cfg_mgr.h b/src/bin/d2/d2_cfg_mgr.h index 08032213a8..dcc03e1e8f 100644 --- a/src/bin/d2/d2_cfg_mgr.h +++ b/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(); }; diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index 3865063dec..c8e331e30e 100644 --- a/src/bin/d2/d2_config.cc +++ b/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) { diff --git a/src/bin/d2/d2_config.h b/src/bin/d2/d2_config.h index 8c5dd29cd4..168559b192 100644 --- a/src/bin/d2/d2_config.h +++ b/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); diff --git a/src/bin/d2/d_cfg_mgr.cc b/src/bin/d2/d_cfg_mgr.cc index 6be057eaca..151202f360 100644 --- a/src/bin/d2/d_cfg_mgr.cc +++ b/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::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); } diff --git a/src/bin/d2/d_cfg_mgr.h b/src/bin/d2/d_cfg_mgr.h index 92468835a4..d088feb8db 100644 --- a/src/bin/d2/d_cfg_mgr.h +++ b/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 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 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; diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index 71bcce3454..17f7f46ce9 100644 --- a/src/bin/d2/nc_trans.h +++ b/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 diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index be9caa7fdd..79d162ba7b 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/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 diff --git a/src/bin/d2/tests/d_cfg_mgr_unittests.cc b/src/bin/d2/tests/d_cfg_mgr_unittests.cc index 235e954e78..88438d215d 100644 --- a/src/bin/d2/tests/d_cfg_mgr_unittests.cc +++ b/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();