From: Francis Dupont Date: Wed, 18 Jan 2017 01:27:14 +0000 (+0100) Subject: [5098] Improved checks and unit tests X-Git-Tag: trac102_base~2^2~2 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=2b6317f5ddc1a05b711472cdd025047b339a99cf;p=thirdparty%2Fkea.git [5098] Improved checks and unit tests --- diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index bee3613583..39f1d09267 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -647,7 +647,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { if (config_pair.first == "client-classes") { ClientClassDefListParser parser; - parser.parse(config_pair.second, AF_INET); + ClientClassDictionaryPtr dictionary = + parser.parse(config_pair.second, AF_INET); + CfgMgr::instance().getStagingCfg()->setClientClassDictionary(dictionary); continue; } diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index abdbcbe91f..923c06007c 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -923,11 +923,13 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { continue; } - if (config_pair.first =="client-classes") { - ClientClassDefListParser parser; - parser.parse(config_pair.second, AF_INET6); - continue; - } + if (config_pair.first =="client-classes") { + ClientClassDefListParser parser; + ClientClassDictionaryPtr dictionary = + parser.parse(config_pair.second, AF_INET6); + CfgMgr::instance().getStagingCfg()->setClientClassDictionary(dictionary); + continue; + } ParserPtr parser(createGlobal6DhcpConfigParser(config_pair.first, config_pair.second)); diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index 0891aee1ad..2194cffc31 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -28,12 +28,10 @@ namespace dhcp { // ********************** ExpressionParser **************************** -ExpressionParser::ExpressionParser(ExpressionPtr& expression) - : local_expression_(ExpressionPtr()), expression_(expression) { -} - void -ExpressionParser::parse(ConstElementPtr expression_cfg, uint16_t family) { +ExpressionParser::parse(ExpressionPtr& expression, + ConstElementPtr expression_cfg, + uint16_t family) { if (expression_cfg->getType() != Element::string) { isc_throw(DhcpConfigError, "expression [" << expression_cfg->str() << "] must be a string, at (" @@ -47,8 +45,8 @@ ExpressionParser::parse(ConstElementPtr expression_cfg, uint16_t family) { try { EvalContext eval_ctx(family == AF_INET ? Option::V4 : Option::V6); eval_ctx.parseString(value); - local_expression_.reset(new Expression()); - *local_expression_ = eval_ctx.expression; + expression.reset(new Expression()); + *expression = eval_ctx.expression; } catch (const std::exception& ex) { // Append position if there is a failure. isc_throw(DhcpConfigError, @@ -56,27 +54,22 @@ ExpressionParser::parse(ConstElementPtr expression_cfg, uint16_t family) { << "] error: " << ex.what() << " at (" << expression_cfg->getPosition() << ")"); } - - // Success so commit. - expression_ = local_expression_; } // ********************** ClientClassDefParser **************************** -ClientClassDefParser::ClientClassDefParser(ClientClassDictionaryPtr& class_dictionary) - : match_expr_(ExpressionPtr()), - options_(new CfgOption()), - class_dictionary_(class_dictionary) { -} - void -ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) { +ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary, + ConstElementPtr class_def_cfg, + uint16_t family) { try { std::string name; std::string next_server_txt = "0.0.0.0"; std::string sname; std::string filename; + ExpressionPtr match_expr; + CfgOptionPtr options(new CfgOption()); // Parse the elements that make up the client class definition. BOOST_FOREACH(ConfigPair param, class_def_cfg->mapValue()) { @@ -87,12 +80,12 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) { name = value->stringValue(); } else if (entry == "test") { - ExpressionParser parser(match_expr_); - parser.parse(value, family); + ExpressionParser parser; + parser.parse(match_expr, value, family); } else if (entry == "option-data") { OptionDataListParser opts_parser(family); - opts_parser.parse(options_, value); + opts_parser.parse(options, value); } else if (entry == "next-server") { next_server_txt = value->stringValue(); @@ -109,7 +102,11 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) { } } - // Make name mandatory? + // name is now mandatory + if (name.empty()) { + isc_throw(DhcpConfigError, + "not empty parameter 'name' is required"); + } // Let's parse the next-server field IOAddress next_server("0.0.0.0"); @@ -145,8 +142,8 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) { } // Add the client class definition - class_dictionary_->addClass(name, match_expr_, options_, next_server, - sname, filename); + class_dictionary->addClass(name, match_expr, options, next_server, + sname, filename); } catch (const std::exception& ex) { isc_throw(DhcpConfigError, ex.what() << " (" << class_def_cfg->getPosition() << ")"); @@ -155,20 +152,16 @@ ClientClassDefParser::parse(ConstElementPtr class_def_cfg, uint16_t family) { // ****************** ClientClassDefListParser ************************ -ClientClassDefListParser::ClientClassDefListParser() - : local_dictionary_(new ClientClassDictionary()) { -} - -void +ClientClassDictionaryPtr ClientClassDefListParser::parse(ConstElementPtr client_class_def_list, uint16_t family) { + ClientClassDictionaryPtr dictionary(new ClientClassDictionary()); BOOST_FOREACH(ConstElementPtr client_class_def, client_class_def_list->listValue()) { - ClientClassDefParser parser(local_dictionary_); - parser.parse(client_class_def, family); + ClientClassDefParser parser; + parser.parse(dictionary, client_class_def, family); } - // Success so commit - CfgMgr::instance().getStagingCfg()->setClientClassDictionary(local_dictionary_); + return (dictionary); } } // end of namespace isc::dhcp diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.h b/src/lib/dhcpsrv/parsers/client_class_def_parser.h index 47643517e9..392162bc0b 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.h +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.h @@ -53,25 +53,16 @@ namespace dhcp { /// stored into the ExpressionPtr reference passed into the constructor. class ExpressionParser : public isc::data::SimpleParser { public: - /// @brief Constructor. - /// - /// @param expression variable in which to store the new expression - ExpressionParser(ExpressionPtr& expression); /// @brief Parses an expression configuration element into an Expression /// + /// @param expression variable in which to store the new expression /// @param expression_cfg the configuration entry to be parsed. /// @param family the address family of the expression. /// /// @throw DhcpConfigError if parsing was unsuccessful. - void parse(isc::data::ConstElementPtr expression_cfg, uint16_t family); - -private: - /// @brief Local storage for the parsed expression - ExpressionPtr local_expression_; - - /// @brief Storage into which the parsed expression should be committed - ExpressionPtr& expression_; + void parse(ExpressionPtr& expression, + isc::data::ConstElementPtr expression_cfg, uint16_t family); }; /// @brief Parser for a single client class definition. @@ -79,31 +70,18 @@ private: /// This parser creates an instance of a client class definition. class ClientClassDefParser : public isc::data::SimpleParser { public: - /// @brief Constructor. - /// - /// @param class_dictionary dictionary into which the class should be added - ClientClassDefParser(ClientClassDictionaryPtr& class_dictionary); /// @brief Parses an entry that describes single client class definition. /// /// Attempts to add the new class directly into the given dictionary. /// This done here to detect duplicate classes prior to commit(). + /// @param class_dictionary dictionary into which the class should be added /// @param client_class_def a configuration entry to be parsed. /// @param family the address family of the client class. /// /// @throw DhcpConfigError if parsing was unsuccessful. - void parse(isc::data::ConstElementPtr client_class_def, uint16_t family); - -private: - - /// @brief Storage for the class match expression - ExpressionPtr match_expr_; - - /// @brief Storage for the class options - CfgOptionPtr options_; - - /// @brief Dictionary to which the new class should be added - ClientClassDictionaryPtr& class_dictionary_; + void parse(ClientClassDictionaryPtr& class_dictionary, + isc::data::ConstElementPtr client_class_def, uint16_t family); }; /// @brief Defines a pointer to a ClientClassDefParser @@ -118,26 +96,19 @@ typedef boost::shared_ptr ClientClassDefParserPtr; class ClientClassDefListParser : public isc::data::SimpleParser { public: - /// @brief Constructor. - ClientClassDefListParser(); - /// @brief Parse configuration entries. /// /// This function parses configuration entries, creates instances - /// of client class definitions and tries to adds them to the a - /// local dictionary. At the end class definitions are committed - /// to CfgMgr's global storage. + /// of client class definitions and tries to adds them to the + /// local dictionary. At the end the dictionary is returned. /// /// @param class_def_list pointer to an element that holds entries /// for client class definitions. /// @param family the address family of the client class definitions. + /// @return a pointer to the filled dictionary /// @throw DhcpConfigError if configuration parsing fails. - void parse(isc::data::ConstElementPtr class_def_list, uint16_t family); - - /// @brief Local class dictionary to store classes as they are being parsed - /// - /// Left public for easier unit testing. - ClientClassDictionaryPtr local_dictionary_; + ClientClassDictionaryPtr + parse(isc::data::ConstElementPtr class_def_list, uint16_t family); }; } // end of namespace isc::dhcp diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 5ab02d1b15..20fc7b4896 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -364,7 +364,7 @@ public: class_dictionary_ = dictionary; } - /// @brief Copies the currnet configuration to a new configuration. + /// @brief Copies the current configuration to a new configuration. /// /// This method copies the parameters stored in the configuration to /// an object passed as parameter. The configuration sequence is not diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc index 5a692892e4..17eeb9a97f 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc @@ -51,13 +51,13 @@ protected: const std::string& expression, const std::string& option_string) { ExpressionPtr parsed_expr; - ExpressionParser parser(parsed_expr); + ExpressionParser parser; // Turn config into elements. This may emit exceptions. ElementPtr config_element = Element::fromJSON(expression); // Expression should parse. - ASSERT_NO_THROW(parser.parse(config_element, family)); + ASSERT_NO_THROW(parser.parse(parsed_expr, config_element, family)); // Parsed expression should exist. ASSERT_TRUE(parsed_expr); @@ -98,15 +98,15 @@ protected: /// or by the parsing itself are not caught ClientClassDefPtr parseClientClassDef(const std::string& config, uint16_t family) { - // Create local dicitonary to which the parser add the class. + // Create local dictionary to which the parser add the class. ClientClassDictionaryPtr dictionary(new ClientClassDictionary()); // Turn config into elements. This may emit exceptions. ElementPtr config_element = Element::fromJSON(config); // Parse the configuration. This may emit exceptions. - ClientClassDefParser parser(dictionary); - parser.parse(config_element, family); + ClientClassDefParser parser; + parser.parse(dictionary, config_element, family); // If we didn't throw, then return the first and only class ClientClassDefMapPtr classes = dictionary->getClasses(); @@ -144,10 +144,7 @@ protected: // Parse the configuration. This may emit exceptions. ClientClassDefListParser parser; - parser.parse(config_element, family); - - // Return the parser's local dicationary - return (parser.local_dictionary_); + return (parser.parse(config_element, family)); } }; @@ -223,10 +220,11 @@ TEST_F(ExpressionParserTest, invalidExpressionElement) { // Create the parser. ExpressionPtr parsed_expr; - ExpressionParser parser(parsed_expr); + ExpressionParser parser; // Expression parsing should fail. - ASSERT_THROW(parser.parse(config_element, AF_INET), DhcpConfigError); + ASSERT_THROW(parser.parse(parsed_expr, config_element, AF_INET6), + DhcpConfigError); } // Verifies that given an invalid expression with a syntax error, @@ -241,10 +239,25 @@ TEST_F(ExpressionParserTest, expressionSyntaxError) { // Create the parser. ExpressionPtr parsed_expr; - ExpressionParser parser(parsed_expr); + ExpressionParser parser; // Expression parsing should fail. - ASSERT_THROW(parser.parse(config_element, AF_INET), DhcpConfigError); + ASSERT_THROW(parser.parse(parsed_expr, config_element, AF_INET), + DhcpConfigError); +} + +// Verifies that the name parameter is required and must not be empty +TEST_F(ExpressionParserTest, nameEmpty) { + std::string cfg_txt = "{ \"name\": \"\" }"; + ElementPtr config_element = Element::fromJSON(cfg_txt); + + // Create the parser. + ExpressionPtr parsed_expr; + ExpressionParser parser; + + // Expression parsing should fail. + ASSERT_THROW(parser.parse(parsed_expr, config_element, AF_INET6), + DhcpConfigError); } // Verifies you can create a class with only a name @@ -279,6 +292,7 @@ TEST_F(ClientClassDefParserTest, nameOnlyValid) { // Verifies you can create a class with a name, expression, // but no options. +// @todo same with AF_INET6 TEST_F(ClientClassDefParserTest, nameAndExpressionClass) { std::string cfg_text = @@ -320,6 +334,7 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) { // Verifies you can create a class with a name and options, // but no expression. +// @todo same with AF_INET6 TEST_F(ClientClassDefParserTest, nameAndOptionsClass) { std::string cfg_text = @@ -355,6 +370,7 @@ TEST_F(ClientClassDefParserTest, nameAndOptionsClass) { // Verifies you can create a class with a name, expression, // and options. +// @todo same with AF_INET6 TEST_F(ClientClassDefParserTest, basicValidClass) { std::string cfg_text = @@ -466,7 +482,7 @@ TEST_F(ClientClassDefParserTest, invalidExpression) { "} \n"; ClientClassDefPtr cclass; - ASSERT_THROW(cclass = parseClientClassDef(cfg_text, AF_INET), + ASSERT_THROW(cclass = parseClientClassDef(cfg_text, AF_INET6), DhcpConfigError); } @@ -503,7 +519,7 @@ TEST_F(ClientClassDefListParserTest, simpleValidList) { // Parsing the list should succeed. ClientClassDictionaryPtr dictionary; - ASSERT_NO_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET)); + ASSERT_NO_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET6)); ASSERT_TRUE(dictionary); // We should have three classes in the dictionary. @@ -526,13 +542,6 @@ TEST_F(ClientClassDefListParserTest, simpleValidList) { // For good measure, make sure we can't find a non-existant class. ASSERT_NO_THROW(cclass = dictionary->findClass("bogus")); EXPECT_FALSE(cclass); - - // Verify that the dictionary was pushed to the CfgMgr's staging config. - SrvConfigPtr staging = CfgMgr::instance().getStagingCfg(); - ASSERT_TRUE(staging); - ClientClassDictionaryPtr staged_dictionary = staging->getClientClassDictionary(); - ASSERT_TRUE(staged_dictionary); - EXPECT_TRUE(*staged_dictionary == *dictionary); } // Verifies that class list containing a duplicate class entries, fails @@ -568,12 +577,13 @@ TEST_F(ClientClassDefListParserTest, invalidClass) { "] \n"; ClientClassDictionaryPtr dictionary; - ASSERT_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET), + ASSERT_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET6), DhcpConfigError); } // Test verifies that without any class specified, the fixed fields have their // default, empty value. +// @todo same with AF_INET6 TEST_F(ClientClassDefParserTest, noFixedFields) { std::string cfg_text =