[4626] Changes after review

 - Dhcp4Srv::vendorClassSpecificProcessing removed
 - User's Guide updated
 - disabled obsolete test (will need to be rewritten to take advantage
   of new classification code)
 - classes definitions now use strings for server-name and filename
 - unused configuration removed from unit-tests
 - textFixedFields is now documented
 - Unit-tests now have short descriptions

doc/guide/dhcp4-srv.xml

@@ -1745,12 +1745,11 @@ It is merely echoed by the server
         </para>
 
         <para>
-        For clients that belong to the VENDOR_CLASS_docsis3.0 class, the siaddr
-        field is set to the value of next-server (if specified in a subnet). If
-        there is a boot-file-name option specified, its value is also set in the
-        file field in the DHCPv4 packet. For eRouter1.0 class, the siaddr is
-        always set to 0.0.0.0. That capability is expected to be moved to
-        an external hook library that will be dedicated to cable modems.
+          Note: Kea 1.0 and earlier versions performed special actions for
+          clients that were in VENDOR_CLASS_docsis3.0. This is no longer the
+          case in Kea 1.1 and newer. In those newer versions the old behavior
+          can be achieved by defining VENDOR_CLASS_docsis3.0 and setting
+          its next-server and boot-file-name values appropriately.
         </para>
 
         <para>

src/bin/dhcp4/dhcp4_messages.mes

@@ -233,11 +233,6 @@ information. The second and third argument contains the packet name
 and type respectively. The fourth argument contains detailed packet
 information.
 
-% DHCP4_DISCOVER_CLASS_PROCESSING_FAILED %1: client class specific processing failed for DHCPDISCOVER
-This debug message means that the server processing that is unique for each
-client class has reported a failure. The response packet will not be sent.
-The argument holds the client and transaction identification information.
-
 % DHCP4_DYNAMIC_RECONFIGURATION initiate server reconfiguration using file: %1, after receiving SIGHUP signal
 This is the info message logged when the DHCPv4 server starts reconfiguration
 as a result of receiving SIGHUP signal.
@@ -314,12 +309,6 @@ will be only able to offer global options - no addresses will be assigned.
 The argument specifies the client and transaction identification
 information.
 
-% DHCP4_INFORM_CLASS_PROCESSING_FAILED %1: client class specific processing failed for DHCPINFORM
-This debug message means that the server processing that is unique for each
-client class has reported a failure. The response packet will not be sent.
-The argument specifies the client and the transaction identification
-information.
-
 % DHCP4_INFORM_DIRECT_REPLY %1: DHCPACK in reply to the DHCPINFORM will be sent directly to %2 over %3
 This debug message is issued when the DHCPACK will be sent directly to the
 client, rather than via a relay. The first argument contains the client
@@ -615,12 +604,6 @@ and the lease. The first argument includes the client and the
 transaction identification information. The second argument specifies
 the leased address.
 
-% DHCP4_REQUEST_CLASS_PROCESSING_FAILED %1: client class specific processing failed for DHCPREQUEST
-This debug message means that the server processing that is unique for each
-client class has reported a failure. The response packet will not be sent.
-The argument contains the client and transaction identification
-information.
-
 % DHCP4_RESPONSE_DATA %1: responding with packet %2 (type %3), packet details: %4
 A debug message including the detailed data about the packet being sent
 to the client. The first argument contains the client and the transaction

src/bin/dhcp4/dhcp4_srv.cc

@@ -1964,14 +1964,26 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
             response->setSiaddr(next_server);
         }
 
-        const vector<uint8_t>& sname = cl->second->getSname();
+        const string& sname = cl->second->getSname();
         if (!sname.empty()) {
-            response->setSname(&sname[0], sname.size());
+            // Converting string to (const uint8_t*, size_t len) format is
+            // tricky. reineterpret_cast is not the most elegant solution,
+            // but it does avoid us making unnecessary copy. We will convert
+            // sname and file fields in Pkt4 to string one day and live
+            // will be easier.
+            response->setSname(reinterpret_cast<const uint8_t*>(sname.c_str()),
+                               sname.size());
         }
 
