From c519aa88c8cf4edb25a53ec761364b076a6ae497 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 6 May 2013 11:45:27 -0400 Subject: [PATCH] [2355] Minor changes in response to review comments. --- src/bin/dhcp4/config_parser.cc | 63 +++++----- src/bin/dhcp4/config_parser.h | 7 +- src/bin/dhcp4/tests/config_parser_unittest.cc | 4 + src/bin/dhcp6/config_parser.cc | 44 +++---- src/bin/dhcp6/config_parser.h | 5 +- src/lib/dhcpsrv/dhcp_config_parser.h | 76 ------------ src/lib/dhcpsrv/dhcp_parsers.cc | 84 ++++++++++---- src/lib/dhcpsrv/dhcp_parsers.h | 108 +++++++++++++++--- src/lib/dhcpsrv/subnet.cc | 60 +++++----- src/lib/dhcpsrv/tests/cfgmgr_unittest.cc | 2 +- .../dhcpsrv/tests/dhcp_parsers_unittest.cc | 16 ++- 11 files changed, 263 insertions(+), 206 deletions(-) diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc index 8099d7170..e7eb93b7c 100644 --- a/src/bin/dhcp4/config_parser.cc +++ b/src/bin/dhcp4/config_parser.cc @@ -41,10 +41,6 @@ using namespace isc::asiolink; namespace { -/// @brief Create the global parser context which stores global -/// parameters, options, and option definitions. -ParserContextPtr global_context_ptr(new ParserContext(Option::V4)); - /// @brief Parser for DHCP4 option data value. /// /// This parser parses configuration entries that specify value of @@ -66,7 +62,7 @@ public: /// @brief static factory method for instantiating Dhcp4OptionDataParsers /// - /// @param param_name name fo the parameter to be parsed. + /// @param param_name name of the parameter to be parsed. /// @param options storage where the parameter value is to be stored. /// @param global_context is a pointer to the global context which /// stores global scope parameters, options, option defintions. @@ -130,10 +126,10 @@ protected: /// /// @param addr is the IPv4 prefix of the pool. /// @param len is the prefix length. - /// @param ignored dummy parameter to provide symmetry between + /// @param ignored dummy parameter to provide symmetry between the + /// PoolParser derivations. The V6 derivation requires a third value. /// @return returns a PoolPtr to the new Pool4 object. - PoolPtr poolMaker (IOAddress &addr, uint32_t len, int32_t) - { + PoolPtr poolMaker (IOAddress &addr, uint32_t len, int32_t) { return (PoolPtr(new Pool4(addr, len))); } @@ -144,8 +140,7 @@ protected: /// @param ignored dummy parameter to provide symmetry between the /// PoolParser derivations. The V6 derivation requires a third value. /// @return returns a PoolPtr to the new Pool4 object. - PoolPtr poolMaker (IOAddress &min, IOAddress &max, int32_t) - { + PoolPtr poolMaker (IOAddress &min, IOAddress &max, int32_t) { return (PoolPtr(new Pool4(min, max))); } }; @@ -162,15 +157,22 @@ public: /// @param ignored first parameter /// @param global_context is a pointer to the global context which /// stores global scope parameters, options, option defintions. - Subnet4ConfigParser(const std::string&, ParserContextPtr global_context) - :SubnetConfigParser("", global_context) { + Subnet4ConfigParser(const std::string&) + :SubnetConfigParser("", globalContext()) { } /// @brief Adds the created subnet to a server's configuration. + /// @throw throws Unexpected if dynamic cast fails. void commit() { if (subnet_) { - Subnet4Ptr bs = boost::dynamic_pointer_cast(subnet_); - isc::dhcp::CfgMgr::instance().addSubnet4(bs); + Subnet4Ptr sub4ptr = boost::dynamic_pointer_cast(subnet_); + if (!sub4ptr) { + // If we hit this, it is a programming error. + isc_throw(Unexpected, + "Invalid cast in Subnet4ConfigParser::commit"); + } + + isc::dhcp::CfgMgr::instance().addSubnet4(sub4ptr); } } @@ -191,14 +193,14 @@ protected: (config_id.compare("rebind-timer") == 0)) { parser = new Uint32Parser(config_id, uint32_values_); } else if ((config_id.compare("subnet") == 0) || - (config_id.compare("interface") == 0)) { + (config_id.compare("interface") == 0)) { parser = new StringParser(config_id, string_values_); } else if (config_id.compare("pool") == 0) { parser = new Pool4Parser(config_id, pools_); } else if (config_id.compare("option-data") == 0) { parser = new OptionDataListParser(config_id, options_, - global_context_, - Dhcp4OptionDataParser::factory); + global_context_, + Dhcp4OptionDataParser::factory); } else { isc_throw(NotImplemented, "parser error: Subnet4 parameter not supported: " << config_id); @@ -277,7 +279,7 @@ public: /// @brief constructor /// - /// @param dummy first argument, always ingored. All parsers accept a + /// @param dummy first argument, always ignored. All parsers accept a /// string parameter "name" as their first argument. Subnets4ListConfigParser(const std::string&) { } @@ -290,8 +292,7 @@ public: /// @param subnets_list pointer to a list of IPv4 subnets void build(ConstElementPtr subnets_list) { BOOST_FOREACH(ConstElementPtr subnet, subnets_list->listValue()) { - ParserPtr parser(new Subnet4ConfigParser("subnet", - global_context_ptr)); + ParserPtr parser(new Subnet4ConfigParser("subnet")); parser->build(subnet); subnets_.push_back(parser); } @@ -346,22 +347,22 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) { (config_id.compare("renew-timer") == 0) || (config_id.compare("rebind-timer") == 0)) { parser = new Uint32Parser(config_id, - global_context_ptr->uint32_values_); + globalContext()->uint32_values_); } else if (config_id.compare("interface") == 0) { parser = new InterfaceListConfigParser(config_id); } else if (config_id.compare("subnet4") == 0) { parser = new Subnets4ListConfigParser(config_id); } else if (config_id.compare("option-data") == 0) { parser = new OptionDataListParser(config_id, - global_context_ptr->options_, - global_context_ptr, + globalContext()->options_, + globalContext(), Dhcp4OptionDataParser::factory); } else if (config_id.compare("option-def") == 0) { parser = new OptionDefListParser(config_id, - global_context_ptr->option_defs_); + globalContext()->option_defs_); } else if (config_id.compare("version") == 0) { parser = new StringParser(config_id, - global_context_ptr->string_values_); + globalContext()->string_values_); } else if (config_id.compare("lease-database") == 0) { parser = new DbAccessParser(config_id); } else { @@ -405,7 +406,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { // parsing operation fails after the global storage has been // modified. We need to preserve the original global data here // so as we can rollback changes when an error occurs. - ParserContext original_context(*global_context_ptr); + ParserContext original_context(*globalContext()); // Answer will hold the result. ConstElementPtr answer; @@ -500,7 +501,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { // Rollback changes as the configuration parsing failed. if (rollback) { - global_context_ptr.reset(new ParserContext(original_context)); + globalContext().reset(new ParserContext(original_context)); return (answer); } @@ -511,11 +512,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { return (answer); } -// Makes global context accessible for unit tests. -const ParserContext& getGlobalParserContext() { - return (*global_context_ptr); +ParserContextPtr globalContext() { + static ParserContextPtr global_context_ptr(new ParserContext(Option::V4)); + return (global_context_ptr); } + + }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/bin/dhcp4/config_parser.h b/src/bin/dhcp4/config_parser.h index 748efbeae..692a908ef 100644 --- a/src/bin/dhcp4/config_parser.h +++ b/src/bin/dhcp4/config_parser.h @@ -63,11 +63,8 @@ configureDhcp4Server(Dhcpv4Srv&, /// @brief Returns the global context /// -/// This function must be only used by unit tests that need -/// to access global context. -/// -/// @returns a const reference to the global context -const ParserContext& getGlobalParserContext(); +/// @return a const reference to the global context +ParserContextPtr globalContext(); }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 03c8346f7..171f43106 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -53,7 +53,11 @@ public: // Checks if global parameter of name have expected_value void checkGlobalUint32(string name, uint32_t expected_value) { const Uint32StoragePtr uint32_defaults = +#if 0 getGlobalParserContext().uint32_values_; +#else + globalContext()->uint32_values_; +#endif try { uint32_t actual_value = uint32_defaults->getParam(name); EXPECT_EQ(expected_value, actual_value); diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index efc2ceccf..f50c733b0 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -55,9 +55,6 @@ typedef boost::shared_ptr BooleanParserPtr; typedef boost::shared_ptr StringParserPtr; typedef boost::shared_ptr Uint32ParserPtr; -// TKM - declare a global parser context -ParserContextPtr global_context_ptr(new ParserContext(Option::V6)); - /// @brief Parser for DHCP6 option data value. /// /// This parser parses configuration entries that specify value of @@ -182,15 +179,21 @@ public: /// @param ignored first parameter /// @param global_context is a pointer to the global context which /// stores global scope parameters, options, option defintions. - Subnet6ConfigParser(const std::string&, ParserContextPtr global_context) - :SubnetConfigParser("", global_context) { + Subnet6ConfigParser(const std::string&) + :SubnetConfigParser("", globalContext()) { } /// @brief Adds the created subnet to a server's configuration. + /// @throw throws Unexpected if dynamic cast fails. void commit() { if (subnet_) { - Subnet6Ptr bs = boost::dynamic_pointer_cast(subnet_); - isc::dhcp::CfgMgr::instance().addSubnet6(bs); + Subnet6Ptr sub6ptr = boost::dynamic_pointer_cast(subnet_); + if (!sub6ptr) { + // If we hit this, it is a programming error. + isc_throw(Unexpected, + "Invalid cast in Subnet4ConfigParser::commit"); + } + isc::dhcp::CfgMgr::instance().addSubnet6(sub6ptr); } } @@ -212,7 +215,7 @@ protected: (config_id.compare("rebind-timer") == 0)) { parser = new Uint32Parser(config_id, uint32_values_); } else if ((config_id.compare("subnet") == 0) || - (config_id.compare("interface") == 0)) { + (config_id.compare("interface") == 0)) { parser = new StringParser(config_id, string_values_); } else if (config_id.compare("pool") == 0) { parser = new Pool6Parser(config_id, pools_); @@ -303,7 +306,7 @@ public: /// @brief constructor /// - /// @param dummy first argument, always ingored. All parsers accept a + /// @param dummy first argument, always ignored. All parsers accept a /// string parameter "name" as their first argument. Subnets6ListConfigParser(const std::string&) { } @@ -316,8 +319,7 @@ public: /// @param subnets_list pointer to a list of IPv6 subnets void build(ConstElementPtr subnets_list) { BOOST_FOREACH(ConstElementPtr subnet, subnets_list->listValue()) { - ParserPtr parser(new Subnet6ConfigParser("subnet", - global_context_ptr)); + ParserPtr parser(new Subnet6ConfigParser("subnet" )); parser->build(subnet); subnets_.push_back(parser); } @@ -373,22 +375,22 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id) { (config_id.compare("renew-timer") == 0) || (config_id.compare("rebind-timer") == 0)) { parser = new Uint32Parser(config_id, - global_context_ptr->uint32_values_); + globalContext()->uint32_values_); } else if (config_id.compare("interface") == 0) { parser = new InterfaceListConfigParser(config_id); } else if (config_id.compare("subnet6") == 0) { parser = new Subnets6ListConfigParser(config_id); } else if (config_id.compare("option-data") == 0) { parser = new OptionDataListParser(config_id, - global_context_ptr->options_, - global_context_ptr, + globalContext()->options_, + globalContext(), Dhcp6OptionDataParser::factory); } else if (config_id.compare("option-def") == 0) { parser = new OptionDefListParser(config_id, - global_context_ptr->option_defs_); + globalContext()->option_defs_); } else if (config_id.compare("version") == 0) { parser = new StringParser(config_id, - global_context_ptr->string_values_); + globalContext()->string_values_); } else if (config_id.compare("lease-database") == 0) { parser = new DbAccessParser(config_id); } else { @@ -432,7 +434,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { // parsing operation fails after the global storage has been // modified. We need to preserve the original global data here // so as we can rollback changes when an error occurs. - ParserContext original_context(*global_context_ptr); + ParserContext original_context(*globalContext()); // answer will hold the result. ConstElementPtr answer; @@ -528,7 +530,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { // Rollback changes as the configuration parsing failed. if (rollback) { - global_context_ptr.reset(new ParserContext(original_context)); + globalContext().reset(new ParserContext(original_context)); return (answer); } @@ -539,9 +541,9 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { return (answer); } -// Makes global context accessible for unit tests. -const ParserContext& getGlobalParserContext() { - return (*global_context_ptr); +ParserContextPtr globalContext() { + static ParserContextPtr global_context_ptr(new ParserContext(Option::V6)); + return (global_context_ptr); } }; // end of isc::dhcp namespace diff --git a/src/bin/dhcp6/config_parser.h b/src/bin/dhcp6/config_parser.h index 7e215b162..1d7775794 100644 --- a/src/bin/dhcp6/config_parser.h +++ b/src/bin/dhcp6/config_parser.h @@ -51,11 +51,8 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set); /// @brief Returns the global context /// -/// This function must be only used by unit tests that need -/// to access global context. -/// /// @returns a const reference to the global context -const ParserContext& getGlobalParserContext(); +ParserContextPtr globalContext(); }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/lib/dhcpsrv/dhcp_config_parser.h b/src/lib/dhcpsrv/dhcp_config_parser.h index 86eab4d10..77e46c835 100644 --- a/src/lib/dhcpsrv/dhcp_config_parser.h +++ b/src/lib/dhcpsrv/dhcp_config_parser.h @@ -130,82 +130,6 @@ public: virtual void commit() = 0; }; -/// @brief A template class that stores named elements of a given data type. -/// -/// This template class is provides data value storage for configuration parameters -/// of a given data type. The values are stored by parameter name and as instances -/// of type "ValueType". -/// -/// @param ValueType is the data type of the elements to store. -template -class ValueStorage { - public: - /// @brief Stores the the parameter and its value in the store. - /// - /// If the parameter does not exist in the store, then it will be added, - /// otherwise its data value will be updated with the given value. - /// - /// @param name is the name of the paramater to store. - /// @param value is the data value to store. - void setParam(const std::string& name, const ValueType& value) { - values_[name] = value; - } - - /// @brief Returns the data value for the given parameter. - /// - /// Finds and returns the data value for the given parameter. - /// @param name is the name of the parameter for which the data - /// value is desired. - /// - /// @return The paramater's data value of type . - /// @throw DhcpConfigError if the parameter is not found. - ValueType getParam(const std::string& name) const { - typename std::map::const_iterator param - = values_.find(name); - - if (param == values_.end()) { - isc_throw(DhcpConfigError, "Missing parameter '" - << name << "'"); - } - - return (param->second); - } - - /// @brief Remove the parameter from the store. - /// - /// Deletes the entry for the given parameter from the store if it - /// exists. - /// - /// @param name is the name of the paramater to delete. - void delParam(const std::string& name) { - values_.erase(name); - } - - /// @brief Deletes all of the entries from the store. - /// - void clear() { - values_.clear(); - } - - - private: - /// @brief An std::map of the data values, keyed by parameter names. - std::map values_; -}; - - -/// @brief a collection of elements that store uint32 values (e.g. renew-timer = 900) -typedef ValueStorage Uint32Storage; -typedef boost::shared_ptr Uint32StoragePtr; - -/// @brief a collection of elements that store string values -typedef ValueStorage StringStorage; -typedef boost::shared_ptr StringStoragePtr; - -/// @brief Storage for parsed boolean values. -typedef ValueStorage BooleanStorage; -typedef boost::shared_ptr BooleanStoragePtr; - }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index 1c620921c..a7e0b1bb6 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -44,7 +44,7 @@ ParserContext::ParserContext(Option::Universe universe): universe_(universe) { } -ParserContext::ParserContext(ParserContext& rhs): +ParserContext::ParserContext(const ParserContext& rhs): boolean_values_(new BooleanStorage(*(rhs.boolean_values_))), uint32_values_(new Uint32Storage(*(rhs.uint32_values_))), string_values_(new StringStorage(*(rhs.string_values_))), @@ -53,19 +53,38 @@ ParserContext::ParserContext(ParserContext& rhs): universe_(rhs.universe_) { } +ParserContext& +ParserContext::operator=(const ParserContext& rhs) { + ParserContext tmp(rhs); + boolean_values_ = + BooleanStoragePtr(new BooleanStorage(*(rhs.boolean_values_))); + uint32_values_ = + Uint32StoragePtr(new Uint32Storage(*(tmp.uint32_values_))); + string_values_ = + StringStoragePtr(new StringStorage(*(tmp.string_values_))); + options_ = OptionStoragePtr(new OptionStorage(*(tmp.options_))); + option_defs_ = + OptionDefStoragePtr(new OptionDefStorage(*(tmp.option_defs_))); + universe_ = rhs.universe_; + return (*this); + } + + // **************************** DebugParser ************************* DebugParser::DebugParser(const std::string& param_name) :param_name_(param_name) { } -void DebugParser::build(ConstElementPtr new_config) { +void +DebugParser::build(ConstElementPtr new_config) { std::cout << "Build for token: [" << param_name_ << "] = [" << value_->str() << "]" << std::endl; value_ = new_config; } -void DebugParser::commit() { +void +DebugParser::commit() { // Debug message. The whole DebugParser class is used only for parser // debugging, and is not used in production code. It is very convenient // to keep it around. Please do not turn this cout into logger calls. @@ -128,13 +147,15 @@ InterfaceListConfigParser::InterfaceListConfigParser(const std::string& } } -void InterfaceListConfigParser::build(ConstElementPtr value) { +void +InterfaceListConfigParser::build(ConstElementPtr value) { BOOST_FOREACH(ConstElementPtr iface, value->listValue()) { interfaces_.push_back(iface->str()); } } -void InterfaceListConfigParser::commit() { +void +InterfaceListConfigParser::commit() { /// @todo: Implement per interface listening. Currently always listening /// on all interfaces. } @@ -157,7 +178,8 @@ OptionDataParser::OptionDataParser(const std::string&, OptionStoragePtr options, } } -void OptionDataParser::build(ConstElementPtr option_data_entries) { +void +OptionDataParser::build(ConstElementPtr option_data_entries) { BOOST_FOREACH(ConfigPair param, option_data_entries->mapValue()) { ParserPtr parser; if (param.first == "name" || param.first == "data" || @@ -193,7 +215,8 @@ void OptionDataParser::build(ConstElementPtr option_data_entries) { createOption(); } -void OptionDataParser::commit() { +void +OptionDataParser::commit() { if (!option_descriptor_.option) { // Before we can commit the new option should be configured. If it is // not than somebody must have called commit() before build(). @@ -221,7 +244,8 @@ void OptionDataParser::commit() { options_->addItem(option_descriptor_, option_space_); } -void OptionDataParser::createOption() { +void +OptionDataParser::createOption() { // Option code is held in the uint32_t storage but is supposed to // be uint16_t value. We need to check that value in the configuration // does not exceed range of uint8_t and is not zero. @@ -389,7 +413,8 @@ OptionDataListParser::OptionDataListParser(const std::string&, } } -void OptionDataListParser::build(ConstElementPtr option_data_list) { +void +OptionDataListParser::build(ConstElementPtr option_data_list) { BOOST_FOREACH(ConstElementPtr option_value, option_data_list->listValue()) { boost::shared_ptr parser((*optionDataParserFactory_)("option-data", @@ -404,7 +429,8 @@ void OptionDataListParser::build(ConstElementPtr option_data_list) { } } -void OptionDataListParser::commit() { +void +OptionDataListParser::commit() { BOOST_FOREACH(ParserPtr parser, parsers_) { parser->commit(); } @@ -426,7 +452,8 @@ OptionDefParser::OptionDefParser(const std::string&, } } -void OptionDefParser::build(ConstElementPtr option_def) { +void +OptionDefParser::build(ConstElementPtr option_def) { // Parse the elements that make up the option definition. BOOST_FOREACH(ConfigPair param, option_def->mapValue()) { std::string entry(param.first); @@ -473,14 +500,16 @@ void OptionDefParser::build(ConstElementPtr option_def) { } } -void OptionDefParser::commit() { +void +OptionDefParser::commit() { if (storage_ && option_definition_ && OptionSpace::validateName(option_space_name_)) { storage_->addItem(option_definition_, option_space_name_); } } -void OptionDefParser::createOptionDef() { +void +OptionDefParser::createOptionDef() { // Get the option space name and validate it. std::string space = string_values_->getParam("space"); if (!OptionSpace::validateName(space)) { @@ -567,7 +596,8 @@ OptionDefListParser::OptionDefListParser(const std::string&, } } -void OptionDefListParser::build(ConstElementPtr option_def_list) { +void +OptionDefListParser::build(ConstElementPtr option_def_list) { // Clear existing items in the storage. // We are going to replace all of them. storage_->clearItems(); @@ -585,7 +615,8 @@ void OptionDefListParser::build(ConstElementPtr option_def_list) { } } -void OptionDefListParser::commit() { +void +OptionDefListParser::commit() { CfgMgr& cfg_mgr = CfgMgr::instance(); cfg_mgr.deleteOptionDefs(); @@ -616,7 +647,8 @@ PoolParser::PoolParser(const std::string&, PoolStoragePtr pools) } } -void PoolParser::build(ConstElementPtr pools_list) { +void +PoolParser::build(ConstElementPtr pools_list) { BOOST_FOREACH(ConstElementPtr text_pool, pools_list->listValue()) { // That should be a single pool representation. It should contain // text is form prefix/len or first - last. Note that spaces @@ -673,9 +705,10 @@ void PoolParser::build(ConstElementPtr pools_list) { << text_pool->stringValue() << ". Does not contain - (for min-max) nor / (prefix/len)"); } - } +} -void PoolParser::commit() { +void +PoolParser::commit() { if (pools_) { // local_pools_ holds the values produced by the build function. // At this point parsing should have completed successfuly so @@ -699,7 +732,8 @@ SubnetConfigParser::SubnetConfigParser(const std::string&, } } -void SubnetConfigParser::build(ConstElementPtr subnet) { +void +SubnetConfigParser::build(ConstElementPtr subnet) { BOOST_FOREACH(ConfigPair param, subnet->mapValue()) { ParserPtr parser(createSubnetConfigParser(param.first)); parser->build(param.second); @@ -722,8 +756,9 @@ void SubnetConfigParser::build(ConstElementPtr subnet) { createSubnet(); } -void SubnetConfigParser::appendSubOptions(const std::string& option_space, - OptionPtr& option) { +void +SubnetConfigParser::appendSubOptions(const std::string& option_space, + OptionPtr& option) { // Only non-NULL options are stored in option container. // If this option pointer is NULL this is a serious error. assert(option); @@ -776,7 +811,8 @@ void SubnetConfigParser::appendSubOptions(const std::string& option_space, } } -void SubnetConfigParser::createSubnet() { +void +SubnetConfigParser::createSubnet() { std::string subnet_txt; try { subnet_txt = string_values_->getParam("subnet"); @@ -896,8 +932,8 @@ void SubnetConfigParser::createSubnet() { } } -isc::dhcp::Triplet SubnetConfigParser::getParam(const - std::string& name) { +isc::dhcp::Triplet +SubnetConfigParser::getParam(const std::string& name) { uint32_t value = 0; try { // look for local value diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 1fe2867b5..e453204bd 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -34,19 +34,94 @@ namespace dhcp { typedef OptionSpaceContainer OptionDefStorage; -/// @brief Shared pointer to option definitions storage. +/// @brief Shared pointer to option definitions storage. typedef boost::shared_ptr OptionDefStoragePtr; /// Collection of containers holding option spaces. Each container within /// a particular option space holds so-called option descriptors. typedef OptionSpaceContainer OptionStorage; -/// @brief Shared pointer to option storage. +/// @brief Shared pointer to option storage. typedef boost::shared_ptr OptionStoragePtr; +/// @brief A template class that stores named elements of a given data type. +/// +/// This template class is provides data value storage for configuration parameters +/// of a given data type. The values are stored by parameter name and as instances +/// of type "ValueType". +/// +/// @param ValueType is the data type of the elements to store. +template +class ValueStorage { + public: + /// @brief Stores the the parameter and its value in the store. + /// + /// If the parameter does not exist in the store, then it will be added, + /// otherwise its data value will be updated with the given value. + /// + /// @param name is the name of the paramater to store. + /// @param value is the data value to store. + void setParam(const std::string& name, const ValueType& value) { + values_[name] = value; + } + + /// @brief Returns the data value for the given parameter. + /// + /// Finds and returns the data value for the given parameter. + /// @param name is the name of the parameter for which the data + /// value is desired. + /// + /// @return The paramater's data value of type . + /// @throw DhcpConfigError if the parameter is not found. + ValueType getParam(const std::string& name) const { + typename std::map::const_iterator param + = values_.find(name); + + if (param == values_.end()) { + isc_throw(DhcpConfigError, "Missing parameter '" + << name << "'"); + } + + return (param->second); + } + + /// @brief Remove the parameter from the store. + /// + /// Deletes the entry for the given parameter from the store if it + /// exists. + /// + /// @param name is the name of the paramater to delete. + void delParam(const std::string& name) { + values_.erase(name); + } + + /// @brief Deletes all of the entries from the store. + /// + void clear() { + values_.clear(); + } + + + private: + /// @brief An std::map of the data values, keyed by parameter names. + std::map values_; +}; + + +/// @brief a collection of elements that store uint32 values +typedef ValueStorage Uint32Storage; +typedef boost::shared_ptr Uint32StoragePtr; + +/// @brief a collection of elements that store string values +typedef ValueStorage StringStorage; +typedef boost::shared_ptr StringStoragePtr; + +/// @brief Storage for parsed boolean values. +typedef ValueStorage BooleanStorage; +typedef boost::shared_ptr BooleanStoragePtr; /// @brief Container for the current parsing context. It provides a -/// single enclosure for the storage of configuration paramaters, +/// single enclosure for the storage of configuration parameters, /// options, option definitions, and other context specific information /// that needs to be accessible throughout the parsing and parsing /// constructs. @@ -59,7 +134,7 @@ public: ParserContext(Option::Universe universe); /// @brief Copy constructor - ParserContext(ParserContext& rhs); + ParserContext(const ParserContext& rhs); /// @brief Storage for boolean parameters. BooleanStoragePtr boolean_values_; @@ -78,16 +153,23 @@ public: /// @brief The parsing universe of this context. Option::Universe universe_; + + /// @brief Assignment operator + ParserContext& operator=(const ParserContext& rhs); }; /// @brief Pointer to various parser context. typedef boost::shared_ptr ParserContextPtr; -/// @brief - TKM - simple type parser template class +/// @brief Simple data-type parser template class /// -/// This parser handles configuration values of the boolean type. -/// Parsed values are stored in a provided storage. If no storage -/// is provided then the build function throws an exception. +/// This is the template class for simple data-type parsers. It supports +/// parsing a configuration parameter with specific data-type for its +/// possible values. It provides a common constructor, commit, and templated +/// data storage. The "build" method implementation must be provided by a +/// declaring type. +/// @param ValueType is the data type of the configuration paramater value +/// the parser should handle. template class ValueParser : public DhcpConfigParser { public: @@ -117,11 +199,12 @@ public: } - /// @brief Parse a boolean value. + /// @brief Parse a given element into a value of type /// /// @param value a value to be parsed. /// - /// @throw isc::BadValue if value is not a boolean type Element. + /// @throw isc::BadValue Typically the implementing type will throw + /// a BadValue exception when given an invalid Element to parse. void build(isc::data::ConstElementPtr value); /// @brief Put a parsed value to the storage. @@ -132,7 +215,7 @@ public: } private: - /// Pointer to the storage where parsed value is stored. + /// Pointer to the storage where committed value is stored. boost::shared_ptr > storage_; /// Name of the parameter which value is parsed with this parser. @@ -499,9 +582,6 @@ typedef boost::shared_ptr PoolStoragePtr; /// of two syntaxes: min-max and prefix/len. Pool objects are created /// and stored in chosen PoolStorage container. /// -/// As there are no default values for pool, setStorage() must be called -/// before build(). Otherwise exception will be thrown. -/// /// It is useful for parsing Dhcp<4/6>/subnet<4/6>[X]/pool parameters. class PoolParser : public DhcpConfigParser { public: diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index b60ee60af..50a0feec3 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -33,11 +33,13 @@ Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len, last_allocated_(lastAddrInPrefix(prefix, len)) { if ((prefix.isV6() && len > 128) || (prefix.isV4() && len > 32)) { - isc_throw(BadValue, "Invalid prefix length specified for subnet: " << len); + isc_throw(BadValue, + "Invalid prefix length specified for subnet: " << len); } } -bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const { +bool +Subnet::inRange(const isc::asiolink::IOAddress& addr) const { IOAddress first = firstAddrInPrefix(prefix_, prefix_len_); IOAddress last = lastAddrInPrefix(prefix_, prefix_len_); @@ -84,7 +86,8 @@ Subnet::getOptionDescriptor(const std::string& option_space, return (*range.first); } -std::string Subnet::toText() const { +std::string +Subnet::toText() const { std::stringstream tmp; tmp << prefix_.toText() << "/" << static_cast(prefix_len_); return (tmp.str()); @@ -101,12 +104,14 @@ Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length, } } -void Subnet::addPool(const PoolPtr& pool) { +void +Subnet::addPool(const PoolPtr& pool) { IOAddress first_addr = pool->getFirstAddress(); IOAddress last_addr = pool->getLastAddress(); if (!inRange(first_addr) || !inRange(last_addr)) { - isc_throw(BadValue, "Pool (" << first_addr.toText() << "-" << last_addr.toText() + isc_throw(BadValue, "Pool (" << first_addr.toText() << "-" + << last_addr.toText() << " does not belong in this (" << prefix_.toText() << "/" << static_cast(prefix_len_) << ") subnet4"); } @@ -119,15 +124,16 @@ void Subnet::addPool(const PoolPtr& pool) { PoolPtr Subnet::getPool(isc::asiolink::IOAddress hint) { PoolPtr candidate; - for (PoolCollection::iterator pool = pools_.begin(); pool != pools_.end(); ++pool) { + for (PoolCollection::iterator pool = pools_.begin(); + pool != pools_.end(); ++pool) { - // if we won't find anything better, then let's just use the first pool + // If we won't find anything better, then let's just use the first pool if (!candidate) { candidate = *pool; } - // if the client provided a pool and there's a pool that hint is valid in, - // then let's use that pool + // If the client provided a pool and there's a pool that hint is valid + // in, then let's use that pool if ((*pool)->inRange(hint)) { return (*pool); } @@ -135,11 +141,13 @@ PoolPtr Subnet::getPool(isc::asiolink::IOAddress hint) { return (candidate); } -void Subnet::setIface(const std::string& iface_name) { +void +Subnet::setIface(const std::string& iface_name) { iface_ = iface_name; } -std::string Subnet::getIface() const { +std::string +Subnet::getIface() const { return (iface_); } @@ -148,25 +156,29 @@ std::string Subnet::getIface() const { void Subnet4::validateOption(const OptionPtr& option) const { if (!option) { - isc_throw(isc::BadValue, "option configured for subnet must not be NULL"); + isc_throw(isc::BadValue, + "option configured for subnet must not be NULL"); } else if (option->getUniverse() != Option::V4) { - isc_throw(isc::BadValue, "expected V4 option to be added to the subnet"); + isc_throw(isc::BadValue, + "expected V4 option to be added to the subnet"); } } -bool Subnet::inPool(const isc::asiolink::IOAddress& addr) const { +bool +Subnet::inPool(const isc::asiolink::IOAddress& addr) const { // Let's start with checking if it even belongs to that subnet. if (!inRange(addr)) { return (false); } - for (PoolCollection::const_iterator pool = pools_.begin(); pool != pools_.end(); ++pool) { + for (PoolCollection::const_iterator pool = pools_.begin(); + pool != pools_.end(); ++pool) { if ((*pool)->inRange(addr)) { return (true); } } - // there's no pool that address belongs to + // There's no pool that address belongs to return (false); } @@ -186,21 +198,13 @@ Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length, void Subnet6::validateOption(const OptionPtr& option) const { if (!option) { - isc_throw(isc::BadValue, "option configured for subnet must not be NULL"); + isc_throw(isc::BadValue, + "option configured for subnet must not be NULL"); } else if (option->getUniverse() != Option::V6) { - isc_throw(isc::BadValue, "expected V6 option to be added to the subnet"); + isc_throw(isc::BadValue, + "expected V6 option to be added to the subnet"); } } -#if 0 -void Subnet6::setIface(const std::string& iface_name) { - iface_ = iface_name; -} - -std::string Subnet6::getIface() const { - return (iface_); -} -#endif - } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index be31bab84..4543c0735 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc @@ -15,7 +15,7 @@ #include #include -#include +#include #include #include diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 13aac24f4..81a36af46 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -76,7 +76,7 @@ TEST_F(DhcpParserTest, booleanParserTest) { ASSERT_NO_THROW(parser.build(element)); // Verify that commit updates storage. - bool actual_value = ~test_value; + bool actual_value = !test_value; parser.commit(); EXPECT_NO_THROW((actual_value = storage->getParam(name))); EXPECT_EQ(test_value, actual_value); @@ -116,6 +116,12 @@ TEST_F(DhcpParserTest, stringParserTest) { ElementPtr element = Element::create(9999); EXPECT_NO_THROW(parser.build(element)); + // Verify that commit updates storage. + parser.commit(); + std::string actual_value; + EXPECT_NO_THROW((actual_value = storage->getParam(name))); + EXPECT_EQ("9999", actual_value); + // Verify that parser will build with a string value. const std::string test_value = "test value"; element = Element::create(test_value); @@ -123,7 +129,6 @@ TEST_F(DhcpParserTest, stringParserTest) { // Verify that commit updates storage. parser.commit(); - std::string actual_value; EXPECT_NO_THROW((actual_value = storage->getParam(name))); EXPECT_EQ(test_value, actual_value); } @@ -168,6 +173,12 @@ TEST_F(DhcpParserTest, uint32ParserTest) { int_element->setValue((long)test_value); ASSERT_NO_THROW(parser.build(int_element)); + // Verify that commit updates storage. + parser.commit(); + uint32_t actual_value = 0; + EXPECT_NO_THROW((actual_value = storage->getParam(name))); + EXPECT_EQ(test_value, actual_value); + // Verify that parser will build with a valid positive value. test_value = 77; int_element->setValue((long)test_value); @@ -175,7 +186,6 @@ TEST_F(DhcpParserTest, uint32ParserTest) { // Verify that commit updates storage. parser.commit(); - uint32_t actual_value = 0; EXPECT_NO_THROW((actual_value = storage->getParam(name))); EXPECT_EQ(test_value, actual_value); } -- GitLab