Skip to content

GitLab

Commit 21794c5d authored by Tomek Mrugalski's avatar Tomek Mrugalski 🛰
Browse files

[2320] Changes after review.

parent d5310a9c
Hide whitespace changes
Inline Side-by-side
doc/guide/bind10-guide.xml
View file @ 21794c5d
......@@ -3504,7 +3504,7 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";</screen>
<itemizedlist>
<listitem>
<simpara>RFC2131: Supported messages are DISCOVER, OFFER,
REQUEST, and ACK.</simpara>
REQUEST, ACK, NACK, RELEASE.</simpara>
</listitem>
<listitem>
<simpara>RFC2132: Supported options are: PAD (0),
......@@ -3512,6 +3512,10 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";</screen>
Domain Name (15), DNS Servers (6), IP Address Lease Time
(51), Subnet mask (1), and Routers (3).</simpara>
</listitem>
<listitem>
<simpara>RFC6842: Server responses include client-id option
if client sent it in its message.</simpara>
</listitem>
</itemizedlist>
</section>
......
src/bin/dhcp4/tests/dhcp4_unittests.cc
View file @ 21794c5d
......@@ -21,16 +21,10 @@ main(int argc, char* argv[]) {
::testing::InitGoogleTest(&argc, argv);
// See the documentation of the B10_* environment variables in
// src/lib/log/README for info on how to tweak logging
isc::log::initLogger();
// Uncomment those to get much more verbose tests
/*
isc::log::initLogger("b10-dhcp4",
isc::log::DEBUG,
isc::log::MAX_DEBUG_LEVEL, NULL, false);
isc::dhcp::dhcp4_logger.setSeverity(isc::log::DEBUG, 99);
*/
int result = RUN_ALL_TESTS();
return (result);
......
src/lib/dhcp/hwaddr.h
View file @ 21794c5d
......@@ -23,10 +23,22 @@
namespace isc {
namespace dhcp {
/// @brief Hardware type that represents information from DHCPv4 packet
struct HWAddr {
public:
/// @brief default constructor
HWAddr();
/// @brief constructor, based on C-style pointer and length
/// @param hwaddr pointer to hardware address
/// @param len length of the address pointed by hwaddr
/// @param htype hardware type
HWAddr(const uint8_t* hwaddr, size_t len, uint8_t htype);
/// @brief constructor, based on C++ vector<uint8_t>
/// @param hwaddr const reference to hardware address
/// @param htype hardware type
HWAddr(const std::vector<uint8_t>& hwaddr, uint8_t htype);
// Vector that keeps the actual hardware address
......@@ -48,7 +60,6 @@ public:
/// @brief Shared pointer to a hardware address structure
typedef boost::shared_ptr<HWAddr> HWAddrPtr;
}; // end of isc::dhcp namespace
}; // end of isc namespace
......
src/lib/dhcp/pkt4.cc
View file @ 21794c5d
......@@ -112,7 +112,7 @@ Pkt4::pack() {
bufferOut_.writeUint8(op_);
bufferOut_.writeUint8(hwaddr_->htype_);
bufferOut_.writeUint8(hw_len < 16 ? hw_len : 16);
bufferOut_.writeUint8(hw_len < MAX_CHADDR_LEN ? hw_len : MAX_CHADDR_LEN);
bufferOut_.writeUint8(hops_);
bufferOut_.writeUint32(transid_);
bufferOut_.writeUint16(secs_);
......@@ -123,13 +123,14 @@ Pkt4::pack() {
bufferOut_.writeUint32(giaddr_);
if (hw_len <=16) {
if (hw_len <= MAX_CHADDR_LEN) {
// write up to 16 bytes of the hardware address (CHADDR field is 16
// bytes long in DHCPv4 message).
bufferOut_.writeData(&hwaddr_->hwaddr_[0], (hw_len<16?hw_len:16) );
hw_len = 16 - hw_len;
bufferOut_.writeData(&hwaddr_->hwaddr_[0],
(hw_len < MAX_CHADDR_LEN ? hw_len : MAX_CHADDR_LEN) );
hw_len = MAX_CHADDR_LEN - hw_len;
} else {
hw_len = 16;
hw_len = MAX_CHADDR_LEN;
}
// write (len) bytes of padding
......@@ -288,7 +289,7 @@ Pkt4::setHWAddr(uint8_t hType, uint8_t hlen,
isc_throw(OutOfRange, "Invalid HW Address specified");
}
hwaddr_ = HWAddrPtr(new HWAddr(mac_addr, hType));
hwaddr_.reset(new HWAddr(mac_addr, hType));
}
void
......@@ -370,7 +371,7 @@ Pkt4::getHlen() const {
isc_throw(InvalidOperation, "Can't get HType. HWAddr not defined");
}
uint8_t len = hwaddr_->hwaddr_.size();
return (len <= 16 ? len : 16);
return (len <= MAX_CHADDR_LEN ? len : MAX_CHADDR_LEN);
}
void
......@@ -395,7 +396,7 @@ Pkt4::getOption(uint8_t type) const {
bool
Pkt4::delOption(uint8_t type) {
isc::dhcp::Option::OptionCollection::iterator x = options_.find(type);
if (x!=options_.end()) {
if (x != options_.end()) {
options_.erase(x);
return (true); // delete successful
}
......
src/lib/dhcp/tests/hwaddr_unittest.cc
View file @ 21794c5d
......@@ -90,8 +90,9 @@ TEST(HWAddrTest, operators) {
EXPECT_TRUE(*hw4 != *hw5);
}
// Checks that toText() method produces appropriate text representation
TEST(HWAddrTest, toText) {
uint8_t data[] = {0, 1, 2, 3, 4, 5}; // last digit different
uint8_t data[] = {0, 1, 2, 3, 4, 5};
uint8_t htype = 15;
HWAddrPtr hw(new HWAddr(data, sizeof(data), htype));
......
src/lib/dhcp/tests/pkt4_unittest.cc
View file @ 21794c5d
......@@ -222,6 +222,7 @@ TEST(Pkt4Test, fixedFields) {
// Chaddr contains link-layer addr (MAC). It is no longer always 16 bytes
// long and its length depends on hlen value (it is up to 16 bytes now).
ASSERT_EQ(pkt->getHWAddr()->hwaddr_.size(), dummyHlen);
EXPECT_EQ(0, memcmp(dummyChaddr, &pkt->getHWAddr()->hwaddr_[0], dummyHlen));
EXPECT_EQ(0, memcmp(dummySname, &pkt->getSname()[0], 64));
......
src/lib/dhcpsrv/alloc_engine.cc
View file @ 21794c5d
......@@ -267,36 +267,32 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
const IOAddress& hint,
bool fake_allocation /* = false */ ) {
// That check is not necessary. We create allocator in AllocEngine
// constructor
// Allocator is always created in AllocEngine constructor and there is
// currently no other way to set it, so that check is not really necessary.
if (!allocator_) {
isc_throw(InvalidOperation, "No allocator selected");
}
// check if there's existing lease for that subnet/clientid/hwaddr combination.
// Check if there's existing lease for that subnet/clientid/hwaddr combination.
Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(hwaddr->hwaddr_, subnet->getID());
if (existing) {
// we have a lease already. This is a returning client, probably after
// his reboot.
// We have a lease already. This is a returning client, probably after
// its reboot.
existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
if (existing) {
return (existing);
}
// If renewal failed (e.g. the lease no longer matches current configuration)
// let's continue allocation process
// let's continue the allocation process
}
if (clientid) {
existing = LeaseMgrFactory::instance().getLease4(*clientid, subnet->getID());
if (existing) {
// we have a lease already. This is a returning client, probably after
// his reboot.
// its reboot.
existing = renewLease4(subnet, clientid, hwaddr, existing, fake_allocation);
// @todo: produce a warning. We haven't found him using MAC address, but
// we found him using client-id
if (existing) {
......@@ -309,10 +305,10 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
if (subnet->inPool(hint)) {
existing = LeaseMgrFactory::instance().getLease4(hint);
if (!existing) {
/// @todo: check if the hint is reserved once we have host support
/// @todo: Check if the hint is reserved once we have host support
/// implemented
// the hint is valid and not currently used, let's create a lease for it
// The hint is valid and not currently used, let's create a lease for it
Lease4Ptr lease = createLease4(subnet, clientid, hwaddr, hint, fake_allocation);
// It can happen that the lease allocation failed (we could have lost
......@@ -334,7 +330,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
// the following occurs:
// - we find a free address
// - we find an address for which the lease has expired
// - we exhaust number of tries
// - we exhaust the number of tries
//
// @todo: Current code does not handle pool exhaustion well. It will be
// improved. Current problems:
......@@ -373,7 +369,7 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
}
}
// continue trying allocation until we run out of attempts
// Continue trying allocation until we run out of attempts
// (or attempts are set to 0, which means infinite)
--i;
} while ( i || !attempts_);
......@@ -545,7 +541,6 @@ Lease4Ptr AllocEngine::createLease4(const SubnetPtr& subnet,
if (!fake_allocation) {
// That is a real (REQUEST) allocation
bool status = LeaseMgrFactory::instance().addLease(lease);
if (status) {
return (lease);
} else {
......@@ -569,7 +564,6 @@ Lease4Ptr AllocEngine::createLease4(const SubnetPtr& subnet,
}
}
AllocEngine::~AllocEngine() {
// no need to delete allocator. smart_ptr will do the trick for us
}
......
src/lib/dhcpsrv/alloc_engine.h
View file @ 21794c5d
......@@ -66,6 +66,12 @@ protected:
/// reserved - AllocEngine will check that and will call pickAddress
/// again if necessary. The number of times this method is called will
/// increase as the number of available leases will decrease.
///
/// @param subnet next address will be returned from pool of that subnet
/// @param duid Client's DUID
/// @param hint client's hint
///
/// @return the next address
virtual isc::asiolink::IOAddress
pickAddress(const SubnetPtr& subnet, const DuidPtr& duid,
const isc::asiolink::IOAddress& hint) = 0;
......@@ -197,12 +203,12 @@ protected:
/// @brief Renews a IPv4 lease
///
/// Since both request and renew are implemented in DHCPv4 as sending
/// REQUEST packet, it is difficult to easily distinguish between those
/// cases. Therefore renew for DHCPv4 is done in allocation engine.
/// Since both request and renew are implemented in DHCPv4 as the sending of
/// a REQUEST packet, it is difficult to easily distinguish between those
/// cases. Therefore renew for DHCPv4 is done in the allocation engine.
/// This method is also used when client crashed/rebooted and tries
/// to get a new lease. It thinks that it gets a new lease, but in fact
/// we are only renewing still valid lease for that client.
/// we are only renewing the still valid lease for that client.
///
/// @param subnet subnet the client is attached to
/// @param clientid client identifier
......@@ -241,10 +247,10 @@ protected:
virtual ~AllocEngine();
private:
/// @brief creates a lease and inserts it in LeaseMgr if necessary
/// @brief Creates a lease and inserts it in LeaseMgr if necessary
///
/// Creates a lease based on specified parameters and tries to insert it
/// into the database. That may fail in some cases, i.e. when there is another
/// into the database. That may fail in some cases, e.g. when there is another
/// allocation process and we lost a race to a specific lease.
///
/// @param subnet subnet the lease is allocated from
......@@ -278,7 +284,7 @@ private:
uint32_t iaid, const isc::asiolink::IOAddress& addr,
bool fake_allocation = false);
/// @brief reuses expired IPv4 lease
/// @brief Reuses expired IPv4 lease
///
/// Updates existing expired lease with new information. Lease database
/// is updated if this is real (i.e. REQUEST, fake_allocation = false), not
......@@ -297,7 +303,7 @@ private:
const HWAddrPtr& hwaddr,
bool fake_allocation = false);
/// @brief reuses expired IPv6 lease
/// @brief Reuses expired IPv6 lease
///
/// Updates existing expired lease with new information. Lease database
/// is updated if this is real (i.e. REQUEST, fake_allocation = false), not
......
src/lib/dhcpsrv/subnet.cc
View file @ 21794c5d
......@@ -93,7 +93,7 @@ PoolPtr Subnet::getPool(isc::asiolink::IOAddress hint) {
// getPool() as pure virtual and have Subnet4 and Subnet6 provide their
// own methods. Those two implementation would only differ by a default
// value, so it would just include duplicate code.
if (dynamic_cast<Subnet6*>(this) && hint.toText() == "::") {
if (dynamic_cast<Subnet4*>(this) && hint.toText() == "::") {
hint = IOAddress("0.0.0.0");
}
......
src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
View file @ 21794c5d
......@@ -31,7 +31,7 @@
#include <iostream>
#include <sstream>
#include <map>
#include <set>
#include <time.h>
using namespace std;
......@@ -42,17 +42,31 @@ using namespace isc::dhcp::test;
namespace {
/// @brief Allocation engine with some internal methods exposed
class NakedAllocEngine : public AllocEngine {
public:
/// @brief the sole constructor
/// @param engine_type specifies engine type (e.g. iterative)
/// @param attempts number of lease selection attempts before giving up
NakedAllocEngine(AllocEngine::AllocType engine_type, unsigned int attempts)
:AllocEngine(engine_type, attempts) {
}
// Expose internal classes for testing purposes
using AllocEngine::Allocator;
using AllocEngine::IterativeAllocator;
};
/// @brief Used in Allocation Engine tests for IPv6
class AllocEngine6Test : public ::testing::Test {
public:
/// @brief Default constructor
///
/// Sets duid_, iaid_, subnet_, pool_ fields to example values used
/// in many tests, initializes cfg_mgr configuration and creates
/// lease database.
AllocEngine6Test() {
duid_ = DuidPtr(new DUID(vector<uint8_t>(8, 0x42)));
iaid_ = 42;
......@@ -69,6 +83,9 @@ public:
factory_.create("type=memfile");
}
/// @brief checks if Lease6 matches expected configuration
///
/// @param lease lease to be checked
void checkLease6(const Lease6Ptr& lease) {
// that is belongs to the right subnet
EXPECT_EQ(lease->subnet_id_, subnet_->getID());
......@@ -92,15 +109,22 @@ public:
factory_.destroy();
}
DuidPtr duid_;
uint32_t iaid_;
Subnet6Ptr subnet_;
Pool6Ptr pool_;
LeaseMgrFactory factory_;
DuidPtr duid_; ///< client-identifier (value used in tests)
uint32_t iaid_; ///< IA identifier (value used in tests)
Subnet6Ptr subnet_; ///< subnet6 (used in tests)
Pool6Ptr pool_; ///< pool belonging to subnet_
LeaseMgrFactory factory_; ///< pointer to LeaseMgr factory
};
/// @brief Used in Allocation Engine tests for IPv4
class AllocEngine4Test : public ::testing::Test {
public:
/// @brief Default constructor
///
/// Sets clientid_, hwaddr_, subnet_, pool_ fields to example values
/// used in many tests, initializes cfg_mgr configuration and creates
/// lease database.
AllocEngine4Test() {
clientid_ = ClientIdPtr(new ClientId(vector<uint8_t>(8, 0x44)));
static uint8_t mac[] = { 0, 1, 22, 33, 44, 55};
......@@ -121,6 +145,9 @@ public:
factory_.create("type=memfile");
}
/// @brief checks if Lease4 matches expected configuration
///
/// @param lease lease to be checked
void checkLease4(const Lease4Ptr& lease) {
// that is belongs to the right subnet
EXPECT_EQ(lease->subnet_id_, subnet_->getID());
......@@ -142,11 +169,11 @@ public:
factory_.destroy();
}
ClientIdPtr clientid_;
HWAddrPtr hwaddr_;
Subnet4Ptr subnet_;
Pool4Ptr pool_;
LeaseMgrFactory factory_;
ClientIdPtr clientid_; ///< client-identifier (value used in tests)
HWAddrPtr hwaddr_; ///< hardware address (value used in tests)
Subnet4Ptr subnet_; ///< subnet4 (used in tests)
Pool4Ptr pool_; ///< pool belonging to subnet_
LeaseMgrFactory factory_; ///< pointer to LeaseMgr factory
};
// This test checks if the Allocation Engine can be instantiated and that it
......@@ -338,7 +365,7 @@ TEST_F(AllocEngine6Test, IterativeAllocator_manyPools6) {
// there are 8 extra pools with 9 addresses in each.
// Let's keep picked addresses here and check their uniqueness.
std::map<IOAddress, int> generated_addrs;
std::set<IOAddress> generated_addrs;
int cnt = 0;
while (++cnt) {
IOAddress candidate = alloc->pickAddress(subnet_, duid_, IOAddress("::"));
......@@ -351,7 +378,7 @@ TEST_F(AllocEngine6Test, IterativeAllocator_manyPools6) {
if (generated_addrs.find(candidate) == generated_addrs.end()) {
// we haven't had this
generated_addrs[candidate] = 0;
generated_addrs.insert(candidate);
} else {
// we have seen this address before. That should mean that we
// iterated over all addresses.
......@@ -538,10 +565,10 @@ TEST_F(AllocEngine4Test, simpleAlloc4) {
Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
IOAddress("0.0.0.0"), false);
// check that we got a lease
// Check that we got a lease
ASSERT_TRUE(lease);
// do all checks on the lease
// Do all checks on the lease
checkLease4(lease);
// Check that the lease is indeed in LeaseMgr
......@@ -561,10 +588,10 @@ TEST_F(AllocEngine4Test, fakeAlloc4) {
Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
IOAddress("0.0.0.0"), true);
// check that we got a lease
// Check that we got a lease
ASSERT_TRUE(lease);
// do all checks on the lease
// Do all checks on the lease
checkLease4(lease);
// Check that the lease is NOT in LeaseMgr
......@@ -584,13 +611,13 @@ TEST_F(AllocEngine4Test, allocWithValidHint4) {
IOAddress("192.0.2.105"),
false);
// check that we got a lease
// Check that we got a lease
ASSERT_TRUE(lease);
// we should get what we asked for
// We should get what we asked for
EXPECT_EQ(lease->addr_.toText(), "192.0.2.105");
// do all checks on the lease
// Do all checks on the lease
checkLease4(lease);
// Check that the lease is indeed in LeaseMgr
......@@ -609,7 +636,7 @@ TEST_F(AllocEngine4Test, allocWithUsedHint4) {
ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
ASSERT_TRUE(engine);
// let's create a lease and put it in the LeaseMgr
// Let's create a lease and put it in the LeaseMgr
uint8_t hwaddr2[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
uint8_t clientid2[] = { 8, 7, 6, 5, 4, 3, 2, 1 };
time_t now = time(NULL);
......@@ -617,22 +644,22 @@ TEST_F(AllocEngine4Test, allocWithUsedHint4) {
clientid2, sizeof(clientid2), 1, 2, 3, now, subnet_->getID()));
ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
// another client comes in and request an address that is in pool, but
// Another client comes in and request an address that is in pool, but
// unfortunately it is used already. The same address must not be allocated
// twice.
Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
IOAddress("192.0.2.106"),
false);
// check that we got a lease
// Check that we got a lease
ASSERT_TRUE(lease);
// allocated address must be different
// Allocated address must be different
EXPECT_TRUE(used->addr_.toText() != lease->addr_.toText());
// we should NOT get what we asked for, because it is used already
// We should NOT get what we asked for, because it is used already
EXPECT_TRUE(lease->addr_.toText() != "192.0.2.106");
// do all checks on the lease
// Do all checks on the lease
checkLease4(lease);
// Check that the lease is indeed in LeaseMgr
......@@ -657,13 +684,13 @@ TEST_F(AllocEngine4Test, allocBogusHint4) {
Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
IOAddress("10.1.1.1"),
false);
// check that we got a lease
// Check that we got a lease
ASSERT_TRUE(lease);
// we should NOT get what we asked for, because it is used already
// We should NOT get what we asked for, because it is used already
EXPECT_TRUE(lease->addr_.toText() != "10.1.1.1");
// do all checks on the lease
// Do all checks on the lease
checkLease4(lease);
// Check that the lease is indeed in LeaseMgr
......@@ -695,12 +722,12 @@ TEST_F(AllocEngine4Test, IterativeAllocator) {
TEST_F(AllocEngine4Test, IterativeAllocator_manyPools4) {
NakedAllocEngine::IterativeAllocator* alloc = new NakedAllocEngine::IterativeAllocator();
// let's start from 2, as there is 2001:db8:1::10 - 2001:db8:1::20 pool already.
// Let's start from 2, as there is 2001:db8:1::10 - 2001:db8:1::20 pool already.
for (int i = 2; i < 10; ++i) {
stringstream min, max;
min << "192.0.2." << i*10 + 1;
max << "192.0.2." << i*10 + 9;
min << "192.0.2." << i * 10 + 1;
max << "192.0.2." << i * 10 + 9;
Pool4Ptr pool(new Pool4(IOAddress(min.str()),
IOAddress(max.str())));
......@@ -708,11 +735,11 @@ TEST_F(AllocEngine4Test, IterativeAllocator_manyPools4) {
subnet_->addPool(pool);
}
int total = 10 + 8*9; // first pool (.100 - .109) has 10 addresses in it,
// there are 8 extra pools with 9 addresses in each.
int total = 10 + 8 * 9; // first pool (.100 - .109) has 10 addresses in it,
// there are 8 extra pools with 9 addresses in each.
// Let's keep picked addresses here and check their uniqueness.
std::map<IOAddress, int> generated_addrs;
std::set<IOAddress> generated_addrs;
int cnt = 0;
while (++cnt) {
IOAddress candidate = alloc->pickAddress(subnet_, clientid_, IOAddress("0.0.0.0"));
......@@ -724,8 +751,8 @@ TEST_F(AllocEngine4Test, IterativeAllocator_manyPools4) {
// cout << candidate.toText() << endl;
if (generated_addrs.find(candidate) == generated_addrs.end()) {
// we haven't had this
generated_addrs[candidate] = 0;
// We haven't had this
generated_addrs.insert(candidate);
} else {
// we have seen this address before. That should mean that we
// iterated over all addresses.
......@@ -771,7 +798,7 @@ TEST_F(AllocEngine4Test, smallPool4) {
EXPECT_EQ("192.0.2.17", lease->addr_.toText());
// do all checks on the lease
// Do all checks on the lease
checkLease4(lease);
// Check that the lease is indeed in LeaseMgr
......@@ -837,7 +864,7 @@ TEST_F(AllocEngine4Test, discoverReuseExpiredLease4) {
time_t now = time(NULL) - 500; // Allocated 500 seconds ago
Lease4Ptr lease(new Lease4(addr, clientid2, sizeof(clientid2), hwaddr2, sizeof(hwaddr2),
495, 100, 200, now, subnet_->getID()));
// lease was assigned 500 seconds ago, but its valid lifetime is 495, so it
// Lease was assigned 500 seconds ago, but its valid lifetime is 495, so it
// is expired already
ASSERT_TRUE(lease->expired());
ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
......@@ -916,7 +943,7 @@ TEST_F(AllocEngine4Test, renewLease4) {
old_timestamp, subnet_->getID()));
ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
// lease was assigned 45 seconds ago and is valid for 100 seconds. Let's
// Lease was assigned 45 seconds ago and is valid for 100 seconds. Let's
// renew it.
ASSERT_FALSE(lease->expired());
lease = engine->renewLease4(subnet_, clientid_, hwaddr_, lease, false);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment