[2342] Miscellaneous changes as a result of review

src/bin/dhcp6/dhcp6_srv.cc

@@ -39,10 +39,6 @@
 #include <dhcp/cfgmgr.h>
 #include <dhcp/option6_iaaddr.h>
 
-// @todo: Replace this with MySQL_LeaseMgr (or a LeaseMgr factory)
-// once it is merged
-#include <dhcp/memfile_lease_mgr.h>
-
 #include <boost/foreach.hpp>
 
 using namespace isc;
@@ -54,8 +50,8 @@ using namespace std;
 namespace isc {
 namespace dhcp {
 
-Dhcpv6Srv::Dhcpv6Srv(uint16_t port) : alloc_engine_(), serverid_(),
-                                      shutdown_(false) {
+Dhcpv6Srv::Dhcpv6Srv(uint16_t port, const char* dbconfig)
+    : alloc_engine_(), serverid_(), shutdown_(true) {
 
     LOG_DEBUG(dhcp6_logger, DBG_DHCP6_START, DHCP6_OPEN_SOCKET).arg(port);
 
@@ -74,7 +70,6 @@ Dhcpv6Srv::Dhcpv6Srv(uint16_t port) : alloc_engine_(), serverid_(),
         if (port > 0) {
             if (IfaceMgr::instance().countIfaces() == 0) {
                 LOG_ERROR(dhcp6_logger, DHCP6_NO_INTERFACES);
-                shutdown_ = true;
                 return;
             }
             IfaceMgr::instance().openSockets6(port);
@@ -82,25 +77,21 @@ Dhcpv6Srv::Dhcpv6Srv(uint16_t port) : alloc_engine_(), serverid_(),
 
         setServerID();
 
+        // Instantiate LeaseMgr
+        LeaseMgrFactory::create(dbconfig);
+        LOG_INFO(dhcp6_logger, DHCP6_DB_BACKEND_STARTED)
+            .arg(LeaseMgrFactory::instance().getName());
+
+        // Instantiate allocation engine
+        alloc_engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100));
+
     } catch (const std::exception &e) {
         LOG_ERROR(dhcp6_logger, DHCP6_SRV_CONSTRUCT_ERROR).arg(e.what());
-        shutdown_ = true;
         return;
     }
 
-    // Instantiate LeaseMgr
-    // @todo: Replace this with MySQL_LeaseMgr (or a LeaseMgr factory)
-    // once it is merged
-#ifdef HAVE_MYSQL
-    LeaseMgrFactory::create("type=mysql user=kea password=kea name=kea host=localhost");
-#else
-    LeaseMgrFactory::create("type=memfile");
-#endif
-    LOG_INFO(dhcp6_logger, DHCP6_DB_BACKEND_STARTED)
-        .arg(LeaseMgrFactory::instance().getName());
-
-    // Instantiate allocation engine
-    alloc_engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100));
+    // All done, so can proceed
+    shutdown_ = false;
 }
 
 Dhcpv6Srv::~Dhcpv6Srv() {

src/bin/dhcp6/dhcp6_srv.h

@@ -57,7 +57,10 @@ public:
     /// old or create new DUID.
     ///
     /// @param port port on will all sockets will listen
-    Dhcpv6Srv(uint16_t port = DHCP6_SERVER_PORT);
+    /// @param dbconfig Lease manager configuration string.  The default
+    ///        of the "memfile" manager is used for testing.
+    Dhcpv6Srv(uint16_t port = DHCP6_SERVER_PORT,
+            const char* dbconfig = "type=memfile");
 
     /// @brief Destructor. Used during DHCPv6 service shutdown.
     virtual ~Dhcpv6Srv();

src/lib/dhcp/Makefile.am

@@ -59,8 +59,7 @@ libb10_dhcpsrv_la_SOURCES += triplet.h
 
 libb10_dhcpsrv_la_CXXFLAGS = $(AM_CXXFLAGS)
 libb10_dhcpsrv_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES)
-libb10_dhcpsrv_la_LIBADD   = $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la
-libb10_dhcpsrv_la_LIBADD  += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la
+libb10_dhcpsrv_la_LIBADD   = $(top_builddir)/src/lib/asiolink/libb10-asiolink.la
 libb10_dhcpsrv_la_LIBADD  += $(top_builddir)/src/lib/util/libb10-util.la
 libb10_dhcpsrv_la_LDFLAGS  = -no-undefined -version-info 2:0:0
 if HAVE_MYSQL

