[4271] Changes after review

 - User's Guide clearified couple things.
 - messages orded alphabetically
 - indentation increased in parser.yy
 - debugging message formats updated slightly ('1234' => 0x1234)
 - one new case in vendorClass4SpecificVendorData added
 - verbose_ flag in log_utils.h commented

doc/guide/classify.xml

@@ -457,12 +457,20 @@
         is for the second data chunk (if present), etc.
       </para>
 
-      <para>Asterisk (*) or 0 can be used to specify wildcard enterprise-id
-      value, i.e. it will match any enterprise-id value.</para>
+      <para>In the vendor and vendor-class constructs Asterisk (*) or 0 can be
+      used to specify a wildcard enterprise-id value, i.e. it will match any
+      enterprise-id value.</para>
 
       <para>Vendor Class Identifier (option 60 in DHCPv4) can be
       accessed using option[60] expression.</para>
 
+      <para>RFC3925 and RFC3315 allow for multiple instances of vendor options
+      to appear in a single message. The client classification code currently
+      examines the first instance if more than one appear. For vendor.enterprise
+      and vendor-class.enterprise expressions, the value from the first instance
+      is returned. Please submit a feature request on Kea website if you need
+      support for multiple instances.</para>
+
       <para>
         <table frame="all" id="classification-expressions-list">
           <title>List of Classification Expressions</title>

src/lib/eval/eval_messages.mes

@@ -107,60 +107,52 @@ string and an empty result will be pushed onto the stack.  The start,
 length and string are still popped from the stack and the result is
 still pushed.  The strings are displayed in hex.
 
-% EVAL_RESULT Expression %1 evaluated to %2
+% EVAL_DEBUG_VENDOR_CLASS_DATA Data %1 (out of %2 received) in vendor class found, pushing result '%3'
+This debug message indicates that vendor class option was found and passed
+enterprise-id checks and has sufficient number of data chunks. The total number
+of chunks and value pushed are reported as debugging aid.
+
+% EVAL_DEBUG_VENDOR_CLASS_DATA_NOT_FOUND Requested data index %1, but option with enterprise-id %2 has only %3 data tuple(s), pushing result '%4'
+This debug message indicates that vendor class option was found and passed
+enterprise-id checks, but does not have sufficient number of data chunks.
+Note that the index starts at 0, so there has to be at least (index + 1)
+data chunks.
+
+% EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID Pushing enterprise-id %1 as result 0x%2
+This debug message indicates that the expression has been evaluated and vendor
+class option was found and its enterprise-id is being reported.
+
+% EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID_MISMATCH Was looking for %1, option had %2, pushing result '%3'
 This debug message indicates that the expression has been evaluated
-to said value. This message is mostly useful during debugging of the
-client classification expressions.
+and vendor class option was found, but has different enterprise-id than specified
+in the expression.
 
-% EVAL_DEBUG_VENDOR_NO_OPTION Option with code %1 missing, pushing result '%2'
+% EVAL_DEBUG_VENDOR_CLASS_EXISTS Option with enterprise-id %1 found, pushing result '%2'
+This debug message indicates that the expression has been evaluated and vendor
+class option was found.
+
+% EVAL_DEBUG_VENDOR_CLASS_NO_OPTION Option with code %1 missing, pushing result '%2'
 This debug message indicates that the expression has been evaluated
-and vendor option was not found. This message is mostly useful during
-debugging of the client classification expressions.
+and vendor class option was not found.
+
+% EVAL_DEBUG_VENDOR_ENTERPRISE_ID Pushing enterprise-id %1 as result 0x%2
+This debug message indicates that the expression has been evaluated and vendor
+option was found and its enterprise-id is being reported.
 
 % EVAL_DEBUG_VENDOR_ENTERPRISE_ID_MISMATCH Was looking for %1, option had %2, pushing result '%3'
 This debug message indicates that the expression has been evaluated
 and vendor option was found, but has different enterprise-id than specified
-in the expression. This message is mostly useful during debugging of the
-client classification expressions.
-
-% EVAL_DEBUG_VENDOR_ENTERPRISE_ID Pushing enterprise-id %1 as result '%2'
-This debug message indicates that the expression has been evaluated and vendor
-option was found and its enterprise-id is being reported. This message is mostly
-useful during debugging of the client classification expressions.
+in the expression.
 
 % EVAL_DEBUG_VENDOR_EXISTS Option with enterprise-id %1 found, pushing result '%2'
 This debug message indicates that the expression has been evaluated and vendor