-        const vector<uint8_t>& filename = cl->second->getFilename();
+        const string& filename = cl->second->getFilename();
         if (!filename.empty()) {
-            response->setFile(&filename[0], filename.size());
+            // Converting string to (const uint8_t*, size_t len) format is
+            // tricky. reineterpret_cast is not the most elegant solution,
+            // but it does avoid us making unnecessary copy. We will convert
+            // sname and file fields in Pkt4 to string one day and live
+            // will be easier.
+            response->setFile(reinterpret_cast<const uint8_t*>(filename.c_str()),
+                              filename.size());
         }
     }
 
@@ -2021,8 +2033,8 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
         // them we append them for him.
         appendBasicOptions(ex);
 
-        // See if the class mandates setting any fixed fields (siaddr, sname,
-        // filename).
+        // Set fixed fields (siaddr, sname, filename) if defined in
+        // the reservation, class or subnet specific configuration.
         setFixedFields(ex);
 
     } else {
@@ -2037,14 +2049,6 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
 
     appendServerID(ex);
 
-    /// @todo: decide whether we want to add a new hook point for
-    /// doing class specific processing.
-    if (!vendorClassSpecificProcessing(ex)) {
-        /// @todo add more verbosity here
-        LOG_DEBUG(options4_logger, DBG_DHCP4_DETAIL, DHCP4_DISCOVER_CLASS_PROCESSING_FAILED)
-            .arg(discover->getLabel());
-    }
-
     return (ex.getResponse());
 }
 
@@ -2081,8 +2085,8 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
         // them we append them for him.
         appendBasicOptions(ex);
 
-        // See if the class mandates setting any fixed fields (siaddr, sname,
-        // filename).
+        // Set fixed fields (siaddr, sname, filename) if defined in
+        // the reservation, class or subnet specific configuration.
         setFixedFields(ex);
     }
 
@@ -2092,14 +2096,6 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) {
 
     appendServerID(ex);
 
-    /// @todo: decide whether we want to add a new hook point for
-    /// doing class specific processing.
-    if (!vendorClassSpecificProcessing(ex)) {
-        /// @todo add more verbosity here
-        LOG_DEBUG(options4_logger, DBG_DHCP4_DETAIL, DHCP4_REQUEST_CLASS_PROCESSING_FAILED)
-            .arg(ex.getQuery()->getLabel());
-    }
-
     return (ex.getResponse());
 }
 
@@ -2369,8 +2365,8 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
     appendBasicOptions(ex);
     adjustIfaceData(ex);
 
-    // See if the class mandates setting any fixed fields (siaddr, sname,
-    // filename).
+    // Set fixed fields (siaddr, sname, filename) if defined in
+    // the reservation, class or subnet specific configuration.
     setFixedFields(ex);
 
     // There are cases for the DHCPINFORM that the server receives it via
@@ -2391,14 +2387,6 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
     // The DHCPACK must contain server id.
     appendServerID(ex);
 
-    /// @todo: decide whether we want to add a new hook point for
-    /// doing class specific processing.
-    if (!vendorClassSpecificProcessing(ex)) {
-        LOG_DEBUG(options4_logger, DBG_DHCP4_DETAIL,
-                  DHCP4_INFORM_CLASS_PROCESSING_FAILED)
-            .arg(inform->getLabel());
-    }
-
     return (ex.getResponse());
 }
 
@@ -2622,17 +2610,8 @@ void Dhcpv4Srv::classifyByVendor(const Pkt4Ptr& pkt, std::string& classes) {
     // 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(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_MODEM);
-        classes += string(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_MODEM) + " ";
-    } else
-    if (vendor_class->getValue().find(DOCSIS3_CLASS_EROUTER) != std::string::npos) {
-        pkt->addClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER);
-        classes += string(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER) + " ";
-    } else {
-        pkt->addClass(VENDOR_CLASS_PREFIX + vendor_class->getValue());
-        classes += VENDOR_CLASS_PREFIX + vendor_class->getValue();
-    }
+    pkt->addClass(VENDOR_CLASS_PREFIX + vendor_class->getValue());
+    classes += VENDOR_CLASS_PREFIX + vendor_class->getValue();
 }
 
 void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) {
@@ -2687,52 +2666,6 @@ void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) {
     }
 }
 
