[#1196] Addressed comments

11 files changed, 62 insertions, 60 deletions

diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc
index db30fa6778..cc874375e6 100644
--- a/src/lib/dhcpsrv/pgsql_host_data_source.cc
+++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc
@@ -2035,8 +2035,8 @@ PgSqlHostDataSourceImpl::PgSqlHostDataSourceImpl(const PgSqlConnection::Paramete
     : parameters_(parameters) {
 
     // Validate the schema version first.
-    std::pair<uint32_t, uint32_t> code_version(PGSQL_SCHEMA_VERSION_MAJOR,
-                                               PGSQL_SCHEMA_VERSION_MINOR);
+    std::pair<uint32_t, uint32_t> code_version(PG_SCHEMA_VERSION_MAJOR,
+                                               PG_SCHEMA_VERSION_MINOR);
     std::pair<uint32_t, uint32_t> db_version = getVersion();
     if (code_version != db_version) {
         isc_throw(DbOpenError,
diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
index b8a3e18516..fd6ab7765d 100644
--- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
@@ -1086,8 +1086,7 @@ public:
     /// calls will return false.
     ///
     /// Checks against negative values for the state count and logs once
-    /// a warning message. Unfortunately not getting the message is not
-    /// a proof that detailed counters are correct.
+    /// a warning message.
     ///
     /// @param row Storage for the fetched row
     ///
@@ -1212,8 +1211,8 @@ PgSqlLeaseMgr::PgSqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters)
     : parameters_(parameters) {
 
     // Validate schema version first.
-    std::pair<uint32_t, uint32_t> code_version(PGSQL_SCHEMA_VERSION_MAJOR,
-                                               PGSQL_SCHEMA_VERSION_MINOR);
+    std::pair<uint32_t, uint32_t> code_version(PG_SCHEMA_VERSION_MAJOR,
+                                               PG_SCHEMA_VERSION_MINOR);
     std::pair<uint32_t, uint32_t> db_version = getVersion();
     if (code_version != db_version) {
         isc_throw(DbOpenError,
@@ -1263,8 +1262,8 @@ PgSqlLeaseMgr::createContext() const {
 std::string
 PgSqlLeaseMgr::getDBVersion() {
     std::stringstream tmp;
-    tmp << "PostgreSQL backend " << PGSQL_SCHEMA_VERSION_MAJOR;
-    tmp << "." << PGSQL_SCHEMA_VERSION_MINOR;
+    tmp << "PostgreSQL backend " << PG_SCHEMA_VERSION_MAJOR;
+    tmp << "." << PG_SCHEMA_VERSION_MINOR;
     tmp << ", library " << PQlibVersion();
     return (tmp.str());
 }
diff --git a/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc
index cc22163612..d79dd8bb69 100644
--- a/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cql_lease_mgr_unittest.cc
@@ -858,14 +858,14 @@ TEST_F(CqlLeaseMgrTest, leaseStatsQuery6) {
     testLeaseStatsQuery6();
 }
 
-/// @brief Tests v4 lease stats to never go negative.
-TEST_F(CqlLeaseMgrTest, leaseStatsQueryNegative4) {
-    testLeaseStatsQueryNegative4();
+/// @brief Tests v4 lease stats to be attributed to the wrong subnet.
+TEST_F(CqlLeaseMgrTest, leaseStatsQueryAttribution4) {
+    testLeaseStatsQueryAttribution4();
 }
 
-/// @brief Tests v6 lease stats to never go negative.
-TEST_F(CqlLeaseMgrTest, leaseStatsQueryNegative6) {
-    testLeaseStatsQueryNegative6();
+/// @brief Tests v6 lease stats to be attributed to the wrong subnet.
+TEST_F(CqlLeaseMgrTest, leaseStatsQueryAttribution6) {
+    testLeaseStatsQueryAttribution6();
 }
 
 }  // namespace
diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
index c717a3e073..e0778ebfbe 100644
--- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
@@ -3630,7 +3630,7 @@ GenericLeaseMgrTest::testLeaseStatsQuery6() {
 }
 
 void
-GenericLeaseMgrTest::testLeaseStatsQueryNegative4() {
+GenericLeaseMgrTest::testLeaseStatsQueryAttribution4() {
     // Create two subnets for the same range.
     CfgSubnets4Ptr cfg = CfgMgr::instance().getStagingCfg()->getCfgSubnets4();
     Subnet4Ptr subnet;
@@ -3638,7 +3638,8 @@ GenericLeaseMgrTest::testLeaseStatsQueryNegative4() {
     subnet.reset(new Subnet4(IOAddress("192.0.1.0"), 24, 1, 2, 3, 1));
     cfg->add(subnet);
 
-    subnet.reset(new Subnet4(IOAddress("192.0.1.1"), 24, 1, 2, 3, 2));
+    // Note it is even allowed to use 192.0.1.1/24 here...
+    subnet.reset(new Subnet4(IOAddress("192.0.1.0"), 25, 1, 2, 3, 2));
     cfg->add(subnet);
 
     ASSERT_NO_THROW(CfgMgr::instance().commit());
@@ -3671,7 +3672,7 @@ GenericLeaseMgrTest::testLeaseStatsQueryNegative4() {
 }
 
 void
-GenericLeaseMgrTest::testLeaseStatsQueryNegative6() {
+GenericLeaseMgrTest::testLeaseStatsQueryAttribution6() {
     // Create two subnets.
     CfgSubnets6Ptr cfg = CfgMgr::instance().getStagingCfg()->getCfgSubnets6();
     Subnet6Ptr subnet;
diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
index 0de0e80dbc..1bf00d41fa 100644
--- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
+++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
@@ -467,23 +467,25 @@ public:
     ///
     void testLeaseStatsQuery6();
 
-    /// @brief Checks if v4 LeaseStatsQuery can get negative counters
+    /// @brief Checks if v4 LeaseStatsQuery can get bad attribution.
     ///
     /// It creates two subnets with leases and move one from the first
     /// to the second. If counters are not updated this can lead to
-    /// negative counters.
+    /// bad attribution i.e. a lease is counted in a subnet when it
+    /// belongs to another one.
     ///
-    void testLeaseStatsQueryNegative4();
+    void testLeaseStatsQueryAttribution4();
 
-    /// @brief Checks if v6 LeaseStatsQuery can get negative counters
+    /// @brief Checks if v6 LeaseStatsQuery can get bad attribution.
     ///
     /// It creates two subnets with leases and move one from the first
     /// to the second. If counters are not updated this can lead to
-    /// negative counters.
+    /// bad attribution i.e. a lease is counted in a subnet when it
+    /// belongs to another one.
     ///
     /// @note We can check the lease type change too but in the real
     /// world this never happens.
-    void testLeaseStatsQueryNegative6();
+    void testLeaseStatsQueryAttribution6();
 
     /// @brief Compares LeaseQueryStats content to expected set of rows
     ///
diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
index 377bb4f0e4..d628279967 100644
--- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
@@ -2258,16 +2258,16 @@ TEST_F(MemfileLeaseMgrTest, leaseStatsQuery6) {
     testLeaseStatsQuery6();
 }
 
-/// @brief Tests v4 lease stats to never go negative.
-TEST_F(MemfileLeaseMgrTest, leaseStatsQueryNegative4) {
+/// @brief Tests v4 lease stats to be attributed to the wrong subnet.
+TEST_F(MemfileLeaseMgrTest, leaseStatsQueryAttribution4) {
     startBackend(V4);
-    testLeaseStatsQueryNegative4();
+    testLeaseStatsQueryAttribution4();
 }
 
-/// @brief Tests v6 lease stats to never go negative.
-TEST_F(MemfileLeaseMgrTest, leaseStatsQueryNegative6) {
+/// @brief Tests v6 lease stats to be attributed to the wrong subnet.
+TEST_F(MemfileLeaseMgrTest, leaseStatsQueryAttribution6) {
     startBackend(V6);
-    testLeaseStatsQueryNegative6();
+    testLeaseStatsQueryAttribution6();
 }
 
 }  // namespace
diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
index e703159c2d..9a167e6952 100644
--- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
@@ -1015,26 +1015,26 @@ TEST_F(MySqlLeaseMgrTest, leaseStatsQuery6MultiThreading) {
     testLeaseStatsQuery6();
 }
 
-/// @brief Tests v4 lease stats to never go negative.
-TEST_F(MySqlLeaseMgrTest, leaseStatsQueryNegative4) {
-    testLeaseStatsQueryNegative4();
+/// @brief Tests v4 lease stats to be attributed to the wrong subnet.
+TEST_F(MySqlLeaseMgrTest, leaseStatsQueryAttribution4) {
+    testLeaseStatsQueryAttribution4();
 }
 
-/// @brief Tests v4 lease stats to never go negative.
-TEST_F(MySqlLeaseMgrTest, leaseStatsQueryNegative4MultiThreading) {
+/// @brief Tests v4 lease stats to be attributed to the wrong subnet.
+TEST_F(MySqlLeaseMgrTest, leaseStatsQueryAttribution4MultiThreading) {
     MultiThreadingMgr::instance().setMode(true);
-    testLeaseStatsQueryNegative4();
+    testLeaseStatsQueryAttribution4();
 }
 
-/// @brief Tests v6 lease stats to never go negative.
-TEST_F(MySqlLeaseMgrTest, leaseStatsQueryNegative6) {
-    testLeaseStatsQueryNegative6();
+/// @brief Tests v6 lease stats to be attributed to the wrong subnet.
+TEST_F(MySqlLeaseMgrTest, leaseStatsQueryAttribution6) {
+    testLeaseStatsQueryAttribution6();
 }
 
-/// @brief Tests v6 lease stats to never go negative.
-TEST_F(MySqlLeaseMgrTest, leaseStatsQueryNegative6MultiThreading) {
+/// @brief Tests v6 lease stats to be attributed to the wrong subnet.
+TEST_F(MySqlLeaseMgrTest, leaseStatsQueryAttribution6MultiThreading) {
     MultiThreadingMgr::instance().setMode(true);
-    testLeaseStatsQueryNegative6();
+    testLeaseStatsQueryAttribution6();
 }
 
 }  // namespace
diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
index 381b194c98..70776ec644 100644
--- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
@@ -246,8 +246,8 @@ TEST_F(PgSqlLeaseMgrTest, checkVersion) {
     // Check version
     pair<uint32_t, uint32_t> version;
     ASSERT_NO_THROW(version = lmptr_->getVersion());
-    EXPECT_EQ(PGSQL_SCHEMA_VERSION_MAJOR, version.first);
-    EXPECT_EQ(PGSQL_SCHEMA_VERSION_MINOR, version.second);
+    EXPECT_EQ(PG_SCHEMA_VERSION_MAJOR, version.first);
+    EXPECT_EQ(PG_SCHEMA_VERSION_MINOR, version.second);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -971,26 +971,26 @@ TEST_F(PgSqlLeaseMgrTest, leaseStatsQuery6MultiThreading) {
     testLeaseStatsQuery6();
 }
 
-/// @brief Tests v4 lease stats to never go negative.
-TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryNegative4) {
-    testLeaseStatsQueryNegative4();
+/// @brief Tests v4 lease stats to be attributed to the wrong subnet.
+TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryAttribution4) {
+    testLeaseStatsQueryAttribution4();
 }
 
-/// @brief Tests v4 lease stats to never go negative.
-TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryNegative4MultiThreading) {
+/// @brief Tests v4 lease stats to be attributed to the wrong subnet.
+TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryAttribution4MultiThreading) {
     MultiThreadingMgr::instance().setMode(true);
-    testLeaseStatsQueryNegative4();
+    testLeaseStatsQueryAttribution4();
 }
 
-/// @brief Tests v6 lease stats to never go negative.
-TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryNegative6) {
-    testLeaseStatsQueryNegative6();
+/// @brief Tests v6 lease stats to be attributed to the wrong subnet.
+TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryAttribution6) {
+    testLeaseStatsQueryAttribution6();
 }
 
-/// @brief Tests v6 lease stats to never go negative.
-TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryNegative6MultiThreading) {
+/// @brief Tests v6 lease stats to be attributed to the wrong subnet.
+TEST_F(PgSqlLeaseMgrTest, leaseStatsQueryAttribution6MultiThreading) {
     MultiThreadingMgr::instance().setMode(true);
-    testLeaseStatsQueryNegative6();
+    testLeaseStatsQueryAttribution6();
 }
 
 }  // namespace
diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h
index 12e500f160..b02346eb81 100644
--- a/src/lib/pgsql/pgsql_connection.h
+++ b/src/lib/pgsql/pgsql_connection.h
@@ -18,8 +18,8 @@ namespace isc {
 namespace db {
 
 /// @brief Define PostgreSQL backend version: 6.1
-const uint32_t PGSQL_SCHEMA_VERSION_MAJOR = 6;
-const uint32_t PGSQL_SCHEMA_VERSION_MINOR = 1;
+const uint32_t PG_SCHEMA_VERSION_MAJOR = 6;
+const uint32_t PG_SCHEMA_VERSION_MINOR = 1;
 
 // Maximum number of parameters that can be used a statement
 // @todo This allows us to use an initializer list (since we can't
diff --git a/src/lib/pgsql/testutils/pgsql_schema.cc b/src/lib/pgsql/testutils/pgsql_schema.cc
index 21b313c1bc..f12bcbfef7 100644
--- a/src/lib/pgsql/testutils/pgsql_schema.cc
+++ b/src/lib/pgsql/testutils/pgsql_schema.cc
@@ -56,7 +56,7 @@ bool wipePgSQLData(bool show_err) {
     cmd << " sh " << DATABASE_SCRIPTS_DIR << "/pgsql/wipe_data.sh";
 
     // Add expected schema version as the wipe script's first argument.
-    cmd  << " " << PGSQL_SCHEMA_VERSION_MAJOR  << "." << PGSQL_SCHEMA_VERSION_MINOR;
+    cmd  << " " << PG_SCHEMA_VERSION_MAJOR  << "." << PG_SCHEMA_VERSION_MINOR;
 
     // Now add command line arguments for psql.
     cmd  << " --set ON_ERROR_STOP=1 -A -t -h localhost -q -U keatest -d keatest";
diff --git a/src/lib/pgsql/testutils/pgsql_schema.h b/src/lib/pgsql/testutils/pgsql_schema.h
index 6d90762162..7a664ab432 100644
--- a/src/lib/pgsql/testutils/pgsql_schema.h
+++ b/src/lib/pgsql/testutils/pgsql_schema.h
@@ -76,8 +76,8 @@ void createPgSQLSchema(bool show_err = false, bool force = false);
 ///  <TEST_ADMIN_SCRIPTS_DIR>/pgsql/wipe_data.sh
 ///
 /// This will fail if there is no schema, if the existing schema
-/// version is incorrect (i.e. does not match PGSQL_SCHEMA_VERSION_MAJOR
-/// and PGSQL_SCHEMA_VERSION_MINOR), or a SQL error occurs.  Otherwise,
+/// version is incorrect (i.e. does not match PG_SCHEMA_VERSION_MAJOR
+/// and PG_SCHEMA_VERSION_MINOR), or a SQL error occurs.  Otherwise,
 /// the script is should delete all transient data, leaving intact
 /// reference tables.
 ///