-option was found. This message is mostly useful during debugging of the client
-classification expressions.
+option was found.
 
-% EVAL_DEBUG_VENDOR_CLASS_NO_OPTION Option with code %1 missing, pushing result '%2'
+% EVAL_DEBUG_VENDOR_NO_OPTION Option with code %1 missing, pushing result '%2'
 This debug message indicates that the expression has been evaluated
-and vendor class option was not found. This message is mostly useful during
-debugging of the client classification expressions.
+and vendor option was not found.
 
-% EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID_MISMATCH Was looking for %1, option had %2, pushing result '%3'
+% EVAL_RESULT Expression %1 evaluated to %2
 This debug message indicates that the expression has been evaluated
-and vendor class option was found, but has different enterprise-id than specified
-in the expression. This message is mostly useful during debugging of the
-client classification expressions.
-
-% EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID Pushing enterprise-id %1 as result '%2'
-This debug message indicates that the expression has been evaluated and vendor
-class option was found and its enterprise-id is being reported. This message is mostly
-useful during debugging of the client classification expressions.
-
-% EVAL_DEBUG_VENDOR_CLASS_EXISTS Option with enterprise-id %1 found, pushing result '%2'
-This debug message indicates that the expression has been evaluated and vendor
-class option was found. This message is mostly useful during debugging of the
+to said value. This message is mostly useful during debugging of the
 client classification expressions.
-
-% EVAL_DEBUG_VENDOR_CLASS_DATA Data %1 (out of %2 received) in vendor class found, pushing result '%3'
-This debug message indicates that vendor class option was found and passed
-enterprise-id checks and has sufficient number of data chunks. The total number
-of chunks and value pushed are reported as debugging aid.
-
-% EVAL_DEBUG_VENDOR_CLASS_DATA_NOT_FOUND Requested data index %1, but option with enterprise-id %2 has only %3 data tuple(s), pushing result '%4'
-This debug message indicates that vendor class option was found and passed
-enterprise-id checks, but does not have sufficient number of data chunks.
-Note that the index starts at 0, so there has to be at least (index + 1)
-data chunks.

src/lib/eval/parser.yy

@@ -166,35 +166,35 @@ bool_expr : "(" bool_expr ")"
                         error(@1, "relay6 can only be used in DHCPv6.");
                     }
                 }
-| VENDOR_CLASS "[" enterprise_id "]" "." EXISTS
-{
-    // Expression: vendor-class[1234].exists
-    //
-    // This token will find option 124 (DHCPv4) or 16 (DHCPv6), and will check
-    // if enterprise-id equals specified value.
-    TokenPtr exist(new TokenVendorClass(ctx.getUniverse(), $3, TokenOption::EXISTS));
-    ctx.expression.push_back(exist);
-}
-| VENDOR "[" enterprise_id "]" "." EXISTS
-{
-    // Expression: vendor[1234].exists
-    //
-    // This token will find option 125 (DHCPv4) or 17 (DHCPv6), and will check
-    // if enterprise-id equals specified value.
-    TokenPtr exist(new TokenVendor(ctx.getUniverse(), $3, TokenOption::EXISTS));
-    ctx.expression.push_back(exist);
+          | VENDOR_CLASS "[" enterprise_id "]" "." EXISTS
+          {
+              // Expression: vendor-class[1234].exists
+              //
+              // This token will find option 124 (DHCPv4) or 16 (DHCPv6), and will check
+              // if enterprise-id equals specified value.
+              TokenPtr exist(new TokenVendorClass(ctx.getUniverse(), $3, TokenOption::EXISTS));
+              ctx.expression.push_back(exist);
+          }
+          | VENDOR "[" enterprise_id "]" "." EXISTS
+          {
+              // Expression: vendor[1234].exists
+              //
+              // This token will find option 125 (DHCPv4) or 17 (DHCPv6), and will check
+              // if enterprise-id equals specified value.
+              TokenPtr exist(new TokenVendor(ctx.getUniverse(), $3, TokenOption::EXISTS));
+              ctx.expression.push_back(exist);
 
-}
-| VENDOR "[" enterprise_id "]" "." OPTION "[" option_code "]" "." EXISTS
-{
-    // Expression vendor[1234].option[123].exists
-    //
-    // This token will check if specified vendor option exists, has specified
-    // enterprise-id and if has specified suboption.
-    TokenPtr exist(new TokenVendor(ctx.getUniverse(), $3, TokenOption::EXISTS, $8));
-    ctx.expression.push_back(exist);
-}
-;
+          }
+          | VENDOR "[" enterprise_id "]" "." OPTION "[" option_code "]" "." EXISTS
+          {
+              // Expression vendor[1234].option[123].exists
+              //
+              // This token will check if specified vendor option exists, has specified
+              // enterprise-id and if has specified suboption.
+              TokenPtr exist(new TokenVendor(ctx.getUniverse(), $3, TokenOption::EXISTS, $8));
+              ctx.expression.push_back(exist);
+          }
+          ;
 
 string_expr : STRING
                   {
@@ -287,59 +287,58 @@ string_expr : STRING
                       TokenPtr conc(new TokenConcat());
                       ctx.expression.push_back(conc);
                   }