-bool
-Dhcpv4Srv::vendorClassSpecificProcessing(const Dhcpv4Exchange& ex) {
-
-    Subnet4Ptr subnet = ex.getContext()->subnet_;
-    Pkt4Ptr query = ex.getQuery();
-    Pkt4Ptr rsp = ex.getResponse();
-
-    // If any of those is missing, there is nothing to do.
-    if (!subnet || !query || !rsp) {
-        return (true);
-    }
-
-    if (query->inClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_MODEM)) {
-
-        // Set next-server. This is TFTP server address. Cable modems will
-        // download their configuration from that server.
-        rsp->setSiaddr(subnet->getSiaddr());
-
-        // Now try to set up file field in DHCPv4 packet. We will just copy
-        // content of the boot-file option, which contains the same information.
-        const CfgOptionList& co_list = ex.getCfgOptionList();
-        for (CfgOptionList::const_iterator copts = co_list.begin();
-             copts != co_list.end(); ++copts) {
-            OptionDescriptor desc = (*copts)->get("dhcp4", DHO_BOOT_FILE_NAME);
-
-            if (desc.option_) {
-                boost::shared_ptr<OptionString> boot =
-                    boost::dynamic_pointer_cast<OptionString>(desc.option_);
-                if (boot) {
-                    std::string filename = boot->getValue();
-                    rsp->setFile((const uint8_t*)filename.c_str(), filename.size());
-                    break;
-                }
-            }
-        }
-    }
-
-    if (query->inClass(VENDOR_CLASS_PREFIX + DOCSIS3_CLASS_EROUTER)) {
-
-        // Do not set TFTP server address for eRouter devices.
-        rsp->setSiaddr(IOAddress::IPV4_ZERO_ADDRESS());
-    }
-
-    return (true);
-}
-
 void
 Dhcpv4Srv::startD2() {
     D2ClientMgr& d2_mgr = CfgMgr::instance().getD2ClientMgr();

src/bin/dhcp4/dhcp4_srv.h

@@ -517,8 +517,10 @@ protected:
     ///
     /// If the incoming packets belongs to a class and that class defines
     /// next-server, server-hostname or boot-file-name, we need to set the
-    /// siaddr, sname or filename fields in the outgoing packet. This
-    /// is what this method does.
+    /// siaddr, sname or filename fields in the outgoing packet. Also, those
+    /// values can be defined for subnet or in reservations. The values
+    /// defined in reservation takes precedence over class values, which
+    /// in turn take precedence over subnet values.
     ///
     /// @param ex DHCPv4 exchange holding the client's message and the server's
     ///           response to be adjusted.
@@ -763,18 +765,6 @@ protected:
     /// @param pkt packet to be classified
     void classifyPacket(const Pkt4Ptr& pkt);
 
-    /// @brief Performs packet processing specific to a vendor class
-    ///
-    /// If the selected subnet, query or response in the @c ex object is NULL
-    /// this method returns immediately and returns true.
-    ///
-    /// @note This processing is a likely candidate to be pushed into hooks.
-    ///
-    /// @param ex The exchange holding both the client's message and the
-    /// server's response.
-    /// @return true if successful, false otherwise (will prevent sending response)
-    bool vendorClassSpecificProcessing(const Dhcpv4Exchange& ex);
-
     /// @brief Allocation Engine.
     /// Pointer to the allocation engine that we are currently using
     /// It must be a pointer, because we will support changing engines

src/bin/dhcp4/tests/classify_unittest.cc

@@ -20,54 +20,19 @@ using namespace std;
 
 namespace {
 
-/// @brief Set of JSON configurations used throughout the DORA tests.
+/// @brief Set of JSON configurations used throughout the classify tests.
 ///
 /// - Configuration 0:
 ///   - Used for testing direct traffic
 ///   - 1 subnet: 10.0.0.0/24
 ///   - 1 pool: 10.0.0.10-10.0.0.100
-///   - Router option present: 10.0.0.200 and 10.0.0.201
-///   - Domain Name Server option present: 10.0.0.202, 10.0.0.203.
-///   - Log Servers option present: 192.0.2.200 and 192.0.2.201
-///   - Quotes Servers option present: 192.0.2.202, 192.0.2.203.
-///   - no classes
-///
-/// - Configuration 1:
-///   - The same as configuration 0, but has the following classes defined:
+///   - the following classes defined:
 ///     option[93].hex == 0x0009, next-server set to 1.2.3.4
 ///     option[93].hex == 0x0007, set server-hostname to deneb
 ///     option[93].hex == 0x0006, set boot-file-name to pxelinux.0
 ///     option[93].hex == 0x0001, set boot-file-name to ipxe.efi
 const char* CONFIGS[] = {
-// Configuration 0
-    "{ \"interfaces-config\": {"
-        "      \"interfaces\": [ \"*\" ]"
-        "},"
-        "\"valid-lifetime\": 600,"
-        "\"subnet4\": [ { "
-        "    \"subnet\": \"10.0.0.0/24\", "
-        "    \"id\": 1,"
-        "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
-        "    \"option-data\": [ {"
-        "        \"name\": \"routers\","
-        "        \"data\": \"10.0.0.200,10.0.0.201\""
-        "    },"
-        "    {"
-        "        \"name\": \"domain-name-servers\","
-        "        \"data\": \"10.0.0.202,10.0.0.203\""
-        "    },"
-        "    {"
-        "        \"name\": \"log-servers\","
-        "        \"data\": \"10.0.0.200,10.0.0.201\""
-        "    },"
-        "    {"
-        "        \"name\": \"cookie-servers\","
-        "        \"data\": \"10.0.0.202,10.0.0.203\""
-        "    } ]"
-        " } ]"
-    "}",
-
-    // Configuration 6
+    // Configuration 0
     "{ \"interfaces-config\": {"
         "   \"interfaces\": [ \"*\" ]"
         "},"
@@ -124,6 +89,19 @@ public:
     ~ClassifyTest() {
     }
 
+    /// @brief Does client exchanges and checks if fixed fields have expected values.
+    ///
+    /// Depending on the value of msgtype (allowed types: DHCPDISCOVER, DHCPREQUEST or
+    /// DHCPINFORM), this method sets up the server, then conducts specified exchange
+    /// and then checks if the response contains expected values of next-server, sname
+    /// and filename fields.
+    ///
+    /// @param config server configuration to be used
+    /// @param msgtype DHCPDISCOVER, DHCPREQUEST or DHCPINFORM
+    /// @param extra_opt option to include in client messages (optional)
+    /// @param exp_next_server expected value of the next-server field
+    /// @param exp_sname expected value of the sname field
+    /// @param exp_filename expected value of the filename field
     void
     testFixedFields(const char* config, uint8_t msgtype, const OptionPtr& extra_opt,
                     const std::string& exp_next_server, const std::string& exp_sname,
@@ -181,17 +159,17 @@ public:
 // This test checks that an incoming DISCOVER that does not match any classes
 // will get the fixed fields empty.
 TEST_F(ClassifyTest, fixedFieldsDiscoverNoClasses) {
-    testFixedFields(CONFIGS[1], DHCPDISCOVER, OptionPtr(), "0.0.0.0", "", "");
+    testFixedFields(CONFIGS[0], DHCPDISCOVER, OptionPtr(), "0.0.0.0", "", "");
 }
 // This test checks that an incoming REQUEST that does not match any classes
 // will get the fixed fields empty.
 TEST_F(ClassifyTest, fixedFieldsRequestNoClasses) {
-    testFixedFields(CONFIGS[1], DHCPREQUEST, OptionPtr(), "0.0.0.0", "", "");
+    testFixedFields(CONFIGS[0], DHCPREQUEST, OptionPtr(), "0.0.0.0", "", "");
 }
 // This test checks that an incoming INFORM that does not match any classes
 // will get the fixed fields empty.
 TEST_F(ClassifyTest, fixedFieldsInformNoClasses) {
-    testFixedFields(CONFIGS[1], DHCPINFORM, OptionPtr(), "0.0.0.0", "", "");
+    testFixedFields(CONFIGS[0], DHCPINFORM, OptionPtr(), "0.0.0.0", "", "");
 }
 
 
@@ -200,21 +178,21 @@ TEST_F(ClassifyTest, fixedFieldsInformNoClasses) {
 TEST_F(ClassifyTest, fixedFieldsDiscoverNextServer) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0009));
 
-    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "1.2.3.4", "", "");
+    testFixedFields(CONFIGS[0], DHCPDISCOVER, pxe, "1.2.3.4", "", "");
 }
 // This test checks that an incoming REQUEST that does match a class that has
 // next-server specified will result in a response that has the next-server set.
 TEST_F(ClassifyTest, fixedFieldsRequestNextServer) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0009));
 
