From 9394d67b638a84409c3491fee821f347ca2acbcf Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Fri, 26 Oct 2012 15:37:45 +0200 Subject: [PATCH] [2324] Changes after review. --- src/lib/dhcp/alloc_engine.cc | 28 +++++++-------- src/lib/dhcp/alloc_engine.h | 45 +++++++++++------------- src/lib/dhcp/lease_mgr.cc | 2 +- src/lib/dhcp/lease_mgr.h | 14 ++++---- src/lib/dhcp/subnet.cc | 1 - src/lib/dhcp/subnet.h | 33 ++++++++++++++--- src/lib/dhcp/tests/lease_mgr_unittest.cc | 18 +++++----- src/lib/dhcp/tests/memfile_lease_mgr.cc | 8 ++--- src/lib/dhcp/tests/memfile_lease_mgr.h | 30 +++++++++++++--- 9 files changed, 109 insertions(+), 70 deletions(-) diff --git a/src/lib/dhcp/alloc_engine.cc b/src/lib/dhcp/alloc_engine.cc index 1ab677f54e..5e8d90d1ea 100644 --- a/src/lib/dhcp/alloc_engine.cc +++ b/src/lib/dhcp/alloc_engine.cc @@ -12,7 +12,6 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -#include #include using namespace isc::asiolink; @@ -41,7 +40,7 @@ AllocEngine::IterativeAllocator::increaseAddress(const isc::asiolink::IOAddress& } for (int i = len - 1; i >= 0; --i) { - packed[i]++; + ++packed[i]; if (packed[i] != 0) { break; } @@ -67,8 +66,6 @@ AllocEngine::IterativeAllocator::pickAddress(const Subnet6Ptr& subnet, isc_throw(AllocFailed, "No pools defined in selected subnet"); } - Pool6Ptr pool = Pool6Ptr(); // null - // first we need to find a pool the last address belongs to. Pool6Collection::const_iterator it; for (it = pools.begin(); it != pools.end(); ++it) { @@ -144,13 +141,13 @@ AllocEngine::AllocEngine(AllocType engine_type, unsigned int attempts) :attempts_(attempts) { switch (engine_type) { case ALLOC_ITERATIVE: - allocator_ = new IterativeAllocator(); + allocator_ = boost::shared_ptr(new IterativeAllocator()); break; case ALLOC_HASHED: - allocator_ = new HashedAllocator(); + allocator_ = boost::shared_ptr(new HashedAllocator()); break; case ALLOC_RANDOM: - allocator_ = new RandomAllocator(); + allocator_ = boost::shared_ptr(new RandomAllocator()); break; default: @@ -163,7 +160,8 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet, const DuidPtr& duid, uint32_t iaid, const IOAddress& hint, - bool fake /* = false */ ) { + bool fake_allocation /* = false */ ) { + // That check is not necessary. We create allocator in AllocEngine // constructor if (!allocator_) { @@ -186,7 +184,7 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet, /// implemented // the hint is valid and not currently used, let's create a lease for it - Lease6Ptr lease = createLease(subnet, duid, iaid, hint, fake); + Lease6Ptr lease = createLease(subnet, duid, iaid, hint, fake_allocation); // It can happen that the lease allocation failed (we could have lost // the race condition. That means that the hint is lo longer usable and @@ -208,7 +206,8 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet, // there's no existing lease for selected candidate, so it is // free. Let's allocate it. if (!existing) { - Lease6Ptr lease = createLease(subnet, duid, iaid, candidate, fake); + Lease6Ptr lease = createLease(subnet, duid, iaid, candidate, + fake_allocation); if (lease) { return (lease); } @@ -231,13 +230,13 @@ Lease6Ptr AllocEngine::createLease(const Subnet6Ptr& subnet, const DuidPtr& duid, uint32_t iaid, const IOAddress& addr, - bool fake /*= false */ ) { + bool fake_allocation /*= false */ ) { Lease6Ptr lease(new Lease6(Lease6::LEASE_IA_NA, addr, duid, iaid, subnet->getPreferred(), subnet->getValid(), subnet->getT1(), subnet->getT2(), subnet->getID())); - if (!fake) { + if (!fake_allocation) { // That is a real (REQUEST) allocation bool status = LeaseMgr::instance().addLease(lease); @@ -266,10 +265,7 @@ Lease6Ptr AllocEngine::createLease(const Subnet6Ptr& subnet, } AllocEngine::~AllocEngine() { - if (allocator_) { - delete allocator_; - allocator_ = NULL; - } + // no need to delete allocator. smart_ptr will do the trick for us } }; // end of isc::dhcp namespace diff --git a/src/lib/dhcp/alloc_engine.h b/src/lib/dhcp/alloc_engine.h index 5df70cbea6..74e30dd8b0 100644 --- a/src/lib/dhcp/alloc_engine.h +++ b/src/lib/dhcp/alloc_engine.h @@ -15,12 +15,12 @@ #ifndef ALLOC_ENGINE_H #define ALLOC_ENGINE_H +#include #include #include #include #include #include -#include namespace isc { namespace dhcp { @@ -30,13 +30,13 @@ namespace dhcp { class AllocFailed : public isc::Exception { public: -/// @brief constructor -/// -/// @param file name of the file, where exception occurred -/// @param line line of the file, where exception occurred -/// @param what text description of the issue that caused exception -AllocFailed(const char* file, size_t line, const char* what) - : isc::Exception(file, line, what) {} + /// @brief constructor + /// + /// @param file name of the file, where exception occurred + /// @param line line of the file, where exception occurred + /// @param what text description of the issue that caused exception + AllocFailed(const char* file, size_t line, const char* what) + : isc::Exception(file, line, what) {} }; /// @brief DHCPv4 and DHCPv6 allocation engine @@ -65,22 +65,17 @@ protected: /// again if necessary. The number of times this method is called will /// increase as the number of available leases will decrease. virtual isc::asiolink::IOAddress - pickAddress(const Subnet6Ptr& subnet, - const DuidPtr& duid, - const isc::asiolink::IOAddress& hint) = 0; + pickAddress(const Subnet6Ptr& subnet, const DuidPtr& duid, + const isc::asiolink::IOAddress& hint) = 0; protected: - /// @brief protected constructor - /// - /// Prevents anyone from attempting to instantiate Allocator objects - /// directly. Derived classes should be used instead. - Allocator() { - } }; /// @brief Address/prefix allocator that iterates over all addresses /// /// This class implements iterative algorithm that returns all addresses in - /// a pool iteratively, one after another. + /// a pool iteratively, one after another. Once the last address is reached, + /// it starts allocating from the beginning of the first pool (i.e. it loops + /// over). class IterativeAllocator : public Allocator { public: @@ -184,14 +179,15 @@ protected: /// @param duid Client'd DUID /// @param iaid iaid field from the IA_NA container that client sent /// @param hint a hint that the client provided - /// @param fake is this real (REQUEST) or fake (SOLICIT) allocation + /// @param fake_allocation is this real i.e. REQUEST (false) or just picking + /// an address for SOLICIT that is not really allocated (true) /// @return Allocated IPv6 lease (or NULL if allocation failed) Lease6Ptr allocateAddress6(const Subnet6Ptr& subnet, const DuidPtr& duid, uint32_t iaid, const isc::asiolink::IOAddress& hint, - bool fake); + bool fake_allocation); /// @brief Destructor. Used during DHCPv6 service shutdown. virtual ~AllocEngine(); @@ -207,15 +203,16 @@ private: /// @param duid client's DUID /// @param iaid IAID from the IA_NA container the client sent to us /// @param addr an address that was selected and is confirmed to be available - /// @param fake is this SOLICIT (true) or a real/REQUEST allocation (false)? + /// @param fake_allocation is this real i.e. REQUEST (false) or just picking + /// an address for SOLICIT that is not really allocated (true) /// @return allocated lease (or NULL in the unlikely case of the lease just /// becomed unavailable) Lease6Ptr createLease(const Subnet6Ptr& subnet, const DuidPtr& duid, uint32_t iaid, const isc::asiolink::IOAddress& addr, - bool fake = false); + bool fake_allocation = false); /// @brief a pointer to currently used allocator - Allocator* allocator_; + boost::shared_ptr allocator_; /// @brief number of attempts before we give up lease allocation (0=unlimited) unsigned int attempts_; @@ -224,4 +221,4 @@ private: }; // namespace isc::dhcp }; // namespace isc -#endif // DHCP6_SRV_H +#endif // ALLOC_ENGINE_H diff --git a/src/lib/dhcp/lease_mgr.cc b/src/lib/dhcp/lease_mgr.cc index 2e77ca6f28..59582fb115 100644 --- a/src/lib/dhcp/lease_mgr.cc +++ b/src/lib/dhcp/lease_mgr.cc @@ -39,7 +39,7 @@ Lease6::Lease6(LeaseType type, const isc::asiolink::IOAddress& addr, DuidPtr dui preferred_lft_(preferred), valid_lft_(valid), t1_(t1), t2_(t2), subnet_id_(subnet_id), fixed_(false), fqdn_fwd_(false), fqdn_rev_(false) { - if (duid == DuidPtr()) { + if (!duid) { isc_throw(InvalidOperation, "DUID must be specified for a lease"); } diff --git a/src/lib/dhcp/lease_mgr.h b/src/lib/dhcp/lease_mgr.h index ec2853c8bc..7541a15f7b 100644 --- a/src/lib/dhcp/lease_mgr.h +++ b/src/lib/dhcp/lease_mgr.h @@ -289,9 +289,9 @@ public: /// @brief returns a single instance of LeaseMgr /// /// LeaseMgr is a singleton and this method is the only way of - /// accessing it. LeaseMgr must be create first using - /// instantiate() method, otherwise instance() will throw - /// InvalidOperation exception. + /// accessing it. LeaseMgr must be created first. See + /// isc::dhcp::LeaseMgrFactory class (work of ticket #2342. + /// Otherwise instance() will throw InvalidOperation exception. /// @throw InvalidOperation if LeaseMgr not instantiated static LeaseMgr& instance(); @@ -305,12 +305,12 @@ public: /// @brief Adds an IPv4 lease. /// /// @param lease lease to be added - virtual bool addLease(Lease4Ptr lease) = 0; + virtual bool addLease(const Lease4Ptr& lease) = 0; /// @brief Adds an IPv6 lease. /// /// @param lease lease to be added - virtual bool addLease(Lease6Ptr lease) = 0; + virtual bool addLease(const Lease6Ptr& lease) = 0; /// @brief Returns existing IPv4 lease for specified IPv4 address and subnet_id /// @@ -429,14 +429,14 @@ public: /// @param lease4 The lease to be updated. /// /// If no such lease is present, an exception will be thrown. - virtual void updateLease4(Lease4Ptr lease4) = 0; + virtual void updateLease4(const Lease4Ptr& lease4) = 0; /// @brief Updates IPv4 lease. /// /// @param lease4 The lease to be updated. /// /// If no such lease is present, an exception will be thrown. - virtual void updateLease6(Lease6Ptr lease6) = 0; + virtual void updateLease6(const Lease6Ptr& lease6) = 0; /// @brief Deletes a lease. /// diff --git a/src/lib/dhcp/subnet.cc b/src/lib/dhcp/subnet.cc index 5fb1d43dad..230a1b9ebd 100644 --- a/src/lib/dhcp/subnet.cc +++ b/src/lib/dhcp/subnet.cc @@ -15,7 +15,6 @@ #include #include #include -#include using namespace isc::asiolink; diff --git a/src/lib/dhcp/subnet.h b/src/lib/dhcp/subnet.h index 9238240c53..714ed9e502 100644 --- a/src/lib/dhcp/subnet.h +++ b/src/lib/dhcp/subnet.h @@ -49,10 +49,11 @@ public: /// subnet (e.g. 2001::/64) there may be one or more pools defined /// that may or may not cover entire subnet, e.g. pool 2001::1-2001::10). /// inPool() returning true implies inSubnet(), but the reverse implication - /// is not always true. For the given example, 2001::abc would return + /// is not always true. For the given example, 2001::1234:abcd would return /// true for inSubnet(), but false for inPool() check. /// - /// @param addr this address will be checked if it belongs to any pools in that subnet + /// @param addr this address will be checked if it belongs to any pools in + /// that subnet /// @return true if the address is in any of the pools virtual bool inPool(const isc::asiolink::IOAddress& addr) const = 0; @@ -71,15 +72,35 @@ public: return (t2_); } - isc::asiolink::IOAddress getLastAllocated() { + /// @brief returns the last address that was tried from this pool + /// + /// This method returns the last address that was attempted to be allocated + /// from this subnet. This is used as helper information for the next + /// iteration of the allocation algorithm. + /// + /// @todo: Define map somewhere in the + /// AllocEngine::IterativeAllocator and keep the data there + /// + /// @return address that was last tried from this pool + isc::asiolink::IOAddress getLastAllocated() const { return (last_allocated_); } + /// @brief sets the last address that was tried from this pool + /// + /// This method sets the last address that was attempted to be allocated + /// from this subnet. This is used as helper information for the next + /// iteration of the allocation algorithm. + /// + /// @todo: Define map somewhere in the + /// AllocEngine::IterativeAllocator and keep the data there void setLastAllocated(const isc::asiolink::IOAddress& addr) { last_allocated_ = addr; } - SubnetID getID() { return id_; } + /// @brief returns unique ID for that subnet + /// @return unique ID for that subnet + SubnetID getID() const { return (id_); } protected: /// @brief protected constructor @@ -91,6 +112,10 @@ protected: const Triplet& t2, const Triplet& valid_lifetime); + /// @brief virtual destructor + virtual ~Subnet() { + }; + /// @brief returns the next unique Subnet-ID /// /// @return the next unique Subnet-ID diff --git a/src/lib/dhcp/tests/lease_mgr_unittest.cc b/src/lib/dhcp/tests/lease_mgr_unittest.cc index 8446160051..743b34a8c0 100644 --- a/src/lib/dhcp/tests/lease_mgr_unittest.cc +++ b/src/lib/dhcp/tests/lease_mgr_unittest.cc @@ -86,18 +86,18 @@ TEST_F(LeaseMgrTest, addGetDelete) { x = leaseMgr->getLease6(IOAddress("2001:db8:1::456")); ASSERT_TRUE(x); - EXPECT_TRUE(x->addr_ == addr); - EXPECT_TRUE(*x->duid_ == *duid); - EXPECT_TRUE(x->iaid_ == iaid); - EXPECT_TRUE(x->subnet_id_ == subnet_id); + EXPECT_EQ(x->addr_.toText(), addr.toText()); + EXPECT_EQ(*x->duid_, *duid); + EXPECT_EQ(x->iaid_, iaid); + EXPECT_EQ(x->subnet_id_, subnet_id); // These are not important from lease management perspective, but // let's check them anyway. - EXPECT_TRUE(x->type_ == Lease6::LEASE_IA_NA); - EXPECT_TRUE(x->preferred_lft_ == 100); - EXPECT_TRUE(x->valid_lft_ == 200); - EXPECT_TRUE(x->t1_ == 50); - EXPECT_TRUE(x->t2_ == 80); + EXPECT_EQ(x->type_, Lease6::LEASE_IA_NA); + EXPECT_EQ(x->preferred_lft_, 100); + EXPECT_EQ(x->valid_lft_, 200); + EXPECT_EQ(x->t1_, 50); + EXPECT_EQ(x->t2_, 80); // should return false - there's no such address EXPECT_FALSE(leaseMgr->deleteLease6(IOAddress("2001:db8:1::789"))); diff --git a/src/lib/dhcp/tests/memfile_lease_mgr.cc b/src/lib/dhcp/tests/memfile_lease_mgr.cc index d0a8b9994e..ac3316d6ca 100644 --- a/src/lib/dhcp/tests/memfile_lease_mgr.cc +++ b/src/lib/dhcp/tests/memfile_lease_mgr.cc @@ -24,11 +24,11 @@ Memfile_LeaseMgr::Memfile_LeaseMgr(const std::string& dbconfig) Memfile_LeaseMgr::~Memfile_LeaseMgr() { } -bool Memfile_LeaseMgr::addLease(Lease4Ptr) { +bool Memfile_LeaseMgr::addLease(const Lease4Ptr&) { return (false); } -bool Memfile_LeaseMgr::addLease(Lease6Ptr lease) { +bool Memfile_LeaseMgr::addLease(const Lease6Ptr& lease) { if (getLease6(lease->addr_)) { // there is a lease with specified address already return (false); @@ -84,10 +84,10 @@ Lease6Ptr Memfile_LeaseMgr::getLease6(const DUID&, uint32_t, return (Lease6Ptr()); } -void Memfile_LeaseMgr::updateLease4(Lease4Ptr ) { +void Memfile_LeaseMgr::updateLease4(const Lease4Ptr& ) { } -void Memfile_LeaseMgr::updateLease6(Lease6Ptr ) { +void Memfile_LeaseMgr::updateLease6(const Lease6Ptr& ) { } diff --git a/src/lib/dhcp/tests/memfile_lease_mgr.h b/src/lib/dhcp/tests/memfile_lease_mgr.h index d39e3eb1d8..666db6ace7 100644 --- a/src/lib/dhcp/tests/memfile_lease_mgr.h +++ b/src/lib/dhcp/tests/memfile_lease_mgr.h @@ -50,22 +50,26 @@ public: /// @brief Adds an IPv4 lease. /// + /// @todo Not implemented yet /// @param lease lease to be added - virtual bool addLease(Lease4Ptr lease); + virtual bool addLease(const Lease4Ptr& lease); /// @brief Adds an IPv6 lease. /// /// @param lease lease to be added - virtual bool addLease(Lease6Ptr lease); + virtual bool addLease(const Lease6Ptr& lease); /// @brief Returns existing IPv4 lease for specified IPv4 address. /// + /// @todo Not implemented yet /// @param addr address of the searched lease /// /// @return a collection of leases virtual Lease4Ptr getLease4(isc::asiolink::IOAddress addr) const; /// @brief Returns existing IPv4 lease for specific address and subnet + /// + /// @todo Not implemented yet /// @param addr address of the searched lease /// @param subnet_id ID of the subnet the lease must belong to /// @@ -75,6 +79,8 @@ public: /// @brief Returns existing IPv4 leases for specified hardware address. /// + /// @todo Not implemented yet + /// /// Although in the usual case there will be only one lease, for mobile /// clients or clients with multiple static/fixed/reserved leases there /// can be more than one. Thus return type is a container, not a single @@ -88,6 +94,8 @@ public: /// @brief Returns existing IPv4 leases for specified hardware address /// and a subnet /// + /// @todo Not implemented yet + /// /// There can be at most one lease for a given HW address in a single /// pool, so this method with either return a single lease or NULL. /// @@ -100,6 +108,8 @@ public: /// @brief Returns existing IPv4 lease for specified client-id /// + /// @todo Not implemented yet + /// /// @param clientid client identifier virtual Lease4Collection getLease4(const ClientId& clientid) const; @@ -108,6 +118,8 @@ public: /// There can be at most one lease for a given HW address in a single /// pool, so this method with either return a single lease or NULL. /// + /// @todo Not implemented yet + /// /// @param clientid client identifier /// @param subnet_id identifier of the subnet that lease must belong to /// @@ -124,6 +136,8 @@ public: /// @brief Returns existing IPv6 lease for a given DUID+IA combination /// + /// @todo Not implemented yet + /// /// @param duid client DUID /// @param iaid IA identifier /// @@ -132,6 +146,8 @@ public: /// @brief Returns existing IPv6 lease for a given DUID+IA combination /// + /// @todo Not implemented yet + /// /// @param duid client DUID /// @param iaid IA identifier /// @param subnet_id identifier of the subnet the lease must belong to @@ -141,20 +157,26 @@ public: /// @brief Updates IPv4 lease. /// + /// @todo Not implemented yet + /// /// @param lease4 The lease to be updated. /// /// If no such lease is present, an exception will be thrown. - void updateLease4(Lease4Ptr lease4); + void updateLease4(const Lease4Ptr& lease4); /// @brief Updates IPv4 lease. /// + /// @todo Not implemented yet + /// /// @param lease4 The lease to be updated. /// /// If no such lease is present, an exception will be thrown. - void updateLease6(Lease6Ptr lease6); + void updateLease6(const Lease6Ptr& lease6); /// @brief Deletes a lease. /// + /// @todo Not implemented yet + /// /// @param addr IPv4 address of the lease to be deleted. /// /// @return true if deletion was successful, false if no such lease exists -- GitLab