BIND 10 trac3203, updated. afea612c23143f81a4201e39ba793bc837c5c9f1 [3203] Changes after review:
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Jan 17 19:52:06 UTC 2014
The branch, trac3203 has been updated
via afea612c23143f81a4201e39ba793bc837c5c9f1 (commit)
from 4e3b085d9487815577d6b3319b51e51111321993 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit afea612c23143f81a4201e39ba793bc837c5c9f1
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Fri Jan 17 20:51:34 2014 +0100
[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
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/dhcp4_srv.cc | 39 ++++++++++++++++++++++--------
src/bin/dhcp6/dhcp6_srv.cc | 18 ++++++++------
src/lib/dhcp/docsis3_option_defs.h | 11 ++++++---
src/lib/dhcp/libdhcp++.cc | 8 ++++++
src/lib/dhcp/option_vendor.h | 5 ++++
src/lib/dhcp/pkt4.h | 8 ++++++
src/lib/dhcp/pkt6.h | 8 ++++++
src/lib/dhcp/tests/libdhcp++_unittest.cc | 11 ++++++---
src/lib/dhcp/tests/pkt4_unittest.cc | 20 +++++++++------
src/lib/dhcp/tests/pkt6_unittest.cc | 20 +++++++++------
10 files changed, 107 insertions(+), 41 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 4da9f82..d74c3db 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/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"));
diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index d659c43..0319550 100644
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/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()) {
diff --git a/src/lib/dhcp/docsis3_option_defs.h b/src/lib/dhcp/docsis3_option_defs.h
index 19bdaa0..6031f8d 100644
--- a/src/lib/dhcp/docsis3_option_defs.h
+++ b/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
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 4577c64..f6ba978 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/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,
diff --git a/src/lib/dhcp/option_vendor.h b/src/lib/dhcp/option_vendor.h
index b1f34be..bb8395c 100644
--- a/src/lib/dhcp/option_vendor.h
+++ b/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
diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h
index 37c326d..1171111 100644
--- a/src/lib/dhcp/pkt4.h
+++ b/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);
diff --git a/src/lib/dhcp/pkt6.h b/src/lib/dhcp/pkt6.h
index 1e876dc..db80fb9 100644
--- a/src/lib/dhcp/pkt6.h
+++ b/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);
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index 1f711b1..4d645a4 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/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
diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index 365b10f..2c8a205 100644
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/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
diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc
index a0139df..17cb629 100644
--- a/src/lib/dhcp/tests/pkt6_unittest.cc
+++ b/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"));
}
}
More information about the bind10-changes
mailing list