-    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "1.2.3.4", "", "");
+    testFixedFields(CONFIGS[0], DHCPREQUEST, pxe, "1.2.3.4", "", "");
 }
 // This test checks that an incoming INFORM that does match a class that has
 // next-server specified will result in a response that has the next-server set.
 TEST_F(ClassifyTest, fixedFieldsInformNextServer) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0009));
 
-    testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "1.2.3.4", "", "");
+    testFixedFields(CONFIGS[0], DHCPINFORM, pxe, "1.2.3.4", "", "");
 }
 
 
@@ -223,21 +201,21 @@ TEST_F(ClassifyTest, fixedFieldsInformNextServer) {
 TEST_F(ClassifyTest, fixedFieldsDiscoverHostname) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0007));
 
-    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "deneb", "");
+    testFixedFields(CONFIGS[0], DHCPDISCOVER, pxe, "0.0.0.0", "deneb", "");
 }
 // This test checks that an incoming REQUEST that does match a class that has
 // server-hostname specified will result in a response that has the sname field set.
 TEST_F(ClassifyTest, fixedFieldsRequestHostname) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0007));
 
-    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "0.0.0.0", "deneb", "");
+    testFixedFields(CONFIGS[0], DHCPREQUEST, pxe, "0.0.0.0", "deneb", "");
 }
 // This test checks that an incoming INFORM that does match a class that has
 // server-hostname specified will result in a response that has the sname field set.
 TEST_F(ClassifyTest, fixedFieldsInformHostname) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0007));
 