-| VENDOR "." ENTERPRISE
-{
-    // expression: vendor.enterprise
-    //
-    // This token will return enterprise-id number of received vendor option.
-    TokenPtr vendor(new TokenVendor(ctx.getUniverse(), 0, TokenVendor::ENTERPRISE_ID));
-    ctx.expression.push_back(vendor);
-}
-| VENDOR_CLASS "." ENTERPRISE
-{
-    // expression: vendor-class.enterprise
-    //
-    // This token will return enterprise-id number of received vendor class option.
-    TokenPtr vendor(new TokenVendorClass(ctx.getUniverse(), 0,
-                                         TokenVendor::ENTERPRISE_ID));
-    ctx.expression.push_back(vendor);
-}
-| VENDOR "[" enterprise_id "]" "." OPTION "[" option_code "]" "." option_repr_type
-{
-    // expression: vendor[1234].option[56].exists
-    // expression: vendor[1234].option[56].hex
-    //
-    // This token will search for vendor option with specified enterprise-id.
-    // If found, will search for specified suboption and finally will return
-    // if it exists ('exists') or its content ('hex')
-    TokenPtr opt(new TokenVendor(ctx.getUniverse(), $3, $11, $8));
-    ctx.expression.push_back(opt);
-}
-| VENDOR_CLASS "[" enterprise_id "]" "." DATA
-{
-    // expression: vendor-class[1234].data
-    //
-    // Vendor class option does not have suboptions, but chunks of data (typically 1,
-    // but the option structure allows multiple of them). If chunk offset is not
-    // specified, we assume the first (0th) is requested.
-    TokenPtr vendor_class(new TokenVendorClass(ctx.getUniverse(), $3,
-                                               TokenVendor::DATA, 0));
-    ctx.expression.push_back(vendor_class);
-}
-| VENDOR_CLASS "[" enterprise_id "]" "." DATA "[" INTEGER "]"
-{
-    // expression: vendor-class[1234].data[5]
-    //
-    // Vendor class option does not have suboptions, but chunks of data (typically 1,
-    // but the option structure allows multiple of them). This syntax specifies
-    // which data chunk (tuple) we want.
-    uint8_t index = ctx.convertUint8($8, @8);
-    TokenPtr vendor_class(new TokenVendorClass(ctx.getUniverse(), $3,
-                                               TokenVendor::DATA, index));
-    ctx.expression.push_back(vendor_class);
-}
-
-;
+          | VENDOR "." ENTERPRISE
+          {
+              // expression: vendor.enterprise
+              //
+              // This token will return enterprise-id number of received vendor option.
+              TokenPtr vendor(new TokenVendor(ctx.getUniverse(), 0, TokenVendor::ENTERPRISE_ID));
+              ctx.expression.push_back(vendor);
+          }
+          | VENDOR_CLASS "." ENTERPRISE
+          {
+              // expression: vendor-class.enterprise
+              //
+              // This token will return enterprise-id number of received vendor class option.
+              TokenPtr vendor(new TokenVendorClass(ctx.getUniverse(), 0,
+                                                   TokenVendor::ENTERPRISE_ID));
+              ctx.expression.push_back(vendor);
+          }
+          | VENDOR "[" enterprise_id "]" "." OPTION "[" option_code "]" "." option_repr_type
+          {
+              // expression: vendor[1234].option[56].exists
+              // expression: vendor[1234].option[56].hex
+              //
+              // This token will search for vendor option with specified enterprise-id.
+              // If found, will search for specified suboption and finally will return
+              // if it exists ('exists') or its content ('hex')
+              TokenPtr opt(new TokenVendor(ctx.getUniverse(), $3, $11, $8));
+              ctx.expression.push_back(opt);
+          }
+          | VENDOR_CLASS "[" enterprise_id "]" "." DATA
+          {
+              // expression: vendor-class[1234].data
+              //
+              // Vendor class option does not have suboptions, but chunks of data (typically 1,
+              // but the option structure allows multiple of them). If chunk offset is not
+              // specified, we assume the first (0th) is requested.
+              TokenPtr vendor_class(new TokenVendorClass(ctx.getUniverse(), $3,
+                                                         TokenVendor::DATA, 0));
+              ctx.expression.push_back(vendor_class);
+          }
+          | VENDOR_CLASS "[" enterprise_id "]" "." DATA "[" INTEGER "]"
+          {
+              // expression: vendor-class[1234].data[5]
+              //
+              // Vendor class option does not have suboptions, but chunks of data (typically 1,
+              // but the option structure allows multiple of them). This syntax specifies
+              // which data chunk (tuple) we want.
+              uint8_t index = ctx.convertUint8($8, @8);
+              TokenPtr vendor_class(new TokenVendorClass(ctx.getUniverse(), $3,
+                                                         TokenVendor::DATA, index));
+              ctx.expression.push_back(vendor_class);
+          }
+          ;
 
 option_code : INTEGER
                  {

src/lib/eval/tests/token_unittest.cc

@@ -437,6 +437,7 @@ public:
     /// @param option_vendor_id enterprise-id used in option (0 means don't
     ///        create the option)
     /// @param option_code sub-option code (0 means don't create suboption)
+    /// @param repr representation (TokenOption::EXISTS or HEXADECIMAL)
     /// @param expected_result text representation of the expected outcome
     void testVendorSuboption(Option::Universe u,
                              uint32_t token_vendor_id, uint16_t token_option_code,
@@ -2017,9 +2018,9 @@ TEST_F(TokenTest, vendor4enterprise) {
     addString("EVAL_DEBUG_VENDOR_NO_OPTION Option with code 125 missing, pushing"
               " result ''");
     addString("EVAL_DEBUG_VENDOR_ENTERPRISE_ID Pushing enterprise-id 1234 as "
-              "result '000004D2'");
+              "result 0x000004D2");
     addString("EVAL_DEBUG_VENDOR_ENTERPRISE_ID Pushing enterprise-id 4294967295"
-              " as result 'FFFFFFFF'");
+              " as result 0xFFFFFFFF");
     EXPECT_TRUE(checkFile());
 }
 
