[2238] Changes after review.

ChangeLog

 4XX.	[func]		tomek
-	A new library (libb10-dhcpsrv) has been create. Currently its
-	functionality is limited to a Configuration Manager. CfgMgr
-	currently only supports basic configuration storage for DHCPv6
-	server,	but that capability is expected to be expanded in a near
-	future.
+	A new library (libb10-dhcpsrv) has been created. At present, it
+	only holds the code for the DHCP Configuration Manager. Currently
+	this object only supports basic configuration storage for the DHCPv6
+	server,	but that capability will be expanded.
 	(Trac #2238, git TBD)
 
 475.	[func]		naokikambe

src/lib/asiolink/io_address.h

@@ -150,18 +150,15 @@ public:
     /// Operations within one protocol family are obvious.
     /// Comparisons between v4 and v6 will allways return v4
     /// being smaller. This follows boost::asio::ip implementation
-    bool smallerThan(const IOAddress& other) const {
-        if (this->getFamily() < other.getFamily()) {
-            return (true);
-        }
-        if (this->getFamily() > other.getFamily()) {
-            return (false);
-        }
-        if (this->getFamily() == AF_INET6) {
-            return (this->asio_address_.to_v6() < other.asio_address_.to_v6());
-        } else {
-            return (this->asio_address_.to_v4() < other.asio_address_.to_v4());
+    bool lessThan(const IOAddress& other) const {
+        if (this->getFamily() == other.getFamily()) {
+            if (this->getFamily() == AF_INET6) {
+                return (this->asio_address_.to_v6() < other.asio_address_.to_v6());
+            } else {
+                return (this->asio_address_.to_v4() < other.asio_address_.to_v4());
+            }
         }
+        return (this->getFamily() < other.getFamily());
     }
 
     /// \brief Checks if one address is smaller or equal than the other
@@ -173,7 +170,7 @@ public:
         if (equals(other)) {
             return (true);
         }
-        return (smallerThan(other));
+        return (lessThan(other));
     }
 
     /// \brief Checks if one address is smaller than the other
@@ -182,7 +179,7 @@ public:
     ///
     /// See \ref smaller_than method for details.
     bool operator<(const IOAddress& other) const {
-        return (smallerThan(other));
+        return (lessThan(other));
     }
 
     /// \brief Checks if one address is smaller or equal than the other

src/lib/asiolink/tests/io_address_unittest.cc

@@ -100,7 +100,7 @@ TEST(IOAddressTest, uint32) {
     EXPECT_EQ(addr3.toText(), "192.0.2.5");
 }
 
-TEST(IOAddressTest, compare) {
+TEST(IOAddressTest, lessThanEqual) {
     IOAddress addr1("192.0.2.5");
     IOAddress addr2("192.0.2.6");
     IOAddress addr3("0.0.0.0");

src/lib/dhcp/addr_utilities.cc

@@ -14,46 +14,80 @@
 
 #include <dhcp/addr_utilities.h>
 
+using namespace isc::asiolink;
+
 namespace isc {
 namespace dhcp {
 
 isc::asiolink::IOAddress firstAddrInPrefix(const isc::asiolink::IOAddress& prefix,
-                                           uint8_t len) {
+                                            uint8_t len) {
 
     static char bitMask[]= { 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff };
-    uint8_t packed[16];
+    uint8_t packed[V6ADDRESS_LEN];
 
+    // First we copy the whole address as 16 bytes.
     memcpy(packed, prefix.getAddress().to_v6().to_bytes().data(), 16);
 
+    // If the length is divisible by 8, it is simple. We just zero out the host
+    // part. Otherwise we need to handle the byte that has to be partially
+    // zeroed.
     if (len % 8 != 0) {
+
+        // Get the appropriate mask. It has relevant bits (those that should
+        // stay) set and irrelevant (those that should be wiped) cleared.
         uint8_t mask = bitMask[len % 8];
+
+        // Let's leave only whatever the mask says should not be cleared.
         packed[len / 8] = packed[len / 8] & mask;
-        len = (len/8 + 1) * 8;
+
+        // Since we have just dealt with this byte, let's move the prefix length
+        // to the beginning of the next byte (len is expressed in bits).
+        len = (len / 8 + 1) * 8;
     }
-    for (int i = len / 8; i < 16; ++i) {
+
+    // Clear out the remaining bits.
+    for (int i = len / 8; i < sizeof(packed); ++i) {
         packed[i] = 0x0;
     }
 
+    // Finally, let's wrap this into nice and easy IOAddress object.
     return (isc::asiolink::IOAddress::from_bytes(AF_INET6, packed));
 }
 
 isc::asiolink::IOAddress lastAddrInPrefix(const isc::asiolink::IOAddress& prefix,
-                                          uint8_t len) {
+                                           uint8_t len) {
 
     static char bitMask[]= { 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff };
-    uint8_t packed[16];
+    uint8_t packed[V6ADDRESS_LEN];
 
+    // First we copy the whole address as 16 bytes.
     memcpy(packed, prefix.getAddress().to_v6().to_bytes().data(), 16);
 
+    // if the length is divisible by 8, it is simple. We just fill the host part
+    // with ones. Otherwise we need to handle the byte that has to be partially
+    // zeroed.
     if (len % 8 != 0) {
+        // Get the appropriate mask. It has relevant bits (those that should
+        // stay) set and irrelevant (those that should be set to 1) cleared.
         uint8_t mask = bitMask[len % 8];
+
+        // Let's set those irrelevant bits with 1. It would be perhaps
+        // easier to not use negation here and invert bitMask content. However,
+        // with this approach, we can use the same mask in first and last
+        // address calculations.
         packed[len / 8] = packed[len / 8] | ~mask;
-        len = (len/8 + 1) * 8;
+
+        // Since we have just dealt with this byte, let's move the prefix length
+        // to the beginning of the next byte (len is expressed in bits).
+        len = (len / 8 + 1) * 8;
     }
-    for (int i = len / 8; i < 16; ++i) {
+
+    // Finally set remaining bits to 1.
+    for (int i = len / 8; i < sizeof(packed); ++i) {
         packed[i] = 0xff;
     }
 
+    // Finally, let's wrap this into nice and easy IOAddress object.
     return (isc::asiolink::IOAddress::from_bytes(AF_INET6, packed));
 }
 

src/lib/dhcp/addr_utilities.h

@@ -18,32 +18,36 @@ namespace isc {
 namespace dhcp {
 
 /// This code is based on similar code from the Dibbler project. I, Tomasz Mrugalski,
-/// as a sole creater of that code hereby release it under BSD license for the benefit
+/// as a sole creator of that code hereby release it under BSD license for the benefit
 /// of the BIND10 project.
 
 /// @brief returns a first address in a given prefix
 ///
 /// Example: For 2001:db8:1::deaf:beef and length /120 the function will return
-/// 2001:db8:1::dead:bee0. See also @ref lastAddrInPrefix.
+/// 2001:db8:1::dead:be00. See also @ref lastAddrInPrefix.
+///
+/// @todo It currently works for v6 only and will throw if v4 address is passed.
 ///
 /// @param prefix and address that belongs to a prefix
 /// @param len prefix length
 ///
 /// @return first address from a prefix
 isc::asiolink::IOAddress firstAddrInPrefix(const isc::asiolink::IOAddress& prefix,
-                                           uint8_t len);
+                                            uint8_t len);
 
 /// @brief returns a last address in a given prefix
 ///
 /// Example: For 2001:db8:1::deaf:beef and length /112 the function will return
 /// 2001:db8:1::dead:ffff. See also @ref firstAddrInPrefix.
 ///
+/// @todo It currently works for v6 only and will throw if v4 address is passed.
+///
 /// @param prefix and address that belongs to a prefix
 /// @param len prefix length
 ///
 /// @return first address from a prefix
 isc::asiolink::IOAddress lastAddrInPrefix(const isc::asiolink::IOAddress& prefix,
-                                          uint8_t len);
+                                           uint8_t len);
 
 };
 };

src/lib/dhcp/cfgmgr.cc

@@ -27,8 +27,8 @@ Pool::Pool(const isc::asiolink::IOAddress& first,
     :id_(getNextID()), first_(first), last_(last) {
 }
 
-bool Pool::inRange(const isc::asiolink::IOAddress& addr) {
-    return ( first_.smallerEqual(addr) && addr.smallerEqual(last_) );
+bool Pool::inRange(const isc::asiolink::IOAddress& addr) const {
+    return (first_.smallerEqual(addr) && addr.smallerEqual(last_));
 }
 
 Pool6::Pool6(Pool6Type type, const isc::asiolink::IOAddress& first,
@@ -46,12 +46,14 @@ Pool6::Pool6(Pool6Type type, const isc::asiolink::IOAddress& first,
         // we need to comment it and uncomment lines below.
         // On one hand, letting the user specify 2001::f - 2001::1 is nice, but
         // on the other hand, 2001::1 may be a typo and the user really meant
-        // 2001::1:0 (or 1something), so a at least a warning would be useful.
+        // 2001::1:0 (or 1 followed by some hex digit), so a at least a warning
+        // would be useful.
 
         // first_  = last;
         // last_ = first;
     }
 
+
     // TYPE_PD is not supported by this constructor. first-last style
     // parameters are for IA and TA only. There is another dedicated
     // constructor for that (it uses prefix/length)
@@ -75,6 +77,9 @@ Pool6::Pool6(Pool6Type type, const isc::asiolink::IOAddress& prefix,
         isc_throw(BadValue, "Invalid prefix length");
     }
 
+    /// @todo: We should probably implement checks against weird addresses
+    /// here, like ::, starting with fe80, starting with ff etc. .
+
     // Let's now calculate the last address in defined pool
     last_ = lastAddrInPrefix(prefix, prefix_len);
 }
@@ -91,11 +96,11 @@ Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len,
     }
 }
 
-bool Subnet::inRange(const isc::asiolink::IOAddress& addr) {
+bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const {
     IOAddress first = firstAddrInPrefix(prefix_, prefix_len_);
     IOAddress last = lastAddrInPrefix(prefix_, prefix_len_);
 
-    return ( (first <= addr) && (addr <= last) );
+    return ((first <= addr) && (addr <= last));
 }
 
 Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length,
@@ -106,7 +111,7 @@ Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length,
     :Subnet(prefix, length, t1, t2, valid_lifetime),
      preferred_(preferred_lifetime){
     if (prefix.getFamily() != AF_INET6) {
-        isc_throw(BadValue, "Invalid prefix " << prefix.toText()
+        isc_throw(BadValue, "Non IPv6 prefix " << prefix.toText()
                   << " specified in subnet6");
     }
 }
@@ -121,6 +126,8 @@ void Subnet6::addPool6(const Pool6Ptr& pool) {
                   << ") subnet6");
     }
 
+    /// @todo: Check that pools do not overlap
+
     pools_.push_back(pool);
 }
 
@@ -153,6 +160,13 @@ Subnet6Ptr
 CfgMgr::getSubnet6(const isc::asiolink::IOAddress& hint) {
 
     // If there's only one subnet configured, let's just use it
+    // The idea is to keep small deployments easy. In a small network - one
+    // router that also runs DHCPv6 server. Users specifies a single pool and
+    // expects it to just work. Without this, the server would complain that it
+    // doesn't have IP address on its interfaces that matches that
+    // configuration. Such requirement makes sense in IPv4, but not in IPv6.
+    // The server does not need to have a global address (using just link-local
+    // is ok for DHCPv6 server) from the pool it serves.
     if (subnets6_.size() == 1) {
         return (subnets6_[0]);
     }

src/lib/dhcp/cfgmgr.h

@@ -31,7 +31,7 @@ class Pool6;
 
 class Subnet6;
 
-/// @brief this template specifes a parameter value
+/// @brief this template specifies a parameter value
 ///
 /// This template class is used to store configuration parameters, like lifetime or T1.
 /// It defines 3 parameters: min, default, and max value. There are 2 constructors:
@@ -48,15 +48,18 @@ public:
     ///
     /// Typically: uint32_t to Triplet assignment. It is very convenient
     /// to be able to simply write Triplet<uint32_t> x = 7;
-    Triplet<T>& operator = (T base_type) {
-        return Triplet<T>(base_type);
+    Triplet<T> operator=(T other) {
+        min_ = other;
+        default_ = other;
+        max_ = other;
+        return *this;
     }
 
     /// @brief triplet to base type conversion
     ///
     /// Typically: Triplet to uint32_t assignment. It is very convenient
     /// to be able to simply write uint32_t z = x; (where x is a Triplet)
-    operator T () const {
+    operator T() const {
         return (default_);
     }
 
@@ -73,7 +76,7 @@ public:
     /// @throw BadValue if min <= def <= max rule is violated
     Triplet(T min, T def, T max)
         :min_(min), default_(def), max_(max) {
-        if ( (min_>def) || (def > max_) ) {
+        if ( (min_ > def) || (def > max_) ) {
             isc_throw(BadValue, "Invalid triplet values.");
         }
     }
@@ -127,6 +130,7 @@ public:
 
     /// @brief returns Pool-id
     ///
+    /// @return pool-id value
     /// Pool-id is an unique value that can be used to identify a pool.
     uint32_t getId() const {
         return (id_);
@@ -148,7 +152,7 @@ public:
     /// @brief Checks if a given address is in the range.
     ///
     /// @return true, if the address is in pool
-    bool inRange(const isc::asiolink::IOAddress& addr);
+    bool inRange(const isc::asiolink::IOAddress& addr) const;
 
 protected:
 
@@ -170,7 +174,7 @@ protected:
 
     /// @brief pool-id
     ///
-    /// This ID is used to indentify this specific pool.
+    /// This ID is used to identify this specific pool.
     uint32_t id_;
 
     /// @brief The first address in a pool
@@ -227,7 +231,7 @@ public:
         return (type_);
     }
 
-protected:
+private:
     /// @brief defines a pool type
     Pool6Type type_;
 
@@ -255,7 +259,7 @@ typedef std::vector<Pool6Ptr> Pool6Collection;
 class Subnet {
 public:
     /// @brief checks if specified address is in range
-    bool inRange(const isc::asiolink::IOAddress& addr);
+    bool inRange(const isc::asiolink::IOAddress& addr) const;
 
     /// @brief return valid-lifetime for addresses in that prefix
     Triplet<uint32_t> getValid() const {
@@ -317,29 +321,54 @@ protected:
 /// This class represents an IPv6 subnet.
 class Subnet6 : public Subnet {
 public:
+
+    /// @brief Constructor with all parameters
+    ///
+    /// @param prefix Subnet6 prefix
+    /// @param length prefix length
+    /// @param t1 renewal timer (in seconds)
+    /// @param t2 rebind timer (in seconds)
+    /// @param preferred_lifetime preferred lifetime of leases (in seconds)
+    /// @param valid_lifetime preferred lifetime of leases (in seconds)
     Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length,
             const Triplet<uint32_t>& t1,
             const Triplet<uint32_t>& t2,
             const Triplet<uint32_t>& preferred_lifetime,
             const Triplet<uint32_t>& valid_lifetime);
 
+    /// @brief Returns preverred lifetime (in seconds)
+    ///
+    /// @return a triplet with preferred lifetime
     Triplet<uint32_t> getPreferred() const {
         return (preferred_);
     }
 
+    /// @brief Returns a pool that specified address belongs to
+    ///
+    /// @param hint address that the returned pool should cover (optional)
+    /// @return Pointer to found pool6 (or NULL)
     Pool6Ptr getPool6(const isc::asiolink::IOAddress& hint =
                       isc::asiolink::IOAddress("::"));
 
+    /// @brief Adds a new pool.
+    /// @param pool pool to be added
     void addPool6(const Pool6Ptr& pool);
 
+    /// @brief returns all pools
+    ///
+    /// The reference is only valid as long as the object that
+    /// returned it.
+    ///
+    /// @return a collection of all pools
     const Pool6Collection& getPools() const {
         return pools_;
     }
 
 protected:
-    /// collection of pools in that list
+    /// @brief collection of pools in that list
     Pool6Collection pools_;
 
+    /// @brief a triplet with preferred lifetime (in seconds)
     Triplet<uint32_t> preferred_;
 };
 

src/lib/dhcp/tests/Makefile.am

@@ -51,6 +51,7 @@ libdhcpsrv_unittests_LDADD  = $(GTEST_LDADD)
 libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la
 libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la
 libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcpsrv.la
+libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la
 
 
 if USE_CLANGPP
@@ -66,6 +67,7 @@ libdhcp___unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/util/libb10-util.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la
+libdhcp___unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la
 libdhcp___unittests_LDADD += $(GTEST_LDADD)
 endif
 

src/lib/dhcp/tests/addr_utilities_unittest.cc

@@ -53,6 +53,14 @@ TEST(Pool6Test, lastAddrInPrefix) {
     EXPECT_EQ("2001::3f", lastAddrInPrefix(addr2, 122).toText());
     EXPECT_EQ("2001::7f", lastAddrInPrefix(addr2, 121).toText());
     EXPECT_EQ("2001::ff", lastAddrInPrefix(addr2, 120).toText());
+
+    // Let's check extreme cases
+    IOAddress anyAddr("::");
+    EXPECT_EQ("7fff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
+              lastAddrInPrefix(anyAddr, 1).toText());
+    EXPECT_EQ("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
+              lastAddrInPrefix(anyAddr, 0).toText());
+    EXPECT_EQ("::", lastAddrInPrefix(anyAddr, 128).toText());
 }
 
 TEST(Pool6Test, firstAddrInPrefix) {

src/lib/dhcp/tests/cfgmgr_unittest.cc

@@ -35,28 +35,28 @@ namespace {
 // constructor validation
 TEST(TripletTest, constructor) {
 
-    uint32_t min = 10;
-    uint32_t value = 20;
-    uint32_t max = 30;
+    const uint32_t min = 10;
+    const uint32_t value = 20;
+    const uint32_t max = 30;
 
     Triplet<uint32_t> x(min, value, max);
 
-    EXPECT_EQ(10, x.getMin());
-    EXPECT_EQ(20, x.get());
-    EXPECT_EQ(30, x.getMax());
+    EXPECT_EQ(min, x.getMin());
+    EXPECT_EQ(value, x.get());
+    EXPECT_EQ(max, x.getMax());
 
     // requested values below min should return allowed min value
-    EXPECT_EQ(10, x.get(5));
+    EXPECT_EQ(min, x.get(min - 5));
 
-    EXPECT_EQ(10, x.get(10));
+    EXPECT_EQ(min, x.get(min));
 
     // requesting a value from within the range (min < x < max) should
     // return the requested value
     EXPECT_EQ(17, x.get(17));
 
-    EXPECT_EQ(30, x.get(30));
+    EXPECT_EQ(max, x.get(max));
 
-    EXPECT_EQ(30, x.get(35));
+    EXPECT_EQ(max, x.get(max + 5));
 
     // this will be boring. It is expected to return 42 no matter what
     Triplet<uint32_t> y(42);
@@ -77,15 +77,26 @@ TEST(TripletTest, operator) {
 
     uint32_t x = 47;
 
+    Triplet<uint32_t> foo(1,2,3);
+    Triplet<uint32_t> bar(4,5,6);
+
+    foo = bar;
+
+    EXPECT_EQ(4, foo.getMin());
+    EXPECT_EQ(5, foo.get());
+    EXPECT_EQ(6, foo.getMax());
+
     // assignment operator: uint32_t => triplet
-    Triplet<uint32_t> y = x;
+    Triplet<uint32_t> y(0);
+    y = x;
 
-    EXPECT_EQ(47, y.get());
+    EXPECT_EQ(x, y.get());
 
     // let's try the other way around: triplet => uint32_t
-    uint32_t z = y;
+    uint32_t z = 0;
+    z = y;
 
-    EXPECT_EQ(47, z);
+    EXPECT_EQ(x, z);
 }
 
 // check if specified values are sane
@@ -131,6 +142,14 @@ TEST(Pool6Test, constructor_prefix_len) {
     EXPECT_EQ("2001:db8:1::", pool1.getFirstAddress().toText());
     EXPECT_EQ("2001:db8:1::ffff:ffff", pool1.getLastAddress().toText());
 
+    // No such thing as /130 prefix
+    EXPECT_THROW(Pool6(Pool6::TYPE_IA, IOAddress("2001:db8::"), 130),
+                 BadValue);
+
+    // /0 prefix does not make sense
+    EXPECT_THROW(Pool6(Pool6::TYPE_IA, IOAddress("2001:db8::"), 0),
+                 BadValue);
+
     // This is Pool6, IPv4 addresses do not belong here
     EXPECT_THROW(Pool6(Pool6::TYPE_IA, IOAddress("192.168.0.2"), 96),
                  BadValue);

src/lib/dhcp/tests/run_unittests.cc

@@ -13,10 +13,12 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <gtest/gtest.h>
+#include <log/logger_support.h>
 
 int
 main(int argc, char* argv[]) {
     ::testing::InitGoogleTest(&argc, argv);
+    isc::log::initLogger();
 
     int result = RUN_ALL_TESTS();