-    testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "0.0.0.0", "deneb", "");
+    testFixedFields(CONFIGS[0], DHCPINFORM, pxe, "0.0.0.0", "deneb", "");
 }
 
 
@@ -246,21 +224,21 @@ TEST_F(ClassifyTest, fixedFieldsInformHostname) {
 TEST_F(ClassifyTest, fixedFieldsDiscoverFile1) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0006));
 
-    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0");
+    testFixedFields(CONFIGS[0], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0");
 }
 // This test checks that an incoming REQUEST that does match a class that has
 // boot-file-name specified will result in a response that has the filename field set.
 TEST_F(ClassifyTest, fixedFieldsRequestFile1) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0006));
 
-    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "0.0.0.0", "", "pxelinux.0");
+    testFixedFields(CONFIGS[0], DHCPREQUEST, pxe, "0.0.0.0", "", "pxelinux.0");
 }
 // This test checks that an incoming INFORM that does match a class that has
 // boot-file-name specified will result in a response that has the filename field set.
 TEST_F(ClassifyTest, fixedFieldsInformFile1) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0006));
 
-    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0");
+    testFixedFields(CONFIGS[0], DHCPDISCOVER, pxe, "0.0.0.0", "", "pxelinux.0");
 }
 
 
@@ -269,21 +247,21 @@ TEST_F(ClassifyTest, fixedFieldsInformFile1) {
 TEST_F(ClassifyTest, fixedFieldsDiscoverFile2) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0001));
 
-    testFixedFields(CONFIGS[1], DHCPDISCOVER, pxe, "0.0.0.0", "", "ipxe.efi");
+    testFixedFields(CONFIGS[0], DHCPDISCOVER, pxe, "0.0.0.0", "", "ipxe.efi");
 }
 // This test checks that an incoming REQUEST that does match a different class that has
 // boot-file-name specified will result in a response that has the filename field set.
 TEST_F(ClassifyTest, fixedFieldsRequestFile2) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0001));
 
-    testFixedFields(CONFIGS[1], DHCPREQUEST, pxe, "0.0.0.0", "", "ipxe.efi");
+    testFixedFields(CONFIGS[0], DHCPREQUEST, pxe, "0.0.0.0", "", "ipxe.efi");
 }
 // This test checks that an incoming INFORM that does match a different class that has
 // boot-file-name specified will result in a response that has the filename field set.
 TEST_F(ClassifyTest, fixedFieldsInformFile2) {
     OptionPtr pxe(new OptionInt<uint16_t>(Option::V4, 93, 0x0001));
 
-    testFixedFields(CONFIGS[1], DHCPINFORM, pxe, "0.0.0.0", "", "ipxe.efi");
+    testFixedFields(CONFIGS[0], DHCPINFORM, pxe, "0.0.0.0", "", "ipxe.efi");
 }
 
 

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -1486,8 +1486,13 @@ TEST_F(Dhcpv4SrvTest, vendorOptionsDocsisDefinitions) {
     ASSERT_EQ(0, rcode_);
 }
 
