From: Thomas Markwalder Date: Thu, 19 Nov 2015 14:32:21 +0000 (-0500) Subject: [4096] Integrated use of Eval expression parsing X-Git-Tag: trac4097a_base~1^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5305d4ea4d87383ffdc97285c1f226a2846acbd5;p=thirdparty%2Fkea.git [4096] Integrated use of Eval expression parsing src/lib/dhcpsrv/client_class_def.cc - updated comment on empty expressions in ctor, - cleaned up whitespace src/lib/dhcpsrv/parsers/client_class_def_parser.cc - ExpressionParser::build() - integrated use of Eval parsing in ExpressionParser - cleaned up whitespace src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc Updated tests to verify that expressions are actually parsed and function. --- diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index 45fcccaf33..87b2610a88 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -29,7 +29,9 @@ ClientClassDef::ClientClassDef(const std::string& name, if (name_.empty()) { isc_throw(BadValue, "ClientClassDef name cannot be empty"); } - // @todo Does it make sense for a class to NOT have match expression? + + // We permit an empty expression for now. This will likely be useful + // for automatic classes such as vendor class. // For classes without options, make sure we have an empty collection if (!cfg_option_) { @@ -37,7 +39,7 @@ ClientClassDef::ClientClassDef(const std::string& name, } } -ClientClassDef::ClientClassDef(const ClientClassDef& rhs) +ClientClassDef::ClientClassDef(const ClientClassDef& rhs) : name_(rhs.name_), match_expr_(ExpressionPtr()), cfg_option_(new CfgOption()) { @@ -88,10 +90,10 @@ bool ClientClassDef::equals(const ClientClassDef& other) const { return ((name_ == other.name_) && ((!match_expr_ && !other.match_expr_) || - (match_expr_ && other.match_expr_ && + (match_expr_ && other.match_expr_ && (*match_expr_ == *(other.match_expr_)))) && ((!cfg_option_ && !other.cfg_option_) || - (cfg_option_ && other.cfg_option_ && + (cfg_option_ && other.cfg_option_ && (*cfg_option_ == *other.cfg_option_)))); } @@ -168,9 +170,9 @@ ClientClassDictionary::equals(const ClientClassDictionary& other) const { ClientClassDefMap::iterator this_class = classes_->begin(); ClientClassDefMap::iterator other_class = other.classes_->begin(); - while (this_class != classes_->end() && + while (this_class != classes_->end() && other_class != other.classes_->end()) { - if (!(*this_class).second || !(*other_class).second || + if (!(*this_class).second || !(*other_class).second || (*(*this_class).second) != (*(*other_class).second)) { return false; } diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index c8f8d5eb50..96f10cdc29 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -16,6 +16,8 @@ #include #include #include +#include + #include using namespace isc::data; @@ -31,27 +33,33 @@ namespace dhcp { ExpressionParser::ExpressionParser(const std::string&, ExpressionPtr& expression, ParserContextPtr global_context) - : local_expression_(ExpressionPtr()), expression_(expression), + : local_expression_(ExpressionPtr()), expression_(expression), global_context_(global_context) { } void ExpressionParser::build(ConstElementPtr expression_cfg) { - // Here is where we would call bison parser with our string - // For now we'll just create an expression with a string token - // containing the expression text + if (expression_cfg->getType() != Element::string) { + isc_throw(DhcpConfigError, "expression [" + << expression_cfg->str() << "] must be a string, at (" + << expression_cfg->getPosition() << ")"); + } + + // Get the expression's text via getValue() as the text returned + // by str() enclosed in quotes. + std::string value; + expression_cfg->getValue(value); try { - if (expression_cfg->getType() != Element::string) { - isc_throw(TypeError, "expression value must be a string"); - } - std::string expression_text = expression_cfg->str(); - TokenPtr dummy(new TokenString(expression_text)); + EvalContext eval_ctx; + eval_ctx.parseString(value); local_expression_.reset(new Expression()); - local_expression_->push_back(dummy); + *local_expression_ = eval_ctx.expression; } catch (const std::exception& ex) { // Append position if there is a failure. - isc_throw(DhcpConfigError, ex.what() << " (" - << expression_cfg->getPosition() << ")"); + isc_throw(DhcpConfigError, + "expression: [" << value + << "] error: " << ex.what() << " at (" + << expression_cfg->getPosition() << ")"); } } @@ -121,9 +129,9 @@ ClientClassDefParser::build(ConstElementPtr class_def_cfg) { // ****************** ClientClassDefListParser ************************ ClientClassDefListParser::ClientClassDefListParser(const std::string&, - ParserContextPtr + ParserContextPtr global_context) - : local_dictionary_(new ClientClassDictionary()), + : local_dictionary_(new ClientClassDictionary()), global_context_(global_context) { } @@ -135,10 +143,10 @@ ClientClassDefListParser::build(ConstElementPtr client_class_def_list) { << client_class_def_list->getPosition() << ")"); } - BOOST_FOREACH(ConstElementPtr client_class_def, + BOOST_FOREACH(ConstElementPtr client_class_def, client_class_def_list->listValue()) { boost::shared_ptr - parser(new ClientClassDefParser("client-class-def", + parser(new ClientClassDefParser("client-class-def", local_dictionary_, global_context_)); parser->build(client_class_def); 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 25936ccda5..fd831aaf98 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc @@ -15,8 +15,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -109,50 +111,78 @@ protected: } }; - -// Verifies basic operation of an ExpressionParser. Until we tie -// this into the actual Bison parsing there's not much to test. -TEST(ExpressionParserTest, simpleStringExpression) { +// Verifies that given a valid expression, the ExpressionParser +// produces an Expression which can be evaluated against a v4 packet. +TEST(ExpressionParserTest, validExpression4) { ParserContextPtr context(new ParserContext(Option::V4)); ExpressionParserPtr parser; ExpressionPtr parsed_expr; // Turn config into elements. This may emit exceptions. - std::string cfg_txt = "\"astring\""; + std::string cfg_txt = "\"option[100] == 'hundred4'\""; ElementPtr config_element = Element::fromJSON(cfg_txt); // Create the parser. ASSERT_NO_THROW(parser.reset(new ExpressionParser("", parsed_expr, context))); // Expression should parse and commit. + (parser->build(config_element)); ASSERT_NO_THROW(parser->build(config_element)); ASSERT_NO_THROW(parser->commit()); // Parsed expression should exist. ASSERT_TRUE(parsed_expr); - // Evaluate it. For now the result will be the - // expression string as dummy ExpressionParser - // just makes an expression of one TokenString - // containing the expression string itself. - ValueStack vstack; - Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 12345)); - (*parsed_expr)[0]->evaluate(*pkt4, vstack); - EXPECT_EQ(vstack.top(), "\"astring\""); + // Build a packet that will fail evaluation. + Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 123)); + EXPECT_FALSE(evaluate(*parsed_expr, *pkt4)); + + // Now add the option so it will pass. + OptionPtr opt(new OptionString(Option::V4, 100, "hundred4")); + pkt4->addOption(opt); + EXPECT_TRUE(evaluate(*parsed_expr, *pkt4)); +} + +// Verifies that given a valid expression, the ExpressionParser +// produces an Expression which can be evaluated against a v6 packet. +TEST(ExpressionParserTest, validExpression6) { + ParserContextPtr context(new ParserContext(Option::V6)); + ExpressionParserPtr parser; + ExpressionPtr parsed_expr; + + // Turn config into elements. This may emit exceptions. + std::string cfg_txt = "\"option[100] == 'hundred6'\""; + ElementPtr config_element = Element::fromJSON(cfg_txt); + + // Create the parser. + ASSERT_NO_THROW(parser.reset(new ExpressionParser("", parsed_expr, + context))); + // Expression should parse and commit. + (parser->build(config_element)); + ASSERT_NO_THROW(parser->build(config_element)); + ASSERT_NO_THROW(parser->commit()); + + // Parsed expression should exist. + ASSERT_TRUE(parsed_expr); + + // Build a packet that will fail evaluation. + Pkt6Ptr pkt6(new Pkt6(DHCPDISCOVER, 123)); + EXPECT_FALSE(evaluate(*parsed_expr, *pkt6)); + + // Now add the option so it will pass. + OptionPtr opt(new OptionString(Option::V6, 100, "hundred6")); + pkt6->addOption(opt); + EXPECT_TRUE(evaluate(*parsed_expr, *pkt6)); } -// Verifies that given an invalid expression, the Expression parser -// will throw a DhdpConfigError. Note this is not intended to be -// an exhaustive test or expression syntax. It is simply to ensure -// that if the parser fails, it does so properly. For now, since -// our parser is a dummy parser which only checks that it's given -// Element::string so send it an integer. -TEST(ExpressionParserTest, invalidExpression) { + +// Verifies that an the ExpressionParser only accepts StringElements. +TEST(ExpressionParserTest, invalidExpressionElement) { ParserContextPtr context(new ParserContext(Option::V4)); ExpressionParserPtr parser; ExpressionPtr parsed_expr; - // Turn config into elements. + // This will create an integer element should fail. std::string cfg_txt = "777"; ElementPtr config_element = Element::fromJSON(cfg_txt); @@ -163,6 +193,27 @@ TEST(ExpressionParserTest, invalidExpression) { ASSERT_THROW(parser->build(config_element), DhcpConfigError); } +// Verifies that given an invalid expression with a syntax error, +// the Expression parser will throw a DhdpConfigError. Note this +// is not intended to be an exhaustive test or expression syntax. +// It is simply to ensure that if the parser fails, it does so +// Properly. +TEST(ExpressionParserTest, expressionSyntaxError) { + ParserContextPtr context(new ParserContext(Option::V4)); + ExpressionParserPtr parser; + ExpressionPtr parsed_expr; + + // Turn config into elements. + std::string cfg_txt = "\"option 'bogus'\""; + ElementPtr config_element = Element::fromJSON(cfg_txt); + + // Create the parser. + ASSERT_NO_THROW(parser.reset(new ExpressionParser("", parsed_expr, + context))); + // Expressionn build() should fail. + ASSERT_THROW(parser->build(config_element), DhcpConfigError); +} + // Verifies you can create a class with only a name // Whether that's useful or not, remains to be seen. // For now the class allows it. @@ -199,8 +250,8 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) { std::string cfg_text = "{ \n" - " \"name\": \"MICROSOFT\", \n" - " \"test\": \"vendor-class-identifier == 'MSFT'\" \n" + " \"name\": \"class_one\", \n" + " \"test\": \"option[100] == 'works right'\" \n" "} \n"; ClientClassDefPtr cclass; @@ -208,7 +259,7 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) { // We should find our class. ASSERT_TRUE(cclass); - EXPECT_EQ("MICROSOFT", cclass->getName()); + EXPECT_EQ("class_one", cclass->getName()); // CfgOption should be a non-null pointer but there // should be no options. Currently there's no good @@ -224,15 +275,14 @@ TEST_F(ClientClassDefParserTest, nameAndExpressionClass) { ExpressionPtr match_expr = cclass->getMatchExpr(); ASSERT_TRUE(match_expr); - // Evaluate it. For now the result will be the - // expression string as dummy ExpressionParser - // just makes an expression of one TokenString - // containing the expression string itself. - ValueStack vstack; - Pkt4Ptr pkt4; - pkt4.reset(new Pkt4(DHCPDISCOVER, 12345)); - (*match_expr)[0]->evaluate(*pkt4, vstack); - EXPECT_EQ(vstack.top(), "\"vendor-class-identifier == 'MSFT'\""); + // Build a packet that will fail evaluation. + Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 123)); + EXPECT_FALSE(evaluate(*match_expr, *pkt4)); + + // Now add the option so it will pass. + OptionPtr opt(new OptionString(Option::V4, 100, "works right")); + pkt4->addOption(opt); + EXPECT_TRUE(evaluate(*match_expr, *pkt4)); } // Verifies you can create a class with a name and options, @@ -277,7 +327,7 @@ TEST_F(ClientClassDefParserTest, basicValidClass) { std::string cfg_text = "{ \n" " \"name\": \"MICROSOFT\", \n" - " \"test\": \"vendor-class-identifier == 'MSFT'\", \n" + " \"test\": \"option[100] == 'booya'\", \n" " \"option-data\": [ \n" " { \n" " \"name\": \"domain-name-servers\", \n" @@ -305,15 +355,14 @@ TEST_F(ClientClassDefParserTest, basicValidClass) { ExpressionPtr match_expr = cclass->getMatchExpr(); ASSERT_TRUE(match_expr); - // Evaluate it. For now the result will be the - // expression string as dummy ExpressionParser - // just makes an expression of one TokenString - // containing the expression string itself. - ValueStack vstack; - Pkt4Ptr pkt4; - pkt4.reset(new Pkt4(DHCPDISCOVER, 12345)); - (*match_expr)[0]->evaluate(*pkt4, vstack); - EXPECT_EQ(vstack.top(), "\"vendor-class-identifier == 'MSFT'\""); + // Build a packet that will fail evaluation. + Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 123)); + EXPECT_FALSE(evaluate(*match_expr, *pkt4)); + + // Now add the option so it will pass. + OptionPtr opt(new OptionString(Option::V4, 100, "booya")); + pkt4->addOption(opt); + EXPECT_TRUE(evaluate(*match_expr, *pkt4)); } // Verifies that a class with no name, fails to parse.