diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml
index f4189cfef4..681481fed9 100644
--- a/doc/guide/dhcp4-srv.xml
+++ b/doc/guide/dhcp4-srv.xml
@@ -1745,12 +1745,11 @@ It is merely echoed by the server
- 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.
diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes
index 708a9256da..a8aa19fa30 100644
--- a/src/bin/dhcp4/dhcp4_messages.mes
+++ b/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
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 8dae9d5f37..18d3923794 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1964,14 +1964,26 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
response->setSiaddr(next_server);
}
- const vector& 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(sname.c_str()),
+ sname.size());
}
- const vector& 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(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 boot =
- boost::dynamic_pointer_cast(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();
diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index 813e66635c..b6646b4dec 100644
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/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
diff --git a/src/bin/dhcp4/tests/classify_unittest.cc b/src/bin/dhcp4/tests/classify_unittest.cc
index fcea893146..c3862f3090 100644
--- a/src/bin/dhcp4/tests/classify_unittest.cc
+++ b/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(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(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(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(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(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(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(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(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(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(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(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(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");
}
diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index b8e5575019..af3c8667b0 100644
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/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);
diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc
index bc9cd48641..010457eaed 100644
--- a/src/lib/dhcpsrv/client_class_def.cc
+++ b/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& sname,
- const std::vector& 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);
diff --git a/src/lib/dhcpsrv/client_class_def.h b/src/lib/dhcpsrv/client_class_def.h
index 36d1237807..82084aded1 100644
--- a/src/lib/dhcpsrv/client_class_def.h
+++ b/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& 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& 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& 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& 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 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 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& sname = std::vector(),
- const std::vector& filename = std::vector());
+ const std::string& sname = std::string(),
+ const std::string& filename = std::string());
/// @brief Adds a new class to the list
///
diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
index 218457ce85..39b298f7cf 100644
--- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc
+++ b/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 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 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);
diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
index f61d0074ba..5949cdbb43 100644
--- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
+++ b/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 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 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.
diff --git a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc
index 04523c2152..734b9551dd 100644
--- a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc
+++ b/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 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 sname(sname_txt.begin(), sname_txt.end());
-
- string filename_txt = "this-is-a-slightly-longish-name-of-a-file.txt";
- vector 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);