[4483] Review comments addressed

 - Implemented new test for checking integer boundaries
 - checkTokenInteger documented
 - indentation fixed in parser.yy
 - integer check in lexer.ll extended from int to int64_t
 - a bug fixed in EvalContext::convertUint32
 - User's Guide now explicitly mentions network order

diff --git a/doc/guide/classify.xml b/doc/guide/classify.xml
index 7876aed05911cb305cace97b59931f219f69b6fc..765daf71d2ff6c8ea58aeb9fb819663a508acb72 100644 (file)
--- a/doc/guide/classify.xml
+++ b/doc/guide/classify.xml
@@ -433,7 +433,9 @@
       it is easier to use decimal notation to represent integers, but it is also
       possible to use hex notation. When using hex notation to represent an
       integer care should be taken to make sure the value is represented as 32
-      bits, e.g. use 0x00000001 instead of 0x1 or 0x01.
+      bits, e.g. use 0x00000001 instead of 0x1 or 0x01. Also, make
+      sure the value is specified in network order, e.g. 1 is
+      represented as 0x00000001.
       </para>
 
       <para>

diff --git a/src/lib/eval/eval_context.cc b/src/lib/eval/eval_context.cc
index ff1a0c86099a64d773e8a2e007af181c78a0fe44..8cd13653b5707f98f3b1f8f66b940fde06fffd59 100644 (file)
--- a/src/lib/eval/eval_context.cc
+++ b/src/lib/eval/eval_context.cc
@@ -139,7 +139,7 @@ EvalContext::convertUint32(const std::string& number,
     } catch (const boost::bad_lexical_cast &) {
         error(loc, "Invalid value in " + number);
     }
-    if (n >= std::numeric_limits<uint32_t>::max()) {
+    if (n > std::numeric_limits<uint32_t>::max()) {
         error(loc, "Invalid value in "
               + number + ". Allowed range: 0..4294967295");
     }

diff --git a/src/lib/eval/lexer.ll b/src/lib/eval/lexer.ll
index fdefcecd6386a4c84ec26477eb8e796962b649e9..f7bd53593aef8a53c8c912307d50a8511ac4c6c3 100644 (file)
--- a/src/lib/eval/lexer.ll
+++ b/src/lib/eval/lexer.ll
@@ -110,7 +110,11 @@ addr6 [0-9a-fA-F]*\:[0-9a-fA-F]*\:[0-9a-fA-F:.]*
     std::string tmp(yytext);
 
     try {
-        static_cast<void>(boost::lexical_cast<int>(tmp));
+        // In substring we want to use negative values (e.g. -1).
+        // In enterprise-id we need to use values up to 0xffffffff.
+        // To cover both of those use cases, we need at least
+        // int64_t.
+        static_cast<void>(boost::lexical_cast<int64_t>(tmp));
     } catch (const boost::bad_lexical_cast &) {
         driver.error(loc, "Failed to convert " + tmp + " to an integer.");
     }

diff --git a/src/lib/eval/parser.yy b/src/lib/eval/parser.yy
index aad6c8e88d4f7a142dd7150bcfae8f700bb60d4d..5bfadcfed055a420475b23d7621dd1613945126e 100644 (file)
--- a/src/lib/eval/parser.yy
+++ b/src/lib/eval/parser.yy
@@ -371,18 +371,18 @@ string_expr : STRING
                                                                TokenVendor::DATA, index));
                     ctx.expression.push_back(vendor_class);
                 }
-             | integer_expr
+            | integer_expr
                 {
                     TokenPtr integer(new TokenInteger($1));
                     ctx.expression.push_back(integer);
                 }
             ;
 
-integer_expr: INTEGER
-              {
-                  $$ = ctx.convertUint32($1, @1);
-              }
-            ;
+integer_expr : INTEGER
+                 {
+                     $$ = ctx.convertUint32($1, @1);
+                 }
+             ;
 
 option_code : INTEGER
                  {

diff --git a/src/lib/eval/tests/context_unittest.cc b/src/lib/eval/tests/context_unittest.cc
index 6272c9d88f330feec323bc8eefb069e9c7d9d1f6..25b02b47f7dbe9a9d3bbb65c211d60cfe708df0e 100644 (file)
--- a/src/lib/eval/tests/context_unittest.cc
+++ b/src/lib/eval/tests/context_unittest.cc
@@ -96,6 +96,10 @@ public:
         EXPECT_TRUE(eq);
     }
 
+    /// @brief Checks if the given token is integer with expected value
+    ///
+    /// @param token token to be inspected
+    /// @param exp_value expected integer value of the token
     void checkTokenInteger(const TokenPtr& token, uint32_t exp_value) {
         ASSERT_TRUE(token);
         boost::shared_ptr<TokenInteger> integer =

diff --git a/src/lib/eval/tests/evaluate_unittest.cc b/src/lib/eval/tests/evaluate_unittest.cc
index d27fd652b6abf9b90303a234aa3cc4528d74f91c..8e40547972a0b808a2c296d228293cefea34ce7e 100644 (file)
--- a/src/lib/eval/tests/evaluate_unittest.cc
+++ b/src/lib/eval/tests/evaluate_unittest.cc
@@ -314,7 +314,8 @@ public:
         bool result;
         bool parsed;
 
-        EXPECT_NO_THROW(parsed = eval.parseString(expr));
+        EXPECT_NO_THROW(parsed = eval.parseString(expr))
+            << " while parsing expression " << expr;
         EXPECT_TRUE(parsed) << " for expression " << expr;
 
         switch (u) {
@@ -330,6 +331,19 @@ public:
 
         EXPECT_EQ(exp_result, result) << " for expression " << expr;
     }
+
+    /// @brief Checks that specified expression throws expected exception.
+    ///
+    /// @tparam ex exception type expected to be thrown
+    /// @param expr expression to be evaluated
+    template<typename ex>
+    void testExpressionNegative(const std::string& expr,
+                                const Option::Universe& u = Option::V4) {
+        EvalContext eval(u);
+
+        EXPECT_THROW(eval.parseString(expr), ex) << "while parsing expression "
+                                                 << expr;
+    }
 };
 
 // This is a quick way to check if certain expressions are valid or not and
@@ -402,6 +416,8 @@ TEST_F(ExpressionsTest, expressionsPkt4Hlen) {
     testExpression(Option::V4, "pkt4.mac == 0x010203040506", true);
 }
 
+// Test if expressions message type can be detected in Pkt4.
+// It also doubles as a check for integer comparison here.
 TEST_F(ExpressionsTest, expressionsPkt4type) {
 
     // We can inspect the option content directly, but
@@ -416,4 +432,17 @@ TEST_F(ExpressionsTest, expressionsPkt4type) {
     testExpression(Option::V4, "pkt4.msgtype == 2", false);
 }
 
+// This tests if inappropriate values (negative, too large) are
+// rejected, but extremal value still allowed for uint32_t are ok.
+TEST_F(ExpressionsTest, invalidIntegers) {
+
+    // These are the extremal uint32_t values that still should be accepted.
+    testExpression(Option::V4, "4294967295 == 0", false);
+
+    // Negative integers should be rejected.
+    testExpressionNegative<EvalParseError>("4294967295 == -1");
+
+    // Oops, one too much.
+    testExpressionNegative<EvalParseError>("4294967296 == 0");
+}
 };