[3560] Addressed review comments.

src/lib/dhcp/duid.cc

@@ -20,6 +20,7 @@
 #include <boost/algorithm/string/constants.hpp>
 #include <boost/algorithm/string/split.hpp>
 #include <iomanip>
+#include <cctype>
 #include <sstream>
 #include <vector>
 
@@ -58,6 +59,17 @@ DUID::decode(const std::string& text) {
 
     std::ostringstream s;
     for (size_t i = 0; i < split_text.size(); ++i) {
+        // Check that only hexadecimal digits are used.
+        size_t ch_index = 0;
+        while (ch_index < split_text[i].length()) {
+            if (!isxdigit(split_text[i][ch_index])) {
+                isc_throw(isc::BadValue, "invalid value '"
+                          << split_text[i][ch_index] << "' in"
+                          << " DUID '" << text << "'");
+            }
+            ++ch_index;
+        }
+
         if (split_text.size() > 1) {
             // If there are multiple tokens and the current one is empty, it
             // means that two consecutive colons were specified. This is not

src/lib/dhcp/duid.h

@@ -98,8 +98,8 @@ class DUID {
     /// representing bytes of DUID must be separated by colons. Usually the
     /// single byte is represented by two hexadecimal digits. However, this
     /// function allows one digit per byte. In this case, a zero is prepended
-    /// before the conversion. For example, a DUID 0:1:2::4:5 equals to
-    /// 00:01:02:00:04:05.
+    /// before the conversion. For example, a DUID 0:1:2:3:4:5 equals to
+    /// 00:01:02:03:04:05.
     ///
     /// @param text DUID in the hexadecimal format with digits representing
     /// individual bytes separated by colons.

src/lib/dhcp/tests/classify_unittest.cc

@@ -55,7 +55,7 @@ TEST(ClassifyTest, ClientClasses) {
 TEST(ClassifyTest, ClientClassesFromString) {
     {
         ClientClasses classes("alpha, beta, gamma");
-
+        EXPECT_EQ(3, classes.size());
         EXPECT_FALSE(classes.contains(""));
         EXPECT_TRUE(classes.contains("alpha"));
         EXPECT_TRUE(classes.contains("beta"));
@@ -64,8 +64,24 @@ TEST(ClassifyTest, ClientClassesFromString) {
 
     {
         ClientClasses classes("alpha, , beta ,");
+        EXPECT_EQ(2, classes.size());
         EXPECT_TRUE(classes.contains("alpha"));
         EXPECT_FALSE(classes.contains(""));
         EXPECT_TRUE(classes.contains("beta"));
     }
+
+    {
+        ClientClasses classes("");
+        EXPECT_TRUE(classes.empty());
+    }
+
+    {
+        ClientClasses classes("    ");
+        EXPECT_TRUE(classes.empty());
+    }
+
+    {
+        ClientClasses classes(", ,, ,");
+        EXPECT_TRUE(classes.empty());
+    }
 }

src/lib/dhcp/tests/duid_unittest.cc

@@ -315,6 +315,23 @@ TEST(ClientIdTest, fromText) {
         ClientId::fromText("00:01:021:03:04:05:06"),
         isc::BadValue
     );
+    // ClientId  with two spaces between the colons should not be allowed.
+    EXPECT_THROW(
+        ClientId::fromText("00:01:  :03:04:05:06"),
+        isc::BadValue
+    );
+
+    // ClientId  with one space between the colons should not be allowed.
+    EXPECT_THROW(
+        ClientId::fromText("00:01: :03:04:05:06"),
+        isc::BadValue
+    );
+
+    // ClientId  with three spaces between the colons should not be allowed.
+    EXPECT_THROW(
+        ClientId::fromText("00:01:   :03:04:05:06"),
+        isc::BadValue
+    );
 }
 
 

src/lib/dhcpsrv/host.cc

@@ -21,11 +21,25 @@ namespace dhcp {
 
 IPv6Resrv::IPv6Resrv(const asiolink::IOAddress& prefix,
                      const uint8_t prefix_len)
-    : prefix_(prefix), prefix_len_(prefix_len) {
+    : prefix_(asiolink::IOAddress("::")), prefix_len_(128) {
+    // Validate and set the actual values.
+    set(prefix, prefix_len);
+}
+
+void
+IPv6Resrv::set(const asiolink::IOAddress& prefix, const uint8_t prefix_len) {
     if (!prefix.isV6() || prefix.isV6Multicast()) {
         isc_throw(isc::BadValue, "invalid prefix '" << prefix
                   << " for new IPv6 reservation");
+
+    } else if (prefix_len > 128) {
+        isc_throw(isc::BadValue, "invalid prefix length '"
+                  << static_cast<int>(prefix_len)
+                  << "' for new IPv6 reservation");
     }
+
+    prefix_ = prefix;
+    prefix_len_ = prefix_len;
 }
 
 bool
@@ -36,8 +50,7 @@ IPv6Resrv::operator==(const IPv6Resrv& other) const {
 
 bool
 IPv6Resrv::operator!=(const IPv6Resrv& other) const {
-    return (prefix_ != other.prefix_ ||
-            prefix_len_ != other.prefix_len_);
+    return (!operator==(other));
 }
 
 Host::Host(const uint8_t* identifier, const size_t identifier_len,

src/lib/dhcpsrv/host.h

@@ -57,8 +57,8 @@ public:
     /// @param prefix Address or prefix to be reserved.
     /// @param prefix_len Prefix length.
     ///
-    /// @throw isc::BadValue if address is not IPv6 address or is a
-    /// multicast address.
+    /// @throw isc::BadValue if prefix is not IPv6 prefix, is a
+    /// multicast address or the prefix length is greater than 128.
     IPv6Resrv(const asiolink::IOAddress& prefix,
               const uint8_t prefix_len = 128);
 
@@ -85,10 +85,10 @@ public:
     ///
     /// @param prefix New prefix.
     /// @param prefix_len New prefix length.
-    void set(const asiolink::IOAddress& prefix, const uint8_t prefix_len) {
-        prefix_ = prefix;
-        prefix_len_ = prefix_len;
-    }
+    ///
+    /// @throw isc::BadValue if prefix is not IPv6 prefix, is a
+    /// multicast address or the prefix length is greater than 128.
+    void set(const asiolink::IOAddress& prefix, const uint8_t prefix_len);
 
     /// @brief Equality operator.
     ///

src/lib/dhcpsrv/tests/host_unittest.cc

@@ -42,11 +42,21 @@ TEST(IPv6ResrvTest, constructorPrefix) {
     EXPECT_EQ(IPv6Resrv::TYPE_PD, resrv.getType());
 }
 
+// This test verifies that invalid prefix is rejected.
 TEST(IPv6ResrvTest, constructorInvalidPrefix) {
+    // IPv4 address is invalid for IPv6 reservation.
     EXPECT_THROW(IPv6Resrv(IOAddress("10.0.0.1"), 128), isc::BadValue);
+    // Multicast address is invalid for IPv6 reservation.
     EXPECT_THROW(IPv6Resrv(IOAddress("ff02:1::2"), 128), isc::BadValue);
 }
 
+// This test verifies that invalid prefix length is rejected.
+TEST(IPv6ResrvTest, constructiorInvalidPrefixLength) {
+    ASSERT_NO_THROW(IPv6Resrv(IOAddress("2001:db8:1::"), 128));
+    EXPECT_THROW(IPv6Resrv(IOAddress("2001:db8:1::"), 129), isc::BadValue);
+    EXPECT_THROW(IPv6Resrv(IOAddress("2001:db8:1::"), 244), isc::BadValue);
+}
+
 // This test verifies that it is possible to modify prefix and its
 // length in an existing reservation.
 TEST(IPv6ResrvTest, setPrefix) {
@@ -61,6 +71,13 @@ TEST(IPv6ResrvTest, setPrefix) {
     EXPECT_EQ("2001:db8::", resrv.getPrefix().toText());
     EXPECT_EQ(48, resrv.getPrefixLen());
     EXPECT_EQ(IPv6Resrv::TYPE_PD, resrv.getType());
+
+    // IPv4 address is invalid for IPv6 reservation.
+    EXPECT_THROW(resrv.set(IOAddress("10.0.0.1"), 128), isc::BadValue);
+    // IPv6 multicast address is invalid for IPv6 reservation.
+    EXPECT_THROW(resrv.set(IOAddress("ff02::1:2"), 128), isc::BadValue);
+    // Prefix length greater than 128 is invalid.
+    EXPECT_THROW(resrv.set(IOAddress("2001:db8:1::"), 129), isc::BadValue);
 }
 
 // This test checks that the equality operators work fine.
@@ -346,19 +363,23 @@ TEST(HostTest, setValues) {
     boost::scoped_ptr<Host> host;
     ASSERT_NO_THROW(host.reset(new Host("01:02:03:04:05:06", "hw-address",
                                         SubnetID(1), SubnetID(2),
-                                        IOAddress("192.0.2.3"))));
+                                        IOAddress("192.0.2.3"),
+                                        "some-host.example.org")));
 
     ASSERT_EQ(1, host->getIPv4SubnetID());
     ASSERT_EQ(2, host->getIPv6SubnetID());
     ASSERT_EQ("192.0.2.3", host->getIPv4Reservation().toText());
+    ASSERT_EQ("some-host.example.org", host->getHostname());
 
     host->setIPv4SubnetID(SubnetID(123));
     host->setIPv6SubnetID(SubnetID(234));
     host->setIPv4Reservation(IOAddress("10.0.0.1"));
+    host->setHostname("other-host.example.org");
 
     EXPECT_EQ(123, host->getIPv4SubnetID());
     EXPECT_EQ(234, host->getIPv6SubnetID());
     EXPECT_EQ("10.0.0.1", host->getIPv4Reservation().toText());
+    EXPECT_EQ("other-host.example.org", host->getHostname());
 }
 
 // Test that Host constructors initialize client classes from string.