-// Checks if DOCSIS client packets are classified properly
-TEST_F(Dhcpv4SrvTest, docsisClientClassification) {
+/// Checks if DOCSIS client packets are classified properly
+///
+/// @todo: With the change in #4626 the vendorClassSpecificProcessing
+/// code was removed and replaced with generic classification. One day
+/// we should rewrite this test to use classes. It would check that the
+/// classification system can be used for docsis packets.
+TEST_F(Dhcpv4SrvTest, DISABLED_docsisClientClassification) {
 
     NakedDhcpv4Srv srv(0);
 

src/lib/dhcpsrv/client_class_def.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -16,7 +16,7 @@ ClientClassDef::ClientClassDef(const std::string& name,
                                const ExpressionPtr& match_expr,
                                const CfgOptionPtr& cfg_option)
     : name_(name), match_expr_(match_expr), cfg_option_(cfg_option),
-      next_server_(asiolink::IOAddress("0.0.0.0")) {
+      next_server_(asiolink::IOAddress::IPV4_ZERO_ADDRESS()) {
 
     // Name can't be blank
     if (name_.empty()) {
@@ -34,7 +34,8 @@ ClientClassDef::ClientClassDef(const std::string& name,
 
 ClientClassDef::ClientClassDef(const ClientClassDef& rhs)
     : name_(rhs.name_), match_expr_(ExpressionPtr()),
-      cfg_option_(new CfgOption()), next_server_(asiolink::IOAddress("0.0.0.0")) {
+      cfg_option_(new CfgOption()),
+      next_server_(asiolink::IOAddress::IPV4_ZERO_ADDRESS()) {
 
     if (rhs.match_expr_) {
         match_expr_.reset(new Expression());
@@ -124,8 +125,8 @@ ClientClassDictionary::addClass(const std::string& name,
                                 const ExpressionPtr& match_expr,
                                 const CfgOptionPtr& cfg_option,
                                 asiolink::IOAddress next_server,
-                                const std::vector<uint8_t>& sname,
-                                const std::vector<uint8_t>& filename) {
+                                const std::string& sname,
+                                const std::string& filename) {
     ClientClassDefPtr cclass(new ClientClassDef(name, match_expr, cfg_option));
     cclass->setNextServer(next_server);
     cclass->setSname(sname);

src/lib/dhcpsrv/client_class_def.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -108,7 +108,7 @@ public:
 
     /// @brief returns next-server value
     /// @return next-server value
-    asiolink::IOAddress getNextServer() const {
+    const asiolink::IOAddress& getNextServer() const {
         return (next_server_);
     }
 
@@ -122,26 +122,26 @@ public:
     /// @brief sets the server-name value
     ///
     /// @param sname the value to be set
-    void setSname(const std::vector<uint8_t>& sname) {
+    void setSname(const std::string& sname) {
         sname_ = sname;
     }
 
     /// @brief sets the boot-file-name value
     ///
     /// @param filename the value to be set
-    void setFilename(const std::vector<uint8_t>& filename) {
+    void setFilename(const std::string& filename) {
         filename_ = filename;
     }
 
     /// @brief returns server-hostname value
     /// @return the vector that contains server-hostname (may be empty if not defined)
-    const std::vector<uint8_t>& getSname() const {
+    const std::string& getSname() const {
         return (sname_);
     }
 
     /// @brief returns boot-file-name value
     /// @return the vector that contains boot-file-name (may be empty if not defined)
-    const std::vector<uint8_t>& getFilename() const {
+    const std::string& getFilename() const {
         return (filename_);
     }
 
@@ -165,13 +165,13 @@ private:
     /// If set by the server-hostname parameter, this value will be
     /// set in the sname field of the DHCPv4 packet.
     /// This can be up to 64 octets long.
-    std::vector<uint8_t> sname_;
+    std::string sname_;
 
     /// @brief boot-file-name
     /// If set by the boot-file-name parameter, this value will be
     /// set in the file field of the DHCPv4 packet.
     /// This can be up to 128 octets long.
-    std::vector<uint8_t> filename_;
+    std::string filename_;
 
 };
 
@@ -214,8 +214,8 @@ public:
     void addClass(const std::string& name, const ExpressionPtr& match_expr,
                   const CfgOptionPtr& options,
                   asiolink::IOAddress next_server = asiolink::IOAddress("0.0.0.0"),
-                  const std::vector<uint8_t>& sname = std::vector<uint8_t>(),
-                  const std::vector<uint8_t>& filename = std::vector<uint8_t>());
+                  const std::string& sname = std::string(),
+                  const std::string& filename = std::string());
 
     /// @brief Adds a new class to the list
     ///

src/lib/dhcpsrv/parsers/client_class_def_parser.cc

@@ -119,43 +119,42 @@ ClientClassDefParser::build(ConstElementPtr class_def_cfg) {
     std::string name;
     try {
         name = string_values_->getParam("name");
-    } catch (const std::exception& ex) {
-        isc_throw(DhcpConfigError, ex.what() << " ("
-                  << class_def_cfg->getPosition() << ")");
-    }
 
-    // Let's parse the next-server field
-    IOAddress next_server("0.0.0.0");
-    string next_server_txt = string_values_->getOptionalParam("next-server", "0.0.0.0");
-    try {
-        next_server = IOAddress(next_server_txt);
-    } catch (const IOError& ex) {
-        isc_throw(DhcpConfigError, "Invalid next-server value specified: '"
-                  << next_server_txt << "'");
-    }
-    if (next_server.getFamily() != AF_INET) {
-        isc_throw(DhcpConfigError, "Invalid next-server value: '"
-                  << next_server_txt << "', must be IPv4 address");
-    }
+        // Let's parse the next-server field
+        IOAddress next_server("0.0.0.0");
+        string next_server_txt = string_values_->getOptionalParam("next-server", "0.0.0.0");
+        try {
+            next_server = IOAddress(next_server_txt);
+        } catch (const IOError& ex) {
+            isc_throw(DhcpConfigError, "Invalid next-server value specified: '"
+                      << next_server_txt);
+        }
 
-    // Let's try to parse sname
-    string sname_txt = string_values_->getOptionalParam("server-hostname", "");
-    if (sname_txt.length() > Pkt4::MAX_SNAME_LEN) {
-        isc_throw(DhcpConfigError, "server-hostname must be at most "
-                  << Pkt4::MAX_SNAME_LEN << " bytes long, it is "
-                  << sname_txt.length());
-    }
-    std::vector<uint8_t> sname(sname_txt.begin(), sname_txt.end());
+        if (next_server.getFamily() != AF_INET) {
+            isc_throw(DhcpConfigError, "Invalid next-server value: '"
+                      << next_server_txt << "', must be IPv4 address");
+        }
 
-    string file_txt = string_values_->getOptionalParam("boot-file-name", "");
-    if (file_txt.length() > Pkt4::MAX_FILE_LEN) {
-        isc_throw(DhcpConfigError, "boot-file-name must be at most "
-                  << Pkt4::MAX_FILE_LEN << " bytes long, it is "
-                  << file_txt.length());
-    }
-    std::vector<uint8_t> filename(file_txt.begin(), file_txt.end());
+        if (next_server.isV4Bcast()) {
+            isc_throw(DhcpConfigError, "Invalid next-server value: '"
+                      << next_server_txt << "', must not be a broadcast");
+        }
+
+        // Let's try to parse sname
+        string sname = string_values_->getOptionalParam("server-hostname", "");
+        if (sname.length() >= Pkt4::MAX_SNAME_LEN) {
+            isc_throw(DhcpConfigError, "server-hostname must be at most "
+                      << Pkt4::MAX_SNAME_LEN - 1 << " bytes long, it is "
+                      << sname.length());
+        }
+
+        string filename = string_values_->getOptionalParam("boot-file-name", "");
+        if (filename.length() > Pkt4::MAX_FILE_LEN) {
+            isc_throw(DhcpConfigError, "boot-file-name must be at most "
+                      << Pkt4::MAX_FILE_LEN - 1 << " bytes long, it is "
+                      << filename.length());
+        }
 
-    try {
         // an OptionCollectionPtr
         class_dictionary_->addClass(name, match_expr_, options_, next_server,
                                     sname, filename);

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -585,6 +585,8 @@ TEST_F(ClientClassDefListParserTest, invalidClass) {
                  DhcpConfigError);
 }
 
+// Test verifies that without any class specified, the fixed fields have their
+// default, empty value.
 TEST_F(ClientClassDefParserTest, noFixedFields) {
 
     std::string cfg_text =
@@ -610,7 +612,8 @@ TEST_F(ClientClassDefParserTest, noFixedFields) {
     EXPECT_EQ(0, cclass->getFilename().size());
 }
 
-
+// Test verifies that it is possible to define next-server field and it
+// is actually set in the class properly.
 TEST_F(ClientClassDefParserTest, nextServer) {
 
     std::string cfg_text =
@@ -637,6 +640,7 @@ TEST_F(ClientClassDefParserTest, nextServer) {
     EXPECT_EQ(0, cclass->getFilename().size());
 }
 
+// Test verifies that the parser rejects bogus next-server value.
 TEST_F(ClientClassDefParserTest, nextServerBogus) {
 
     std::string bogus_v6 =
@@ -666,6 +670,8 @@ TEST_F(ClientClassDefParserTest, nextServerBogus) {
     EXPECT_THROW(parseClientClassDef(bogus_junk, Option::V4), DhcpConfigError);
 }
 
+// Test verifies that it is possible to define server-hostname field and it
+// is actually set in the class properly.
 TEST_F(ClientClassDefParserTest, serverName) {
 
     std::string cfg_text =
@@ -687,12 +693,12 @@ TEST_F(ClientClassDefParserTest, serverName) {
     ASSERT_TRUE(cclass);
 
     // And it should not have any fixed fields set
-    std::string exp_txt("hal9000");
-    std::vector<uint8_t> exp_sname(exp_txt.begin(), exp_txt.end());
+    std::string exp_sname("hal9000");
 
     EXPECT_EQ(exp_sname, cclass->getSname());
 }
 
+// Test verifies that the parser rejects bogus server-hostname value.
 TEST_F(ClientClassDefParserTest, serverNameInvalid) {
 
     std::string cfg_too_long =
@@ -712,6 +718,8 @@ TEST_F(ClientClassDefParserTest, serverNameInvalid) {
 }
 
 
+// Test verifies that it is possible to define boot-file-name field and it
+// is actually set in the class properly.
 TEST_F(ClientClassDefParserTest, filename) {
 
     std::string cfg_text =
@@ -733,12 +741,11 @@ TEST_F(ClientClassDefParserTest, filename) {
     ASSERT_TRUE(cclass);
 
     // And it should not have any fixed fields set
-    std::string exp_txt("ipxe.efi");
-    std::vector<uint8_t> exp_filename(exp_txt.begin(), exp_txt.end());
-
+    std::string exp_filename("ipxe.efi");
     EXPECT_EQ(exp_filename, cclass->getFilename());
 }
 
+// Test verifies that the parser rejects bogus boot-file-name value.
 TEST_F(ClientClassDefParserTest, filenameBogus) {
 
     // boot-file-name is allowed up to 128 bytes, this one is 129.

src/lib/dhcpsrv/tests/client_class_def_unittest.cc

@@ -316,7 +316,7 @@ TEST(ClientClassDef, fixedFieldsDefaults) {
     ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr)));
 
     // Let's checks that it doesn't return any nonsense
-    vector<uint8_t> empty;
+    string empty;
     ASSERT_EQ(IOAddress("0.0.0.0"), cclass->getNextServer());
     EXPECT_EQ(empty, cclass->getSname());
     EXPECT_EQ(empty, cclass->getFilename());
@@ -338,11 +338,8 @@ TEST(ClientClassDef, fixedFieldsBasics) {
     ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr)));
 
 
-    string sname_txt = "This is a very long string that can be a server name";
-    vector<uint8_t> sname(sname_txt.begin(), sname_txt.end());
-
-    string filename_txt = "this-is-a-slightly-longish-name-of-a-file.txt";
-    vector<uint8_t> filename(sname_txt.begin(), sname_txt.end());
+    string sname = "This is a very long string that can be a server name";
+    string filename = "this-is-a-slightly-longish-name-of-a-file.txt";
 
     cclass->setNextServer(IOAddress("1.2.3.4"));
     cclass->setSname(sname);