[2472] Miscellaneous fixes after review

src/lib/dhcp/memfile_lease_mgr.h

@@ -133,7 +133,7 @@ public:
     /// @param addr address of the searched lease
     ///
     /// @return smart pointer to the lease (or NULL if a lease is not found)
-    Lease6Ptr getLease6(const isc::asiolink::IOAddress& addr) const;
+    virtual Lease6Ptr getLease6(const isc::asiolink::IOAddress& addr) const;
 
     /// @brief Returns existing IPv6 lease for a given DUID+IA combination
     ///
@@ -143,7 +143,7 @@ public:
     /// @param iaid IA identifier
     ///
     /// @return collection of IPv6 leases
-    Lease6Collection getLease6(const DUID& duid, uint32_t iaid) const;
+    virtual Lease6Collection getLease6(const DUID& duid, uint32_t iaid) const;
 
     /// @brief Returns existing IPv6 lease for a given DUID+IA combination
     ///
@@ -154,7 +154,7 @@ public:
     /// @param subnet_id identifier of the subnet the lease must belong to
     ///
     /// @return smart pointer to the lease (or NULL if a lease is not found)
-    Lease6Ptr getLease6(const DUID& duid, uint32_t iaid, SubnetID subnet_id) const;
+    virtual Lease6Ptr getLease6(const DUID& duid, uint32_t iaid, SubnetID subnet_id) const;
 
     /// @brief Updates IPv4 lease.
     ///
@@ -163,7 +163,7 @@ public:
     /// @param lease4 The lease to be updated.
     ///
     /// If no such lease is present, an exception will be thrown.
-    void updateLease4(const Lease4Ptr& lease4);
+    virtual void updateLease4(const Lease4Ptr& lease4);
 
     /// @brief Updates IPv4 lease.
     ///
@@ -172,7 +172,7 @@ public:
     /// @param lease6 The lease to be updated.
     ///
     /// If no such lease is present, an exception will be thrown.
-    void updateLease6(const Lease6Ptr& lease6);
+    virtual void updateLease6(const Lease6Ptr& lease6);
 
     /// @brief Deletes a lease.
     ///
@@ -186,7 +186,7 @@ public:
     /// @param addr IPv4 address of the lease to be deleted.
     ///
     /// @return true if deletion was successful, false if no such lease exists
-    bool deleteLease6(const isc::asiolink::IOAddress& addr);
+    virtual bool deleteLease6(const isc::asiolink::IOAddress& addr);
 
     /// @brief Return backend type
     ///
@@ -215,6 +215,9 @@ public:
     virtual std::string getDescription() const;
 
     /// @brief Returns backend version.
+    ///
+    /// @return Version number as a pair of unsigned integers.  "first" is the
+    ///         major version number, "second" the minor number.
     virtual std::pair<uint32_t, uint32_t> getVersion() const {
         return (std::make_pair(1, 0));
     }
@@ -231,8 +234,6 @@ public:
     /// support transactions, this is a no-op.
     virtual void rollback();
 
-    using LeaseMgr::getParameter;
-
 protected:
 
     typedef boost::multi_index_container< // this is a multi-index container...

src/lib/dhcp/mysql_lease_mgr.h

@@ -267,16 +267,6 @@ public:
     /// @return Version number as a pair of unsigned integers.  "first" is the
     ///         major version number, "second" the minor number.
     ///
-    /// @todo: We will need to implement 3 version functions eventually:
-    /// A. abstract API version
-    /// B. backend version
-    /// C. database version (stored in the database scheme)
-    ///
-    /// and then check that:
-    /// B>=A and B=C (it is ok to have newer backend, as it should be backward
-    /// compatible)
-    /// Also if B>C, some database upgrade procedure may be triggered
-    ///
     /// @throw isc::dhcp::DbOperationError An operation on the open database has
     ///        failed.
     virtual std::pair<uint32_t, uint32_t> getVersion() const;

src/lib/dhcp/tests/memfile_lease_mgr_unittest.cc