src/lib/dhcp/lease_mgr.cc

@@ -96,10 +96,3 @@ Lease6::operator==(const Lease6& other) const {
         subnet_id_ == other.subnet_id_
         );
 }
-
-LeaseMgr::LeaseMgr(const LeaseMgr::ParameterMap& parameters)
-    : parameters_(parameters) {
-}
-
-LeaseMgr::~LeaseMgr() {
-}

src/lib/dhcp/lease_mgr.h

@@ -179,7 +179,7 @@ struct Lease4 {
     /// @brief Constructor
     ///
     /// Initialize fields that don't have a default constructor.
-    /// @TODO Remove this
+    /// @todo Remove this
     Lease4() : addr_(0) {}
 };
 
@@ -345,10 +345,12 @@ public:
     ///
     /// @param parameters A data structure relating keywords and values
     ///        concerned with the database.
-    LeaseMgr(const ParameterMap& parameters);
+    LeaseMgr(const ParameterMap& parameters) : parameters_(parameters)
+    {}
 
     /// @brief Destructor
-    ~LeaseMgr();
+    virtual ~LeaseMgr()
+    {}
 
     /// @brief Adds an IPv4 lease.
     ///
@@ -389,7 +391,6 @@ public:
     /// a single lease, not a container of leases.
     ///
     /// @param addr address of the searched lease
-    /// @param subnet_id ID of the subnet the lease must belong to
     ///
     /// @return smart pointer to the lease (or NULL if a lease is not found)
     virtual Lease4Ptr getLease4(const isc::asiolink::IOAddress& addr) const = 0;

src/lib/dhcp/lease_mgr_factory.cc

@@ -14,25 +14,23 @@
 
 #include "config.h"
 
+#include <dhcp/lease_mgr_factory.h>
+#include <dhcp/memfile_lease_mgr.h>
+#ifdef HAVE_MYSQL
+#include <dhcp/mysql_lease_mgr.h>
+#endif
+
+#include <boost/algorithm/string.hpp>
+#include <boost/foreach.hpp>
+#include <boost/scoped_ptr.hpp>
+
 #include <algorithm>
 #include <iostream>
 #include <iterator>
 #include <map>
 #include <sstream>
-#include <string>
 #include <utility>
 
