[3203] Changes after review:

 - Dhcpv{4,6}Srv::classifyPacket() is now better commented.
 - constants are now used instead of magic numbers
 - Pkt{4,6}::addClass is now better commented

src/bin/dhcp4/dhcp4_srv.cc

@@ -395,6 +395,11 @@ Dhcpv4Srv::run() {
 
         adjustRemoteAddr(query, rsp);
 
+        // Let's do class specific processing. This is done before
+        // pkt4_send.
+        //
+        /// @todo: decide whether we want to add a new hook point for
+        /// doing class specific processing.
         if (!classSpecificProcessing(query, rsp)) {
             /// @todo add more verbosity here
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CLASS_PROCESSING_FAILED);
@@ -1805,15 +1810,29 @@ void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) {
     }
 
     // DOCSIS specific section
-    if (vendor_class->getValue().find("docsis3.0") != std::string::npos) {
-        pkt->addClass("docsis3.0");
-        classes += "docsis3.0 ";
+
+    // Let's keep this as a series of checks. So far we're supporting only
+    // docsis3.0, but there are also docsis2.0, docsis1.1 and docsis1.0. We
+    // may come up with adding several classes, e.g. for docsis2.0 we would
+    // add classes docsis2.0, docsis1.1 and docsis1.0.
+
+    // Also we are using find, because we have at least one traffic capture
+    // where the user class was followed by a space ("docsis3.0 ").
+
+    // For now, the code is very simple, but it is expected to get much more
+    // complex soon. One specific case is that the vendor class is an option
+    // sent by the client, so we should not trust it. To confirm that the device
+    // is indeed a modem, John B. suggested to check whether chaddr field
+    // quals subscriber-id option that was inserted by the relay (CMTS).
+    // This kind of logic will appear here soon.
+    if (vendor_class->getValue().find(DOCSIS3_CLASS_MODEM) != std::string::npos) {
+        pkt->addClass(DOCSIS3_CLASS_MODEM);
+        classes += string(DOCSIS3_CLASS_MODEM) + " ";
     } else
-    if (vendor_class->getValue().find("eRouter1.0") != std::string::npos) {
-        pkt->addClass("eRouter1.0");
-        classes += "eRouter1.0 ";
-    }else
-    {
+    if (vendor_class->getValue().find(DOCSIS3_CLASS_EROUTER) != std::string::npos) {
+        pkt->addClass(DOCSIS3_CLASS_EROUTER);
+        classes += string(DOCSIS3_CLASS_EROUTER) + " ";
+    } else {
         classes += vendor_class->getValue();
         pkt->addClass(vendor_class->getValue());
     }
@@ -1831,7 +1850,7 @@ bool Dhcpv4Srv::classSpecificProcessing(const Pkt4Ptr& query, const Pkt4Ptr& rsp
         return (true);
     }
 
-    if (query->inClass("docsis3.0")) {
+    if (query->inClass(DOCSIS3_CLASS_MODEM)) {
 
         // Set next-server. This is TFTP server address. Cable modems will
         // download their configuration from that server.
@@ -1852,7 +1871,7 @@ bool Dhcpv4Srv::classSpecificProcessing(const Pkt4Ptr& query, const Pkt4Ptr& rsp
         }
     }
 
-    if (query->inClass("eRouter1.0")) {
+    if (query->inClass(DOCSIS3_CLASS_EROUTER)) {
 
         // Do not set TFTP server address for eRouter devices.
         rsp->setSiaddr(IOAddress("0.0.0.0"));

src/bin/dhcp6/dhcp6_srv.cc

@@ -2424,18 +2424,20 @@ void Dhcpv6Srv::classifyPacket(const Pkt6Ptr& pkt) {
     string classes = "";
 
     // DOCSIS specific section
-    if (vclass->readString(2).find("docsis3.0") != std::string::npos) {
-        pkt->addClass("docsis3.0");
-        classes += "docsis3.0 ";
+    if (vclass->readString(VENDOR_CLASS_STRING_INDEX)
+        .find(DOCSIS3_CLASS_MODEM) != std::string::npos) {
+        pkt->addClass(DOCSIS3_CLASS_MODEM);
+        classes += string(DOCSIS3_CLASS_MODEM) + " ";
     } else
-    if (vclass->readString(2).find("eRouter1.0") != std::string::npos) {
-        pkt->addClass("eRouter1.0");
-        classes += "eRouter1.0 ";
+    if (vclass->readString(VENDOR_CLASS_STRING_INDEX)
+        .find(DOCSIS3_CLASS_EROUTER) != std::string::npos) {
+        pkt->addClass(DOCSIS3_CLASS_EROUTER);
+        classes += string(DOCSIS3_CLASS_EROUTER) + " ";
     }else
     {
         // Otherwise use the string as is
-        classes += vclass->readString(2);
-        pkt->addClass(vclass->readString(2));
+        classes += vclass->readString(VENDOR_CLASS_STRING_INDEX);
+        pkt->addClass(vclass->readString(VENDOR_CLASS_STRING_INDEX));
     }
 
     if (!classes.empty()) {

src/lib/dhcp/docsis3_option_defs.h

@@ -18,8 +18,8 @@
 #include <dhcp/std_option_defs.h>
 #include <dhcp/option_data_types.h>
 
-
-namespace {
+namespace isc {
+namespace dhcp {
 
 #define VENDOR_ID_CABLE_LABS 4491
 
@@ -61,6 +61,11 @@ const OptionDefParams DOCSIS3_V6_DEFS[] = {
 /// Number of option definitions defined.
 const int DOCSIS3_V6_DEFS_SIZE  = sizeof(DOCSIS3_V6_DEFS) / sizeof(OptionDefParams);
 
-}; // anonymous namespace
+/// The class as specified in vendor-class option by the devices
+extern const char* DOCSIS3_CLASS_EROUTER;
+extern const char* DOCSIS3_CLASS_MODEM;
+
+}; // isc::dhcp namespace
+}; // isc namespace
 
 #endif // DOCSIS3_OPTION_DEFS_H

src/lib/dhcp/libdhcp++.cc

@@ -52,6 +52,14 @@ VendorOptionDefContainers LibDHCP::vendor4_defs_;
 
 VendorOptionDefContainers LibDHCP::vendor6_defs_;
 
+// Those two vendor classes are used for cable modems:
+
+/// DOCSIS3.0 compatible cable modem
+const char* isc::dhcp::DOCSIS3_CLASS_MODEM = "docsis3.0";
+
+/// DOCSIS3.0 cable modem that has router built-in
+const char* isc::dhcp::DOCSIS3_CLASS_EROUTER = "eRouter1.0";
+
 // Let's keep it in .cc file. Moving it to .h would require including optionDefParams
 // definitions there
 void initOptionSpace(OptionDefContainer& defs,

src/lib/dhcp/option_vendor.h

@@ -25,6 +25,11 @@
 namespace isc {
 namespace dhcp {
 
+/// Indexes for fields in vendor-class (17) DHCPv6 option
+const int VENDOR_CLASS_ENTERPRISE_ID_INDEX = 0;
+const int VENDOR_CLASS_DATA_LEN_INDEX = 1;
+const int VENDOR_CLASS_STRING_INDEX = 2;
+
 /// @brief This class represents vendor-specific information option.
 ///
 /// As specified in RFC3925, the option formatting is slightly different

src/lib/dhcp/pkt4.h

@@ -548,6 +548,14 @@ public:
     /// attempts to add to a class the packet already belongs to, will be
     /// ignored silently.
     ///
+    /// @note It is a matter of naming convention. Conceptually, the server
+    /// processes a stream of packets, with some packets belonging to given
+    /// classes. From that perspective, this method adds a packet to specifed
+    /// class. Implementation wise, it looks the opposite - the class name
+    /// is added to the packet. Perhaps the most appropriate name for this
+    /// method would be associateWithClass()? But that seems overly long,
+    /// so I decided to stick with addClass().
+    ///
     /// @param client_class name of the class to be added
     void addClass(const std::string& client_class);
 

src/lib/dhcp/pkt6.h

@@ -442,6 +442,14 @@ public:
     /// attempts to add to a class the packet already belongs to, will be
     /// ignored silently.
     ///
+    /// @note It is a matter of naming convention. Conceptually, the server
+    /// processes a stream of packets, with some packets belonging to given
+    /// classes. From that perspective, this method adds a packet to specifed
+    /// class. Implementation wise, it looks the opposite - the class name
+    /// is added to the packet. Perhaps the most appropriate name for this
+    /// method would be associateWithClass()? But that seems overly long,
+    /// so I decided to stick with addClass().
+    ///
     /// @param client_class name of the class to be added
     void addClass(const std::string& client_class);
 

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -1139,7 +1139,7 @@ TEST_F(LibDhcpTest, vendorClass6) {
     // to be OptionBuffer format)
     isc::util::encode::decodeHex(vendor_class_hex, bin);
 
-    EXPECT_NO_THROW ({
+    ASSERT_NO_THROW ({
             LibDHCP::unpackOptions6(bin, "dhcp6", options);
         });
 
@@ -1158,9 +1158,12 @@ TEST_F(LibDhcpTest, vendorClass6) {
     // 3 fields expected: vendor-id, data-len and data
     ASSERT_EQ(3, vclass->getDataFieldsNum());
 
-    EXPECT_EQ(4491, vclass->readInteger<uint32_t>(0)); // vendor-id=4491
-    EXPECT_EQ(10, vclass->readInteger<uint16_t>(1)); // data len = 10
-    EXPECT_EQ("eRouter1.0", vclass->readString(2)); // data="eRouter1.0"
+    EXPECT_EQ(4491, vclass->readInteger<uint32_t>
+              (VENDOR_CLASS_ENTERPRISE_ID_INDEX)); // vendor-id=4491
+    EXPECT_EQ(10, vclass->readInteger<uint16_t>
+              (VENDOR_CLASS_DATA_LEN_INDEX)); // data len = 10
+    EXPECT_EQ("eRouter1.0", vclass->readString
+              (VENDOR_CLASS_STRING_INDEX)); // data="eRouter1.0"
 }
 
 } // end of anonymous space

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -17,6 +17,7 @@
 #include <asiolink/io_address.h>
 #include <dhcp/dhcp4.h>
 #include <dhcp/libdhcp++.h>
+#include <dhcp/docsis3_option_defs.h>
 #include <dhcp/option_string.h>
 #include <dhcp/pkt4.h>
 #include <exceptions/exceptions.h>
@@ -804,25 +805,28 @@ TEST_F(Pkt4Test, clientClasses) {
     Pkt4 pkt(DHCPOFFER, 1234);
 
     // Default values (do not belong to any class)
-    EXPECT_FALSE(pkt.inClass("eRouter1.0"));
-    EXPECT_FALSE(pkt.inClass("docsis3.0"));
+    EXPECT_FALSE(pkt.inClass(DOCSIS3_CLASS_EROUTER));
+    EXPECT_FALSE(pkt.inClass(DOCSIS3_CLASS_MODEM));
     EXPECT_TRUE(pkt.classes_.empty());
 
     // Add to the first class
-    pkt.addClass("eRouter1.0");
-    EXPECT_TRUE(pkt.inClass("eRouter1.0"));
-    EXPECT_FALSE(pkt.inClass("docsis3.0"));
+    pkt.addClass(DOCSIS3_CLASS_EROUTER);
+    EXPECT_TRUE(pkt.inClass(DOCSIS3_CLASS_EROUTER));
+    EXPECT_FALSE(pkt.inClass(DOCSIS3_CLASS_MODEM));
     ASSERT_FALSE(pkt.classes_.empty());
 
     // Add to a second class
-    pkt.addClass("docsis3.0");
-    EXPECT_TRUE(pkt.inClass("eRouter1.0"));
-    EXPECT_TRUE(pkt.inClass("docsis3.0"));
+    pkt.addClass(DOCSIS3_CLASS_MODEM);
+    EXPECT_TRUE(pkt.inClass(DOCSIS3_CLASS_EROUTER));
+    EXPECT_TRUE(pkt.inClass(DOCSIS3_CLASS_MODEM));
 
     // Check that it's ok to add to the same class repeatedly
     EXPECT_NO_THROW(pkt.addClass("foo"));
     EXPECT_NO_THROW(pkt.addClass("foo"));
     EXPECT_NO_THROW(pkt.addClass("foo"));
+
+    // Check that the packet belongs to 'foo'
+    EXPECT_TRUE(pkt.inClass("foo"));
 }
 
 } // end of anonymous namespace

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -22,6 +22,7 @@
 #include <dhcp/option_int.h>
 #include <dhcp/option_int_array.h>
 #include <dhcp/pkt6.h>
+#include <dhcp/docsis3_option_defs.h>
 #include <util/range_utilities.h>
 
 #include <boost/bind.hpp>
@@ -785,25 +786,28 @@ TEST_F(Pkt6Test, clientClasses) {
     Pkt6 pkt(DHCPV6_ADVERTISE, 1234);
 
     // Default values (do not belong to any class)
-    EXPECT_FALSE(pkt.inClass("eRouter1.0"));
-    EXPECT_FALSE(pkt.inClass("docsis3.0"));
+    EXPECT_FALSE(pkt.inClass(DOCSIS3_CLASS_EROUTER));
+    EXPECT_FALSE(pkt.inClass(DOCSIS3_CLASS_MODEM));
     EXPECT_TRUE(pkt.classes_.empty());
 
     // Add to the first class
-    pkt.addClass("eRouter1.0");
-    EXPECT_TRUE(pkt.inClass("eRouter1.0"));
-    EXPECT_FALSE(pkt.inClass("docsis3.0"));
+    pkt.addClass(DOCSIS3_CLASS_EROUTER);
+    EXPECT_TRUE(pkt.inClass(DOCSIS3_CLASS_EROUTER));
+    EXPECT_FALSE(pkt.inClass(DOCSIS3_CLASS_MODEM));
     ASSERT_FALSE(pkt.classes_.empty());
 
     // Add to a second class
-    pkt.addClass("docsis3.0");
-    EXPECT_TRUE(pkt.inClass("eRouter1.0"));
-    EXPECT_TRUE(pkt.inClass("docsis3.0"));
+    pkt.addClass(DOCSIS3_CLASS_MODEM);
+    EXPECT_TRUE(pkt.inClass(DOCSIS3_CLASS_EROUTER));
+    EXPECT_TRUE(pkt.inClass(DOCSIS3_CLASS_MODEM));
 
     // Check that it's ok to add to the same class repeatedly
     EXPECT_NO_THROW(pkt.addClass("foo"));
     EXPECT_NO_THROW(pkt.addClass("foo"));
     EXPECT_NO_THROW(pkt.addClass("foo"));
+
+    // Check that the packet belongs to 'foo'
+    EXPECT_TRUE(pkt.inClass("foo"));
 }
 
 }