[#2346] updated comments

4 files changed, 71 insertions, 62 deletions

diff --git a/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp4.cc b/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp4.cc
index e09e7c1579..236b4b3946 100644
--- a/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp4.cc
+++ b/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp4.cc
@@ -805,10 +805,10 @@ public:
         std::vector<uint64_t> pool_ids;
 
         if (server_selector.amAny()) {
-                PsqlBindArray in_bindings;
-                in_bindings.addInet4(pool_start_address);
-                in_bindings.addInet4(pool_end_address);
-                getPools(GET_POOL4_RANGE_ANY, in_bindings, pools, pool_ids);
+            PsqlBindArray in_bindings;
+            in_bindings.addInet4(pool_start_address);
+            in_bindings.addInet4(pool_end_address);
+            getPools(GET_POOL4_RANGE_ANY, in_bindings, pools, pool_ids);
         } else {
             auto const& tags = server_selector.getTags();
             for (auto const& tag : tags) {
@@ -1464,8 +1464,8 @@ public:
                       " server is not supported");
 
         } else if (server_selector.amUnassigned()) {
-            isc_throw(NotImplemented, "creating or updating a shared network without"
-                      " assigning it to a server or all servers is not supported");
+            isc_throw(NotImplemented, "managing configuration for no particular server"
+                      " (unassigned) is unsupported at the moment");
         }
 
         PsqlBindArray in_bindings;
@@ -2765,7 +2765,10 @@ TaggedStatementArray tagged_statements = { {
     // Verify that dependency on KNOWN/UNKNOWN class has not changed.
     {
         // PgSqlConfigBackendDHCPv4Impl::CHECK_CLIENT_CLASS_KNOWN_DEPENDENCY_CHANGE,
-        0, { OID_NONE },
+        0,
+        {
+            OID_NONE
+        },
         "CHECK_CLIENT_CLASS_KNOWN_DEPENDENCY_CHANGE",
         "select checkDHCPv4ClientClassKnownDependencyChange()"
     },
@@ -2824,7 +2827,7 @@ TaggedStatementArray tagged_statements = { {
             OID_INT8    // 1 subnet_id
         },
         "GET_SUBNET4_ID_ANY",
-         PGSQL_GET_SUBNET4_ANY(WHERE s.subnet_id = $1)
+        PGSQL_GET_SUBNET4_ANY(WHERE s.subnet_id = $1)
     },
 
     // Select unassigned subnet by id.
@@ -2851,7 +2854,7 @@ TaggedStatementArray tagged_statements = { {
 
     // Select subnet by prefix without specifying server tags.
     {
-        //PgSqlConfigBackendDHCPv4Impl::GET_SUBNET4_PREFIX_ANY,
+        // PgSqlConfigBackendDHCPv4Impl::GET_SUBNET4_PREFIX_ANY,
         1,
         {
             OID_VARCHAR // 1 subnet_prefix
@@ -2941,7 +2944,7 @@ TaggedStatementArray tagged_statements = { {
                                        AND (p.end_address = cast($3 as inet)))
     },
 
-    // Select pool by address range for any server
+    // Select pool by address range for any server.
     {
         // PgSqlConfigBackendDHCPv4Impl::GET_POOL4_RANGE_ANY,
         2,
@@ -2956,7 +2959,7 @@ TaggedStatementArray tagged_statements = { {
 
     // Select shared network by name.
     {
-        //PgSqlConfigBackendDHCPv4Impl::GET_SHARED_NETWORK4_NAME_NO_TAG,
+        // PgSqlConfigBackendDHCPv4Impl::GET_SHARED_NETWORK4_NAME_NO_TAG,
         1,
         {
             OID_VARCHAR // name of network
@@ -2978,7 +2981,7 @@ TaggedStatementArray tagged_statements = { {
 
     // Select unassigned shared network by name.
     {
-        //PgSqlConfigBackendDHCPv4Impl::GET_SHARED_NETWORK4_NAME_UNASSIGNED,
+        // PgSqlConfigBackendDHCPv4Impl::GET_SHARED_NETWORK4_NAME_UNASSIGNED,
         1,
         {
             OID_VARCHAR // name of network
@@ -4043,7 +4046,6 @@ TaggedStatementArray tagged_statements = { {
     // Delete global parameter by name.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_GLOBAL_PARAMETER4,
-        // args: server_tag, name
         2,
         {
             OID_VARCHAR,    // 1 server_tag
@@ -4078,7 +4080,6 @@ TaggedStatementArray tagged_statements = { {
     // Delete subnet by id with specifying server tag.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_SUBNET4_ID_WITH_TAG,
-        // args: server_tag, subnet_id
         2,
         {
             OID_VARCHAR,    // 1 server_tag
@@ -4136,7 +4137,10 @@ TaggedStatementArray tagged_statements = { {
     // Delete all unassigned subnets.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_ALL_SUBNETS4_UNASSIGNED,
-        0, { OID_NONE },
+        0,
+        {
+            OID_NONE
+        },
         "DELETE_ALL_SUBNETS4_UNASSIGNED",
         PGSQL_DELETE_SUBNET_UNASSIGNED(dhcp4)
     },
@@ -4166,7 +4170,6 @@ TaggedStatementArray tagged_statements = { {
     // Delete pools for a subnet.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_POOLS4,
-        // args: subnet_id, subnet_prefix
         2,
         {
             OID_INT8,   // 1 subnet_id
@@ -4213,7 +4216,10 @@ TaggedStatementArray tagged_statements = { {
     // Delete all unassigned shared networks.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_ALL_SHARED_NETWORKS4_UNASSIGNED,
-        0, { OID_NONE },
+        0,
+        {
+            OID_NONE
+        },
         "DELETE_ALL_SHARED_NETWORKS4_UNASSIGNED",
         PGSQL_DELETE_SHARED_NETWORK_UNASSIGNED(dhcp4)
     },
@@ -4245,7 +4251,6 @@ TaggedStatementArray tagged_statements = { {
     // Delete all option definitions.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_ALL_OPTION_DEFS4,
-        // args: server_tag
         1,
         {
             OID_VARCHAR // 1 server_tag
@@ -4286,7 +4291,7 @@ TaggedStatementArray tagged_statements = { {
             OID_VARCHAR     // 3 space
         },
         "DELETE_OPTION4",
-        PGSQL_DELETE_OPTION_WITH_TAG(dhcp4, AND o.scope_id = 0  AND o.code = $2 AND o.space = $3)
+        PGSQL_DELETE_OPTION_WITH_TAG(dhcp4, AND o.scope_id = 0 AND o.code = $2 AND o.space = $3)
     },
 
     // Delete all global options which are unassigned to any servers.
@@ -4457,7 +4462,10 @@ TaggedStatementArray tagged_statements = { {
     // Deletes all servers except logical server 'all'.
     {
         // PgSqlConfigBackendDHCPv4Impl::DELETE_ALL_SERVERS4,
-        0, { OID_NONE },
+        0,
+        {
+            OID_NONE
+        },
         "DELETE_ALL_SERVERS4",
         PGSQL_DELETE_ALL_SERVERS(dhcp4)
     },
@@ -4466,7 +4474,11 @@ TaggedStatementArray tagged_statements = { {
     {
         // PgSqlConfigBackendDHCPv4Impl::GET_LAST_INSERT_ID4,
         // args are: table name, sequence column name
-        2, { OID_VARCHAR, OID_VARCHAR },
+        2,
+        {
+            OID_VARCHAR,
+            OID_VARCHAR
+        },
         "GET_LAST_INSERT_ID4",
         "SELECT CURRVAL(PG_GET_SERIAL_SEQUENCE($1, $2))"
     }
@@ -4496,6 +4508,9 @@ PgSqlConfigBackendDHCPv4Impl::PgSqlConfigBackendDHCPv4Impl(const DatabaseConnect
     conn_.makeReconnectCtl(timer_name_);
 }
 
+PgSqlConfigBackendDHCPv4Impl::~PgSqlConfigBackendDHCPv4Impl() {
+}
+
 PgSqlTaggedStatement&
 PgSqlConfigBackendDHCPv4Impl::getStatement(size_t index) const {
     if (index >= tagged_statements.size()) {
@@ -4506,9 +4521,6 @@ PgSqlConfigBackendDHCPv4Impl::getStatement(size_t index) const {
     return(tagged_statements[index]);
 }
 
-PgSqlConfigBackendDHCPv4Impl::~PgSqlConfigBackendDHCPv4Impl() {
-}
-
 PgSqlConfigBackendDHCPv4::PgSqlConfigBackendDHCPv4(const DatabaseConnection::ParameterMap& parameters)
     : impl_(new PgSqlConfigBackendDHCPv4Impl(parameters)), base_impl_(impl_) {
 }
diff --git a/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp6.cc b/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp6.cc
index 3860ac4995..11c923b18d 100644
--- a/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp6.cc
+++ b/src/hooks/dhcp/pgsql_cb/pgsql_cb_dhcp6.cc
@@ -201,7 +201,7 @@ public:
     /// doesn't exist.
     StampedValuePtr getGlobalParameter6(const ServerSelector& server_selector,
                                         const std::string& name) {
-       StampedValueCollection parameters;
+        StampedValueCollection parameters;
 
         auto const& tags = server_selector.getTags();
         for (auto const& tag : tags) {
@@ -218,8 +218,7 @@ public:
     /// @brief Sends query to insert or update global parameter.
     ///
     /// @param server_selector Server selector.
-    /// @param name Name of the global parameter.
-    /// @param value Value of the global parameter.
+    /// @param value StampedValue describing the parameter to create/update.
     void createUpdateGlobalParameter6(const db::ServerSelector& server_selector,
                                       const StampedValuePtr& value) {
         if (server_selector.amUnassigned()) {
@@ -249,7 +248,6 @@ public:
         // Try to update the existing row.
         if (updateDeleteQuery(PgSqlConfigBackendDHCPv6Impl::UPDATE_GLOBAL_PARAMETER6,
                               in_bindings) == 0) {
-
             // No such parameter found, so let's insert it. We have to adjust the
             // bindings collection to match the prepared statement for insert.
             in_bindings.popBack();
@@ -269,7 +267,6 @@ public:
         }
 
         transaction.commit();
-
     }
 
     /// @brief Sends query to the database to retrieve multiple subnets.
@@ -476,7 +473,6 @@ public:
                                  const std::string& log_message,
                                  const bool cascade_delete,
                                  Args&&... keys) {
-
         PgSqlTransaction transaction(conn_);
 
         // Create scoped audit revision. As long as this instance exists
@@ -491,7 +487,6 @@ public:
         transaction.commit();
 
         return (count);
-
     }
 
     /// @brief Sends query to delete subnet by id.
@@ -602,7 +597,7 @@ public:
     /// @brief Sends query to insert DHCP option.
     ///
     /// This method expects that the server selector contains exactly one
-    /// server tag.
+    /// server tag.  It is intended to be used within a transaction.
     ///
     /// @param server_selector Server selector.
     /// @param in_bindings Collection of bindings representing an option.
@@ -632,7 +627,6 @@ public:
     /// @param option Pointer to the option descriptor encapsulating the option.
     void createUpdateOption6(const ServerSelector& server_selector,
                              const OptionDescriptorPtr& option) {
-
         if (server_selector.amUnassigned()) {
             isc_throw(NotImplemented, "managing configuration for no particular server"
                       " (unassigned) is unsupported at the moment");
@@ -656,10 +650,10 @@ public:
         in_bindings.addTimestamp(option->getModificationTime());
         in_bindings.addNull();
 
-        // Remember the size before we added where clause arguments.
+        // Remember the size before we add where clause arguments.
         size_t pre_where_size = in_bindings.size();
 
-        // Now the add the update where clause parameters
+        // Now we add the update where clause parameters
         in_bindings.add(tag);
         in_bindings.add(option->option_->getType());
         in_bindings.addOptional(option->space_name_);
@@ -783,7 +777,6 @@ public:
     /// @param option_def Pointer to the option definition to be inserted or updated.
     void createUpdateOptionDef6(const ServerSelector& server_selector,
                                 const OptionDefinitionPtr& option_def) {
-
         createUpdateOptionDef(server_selector, option_def, DHCP6_OPTION_SPACE,
                               PgSqlConfigBackendDHCPv6Impl::GET_OPTION_DEF6_CODE_SPACE,
                               PgSqlConfigBackendDHCPv6Impl::INSERT_OPTION_DEF6,
@@ -814,7 +807,6 @@ public:
     uint64_t deleteOptionDef6(const ServerSelector& server_selector,
                               const uint16_t code,
                               const std::string& space) {
-
         PsqlBindArray in_bindings;
         in_bindings.add(code);
         in_bindings.add(space);
@@ -1221,8 +1213,10 @@ namespace {
 typedef std::array<PgSqlTaggedStatement, PgSqlConfigBackendDHCPv6Impl::NUM_STATEMENTS>
 TaggedStatementArray;
 
-/// @brief Prepared PostgreSQL statements used by the backend to insert and
-/// retrieve data from the database.
+/// @brief Prepared PgSQL statements used by the backend to insert and
+/// retrieve data from the database. They must be in the same order as
+/// PgSqlConfigBackendDHCPv6Impl::StatementIndex.  The statement is
+/// the corresponding enum name.
 TaggedStatementArray tagged_statements = { {
     {
         // PgSqlConfigBackendDHCPv6Impl::CREATE_AUDIT_REVISION,
@@ -2859,8 +2853,7 @@ TaggedStatementArray tagged_statements = { {
             OID_VARCHAR     // 3 space
         },
         "DELETE_OPTION6",
-        PGSQL_DELETE_OPTION_WITH_TAG(dhcp6, AND o.scope_id = 0 \
-                                     AND o.code = $2 AND o.space = $3)
+        PGSQL_DELETE_OPTION_WITH_TAG(dhcp6, AND o.scope_id = 0 AND o.code = $2 AND o.space = $3)
     },
 
     // Delete all global options which are unassigned to any servers.
@@ -2884,8 +2877,8 @@ TaggedStatementArray tagged_statements = { {
             OID_VARCHAR // 3 space
         },
         "DELETE_OPTION6_SUBNET_ID",
-        PGSQL_DELETE_OPTION_NO_TAG(dhcp6, WHERE o.scope_id = 1 AND o.dhcp6_subnet_id = $1 \
-                                   AND o.code = $2 AND o.space = $3)
+        PGSQL_DELETE_OPTION_NO_TAG(dhcp6,
+            WHERE o.scope_id = 1 AND o.dhcp6_subnet_id = $1 AND o.code = $2 AND o.space = $3)
     },
 
     // Delete single option from a pool.
@@ -2926,8 +2919,8 @@ TaggedStatementArray tagged_statements = { {
             OID_VARCHAR     // 3 space
         },
         "DELETE_OPTION6_SHARED_NETWORK",
-        PGSQL_DELETE_OPTION_NO_TAG(dhcp6, WHERE o.scope_id = 4 AND o.shared_network_name = $1 \
-                                   AND o.code = $2 AND o.space = $3)
+        PGSQL_DELETE_OPTION_NO_TAG(dhcp6,
+            WHERE o.scope_id = 4 AND o.shared_network_name = $1 AND o.code = $2 AND o.space = $3)
     },
 
     // Delete options belonging to a subnet.
@@ -3276,8 +3269,8 @@ StampedValueCollection
 PgSqlConfigBackendDHCPv6::getAllGlobalParameters6(const ServerSelector& server_selector) const {
     LOG_DEBUG(pgsql_cb_logger, DBGLVL_TRACE_BASIC, PGSQL_CB_GET_ALL_GLOBAL_PARAMETERS6);
     StampedValueCollection parameters;
-    auto tags = server_selector.getTags();
-    for (auto tag : tags) {
+    auto const& tags = server_selector.getTags();
+    for (auto const& tag : tags) {
         PsqlBindArray in_bindings;
         in_bindings.addTempString(tag.get());
         impl_->getGlobalParameters(PgSqlConfigBackendDHCPv6Impl::GET_ALL_GLOBAL_PARAMETERS6,
@@ -3294,8 +3287,8 @@ PgSqlConfigBackendDHCPv6::getModifiedGlobalParameters6(const db::ServerSelector&
     LOG_DEBUG(pgsql_cb_logger, DBGLVL_TRACE_BASIC, PGSQL_CB_GET_MODIFIED_GLOBAL_PARAMETERS6)
         .arg(util::ptimeToText(modification_time));
     StampedValueCollection parameters;
-    auto tags = server_selector.getTags();
-    for (auto tag : tags) {
+    auto const& tags = server_selector.getTags();
+    for (auto const& tag : tags) {
         PsqlBindArray in_bindings;
         in_bindings.addTempString(tag.get());
         in_bindings.addTimestamp(modification_time);
@@ -3530,8 +3523,18 @@ PgSqlConfigBackendDHCPv6::deleteSharedNetworkSubnets6(const db::ServerSelector&
 uint64_t
 PgSqlConfigBackendDHCPv6::deleteSharedNetwork6(const ServerSelector& server_selector,
                                                const std::string& name) {
+    /// @todo Using UNASSIGNED selector is allowed by the CB API but we don't have
+    /// dedicated query for this at the moment. The user should use ANY to delete
+    /// the shared network by name.
+    if (server_selector.amUnassigned()) {
+        isc_throw(NotImplemented, "deleting an unassigned shared network requires "
+                  "an explicit server tag or using ANY server. The UNASSIGNED server "
+                  "selector is currently not supported");
+    }
+
     LOG_DEBUG(pgsql_cb_logger, DBGLVL_TRACE_BASIC, PGSQL_CB_DELETE_SHARED_NETWORK6)
         .arg(name);
+
     int index = (server_selector.amAny() ?
                  PgSqlConfigBackendDHCPv6Impl::DELETE_SHARED_NETWORK6_NAME_ANY :
                  PgSqlConfigBackendDHCPv6Impl::DELETE_SHARED_NETWORK6_NAME_WITH_TAG);
@@ -3555,8 +3558,7 @@ PgSqlConfigBackendDHCPv6::deleteAllSharedNetworks6(const ServerSelector& server_
     int index = (server_selector.amUnassigned() ?
                  PgSqlConfigBackendDHCPv6Impl::DELETE_ALL_SHARED_NETWORKS6_UNASSIGNED :
                  PgSqlConfigBackendDHCPv6Impl::DELETE_ALL_SHARED_NETWORKS6);
-    uint64_t result = impl_->deleteTransactional(index,
-                                                 server_selector, "deleting all shared networks",
+    uint64_t result = impl_->deleteTransactional(index, server_selector, "deleting all shared networks",
                                                  "deleted all shared networks", true);
     LOG_DEBUG(pgsql_cb_logger, DBGLVL_TRACE_BASIC, PGSQL_CB_DELETE_ALL_SHARED_NETWORKS6_RESULT)
         .arg(result);
@@ -3642,8 +3644,8 @@ PgSqlConfigBackendDHCPv6::deleteOption6(const ServerSelector& /* server_selector
     /// just delete it when there is a match with the parent object.
     LOG_DEBUG(pgsql_cb_logger, DBGLVL_TRACE_BASIC, PGSQL_CB_DELETE_BY_POOL_OPTION6)
         .arg(pool_start_address.toText()).arg(pool_end_address.toText()).arg(code).arg(space);
-    uint64_t result = impl_->deleteOption6(ServerSelector::ANY(), pool_start_address, pool_end_address,
-                                           code, space);
+    uint64_t result = impl_->deleteOption6(ServerSelector::ANY(), pool_start_address,
+                                           pool_end_address, code, space);
     LOG_DEBUG(pgsql_cb_logger, DBGLVL_TRACE_BASIC, PGSQL_CB_DELETE_BY_POOL_OPTION6_RESULT)
         .arg(result);
     return (result);
diff --git a/src/hooks/dhcp/pgsql_cb/tests/Makefile.am b/src/hooks/dhcp/pgsql_cb/tests/Makefile.am
index 288980c6fa..146885d2b6 100644
--- a/src/hooks/dhcp/pgsql_cb/tests/Makefile.am
+++ b/src/hooks/dhcp/pgsql_cb/tests/Makefile.am
@@ -24,15 +24,10 @@ if HAVE_GTEST
 TESTS += pgsql_cb_unittests
 
 pgsql_cb_unittests_SOURCES  = pgsql_cb_impl_unittest.cc
-
-# disabled for now, to be added in #95
 pgsql_cb_unittests_SOURCES  += pgsql_cb_dhcp4_unittest.cc
 pgsql_cb_unittests_SOURCES += pgsql_cb_dhcp4_mgr_unittest.cc
-
-# disabled for now, to be added in #96
 pgsql_cb_unittests_SOURCES += pgsql_cb_dhcp6_unittest.cc
 pgsql_cb_unittests_SOURCES += pgsql_cb_dhcp6_mgr_unittest.cc
-
 pgsql_cb_unittests_SOURCES += run_unittests.cc
 
 pgsql_cb_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES) $(LOG4CPLUS_INCLUDES)
diff --git a/src/hooks/dhcp/pgsql_cb/tests/pgsql_cb_dhcp6_mgr_unittest.cc b/src/hooks/dhcp/pgsql_cb/tests/pgsql_cb_dhcp6_mgr_unittest.cc
index ca4a97b070..cb2b699b1e 100644
--- a/src/hooks/dhcp/pgsql_cb/tests/pgsql_cb_dhcp6_mgr_unittest.cc
+++ b/src/hooks/dhcp/pgsql_cb/tests/pgsql_cb_dhcp6_mgr_unittest.cc
@@ -60,15 +60,15 @@ TEST_F(PgSqlConfigBackendDHCPv6MgrTest, factoryRegistration) {
     ASSERT_NO_THROW(mgr.addBackend(validPgSQLConnectionString()));
 
     // Create a PgSQL backend selector for convenience.
-    BackendSelector mysql(BackendSelector::Type::POSTGRESQL);
+    BackendSelector pgsql(BackendSelector::Type::POSTGRESQL);
 
     // Should be able to create a global parameter.
     StampedValuePtr server_tag = StampedValue::create("server-tag", "whale");
-    ASSERT_NO_THROW(mgr.getPool()->createUpdateGlobalParameter6(mysql, ServerSelector::ALL(),
+    ASSERT_NO_THROW(mgr.getPool()->createUpdateGlobalParameter6(pgsql, ServerSelector::ALL(),
                                                                 server_tag));
     // Verify parameter can be fetched.
     server_tag.reset();
-    ASSERT_NO_THROW(server_tag = mgr.getPool()->getGlobalParameter6(mysql, ServerSelector::ALL(),
+    ASSERT_NO_THROW(server_tag = mgr.getPool()->getGlobalParameter6(pgsql, ServerSelector::ALL(),
                                                                     "server-tag"));
     ASSERT_TRUE(server_tag);
     EXPECT_EQ("server-tag", server_tag->getName());
@@ -81,7 +81,7 @@ TEST_F(PgSqlConfigBackendDHCPv6MgrTest, factoryRegistration) {
     ASSERT_THROW(mgr.addBackend(validPgSQLConnectionString()), InvalidType);
 
     // Attempting to read the global parameter should fail.
-    ASSERT_THROW(mgr.getPool()->getGlobalParameter6(mysql, ServerSelector::ALL(), "server-tag"),
+    ASSERT_THROW(mgr.getPool()->getGlobalParameter6(pgsql, ServerSelector::ALL(), "server-tag"),
                  NoSuchDatabase);
 }