[2324] Changes after review.

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 <asiolink/io_address.h>
 #include <alloc_engine.h>
 
 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<Allocator>(new IterativeAllocator());
         break;
     case ALLOC_HASHED:
-        allocator_ = new HashedAllocator();
+        allocator_ = boost::shared_ptr<Allocator>(new HashedAllocator());
         break;
     case ALLOC_RANDOM:
-        allocator_ = new RandomAllocator();
+        allocator_ = boost::shared_ptr<Allocator>(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

src/lib/dhcp/alloc_engine.h

@@ -15,12 +15,12 @@
 #ifndef ALLOC_ENGINE_H
 #define ALLOC_ENGINE_H
 
+#include <boost/shared_ptr.hpp>
 #include <boost/noncopyable.hpp>
 #include <dhcp/duid.h>
 #include <dhcp/subnet.h>
 #include <asiolink/io_address.h>
 #include <dhcp/lease_mgr.h>
-#include <iostream>
 
 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> 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

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");
     }
 

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.
     ///

src/lib/dhcp/subnet.cc

@@ -15,7 +15,6 @@
 #include <dhcp/addr_utilities.h>
 #include <asiolink/io_address.h>
 #include <dhcp/subnet.h>
-#include <dhcp/pool.h>
 
 using namespace isc::asiolink;
 

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<SubnetID, IOAddress> 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<SubnetID, IOAddress> 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<uint32_t>& t2,
            const Triplet<uint32_t>& valid_lifetime);
 
+    /// @brief virtual destructor
+    virtual ~Subnet() {
+    };
+
     /// @brief returns the next unique Subnet-ID
     ///
     /// @return the next unique Subnet-ID

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")));

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& ) {
 
 }
 

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