From: Tomek Mrugalski Date: Wed, 18 Feb 2015 10:35:46 +0000 (+0100) Subject: [3711] Changes after first part of the review: X-Git-Tag: trac3723_base~18^2~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=02c32188f4fbcb98015b3b318f709fde6f4de90b;p=thirdparty%2Fkea.git [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 --- diff --git a/src/lib/asiolink/io_address.cc b/src/lib/asiolink/io_address.cc index 31e3cb4047..08229fda6e 100644 --- a/src/lib/asiolink/io_address.cc +++ b/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(a) - static_cast(b))); } else { // v6 is more involved. // Let's extract the raw data first. - vector a_vec = a.toBytes(); - vector b_vec = b.toBytes(); + const vector& a_vec(a.toBytes()); + const vector& b_vec(b.toBytes()); // ... and prepare the result - vector result(16,0); + vector 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& 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; } } diff --git a/src/lib/asiolink/io_address.h b/src/lib/asiolink/io_address.h index 9ccbd0921f..48fd2d49b7 100644 --- a/src/lib/asiolink/io_address.h +++ b/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 /// diff --git a/src/lib/asiolink/tests/io_address_unittest.cc b/src/lib/asiolink/tests/io_address_unittest.cc index 84b9bcc00c..0fb5d033c2 100644 --- a/src/lib/asiolink/tests/io_address_unittest.cc +++ b/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()); } diff --git a/src/lib/dhcpsrv/addr_utilities.cc b/src/lib/dhcpsrv/addr_utilities.cc index 6505f973d1..ebe3594e8c 100644 --- a/src/lib/dhcpsrv/addr_utilities.cc +++ b/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(max); + uint64_t min_numeric = static_cast(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::max. Let's do it the manual way. - std::vector bin = count.toBytes(); + const std::vector& 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(2)) << (count - 1)); } } diff --git a/src/lib/dhcpsrv/addr_utilities.h b/src/lib/dhcpsrv/addr_utilities.h index 5ad32957ab..0a19b780f8 100644 --- a/src/lib/dhcpsrv/addr_utilities.h +++ b/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); }; }; diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index d05f69dfb5..927ed91971 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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(*it); if (!pool6) { diff --git a/src/lib/dhcpsrv/tests/addr_utilities_unittest.cc b/src/lib/dhcpsrv/tests/addr_utilities_unittest.cc index 906cec301d..af742007fa 100644 --- a/src/lib/dhcpsrv/tests/addr_utilities_unittest.cc +++ b/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::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::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. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h index 543218d51f..c0ec632707 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h +++ b/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()); }