@@ -2039,9 +2040,9 @@ TEST_F(TokenTest, vendor6enterprise) {
     addString("EVAL_DEBUG_VENDOR_NO_OPTION Option with code 17 missing, pushing"
               " result ''");
     addString("EVAL_DEBUG_VENDOR_ENTERPRISE_ID Pushing enterprise-id 1234 as "
-              "result '000004D2'");
+              "result 0x000004D2");
     addString("EVAL_DEBUG_VENDOR_ENTERPRISE_ID Pushing enterprise-id 4294967295 "
-              "as result 'FFFFFFFF'");
+              "as result 0xFFFFFFFF");
     EXPECT_TRUE(checkFile());
 }
 
@@ -2283,9 +2284,9 @@ TEST_F(TokenTest, vendorClass4enterprise) {
     addString("EVAL_DEBUG_VENDOR_CLASS_NO_OPTION Option with code 124 missing, pushing "
               "result ''");
     addString("EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID Pushing enterprise-id "
-              "1234 as result '000004D2'");
+              "1234 as result 0x000004D2");
     addString("EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID Pushing enterprise-id "
-              "4294967295 as result 'FFFFFFFF'");
+              "4294967295 as result 0xFFFFFFFF");
     EXPECT_TRUE(checkFile());
 }
 
