[#3536] addressed review comments

Author: Razvan Becheriu

src/bin/dhcp4/json_config_parser.cc

@@ -900,15 +900,14 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set,
         }
     }
 
-    // Print the list of known backends.
-    LeaseMgrFactory::printRegistered();
+    // Log the list of known backends.
+    LeaseMgrFactory::logRegistered();
 
-    // Print the list of known backends.
-    HostDataSourceFactory::printRegistered();
+    // Log the list of known backends.
+    HostDataSourceFactory::logRegistered();
 
     // Moved from the commit block to add the config backend indication.
     if (status_code == CONTROL_RESULT_SUCCESS && (!check_only || extra_checks)) {
-
         try {
             // If there are config backends, fetch and merge into staging config
             server.getCBControl()->databaseConfigFetch(srv_config,

src/bin/dhcp6/json_config_parser.cc

@@ -1036,11 +1036,11 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set,
         }
     }
 
-    // Print the list of known backends.
-    LeaseMgrFactory::printRegistered();
+    // Log the list of known backends.
+    LeaseMgrFactory::logRegistered();
 
-    // Print the list of known backends.
-    HostDataSourceFactory::printRegistered();
+    // Log the list of known backends.
+    HostDataSourceFactory::logRegistered();
 
     // Moved from the commit block to add the config backend indication.
     if (status_code == CONTROL_RESULT_SUCCESS && (!check_only || extra_checks)) {

src/lib/dhcpsrv/host_data_source_factory.cc

@@ -43,7 +43,7 @@ HostDataSourceFactory::add(HostDataSourceList& sources,
     DatabaseConnection::ParameterMap parameters =
             DatabaseConnection::parse(dbaccess);
 
-    // Get the database type and open the corresponding database
+    // Get the database type and open the corresponding database.
     DatabaseConnection::ParameterMap::iterator it = parameters.find("type");
     if (it == parameters.end()) {
         isc_throw(InvalidParameter, "Host database configuration does not "
@@ -55,8 +55,7 @@ HostDataSourceFactory::add(HostDataSourceList& sources,
 
     // No match?
     if (index == map_.end()) {
-        if ((db_type == "mysql") ||
-            (db_type == "postgresql")) {
+        if ((db_type == "mysql") || (db_type == "postgresql")) {
             string with = (db_type == "postgresql" ? "pgsql" : db_type);
             isc_throw(InvalidType, "The type of host backend: '" << db_type
                       << "' is not compiled in. Did you forget to use --with-"
@@ -69,11 +68,11 @@ HostDataSourceFactory::add(HostDataSourceList& sources,
     // Call the factory and push the pointer on sources.
     sources.push_back(index->second(parameters));
 
-    // Check the factory did not return NULL.
+    // Check the factory did not return null.
     if (!sources.back()) {
         sources.pop_back();
         isc_throw(Unexpected, "Hosts database " << db_type <<
-                  " factory returned NULL");
+                  " factory returned null");
     }
 }
 
@@ -163,14 +162,18 @@ HostDataSourceFactory::registeredFactory(const std::string& db_type) {
 }
 
 void
-HostDataSourceFactory::printRegistered() {
+HostDataSourceFactory::logRegistered() {
     std::stringstream txt;
 
     for (auto const& x : map_) {
-        txt << x.first << " ";
+        if (!txt.str().empty()) {
+            txt << " ";
+        }
+        txt << x.first;
     }
 
-    LOG_INFO(hosts_logger, HOSTS_BACKENDS_REGISTERED).arg(txt.str());
+    LOG_INFO(hosts_logger, HOSTS_BACKENDS_REGISTERED)
+        .arg(txt.str());
 }
 
 }  // namespace dhcp

src/lib/dhcpsrv/host_data_source_factory.h

@@ -96,7 +96,7 @@ class HostDataSourceFactory {
     /// @brief Type of host data source factory
     ///
     /// A factory takes a parameter map and returns a pointer to a host
-    /// data source. In case of failure it must throw and not return NULL.
+    /// data source. In case of failure it must throw and not return null.
     typedef std::function<HostDataSourcePtr (const db::DatabaseConnection::ParameterMap&)> Factory;
 
     /// @brief Register a host data source factory
@@ -131,12 +131,12 @@ class HostDataSourceFactory {
     /// @return true if a factory was registered for db_type, false if not.
     static bool registeredFactory(const std::string& db_type);
 
-    /// @brief Prints out all registered backends.
+    /// @brief Logs out all registered backends.
     ///
     /// We need a dedicated method for this, because we sometimes can't log
     /// the backend type when doing early initialization for backends
     /// initialized statically.
-    static void printRegistered();
+    static void logRegistered();
 
 private:
     /// @brief Factory map

src/lib/dhcpsrv/lease_mgr_factory.cc

@@ -47,7 +47,7 @@ LeaseMgrFactory::create(const std::string& dbaccess) {
     DatabaseConnection::ParameterMap parameters = DatabaseConnection::parse(dbaccess);
     std::string redacted = DatabaseConnection::redactedAccessString(parameters);
 
-    // Get the database type and open the corresponding database
+    // Get the database type and open the corresponding database.
     DatabaseConnection::ParameterMap::iterator it = parameters.find(type);
     if (it == parameters.end()) {
         LOG_ERROR(dhcpsrv_logger, DHCPSRV_NOTYPE_DB).arg(dbaccess);
@@ -88,8 +88,7 @@ LeaseMgrFactory::create(const std::string& dbaccess) {
 
     // No match?
     if (index == map_.end()) {
-        if ((db_type == "mysql") ||
-            (db_type == "postgresql")) {
+        if ((db_type == "mysql") || (db_type == "postgresql")) {
             LOG_ERROR(dhcpsrv_logger, DHCPSRV_UNKNOWN_DB).arg(db_type);
             string with = (db_type == "postgresql" ? "pgsql" : db_type);
             isc_throw(InvalidType, "The Kea server has not been compiled with "
@@ -106,10 +105,10 @@ LeaseMgrFactory::create(const std::string& dbaccess) {
     // Call the factory.
     getLeaseMgrPtr() = index->second(parameters);
 
-    // Check the factory did not return NULL.
+    // Check the factory did not return null.
     if (!getLeaseMgrPtr()) {
         isc_throw(Unexpected, "Lease database " << db_type <<
-                  " factory returned NULL");
+                  " factory returned null");
     }
 }
 
@@ -159,7 +158,7 @@ LeaseMgrFactory::haveInstance() {
 TrackingLeaseMgr&
 LeaseMgrFactory::instance() {
     TrackingLeaseMgr* lmptr = getLeaseMgrPtr().get();
-    if (lmptr == NULL) {
+    if (!lmptr) {
         isc_throw(NoLeaseManager, "no current lease manager is available");
     }
     return (*lmptr);
@@ -208,14 +207,18 @@ LeaseMgrFactory::registeredFactory(const std::string& db_type) {
 }
 
 void
-LeaseMgrFactory::printRegistered() {
+LeaseMgrFactory::logRegistered() {
     std::stringstream txt;
 
     for (auto const& x : map_) {
-        txt << x.first << " ";
+        if (!txt.str().empty()) {
+            txt << " ";
+        }
+        txt << x.first;
     }
 
-    LOG_INFO(dhcpsrv_logger, DHCPSRV_LEASE_MGR_BACKENDS_REGISTERED).arg(txt.str());
+    LOG_INFO(dhcpsrv_logger, DHCPSRV_LEASE_MGR_BACKENDS_REGISTERED)
+        .arg(txt.str());
 }
 
 } // namespace dhcp

src/lib/dhcpsrv/lease_mgr_factory.h

@@ -18,7 +18,6 @@
 namespace isc {
 namespace dhcp {
 
-
 /// @brief No lease manager exception
 ///
 /// Thrown if an attempt is made to get a reference to the current lease
@@ -106,7 +105,7 @@ class LeaseMgrFactory {
     /// @brief Type of lease mgr factory
     ///
     /// A factory takes a parameter map and returns a pointer to a lease mgr.
-    /// In case of failure it must throw and not return NULL.
+    /// In case of failure it must throw and not return null.
     typedef std::function<TrackingLeaseMgrPtr (const db::DatabaseConnection::ParameterMap&)> Factory;
 
     /// @brief Register a lease mgr factory
@@ -121,7 +120,8 @@ class LeaseMgrFactory {
     /// @return true if the factory was successfully added to the map, false
     /// if it already exists.
     static bool registerFactory(const std::string& db_type,
-                                const Factory& factory, bool no_log = false);
+                                const Factory& factory,
+                                bool no_log = false);
 
     /// @brief Deregister a lease mgr factory
     ///
@@ -141,12 +141,12 @@ class LeaseMgrFactory {
     /// @return true if a factory was registered for db_type, false if not.
     static bool registeredFactory(const std::string& db_type);
 
-    /// @brief Prints out all registered backends.
+    /// @brief Logs out all registered backends.
     ///
     /// We need a dedicated method for this, because we sometimes can't log
     /// the backend type when doing early initialization for backends
     /// initialized statically.
-    static void printRegistered();
+    static void logRegistered();
 
 private:
     /// @brief Hold pointer to lease manager

src/lib/dhcpsrv/tests/host_data_source_factory_unittest.cc

@@ -149,7 +149,7 @@ TEST_F(HostDataSourceFactoryTest, notype) {
                  InvalidType);
 }
 
-// Verify that factory must not return NULL
+// Verify that factory must not return null
 TEST_F(HostDataSourceFactoryTest, null) {
     EXPECT_TRUE(HostDataSourceFactory::registerFactory("mem", factory0));
     EXPECT_THROW(HostDataSourceFactory::add(sources_, "type=mem"),

src/lib/dhcpsrv/tests/lease_mgr_factory_unittest.cc

@@ -173,7 +173,7 @@ TEST_F(LeaseMgrFactoryTest, notype) {
     EXPECT_FALSE(LeaseMgrFactory::haveInstance());
 }
 
-// Verify that factory must not return NULL
+// Verify that factory must not return null
 TEST_F(LeaseMgrFactoryTest, null) {
     EXPECT_TRUE(LeaseMgrFactory::registerFactory("mem", factory0));
     EXPECT_THROW(LeaseMgrFactory::create("type=mem"),

src/lib/dhcpsrv/tracking_lease_mgr.h

@@ -303,7 +303,7 @@ class TrackingLeaseMgr : public LeaseMgr {
 };
 
 /// @brief TrackingLeaseMgr pointer
-typedef boost::shared_ptr<TrackingLeaseMgr> TrackingLeaseMgrPtr;
+typedef std::unique_ptr<TrackingLeaseMgr> TrackingLeaseMgrPtr;
 
 } // end of namespace isc::dhcp
 } // end of namespace isc