-#include <boost/foreach.hpp>
-#include <boost/scoped_ptr.hpp>
-#include <boost/algorithm/string.hpp>
-#include <exceptions/exceptions.h>
-#include <dhcp/lease_mgr_factory.h>
-
-#include <dhcp/memfile_lease_mgr.h>
-#ifdef HAVE_MYSQL
-#include <dhcp/mysql_lease_mgr.h>
-#endif
-
 using namespace std;
 
 namespace isc {
@@ -48,7 +46,7 @@ LeaseMgr::ParameterMap
 LeaseMgrFactory::parse(const std::string& dbconfig) {
     LeaseMgr::ParameterMap mapped_tokens;
 
-    if (! dbconfig.empty()) {
+    if (!dbconfig.empty()) {
         vector<string> tokens;
 
         // We need to pass a string to is_any_of, not just char*. Otherwise

src/lib/dhcp/lease_mgr_factory.h

@@ -51,7 +51,7 @@ public:
 /// Strictly speaking these functions could be stand-alone functions.  However,
 /// it is convenient to encapsulate them in a class for naming purposes.
 ///
-/// @TODO: Will need to develop some form of registration mechanism for
+/// @todo: Will need to develop some form of registration mechanism for
 ///        user-supplied backends (so that there is no need to modify the code).
 class LeaseMgrFactory {
 public:
@@ -62,8 +62,8 @@ public:
     /// appropriate type.  The actual lease manager is returned by the
     /// "instance" method.
     ///
-    /// Note: when called, the current lease manager is *always* destroyed
-    /// and a new one created - even if the parameters are the same.
+    /// @note When called, the current lease manager is <b>always</b> destroyed
+    ///       and a new one created - even if the parameters are the same.
     ///
     /// dbconfig is a generic way of passing parameters. Parameters are passed
     /// in the "name=value" format, separated by spaces.  The data MUST include
@@ -104,7 +104,7 @@ public:
     ///
     /// @param dbconfig Database configuration string
     ///
-    /// @return std::map<>std::string, std::string> Map of keyword/value pairs.
+    /// @return std::map<std::string, std::string> Map of keyword/value pairs.
     static LeaseMgr::ParameterMap parse(const std::string& dbconfig);
 
 private:

src/lib/dhcp/memfile_lease_mgr.h

@@ -169,7 +169,7 @@ public:
     ///
     /// @todo Not implemented yet
     ///
-    /// @param lease4 The lease to be updated.
+    /// @param lease6 The lease to be updated.
     ///
     /// If no such lease is present, an exception will be thrown.
     void updateLease6(const Lease6Ptr& lease6);
@@ -237,5 +237,5 @@ protected:
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 
-#endif // MEMFILE_LEASE_MGR_HSE4
+#endif // MEMFILE_LEASE_MGR
 

src/lib/dhcp/mysql_lease_mgr.h

@@ -111,7 +111,6 @@ public:
     /// a single lease, not a container of leases.
     ///
     /// @param addr address of the searched lease
-    /// @param subnet_id ID of the subnet the lease must belong to
     ///
     /// @return smart pointer to the lease (or NULL if a lease is not found)
     virtual Lease4Ptr getLease4(const isc::asiolink::IOAddress& addr) const;
@@ -327,7 +326,7 @@ public:
     ///
     /// @param expire Reference to MYSQL_TIME object from where the expiry
     ///        time of the lease is taken.
-    /// @param lease_time lifetime of the lease.
+    /// @param valid_lifetime lifetime of the lease.
     /// @param cltt Reference to location where client last transmit time
     ///        is put.
     static

src/lib/dhcp/tests/alloc_engine_unittest.cc

@@ -95,15 +95,13 @@ public:
 // This test checks if the Allocation Engine can be instantiated and that it
 // parses parameters string properly.
 TEST_F(AllocEngineTest, constructor) {
-    AllocEngine* x = NULL;
+    boost::scoped_ptr<AllocEngine> x;
 
     // Hashed and random allocators are not supported yet
-    ASSERT_THROW(x = new AllocEngine(AllocEngine::ALLOC_HASHED, 5), NotImplemented);
-    ASSERT_THROW(x = new AllocEngine(AllocEngine::ALLOC_RANDOM, 5), NotImplemented);
+    ASSERT_THROW(x.reset(new AllocEngine(AllocEngine::ALLOC_HASHED, 5)), NotImplemented);
+    ASSERT_THROW(x.reset(new AllocEngine(AllocEngine::ALLOC_RANDOM, 5)), NotImplemented);
 
-    ASSERT_NO_THROW(x = new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100));
-
-    delete x;
+    ASSERT_NO_THROW(x.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
 }
 
 /// @todo: This method is taken from mysql_lease_mgr_utilities.cc from ticket
@@ -269,15 +267,13 @@ TEST_F(AllocEngineTest, allocBogusHint) {
 // This test verifies that the allocator picks addresses that belong to the
 // pool
 TEST_F(AllocEngineTest, IterativeAllocator) {
-    NakedAllocEngine::Allocator* alloc = new NakedAllocEngine::IterativeAllocator();
+    boost::scoped_ptr<NakedAllocEngine::Allocator>
+        alloc(new NakedAllocEngine::IterativeAllocator());
 
     for (int i = 0; i < 1000; ++i) {
         IOAddress candidate = alloc->pickAddress(subnet_, duid_, IOAddress("::"));
-
         EXPECT_TRUE(subnet_->inPool(candidate));
     }
-
-    delete alloc;
 }
 
 

src/lib/dhcp/tests/lease_mgr_factory_unittest.cc

@@ -23,6 +23,9 @@
 using namespace std;
 using namespace isc::dhcp;
 
+// This set of tests only check the parsing functions of LeaseMgrFactory.
+// Tests of the LeaseMgr create/instance/destroy are implicitly carried out
+// in the tests for the different concrete lease managers (e.g. MySqlLeaseMgr).
 
 namespace {
 // empty class for now, but may be extended once Addr6 becomes bigger