From: Marcin Siodelski Date: Fri, 4 Jan 2013 12:47:37 +0000 (+0100) Subject: [2545] Changes as a result of the review. X-Git-Tag: bind10-1.0.0-rc-release~99^2~21^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d83fc2a12f36818bdd60c87e61d01be7a5dfa4e5;p=thirdparty%2Fkea.git [2545] Changes as a result of the review. --- diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc index ede4268679..afaabb8dca 100644 --- a/src/bin/dhcp4/config_parser.cc +++ b/src/bin/dhcp4/config_parser.cc @@ -147,6 +147,11 @@ public: : storage_(NULL), param_name_(param_name), value_(false) { + // Empty parameter name is invalid. + if (param_name_.empty()) { + isc_throw(isc::dhcp::Dhcp4ConfigError, "parser logic error:" + << "empty parameter name provided"); + } } /// @brief Parse a boolean value. @@ -184,7 +189,7 @@ public: /// /// @param param_name name of the parameter for which the /// parser is created. - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new BooleanParser(param_name)); } @@ -226,6 +231,11 @@ public: Uint32Parser(const std::string& param_name) : storage_(&uint32_defaults), param_name_(param_name) { + // Empty parameter name is invalid. + if (param_name_.empty()) { + isc_throw(Dhcp4ConfigError, "parser logic error:" + << "empty parameter name provided"); + } } /// @brief Parses configuration configuration parameter as uint32_t. @@ -235,7 +245,7 @@ public: /// or the parameter name is empty. virtual void build(ConstElementPtr value) { if (param_name_.empty()) { - isc_throw(BadValue, "parser logic error:" + isc_throw(Dhcp4ConfigError, "parser logic error:" << "empty parameter name provided"); } @@ -272,7 +282,7 @@ public: /// @brief factory that constructs Uint32Parser objects /// /// @param param_name name of the parameter to be parsed - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new Uint32Parser(param_name)); } @@ -314,6 +324,11 @@ public: /// @param param_name name of the configuration parameter being parsed StringParser(const std::string& param_name) :storage_(&string_defaults), param_name_(param_name) { + // Empty parameter name is invalid. + if (param_name_.empty()) { + isc_throw(Dhcp4ConfigError, "parser logic error:" + << "empty parameter name provided"); + } } /// @brief parses parameter value @@ -339,7 +354,7 @@ public: /// @brief factory that constructs StringParser objects /// /// @param param_name name of the parameter to be parsed - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new StringParser(param_name)); } @@ -410,7 +425,7 @@ public: /// @brief factory that constructs InterfaceListConfigParser objects /// /// @param param_name name of the parameter to be parsed - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new InterfaceListConfigParser(param_name)); } @@ -537,7 +552,7 @@ public: /// @brief factory that constructs PoolParser objects /// /// @param param_name name of the parameter to be parsed - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new PoolParser(param_name)); } @@ -602,28 +617,28 @@ public: ParserPtr parser; if (param.first == "name") { boost::shared_ptr - name_parser(dynamic_cast(StringParser::Factory(param.first))); + name_parser(dynamic_cast(StringParser::factory(param.first))); if (name_parser) { name_parser->setStorage(&string_values_); parser = name_parser; } } else if (param.first == "code") { boost::shared_ptr - code_parser(dynamic_cast(Uint32Parser::Factory(param.first))); + code_parser(dynamic_cast(Uint32Parser::factory(param.first))); if (code_parser) { code_parser->setStorage(&uint32_values_); parser = code_parser; } } else if (param.first == "data") { boost::shared_ptr - value_parser(dynamic_cast(StringParser::Factory(param.first))); + value_parser(dynamic_cast(StringParser::factory(param.first))); if (value_parser) { value_parser->setStorage(&string_values_); parser = value_parser; } } else if (param.first == "csv-format") { boost::shared_ptr - value_parser(dynamic_cast(BooleanParser::Factory(param.first))); + value_parser(dynamic_cast(BooleanParser::factory(param.first))); if (value_parser) { value_parser->setStorage(&boolean_values_); parser = value_parser; @@ -926,7 +941,7 @@ public: /// @param param_name param name. /// /// @return DhcpConfigParser object. - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new OptionDataListParser(param_name)); } @@ -1059,18 +1074,32 @@ private: isc_throw(Dhcp4ConfigError, "Mandatory subnet definition in subnet missing"); } + // Remove any spaces or tabs. string subnet_txt = it->second; boost::erase_all(subnet_txt, " "); boost::erase_all(subnet_txt, "\t"); + // The subnet format is prefix/len. We are going to extract + // the prefix portion of a subnet string to create IOAddress + // object from it. IOAddress will be passed to the Subnet's + // constructor later on. In order to extract the prefix we + // need to get all characters preceding "/". size_t pos = subnet_txt.find("/"); if (pos == string::npos) { isc_throw(Dhcp4ConfigError, "Invalid subnet syntax (prefix/len expected):" << it->second); } + + // Try to create the address object. It also validates that + // the address syntax is ok. IOAddress addr(subnet_txt.substr(0, pos)); uint8_t len = boost::lexical_cast(subnet_txt.substr(pos + 1)); + // Get all 'time' parameters using inheritance. + // If the subnet-specific value is defined then use it, else + // use the global value. The global value must always be + // present. If it is not, it is an internal error and exception + // is thrown. Triplet t1 = getParam("renew-timer"); Triplet t2 = getParam("rebind-timer"); Triplet valid = getParam("valid-lifetime"); @@ -1133,12 +1162,12 @@ private: Dhcp4ConfigParser* createSubnet4ConfigParser(const std::string& config_id) { FactoryMap factories; - factories["valid-lifetime"] = Uint32Parser::Factory; - factories["renew-timer"] = Uint32Parser::Factory; - factories["rebind-timer"] = Uint32Parser::Factory; - factories["subnet"] = StringParser::Factory; - factories["pool"] = PoolParser::Factory; - factories["option-data"] = OptionDataListParser::Factory; + factories["valid-lifetime"] = Uint32Parser::factory; + factories["renew-timer"] = Uint32Parser::factory; + factories["rebind-timer"] = Uint32Parser::factory; + factories["subnet"] = StringParser::factory; + factories["pool"] = PoolParser::factory; + factories["option-data"] = OptionDataListParser::factory; FactoryMap::iterator f = factories.find(config_id); if (f == factories.end()) { @@ -1257,7 +1286,7 @@ public: /// @brief Returns Subnet4ListConfigParser object /// @param param_name name of the parameter /// @return Subnets4ListConfigParser object - static Dhcp4ConfigParser* Factory(const std::string& param_name) { + static Dhcp4ConfigParser* factory(const std::string& param_name) { return (new Subnets4ListConfigParser(param_name)); } @@ -1282,13 +1311,13 @@ namespace dhcp { Dhcp4ConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) { FactoryMap factories; - factories["valid-lifetime"] = Uint32Parser::Factory; - factories["renew-timer"] = Uint32Parser::Factory; - factories["rebind-timer"] = Uint32Parser::Factory; - factories["interface"] = InterfaceListConfigParser::Factory; - factories["subnet4"] = Subnets4ListConfigParser::Factory; - factories["option-data"] = OptionDataListParser::Factory; - factories["version"] = StringParser::Factory; + factories["valid-lifetime"] = Uint32Parser::factory; + factories["renew-timer"] = Uint32Parser::factory; + factories["rebind-timer"] = Uint32Parser::factory; + factories["interface"] = InterfaceListConfigParser::factory; + factories["subnet4"] = Subnets4ListConfigParser::factory; + factories["option-data"] = OptionDataListParser::factory; + factories["version"] = StringParser::factory; FactoryMap::iterator f = factories.find(config_id); if (f == factories.end()) { @@ -1327,12 +1356,12 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) { ParserCollection dependent_parsers; // The subnet parsers implement data inheritance by directly - // accesing global storages. For this reason the global data + // accessing global storage. For this reason the global data // parsers must store the parsed data into global storages - // immediatelly. This may cause data inconsistency if the - // parsing operation fails after the global storage have been - // already modified. We need to preserve the original global - // data here so as we can rollback changes when an error occurs. + // immediately. This may cause data inconsistency if the + // 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. Uint32Storage uint32_local(uint32_defaults); StringStorage string_local(string_defaults); OptionStorage option_local(option_defaults); @@ -1345,11 +1374,11 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) { try { - // Iterate over all independent parsers first (all but subnet6) + // Iterate over all independent parsers first (all but subnet4) // and try to parse the data. BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) { - ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first)); if (config_pair.first != "subnet4") { + ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first)); independent_parsers.push_back(parser); parser->build(config_pair.second); // The commit operation here may modify the global storage @@ -1361,8 +1390,8 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) { // Process dependent configuration data. BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) { - ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first)); if (config_pair.first == "subnet4") { + ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first)); dependent_parsers.push_back(parser); parser->build(config_pair.second); } diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index baf550dd10..f3df3ab1d8 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -78,7 +78,8 @@ public: /// @brief Create the simple configuration with single option. /// /// This function allows to set one of the parameters that configure - /// option value. These parameters are: "name", "code" and "data". + /// option value. These parameters are: "name", "code", "data" and + /// "csv-format". /// /// @param param_value string holiding option parameter value to be /// injected into the configuration string. diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index 8a4c26c988..9f27941df9 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -131,7 +131,7 @@ 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 DhcpConfigParser* factory(const std::string& param_name) { return (new DebugParser(param_name)); } @@ -158,6 +158,11 @@ public: : storage_(NULL), param_name_(param_name), value_(false) { + // Empty parameter name is invalid. + if (param_name_.empty()) { + isc_throw(isc::dhcp::Dhcp6ConfigError, "parser logic error:" + << "empty parameter name provided"); + } } /// @brief Parse a boolean value. @@ -195,7 +200,7 @@ public: /// /// @param param_name name of the parameter for which the /// parser is created. - static DhcpConfigParser* Factory(const std::string& param_name) { + static DhcpConfigParser* factory(const std::string& param_name) { return (new BooleanParser(param_name)); } @@ -241,6 +246,11 @@ public: Uint32Parser(const std::string& param_name) : storage_(&uint32_defaults), param_name_(param_name) { + // Empty parameter name is invalid. + if (param_name_.empty()) { + isc_throw(Dhcp6ConfigError, "parser logic error:" + << "empty parameter name provided"); + } } /// @brief Parses configuration configuration parameter as uint32_t. @@ -297,7 +307,7 @@ 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 DhcpConfigParser* factory(const std::string& param_name) { return (new Uint32Parser(param_name)); } @@ -339,6 +349,11 @@ public: StringParser(const std::string& param_name) : storage_(&string_defaults), param_name_(param_name) { + // Empty parameter name is invalid. + if (param_name_.empty()) { + isc_throw(Dhcp6ConfigError, "parser logic error:" + << "empty parameter name provided"); + } } /// @brief parses parameter value @@ -368,7 +383,7 @@ 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 DhcpConfigParser* factory(const std::string& param_name) { return (new StringParser(param_name)); } @@ -436,7 +451,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 DhcpConfigParser* factory(const std::string& param_name) { return (new InterfaceListConfigParser(param_name)); } @@ -474,8 +489,6 @@ public: /// (setStorage() not called) void build(ConstElementPtr pools_list) { - using namespace isc::dhcp; - // setStorage() should have been called before build if (!pools_) { isc_throw(isc::InvalidOperation, "parser logic error: no pool storage set," @@ -566,7 +579,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 DhcpConfigParser* factory(const std::string& param_name) { return (new PoolParser(param_name)); } @@ -625,8 +638,6 @@ public: /// @throw isc::BadValue if option data storage is invalid. virtual void build(ConstElementPtr option_data_entries) { - using namespace isc::dhcp; - if (options_ == NULL) { isc_throw(isc::InvalidOperation, "Parser logic error: storage must be set before " "parsing option data."); @@ -635,28 +646,28 @@ public: ParserPtr parser; if (param.first == "name") { boost::shared_ptr - name_parser(dynamic_cast(StringParser::Factory(param.first))); + name_parser(dynamic_cast(StringParser::factory(param.first))); if (name_parser) { name_parser->setStorage(&string_values_); parser = name_parser; } } else if (param.first == "code") { boost::shared_ptr - code_parser(dynamic_cast(Uint32Parser::Factory(param.first))); + code_parser(dynamic_cast(Uint32Parser::factory(param.first))); if (code_parser) { code_parser->setStorage(&uint32_values_); parser = code_parser; } } else if (param.first == "data") { boost::shared_ptr - value_parser(dynamic_cast(StringParser::Factory(param.first))); + value_parser(dynamic_cast(StringParser::factory(param.first))); if (value_parser) { value_parser->setStorage(&string_values_); parser = value_parser; } } else if (param.first == "csv-format") { boost::shared_ptr - value_parser(dynamic_cast(BooleanParser::Factory(param.first))); + value_parser(dynamic_cast(BooleanParser::factory(param.first))); if (value_parser) { value_parser->setStorage(&boolean_values_); parser = value_parser; @@ -740,8 +751,6 @@ private: /// are invalid. void createOption() { - using namespace isc::dhcp; - // 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 uint16_t and is not zero. @@ -963,7 +972,7 @@ public: /// @param param_name param name. /// /// @return DhcpConfigParser object. - static DhcpConfigParser* Factory(const std::string& param_name) { + static DhcpConfigParser* factory(const std::string& param_name) { return (new OptionDataListParser(param_name)); } @@ -998,8 +1007,6 @@ public: /// @throw isc::Dhcp6ConfigError if subnet configuration parsing failed. void build(ConstElementPtr subnet) { - using namespace isc::dhcp; - BOOST_FOREACH(ConfigPair param, subnet->mapValue()) { ParserPtr parser(createSubnet6ConfigParser(param.first)); // The actual type of the parser is unknown here. We have to discover @@ -1091,8 +1098,6 @@ private: /// @throw isc::dhcp::Dhcp6ConfigError if subnet configuration parsing failed. void createSubnet() { - using namespace isc::dhcp; - // Find a subnet string. StringStorage::const_iterator it = string_values_.find("subnet"); if (it == string_values_.end()) { @@ -1103,7 +1108,11 @@ private: string subnet_txt = it->second; boost::erase_all(subnet_txt, " "); boost::erase_all(subnet_txt, "\t"); - // Get the address portion (without length). + // The subnet format is prefix/len. We are going to extract + // the prefix portion of a subnet string to create IOAddress + // object from it. IOAddress will be passed to the Subnet's + // constructor later on. In order to extract the prefix we + // need to get all characters preceding "/". size_t pos = subnet_txt.find("/"); if (pos == string::npos) { isc_throw(Dhcp6ConfigError, @@ -1117,7 +1126,9 @@ private: // Get all 'time' parameters using inheritance. // If the subnet-specific value is defined then use it, else - // use the global value. + // use the global value. The global value must always be + // present. If it is not, it is an internal error and exception + // is thrown. Triplet t1 = getParam("renew-timer"); Triplet t2 = getParam("rebind-timer"); Triplet pref = getParam("preferred-lifetime"); @@ -1185,13 +1196,13 @@ private: DhcpConfigParser* createSubnet6ConfigParser(const std::string& config_id) { FactoryMap factories; - factories["preferred-lifetime"] = Uint32Parser::Factory; - factories["valid-lifetime"] = Uint32Parser::Factory; - factories["renew-timer"] = Uint32Parser::Factory; - factories["rebind-timer"] = Uint32Parser::Factory; - factories["subnet"] = StringParser::Factory; - factories["pool"] = PoolParser::Factory; - factories["option-data"] = OptionDataListParser::Factory; + factories["preferred-lifetime"] = Uint32Parser::factory; + factories["valid-lifetime"] = Uint32Parser::factory; + factories["renew-timer"] = Uint32Parser::factory; + factories["rebind-timer"] = Uint32Parser::factory; + factories["subnet"] = StringParser::factory; + factories["pool"] = PoolParser::factory; + factories["option-data"] = OptionDataListParser::factory; FactoryMap::iterator f = factories.find(config_id); if (f == factories.end()) { @@ -1311,7 +1322,7 @@ public: /// @brief Returns Subnet6ListConfigParser object /// @param param_name name of the parameter /// @return Subnets6ListConfigParser object - static DhcpConfigParser* Factory(const std::string& param_name) { + static DhcpConfigParser* factory(const std::string& param_name) { return (new Subnets6ListConfigParser(param_name)); } @@ -1335,14 +1346,14 @@ namespace dhcp { DhcpConfigParser* createGlobalDhcpConfigParser(const std::string& config_id) { FactoryMap factories; - factories["preferred-lifetime"] = Uint32Parser::Factory; - factories["valid-lifetime"] = Uint32Parser::Factory; - factories["renew-timer"] = Uint32Parser::Factory; - factories["rebind-timer"] = Uint32Parser::Factory; - factories["interface"] = InterfaceListConfigParser::Factory; - factories["subnet6"] = Subnets6ListConfigParser::Factory; - factories["option-data"] = OptionDataListParser::Factory; - factories["version"] = StringParser::Factory; + factories["preferred-lifetime"] = Uint32Parser::factory; + factories["valid-lifetime"] = Uint32Parser::factory; + factories["renew-timer"] = Uint32Parser::factory; + factories["rebind-timer"] = Uint32Parser::factory; + factories["interface"] = InterfaceListConfigParser::factory; + factories["subnet6"] = Subnets6ListConfigParser::factory; + factories["option-data"] = OptionDataListParser::factory; + factories["version"] = StringParser::factory; FactoryMap::iterator f = factories.find(config_id); if (f == factories.end()) { @@ -1381,12 +1392,12 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) { ParserCollection dependent_parsers; // The subnet parsers implement data inheritance by directly - // accesing global storages. For this reason the global data + // accessing global storages. For this reason the global data // parsers must store the parsed data into global storages - // immediatelly. This may cause data inconsistency if the - // parsing operation fails after the global storage have been - // already modified. We need to preserve the original global - // data here so as we can rollback changes when an error occurs. + // immediately. This may cause data inconsistency if the + // 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. Uint32Storage uint32_local(uint32_defaults); StringStorage string_local(string_defaults); OptionStorage option_local(option_defaults); @@ -1400,8 +1411,8 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) { // Iterate over all independent parsers first (all but subnet6) // and try to parse the data. BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) { - ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first)); if (config_pair.first != "subnet6") { + ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first)); independent_parsers.push_back(parser); parser->build(config_pair.second); // The commit operation here may modify the global storage @@ -1413,8 +1424,8 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) { // Process dependent configuration data. BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) { - ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first)); if (config_pair.first == "subnet6") { + ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first)); dependent_parsers.push_back(parser); parser->build(config_pair.second); } diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 9774270cac..1f024a8be6 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -56,7 +56,8 @@ public: /// @brief Create the simple configuration with single option. /// /// This function allows to set one of the parameters that configure - /// option value. These parameters are: "name", "code" and "data". + /// option value. These parameters are: "name", "code", "data" and + /// "csv-format". /// /// @param param_value string holiding option parameter value to be /// injected into the configuration string. @@ -720,7 +721,6 @@ TEST_F(Dhcp6ParserTest, optionDataLowerCase) { EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); ASSERT_TRUE(x); comment_ = parseAnswer(rcode_, x); - std::cout << comment_->str() << std::endl; ASSERT_EQ(0, rcode_); Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"));