@@ -45,7 +45,8 @@ TEST_F(MemfileLeaseMgrTest, constructor) {
     ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap)));
 }
 
-TEST_F(MemfileLeaseMgrTest, GetTypeAndName) {
+// Checks if the getType() and getName() methods both return "memfile".
+TEST_F(MemfileLeaseMgrTest, getTypeAndName) {
     const LeaseMgr::ParameterMap pmap;  // Empty parameter map
     boost::scoped_ptr<Memfile_LeaseMgr> lease_mgr(new Memfile_LeaseMgr(pmap));
 
@@ -53,14 +54,8 @@ TEST_F(MemfileLeaseMgrTest, GetTypeAndName) {
     EXPECT_EQ(std::string("memfile"), lease_mgr->getName());
 }
 
-// There's no point in calling any other methods in LeaseMgr, as they
-// are purely virtual, so we would only call Memfile_LeaseMgr methods.
-// Those methods are just stubs that does not return anything.
-// It seems likely that we will need to extend the memfile code for
-// allocation engine tests, so we may implement tests that call
-// Memfile_LeaseMgr methods then.
-
-TEST_F(MemfileLeaseMgrTest, addGetDelete) {
+// Checks that adding/getting/deleting a Lease6 object works.
+TEST_F(MemfileLeaseMgrTest, addGetDeletei6) {
     const LeaseMgr::ParameterMap pmap;  // Empty parameter map
     boost::scoped_ptr<Memfile_LeaseMgr> lease_mgr(new Memfile_LeaseMgr(pmap));
 

src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc

@@ -470,11 +470,25 @@ TEST(MySqlOpenTest, OpenDatabase) {
     destroySchema();
 }
 
-TEST_F(MySqlLeaseMgrTest, GetType) {
+// @brief Check the getType() method
+//
+// getType() returns a string giving the type of the backend, which should
+// always be "mysql".
+TEST_F(MySqlLeaseMgrTest, getType) {
     EXPECT_EQ(std::string("mysql"), lmptr_->getType());
 }
 
 // @brief Check conversion functions
+//
+// The server works using cltt and valid_filetime.  In the database, the
+// information is stored as expire_time and valid-lifetime, which are
+// related by
+//
+// expire_time = cltt + valid_lifetime
+//
+// This test checks that the conversion is correct.  It does not check that the
+// data is entered into the database correctly, only that the MYSQL_TIME
+// structure used for the entry is correctly set up.
 TEST_F(MySqlLeaseMgrTest, CheckTimeConversion) {
     const time_t cltt = time(NULL);
     const uint32_t valid_lft = 86400;       // 1 day
@@ -513,7 +527,7 @@ TEST_F(MySqlLeaseMgrTest, getName) {
     // @TODO: check for the negative
 }
 
-// @brief Check that getVersion() works
+// @brief Check that getVersion() returns the expected version
 TEST_F(MySqlLeaseMgrTest, CheckVersion) {
     // Check version
     pair<uint32_t, uint32_t> version;
@@ -522,13 +536,15 @@ TEST_F(MySqlLeaseMgrTest, CheckVersion) {
     EXPECT_EQ(CURRENT_VERSION_MINOR, version.second);
 }
 
+// @brief Compare two Lease6 structures for equality
 void
 detailCompareLease6(const Lease6Ptr& first, const Lease6Ptr& second) {
     EXPECT_EQ(first->type_, second->type_);
 
-    // Compare address strings - odd things happen when they are different
-    // as the EXPECT_EQ appears to call the operator uint32_t() function,
-    // which causes an exception to be thrown for IPv6 addresses.
+    // Compare address strings.  Comparison of address objects is not used, as
+    // odd things happen when they are different: the EXPECT_EQ macro appears to
+    // call the operator uint32_t() function, which causes an exception to be
+    // thrown for IPv6 addresses.
     EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
     EXPECT_EQ(first->prefixlen_, second->prefixlen_);
     EXPECT_EQ(first->iaid_, second->iaid_);