[3711] Changes after first part of the review:

 - increaseAddress renamed to increase
 - check that addrsInRange throw when max < min
 - C++ style casts, consts added
 - check that a can be subtracted from b, when a>b
 - documentation for IOAddress::increase, subtract improved
 - couple unnecessary intermediate objects removed

src/lib/asiolink/io_address.cc

@@ -133,21 +133,21 @@ operator<<(std::ostream& os, const IOAddress& address) {
 
 IOAddress
 IOAddress::subtract(const IOAddress& a, const IOAddress& b) {
-    if (a.isV4() != b.isV4()) {
+    if (a.getFamily() != b.getFamily()) {
         isc_throw(BadValue, "Both addresses have to be the same family");
     }
     if (a.isV4()) {
         // Subtracting v4 is easy. We have uint32_t operator.
-        return (IOAddress((uint32_t)a - (uint32_t)b));
+        return (IOAddress(static_cast<uint32_t>(a) - static_cast<uint32_t>(b)));
     } else {
         // v6 is more involved.
 
         // Let's extract the raw data first.
-        vector<uint8_t> a_vec = a.toBytes();
-        vector<uint8_t> b_vec = b.toBytes();
+        const vector<uint8_t>& a_vec(a.toBytes());
+        const vector<uint8_t>& b_vec(b.toBytes());
 
         // ... and prepare the result
-        vector<uint8_t> result(16,0);
+        vector<uint8_t> result(V6ADDRESS_LEN,0);
 
         // Carry is a boolean, but to avoid its frequent casting, let's
         // use uint8_t. Also, some would prefer to call it borrow, but I prefer
@@ -156,14 +156,9 @@ IOAddress::subtract(const IOAddress& a, const IOAddress& b) {
         uint8_t carry = 0;
 
         // Now perform subtraction with borrow.
-        for (int i = 15; i >=0; --i) {
-            if (a_vec[i] >= (b_vec[i] + carry) ) {
-                result[i] = a_vec[i] - b_vec[i] - carry;
-                carry = 0;
-            } else {
-                result[i] = a_vec[i] - b_vec[i] - carry;
-                carry = 1;
-            }
+        for (int i = a_vec.size(); i >= 0; --i) {
+            result[i] = a_vec[i] - b_vec[i] - carry;
+            carry = (a_vec[i] < b_vec[i] + carry);
         }
 
         return (fromBytes(AF_INET6, &result[0]));
@@ -171,7 +166,7 @@ IOAddress::subtract(const IOAddress& a, const IOAddress& b) {
 }
 
 IOAddress
-IOAddress::increaseAddress(const IOAddress& addr) {
+IOAddress::increase(const IOAddress& addr) {
     // Get a buffer holding an address.
     const std::vector<uint8_t>& vec = addr.toBytes();
     // Get the address length.
@@ -188,9 +183,8 @@ IOAddress::increaseAddress(const IOAddress& addr) {
 
     // Start increasing the least significant byte
     for (int i = len - 1; i >= 0; --i) {
-        ++packed[i];
         // if we haven't overflowed (0xff -> 0x0), than we are done
-        if (packed[i] != 0) {
+        if (++packed[i] != 0) {
             break;
         }
     }

src/lib/asiolink/io_address.h

@@ -227,6 +227,13 @@ public:
     /// 192.0.2.5 - 192.0.2.0 = 0.0.0.5
     /// fe80::abcd - fe80:: = ::abcd
     ///
+    /// It is possible to subtract greater from lesser address, e.g.
+    /// 192.168.56.10 - 192.168.67.20, but please do understand that
+    /// the address space is a finite field in mathematical sense, so
+    /// you may end up with a result that is greater then any of the
+    /// addresses you specified. Also, subtraction is not commutative,
+    /// so a - b != b - a.
+    ///
     /// This operation is essential for calculating the number of
     /// leases in a pool, where we need to calculate (max - min).
     /// @throw BadValue if addresses are of different family
@@ -240,10 +247,20 @@ public:
     /// This method works for both IPv4 and IPv6 addresses. For example,
     /// increase 192.0.2.255 will become 192.0.3.0.
     ///
+    /// Address space is a finite field in the mathematical sense, so keep
+    /// in mind that the address space "loops". 255.255.255.255 increased
+    /// by one gives 0.0.0.0. The same is true for maximum value of IPv6
+    /// (all 1's) looping to ::.
+    ///
+    /// @todo Determine if we have a use-case for increasing the address
+    /// by more than one. Increase by one is used in AllocEngine. This method
+    /// could take extra parameter that specifies the value by which the
+    /// address should be increased.
+    ///
     /// @param addr address to be increased
     /// @return address increased by one
     static IOAddress
-    increaseAddress(const IOAddress& addr);
+    increase(const IOAddress& addr);
 
     /// \brief Converts IPv4 address to uint32_t
     ///

src/lib/asiolink/tests/io_address_unittest.cc

@@ -236,6 +236,10 @@ TEST(IOAddressTest, subtract) {
     EXPECT_EQ("192.0.2.13", IOAddress::subtract(addr1, bcast).toText());
     EXPECT_EQ("191.255.255.255", IOAddress::subtract(addr3, addr4).toText());
 
+    // Let's check if we can subtract greater address from smaller.
+    // This will check if we can "loop"
+    EXPECT_EQ("255.255.255.251", IOAddress::subtract(addr3, addr2).toText());
+
     IOAddress addr6("fe80::abcd");
     IOAddress addr7("fe80::");
     IOAddress addr8("fe80::1234");
@@ -260,6 +264,11 @@ TEST(IOAddressTest, subtract) {
     // Subtracting :: is like subtracting 0.
     EXPECT_EQ("2001:db8::face", IOAddress::subtract(addr9, any6).toText());
 
+    // Let's check if we can subtract greater address from smaller.
+    // This will check if we can "loop"
+    EXPECT_EQ("ffff:ffff:ffff:ffff:ffff:ffff:ffff:edcc",
+              IOAddress::subtract(addr7, addr8).toText());
+
     // Inter-family relations are not allowed.
     EXPECT_THROW(IOAddress::subtract(addr1, addr6), isc::BadValue);
     EXPECT_THROW(IOAddress::subtract(addr6, addr1), isc::BadValue);
@@ -275,11 +284,11 @@ TEST(IOAddressTest, increaseAddr) {
     IOAddress any6("::");
     IOAddress the_last_one("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff");
 
-    EXPECT_EQ("192.0.2.13", IOAddress::increaseAddress(addr1).toText());
-    EXPECT_EQ("0.0.0.1", IOAddress::increaseAddress(any4).toText());
-    EXPECT_EQ("0.0.0.0", IOAddress::increaseAddress(bcast).toText());
-    EXPECT_EQ("2001:db8:0:1::", IOAddress::increaseAddress(addr6).toText());
-    EXPECT_EQ("::2", IOAddress::increaseAddress(addr11).toText());
-    EXPECT_EQ("::1", IOAddress::increaseAddress(any6).toText());
-    EXPECT_EQ("::", IOAddress::increaseAddress(the_last_one).toText());
+    EXPECT_EQ("192.0.2.13", IOAddress::increase(addr1).toText());
+    EXPECT_EQ("0.0.0.1", IOAddress::increase(any4).toText());
+    EXPECT_EQ("0.0.0.0", IOAddress::increase(bcast).toText());
+    EXPECT_EQ("2001:db8:0:1::", IOAddress::increase(addr6).toText());
+    EXPECT_EQ("::2", IOAddress::increase(addr11).toText());
+    EXPECT_EQ("::1", IOAddress::increase(any6).toText());
+    EXPECT_EQ("::", IOAddress::increase(the_last_one).toText());
 }

src/lib/dhcpsrv/addr_utilities.cc

@@ -210,16 +210,21 @@ isc::asiolink::IOAddress getNetmask4(uint8_t len) {
 uint64_t
 addrsInRange(const isc::asiolink::IOAddress& min,
              const isc::asiolink::IOAddress& max) {
-    if (min.isV4() != max.isV4()) {
+    if (min.getFamily() != max.getFamily()) {
         isc_throw(BadValue, "Both addresses have to be the same family");
     }
 
+    if (max < min) {
+        isc_throw(BadValue, min.toText() << " must not be greater than "
+                  << max.toText());
+    }
+
     if (min.isV4()) {
         // Let's explicitly cast last_ and first_ (IOAddress). This conversion is
         // automatic, but let's explicitly cast it show that we moved to integer
         // domain and addresses are now substractable.
-        uint64_t max_numeric = uint32_t(max);
-        uint64_t min_numeric = uint32_t(min);
+        uint64_t max_numeric = static_cast<uint32_t>(max);
+        uint64_t min_numeric = static_cast<uint32_t>(min);
 
         // We can simply subtract the values. We need to increase the result
         // by one, as both min and max are included in the range. So even if
@@ -247,11 +252,11 @@ addrsInRange(const isc::asiolink::IOAddress& min,
 
         // Increase it by one (a..a range still contains one address, even though
         // a subtracted from a is zero).
-        count = IOAddress::increaseAddress(count);
+        count = IOAddress::increase(count);
 
         // We don't have uint128, so for anything greater than 2^64, we'll just
         // assume numeric_limits<uint64_t>::max. Let's do it the manual way.
-        std::vector<uint8_t> bin = count.toBytes();
+        const std::vector<uint8_t>& bin(count.toBytes());
 
         // If any of the most significant 64 bits is set, we have more than
         // 2^64 addresses and can't represent it even on uint64_t.
@@ -273,7 +278,7 @@ addrsInRange(const isc::asiolink::IOAddress& min,
     }
 }
 
-uint64_t prefixesInRange(uint8_t pool_len, uint8_t delegated_len) {
+uint64_t prefixesInRange(const uint8_t pool_len, const uint8_t delegated_len) {
     if (delegated_len < pool_len) {
         return (0);
     }
@@ -293,7 +298,7 @@ uint64_t prefixesInRange(uint8_t pool_len, uint8_t delegated_len) {
         // Now count specifies the exponent (e.g. if the difference between the
         // delegated and pool length is 4, we have 16 prefixes), so we need
         // to calculate 2^(count - 1)
-        return (((uint64_t)2) << (count - 1));
+        return ((static_cast<uint64_t>(2)) << (count - 1));
     }
 }
 

src/lib/dhcpsrv/addr_utilities.h

@@ -79,10 +79,10 @@ uint64_t addrsInRange(const isc::asiolink::IOAddress& min,
 /// Example: prefixesInRange(48, 64) returns 65536, as there are /48 pool
 /// can be split into 65536 prefixes, each /64 large.
 ///
-/// @param min the first address in range
-/// @param max the last address in range
-/// @return number of addresses in range
-uint64_t prefixesInRange(uint8_t pool_len, uint8_t delegated_len);
+/// @param pool_len length of the pool in bits
+/// @param delegated_len length of the prefixes to be delegated from the pool
+/// @return number of prefixes in range
+uint64_t prefixesInRange(const uint8_t pool_len, const uint8_t delegated_len);
 };
 };
 

src/lib/dhcpsrv/alloc_engine.cc

@@ -165,7 +165,7 @@ AllocEngine::IterativeAllocator::pickAddress(const SubnetPtr& subnet,
 
     IOAddress next("::");
     if (!prefix) {
-        next = IOAddress::increaseAddress(last); // basically addr++
+        next = IOAddress::increase(last); // basically addr++
     } else {
         Pool6Ptr pool6 = boost::dynamic_pointer_cast<Pool6>(*it);
         if (!pool6) {

src/lib/dhcpsrv/tests/addr_utilities_unittest.cc

@@ -217,6 +217,10 @@ TEST(AddrUtilitiesTest, addrsInRange4) {
     // cannot be stored in uint32_t.
     EXPECT_EQ(uint64_t(std::numeric_limits<uint32_t>::max()) + 1,
               addrsInRange(IOAddress("0.0.0.0"), IOAddress("255.255.255.255")));
+
+    // The upper bound cannot be smaller than the lower bound.
+    EXPECT_THROW(addrsInRange(IOAddress("192.0.2.5"), IOAddress("192.0.2.4")),
+                 isc::BadValue);
 }
 
 // Checks if the calculation for IPv6 addresses in range are correct.
@@ -243,6 +247,9 @@ TEST(AddrUtilitiesTest, addrsInRange6) {
     // at max value of uint64_t.
     EXPECT_EQ(std::numeric_limits<uint64_t>::max(), addrsInRange(IOAddress("::"),
               IOAddress("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff")));
+
+    EXPECT_THROW(addrsInRange(IOAddress("fe80::5"), IOAddress("fe80::4")),
+                 isc::BadValue);
 }
 
 // Checks if prefixInRange returns valid number of prefixes in specified range.

src/lib/dhcpsrv/tests/alloc_engine_utils.h

@@ -181,7 +181,7 @@ public:
     void
     checkAddrIncrease(NakedAllocEngine::NakedIterativeAllocator&,
                       std::string input, std::string exp_output) {
-        EXPECT_EQ(exp_output, asiolink::IOAddress::increaseAddress(
+        EXPECT_EQ(exp_output, asiolink::IOAddress::increase(
                       asiolink::IOAddress(input)).toText());
     }