@@ -2305,9 +2306,9 @@ TEST_F(TokenTest, vendorClass6enterprise) {
     addString("EVAL_DEBUG_VENDOR_CLASS_NO_OPTION Option with code 16 missing, pushing "
               "result ''");
     addString("EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID Pushing enterprise-id "
-              "1234 as result '000004D2'");
+              "1234 as result 0x000004D2");
     addString("EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID Pushing enterprise-id "
-              "4294967295 as result 'FFFFFFFF'");
+              "4294967295 as result 0xFFFFFFFF");
     EXPECT_TRUE(checkFile());
 }
 
@@ -2323,7 +2324,11 @@ TEST_F(TokenTest, vendorClass4SpecificVendorData) {
     testVendorClassData(Option::V4, 4491, 0, 1234, 0, "");
 
     // Case 3: Expression looks for vendor-id 4491, data[0], there is
-    // vendor-class with vendor-id 4491 and no data, expected result is empty string
+    // vendor-class with vendor-id 4491 and no data, expected result is empty string.
+    // Note that vendor option in v4 always have at least one data chunk, even though
+    // it may be empty. The OptionVendor code was told to not create any special
+    // tuples, but it creates one empty on its own. So the code finds that one
+    // tuple and extracts its content (an empty string).
     testVendorClassData(Option::V4, 4491, 0, 4491, 0, "");
 
     // Case 4: Expression looks for vendor-id 4491, data[0], there is
@@ -2486,7 +2491,7 @@ TEST_F(TokenTest, vendorClass4DataIndex) {
     testVendorClassData(Option::V4, 4491, 3, 4491, 0, "");
 
     // Case 4: Expression looks for vendor-id 4491, data[3], there is
-    // vendor-class with vendor-id 1234 and 1 data tuples, expected result is empty string.
+    // vendor-class with vendor-id 1234 and 1 data tuple, expected result is empty string.
     testVendorClassData(Option::V4, 4491, 3, 1234, 1, "");
 
     // Case 5: Expression looks for vendor-id 4491, data[3], there is
@@ -2499,6 +2504,11 @@ TEST_F(TokenTest, vendorClass4DataIndex) {
     // content of that tuple ("gamma")
     testVendorClassData(Option::V4, 4491, 3, 4491, 5, "gamma");
 
+    // Case 6: Expression looks for vendor-id 4491, data[3], there is
+    // vendor-class with vendor-id 1234 and 5 data tuples, expected result is
+    // empty string, because vendor-id does not match.
+    testVendorClassData(Option::V4, 4491, 3, 1234, 5, "");
+
     // Check if the logged messages are correct.
     addString("EVAL_DEBUG_VENDOR_CLASS_NO_OPTION Option with code 124 missing, "
               "pushing result ''");
@@ -2514,6 +2524,8 @@ TEST_F(TokenTest, vendorClass4DataIndex) {
               "pushing result ''");
     addString("EVAL_DEBUG_VENDOR_CLASS_DATA Data 3 (out of 5 received) in vendor "
               "class found, pushing result 'gamma'");
+    addString("EVAL_DEBUG_VENDOR_CLASS_ENTERPRISE_ID_MISMATCH Was looking for "
+              "4491, option had 1234, pushing result ''");
     EXPECT_TRUE(checkFile());
 }
 

src/lib/testutils/log_utils.h

@@ -64,6 +64,9 @@ public:
     void remFile();
 
     /// @brief Enables or disables verbose mode.
+    ///
+    /// See @ref verbose_ for details.
+    ///
     /// @param talk_a_lot (true - as the name says, false - shut up)
     void logCheckVerbose(bool talk_a_lot) {
         verbose_ = talk_a_lot;
@@ -78,6 +81,12 @@ public:
     vector<string> exp_strings_;
     static const char* LOG_FILE;
 
+    /// @brief controls whether the checkFile() should print more details.
+    ///
+    /// If set to true, checkFile() will print each expected line, each
+    /// logged line and will print out a failure message if those two do
+    /// not match. Also, a final verdict is printed. Everything is printed
+    /// on stdout.
     bool verbose_;
 };