[5132] Changes after review (comment #10):

 - Missing break added
 - LAST_IDENTIFIER_TYPE updated
 - auto now includes flex-id
 - HostTest::getIdentifier unit-test updated
 - bool_expr | string_expr removed
 - unit-tests now check that bool expressions are rejected for strings

diff --git a/src/lib/dhcpsrv/host.cc b/src/lib/dhcpsrv/host.cc
index 32b5e39bcd8a1787de4ba22b7a0576ddcc7baec7..702d37d9b85e6e231c50cd5ddce750163362ab64 100644 (file)
--- a/src/lib/dhcpsrv/host.cc
+++ b/src/lib/dhcpsrv/host.cc
@@ -207,6 +207,7 @@ Host::getIdentifierAsText(const IdentifierType& type, const uint8_t* value,
         break;
     case IDENT_FLEX:
         s << "flex-id";
+        break;
     default:
         // This should never happen actually, unless we add new identifier
         // and forget to add a case for it above.

diff --git a/src/lib/dhcpsrv/host.h b/src/lib/dhcpsrv/host.h
index a89242a2941ffc12a02d5580c7f2916aaacfe47d..c3facde0e78f4a012c82ff81b0577020f6ce6157 100644 (file)
--- a/src/lib/dhcpsrv/host.h
+++ b/src/lib/dhcpsrv/host.h
@@ -190,7 +190,7 @@ public:
 
     /// @brief Constant pointing to the last identifier of the
     /// @ref IdentifierType enumeration.
-    static const IdentifierType LAST_IDENTIFIER_TYPE = IDENT_CLIENT_ID;
+    static const IdentifierType LAST_IDENTIFIER_TYPE = IDENT_FLEX;
 
     /// @brief Constructor.
     ///

diff --git a/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc b/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc
index f29ecd417355d70df294a0b5a69abe4e6bbea755..12c6dbfe0bde3098cdf2e10e0b829ee121ae83da 100644 (file)
--- a/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc
+++ b/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc
@@ -1311,13 +1311,14 @@ TEST_F(HostReservationIdsParserTest, dhcp4AutoIdentifiers) {
     ConstCfgHostOperationsPtr cfg = CfgMgr::instance().getStagingCfg()->
         getCfgHostOperations4();
     const CfgHostOperations::IdentifierTypes& ids = cfg->getIdentifierTypes();
-    ASSERT_EQ(4, ids.size());
+    ASSERT_EQ(5, ids.size());
 
     CfgHostOperations::IdentifierTypes::const_iterator id = ids.begin();
     EXPECT_EQ(*id++, Host::IDENT_HWADDR);
     EXPECT_EQ(*id++, Host::IDENT_DUID);
     EXPECT_EQ(*id++, Host::IDENT_CIRCUIT_ID);
     EXPECT_EQ(*id++, Host::IDENT_CLIENT_ID);
+    EXPECT_EQ(*id++, Host::IDENT_FLEX);
 }
 
 // This test verifies that use of "auto" together with an explicit
@@ -1354,11 +1355,12 @@ TEST_F(HostReservationIdsParserTest, dhcp6AutoIdentifiers) {
     ConstCfgHostOperationsPtr cfg = CfgMgr::instance().getStagingCfg()->
         getCfgHostOperations6();
     const CfgHostOperations::IdentifierTypes& ids = cfg->getIdentifierTypes();
-    ASSERT_EQ(2, ids.size());
+    ASSERT_EQ(3, ids.size());
 
     CfgHostOperations::IdentifierTypes::const_iterator id = ids.begin();
     EXPECT_EQ(*id++, Host::IDENT_HWADDR);
     EXPECT_EQ(*id++, Host::IDENT_DUID);
+    EXPECT_EQ(*id++, Host::IDENT_FLEX);
 }
 
 // This test verifies that use of "auto" together with an explicit

diff --git a/src/lib/dhcpsrv/tests/host_unittest.cc b/src/lib/dhcpsrv/tests/host_unittest.cc
index 53caef855bbeed8f4928608002e8cf1094208fb1..2964f888cd9c141720f03cfd8eb4175dd9f5aabc 100644 (file)
--- a/src/lib/dhcpsrv/tests/host_unittest.cc
+++ b/src/lib/dhcpsrv/tests/host_unittest.cc
@@ -182,8 +182,9 @@ TEST_F(HostTest, getIdentifier) {
     EXPECT_EQ(Host::IDENT_DUID, Host::getIdentifierType("duid"));
     EXPECT_EQ(Host::IDENT_CIRCUIT_ID, Host::getIdentifierType("circuit-id"));
     EXPECT_EQ(Host::IDENT_CLIENT_ID, Host::getIdentifierType("client-id"));
+    EXPECT_EQ(Host::IDENT_FLEX, Host::getIdentifierType("flex-id"));
 
-    EXPECT_THROW(Host::getIdentifierType("unuspported"), isc::BadValue);
+    EXPECT_THROW(Host::getIdentifierType("unusupported"), isc::BadValue);
 }
 
 // This test verifies that it is possible to create a Host object

diff --git a/src/lib/eval/parser.yy b/src/lib/eval/parser.yy
index 9d31b01a67c2ecb3dc2c0248e7039aa27c4fc750..26b58424b1a4d6ef45aa55ef2c52389c64454846 100644 (file)
--- a/src/lib/eval/parser.yy
+++ b/src/lib/eval/parser.yy
@@ -115,13 +115,7 @@ using namespace isc::eval;
 // ... that expects either TOPLEVEL_BOOL or TOPLEVEL_STRING. Depending on which
 // token appears first, it will determine what is allowed and what it not.
 start: TOPLEVEL_BOOL expression
-     | TOPLEVEL_STRING string_expression
-;
-
-// string expression can be either a string (proper) or boolean (that is internally
-// stored as "true" or "false")
-string_expression: bool_expr
-      | string_expr
+     | TOPLEVEL_STRING string_expr
 ;
 
 // Expression can either be a single token or a (something == something) expression

diff --git a/src/lib/eval/tests/evaluate_unittest.cc b/src/lib/eval/tests/evaluate_unittest.cc
index b8a69d8180fa60fce290076b6927f999003dc728..37ade2798df7513f2c8b4cad3c1055ff2dbfd927 100644 (file)
--- a/src/lib/eval/tests/evaluate_unittest.cc
+++ b/src/lib/eval/tests/evaluate_unittest.cc
@@ -366,10 +366,11 @@ public:
     /// @param expr expression to be evaluated
     template<typename ex>
     void testExpressionNegative(const std::string& expr,
-                                const Option::Universe& u = Option::V4) {
+                                const Option::Universe& u = Option::V4,
+                                EvalContext::ParserType type = EvalContext::PARSER_BOOL) {
         EvalContext eval(u);
 
-        EXPECT_THROW(eval.parseString(expr), ex) << "while parsing expression "
+        EXPECT_THROW(eval.parseString(expr, type), ex) << "while parsing expression "
                                                  << expr;
     }
 };
@@ -477,10 +478,18 @@ TEST_F(ExpressionsTest, invalidIntegers) {
 // Tests whether expressions can be evaluated to a string.
 TEST_F(ExpressionsTest, evaluateString) {
 
+    // Check that content of the options is returned properly.
     testExpressionString(Option::V4, "option[100].hex", "hundred4");
     testExpressionString(Option::V6, "option[100].hex", "hundred6");
+
+    // Check that content of non-existing option returns empty string.
     testExpressionString(Option::V4, "option[200].hex", "");
     testExpressionString(Option::V6, "option[200].hex", "");
+
+    testExpressionNegative<EvalParseError>("pkt4.msgtype == 1", Option::V4,
+                                           EvalContext::PARSER_STRING);
+    testExpressionNegative<EvalParseError>("pkt6.msgtype == 1", Option::V6,
+                                           EvalContext::PARSER_STRING);
 }
 
 };