[master] Merge branch 'trac3198'

src/hooks/dhcp/user_chk/.gitignore

@@ -0,0 +1,3 @@
+/user_chk_messages.cc
+/user_chk_messages.h
+/s-messages

src/hooks/dhcp/user_chk/Makefile.am

@@ -10,26 +10,21 @@ AM_CXXFLAGS  = $(KEA_CXXFLAGS)
 # But older GCC compilers don't have the flag.
 AM_CXXFLAGS += $(WARNING_NO_MISSING_FIELD_INITIALIZERS_CFLAG)
 
-# Until logging in dynamically loaded libraries is fixed,
 # Define rule to build logging source files from message file
-# user_chk_messages.h user_chk_messages.cc: s-messages
-
-# Until logging in dynamically loaded libraries is fixed, exclude these.
-# s-messages: user_chk_messages.mes
-#	$(top_builddir)/src/lib/log/compiler/message $(top_srcdir)/src/hooks/dhcp/user_chk/user_chk_messages.mes
-#	touch $@
+user_chk_messages.h user_chk_messages.cc: s-messages
+s-messages: user_chk_messages.mes
+	$(top_builddir)/src/lib/log/compiler/message $(top_srcdir)/src/hooks/dhcp/user_chk/user_chk_messages.mes
+	touch $@
 
 # Tell automake that the message files are built as part of the build process
 # (so that they are built before the main library is built).
-# BUILT_SOURCES = user_chk_messages.h user_chk_messages.cc
-BUILT_SOURCES =
+BUILT_SOURCES = user_chk_messages.h user_chk_messages.cc
 
 # Ensure that the message file is included in the distribution
 EXTRA_DIST = libdhcp_user_chk.dox
 
 # Get rid of generated message files on a clean
-#CLEANFILES = *.gcno *.gcda user_chk_messages.h user_chk_messages.cc s-messages
-CLEANFILES = *.gcno *.gcda
+CLEANFILES = *.gcno *.gcda user_chk_messages.h user_chk_messages.cc s-messages
 
 # convenience archive
 
@@ -42,15 +37,13 @@ libduc_la_SOURCES += pkt_send_co.cc
 libduc_la_SOURCES += subnet_select_co.cc
 libduc_la_SOURCES += user.cc user.h
 libduc_la_SOURCES += user_chk.h
-# Until logging in dynamically loaded libraries is fixed, exclude these.
-#libduc_la_SOURCES += user_chk_log.cc user_chk_log.h
+libduc_la_SOURCES += user_chk_log.cc user_chk_log.h
 libduc_la_SOURCES += user_data_source.h
 libduc_la_SOURCES += user_file.cc user_file.h
 libduc_la_SOURCES += user_registry.cc user_registry.h
 libduc_la_SOURCES += version.cc
 
-# Until logging in dynamically loaded libraries is fixed, exclude these.
-#nodist_libduc_la_SOURCES = user_chk_messages.cc user_chk_messages.h
+nodist_libduc_la_SOURCES = user_chk_messages.cc user_chk_messages.h
 
 libduc_la_CXXFLAGS = $(AM_CXXFLAGS)
 libduc_la_CPPFLAGS = $(AM_CPPFLAGS) $(LOG4CPLUS_INCLUDES)

src/hooks/dhcp/user_chk/load_unload.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -15,6 +15,7 @@
 /// @file load_unload.cc Defines the load and unload hooks library functions.
 
 #include <hooks/hooks.h>
+#include <user_chk_log.h>
 #include <user_registry.h>
 #include <user_file.h>
 
@@ -98,8 +99,8 @@ int load(LibraryHandle&) {
     }
     catch (const std::exception& ex) {
         // Log the error and return failure.
-        std::cout << "DHCP UserCheckHook could not be loaded: "
-                  << ex.what() << std::endl;
+        LOG_ERROR(user_chk_logger, USER_CHK_HOOK_LOAD_ERROR)
+            .arg(ex.what());
         ret_val = 1;
     }
 
@@ -120,8 +121,8 @@ int unload() {
     } catch (const std::exception& ex) {
         // On the off chance something goes awry, catch it and log it.
         // @todo Not sure if we should return a non-zero result or not.
-        std::cout << "DHCP UserCheckHook could not be unloaded: "
-                  << ex.what() << std::endl;
+        LOG_ERROR(user_chk_logger, USER_CHK_HOOK_UNLOAD_ERROR)
+            .arg(ex.what());
     }
 
     return (0);

src/hooks/dhcp/user_chk/subnet_select_co.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -20,6 +20,7 @@
 #include <dhcp/pkt6.h>
 #include <dhcpsrv/subnet.h>
 #include <user_chk.h>
+#include <user_chk_log.h>
 
 using namespace isc::dhcp;
 using namespace isc::hooks;
@@ -49,8 +50,7 @@ extern "C" {
 /// @return 0 upon success, non-zero otherwise.
 int subnet4_select(CalloutHandle& handle) {
     if (!user_registry) {
-        std::cout << "DHCP UserCheckHook : subnet4_select UserRegistry is null"
-                  << std::endl;
+        LOG_ERROR(user_chk_logger, USER_CHK_SUBNET4_SELECT_REGISTRY_NULL);
         return (1);
     }
 
@@ -78,8 +78,8 @@ int subnet4_select(CalloutHandle& handle) {
             handle.setArgument("subnet4", subnet);
         }
     } catch (const std::exception& ex) {
-        std::cout << "DHCP UserCheckHook : subnet6_select unexpected error: "
-                  << ex.what() << std::endl;
+        LOG_ERROR(user_chk_logger, USER_CHK_SUBNET4_SELECT_ERROR)
+            .arg(ex.what());
         return (1);
     }
 
@@ -104,8 +104,7 @@ int subnet4_select(CalloutHandle& handle) {
 /// @return 0 upon success, non-zero otherwise.
 int subnet6_select(CalloutHandle& handle) {
     if (!user_registry) {
-        std::cout << "DHCP UserCheckHook : subnet6_select UserRegistry is null"
-                  << std::endl;
+        LOG_ERROR(user_chk_logger, USER_CHK_SUBNET6_SELECT_REGISTRY_NULL);
         return (1);
     }
 
@@ -133,8 +132,8 @@ int subnet6_select(CalloutHandle& handle) {
             handle.setArgument("subnet6", subnet);
         }
     } catch (const std::exception& ex) {
-        std::cout << "DHCP UserCheckHook : subnet6_select unexpected error: "
-                  << ex.what() << std::endl;
+        LOG_ERROR(user_chk_logger, USER_CHK_SUBNET6_SELECT_ERROR)
+            .arg(ex.what());
         return (1);
     }
 

src/hooks/dhcp/user_chk/user_chk_log.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -19,6 +19,8 @@
 #include <log/macros.h>
 #include <user_chk_messages.h>
 
+namespace user_chk {
+
 /// @brief User Check Logger
 ///
 /// Define the logger used to log messages.  We could define it in multiple
@@ -26,4 +28,6 @@
 /// space.
 extern isc::log::Logger user_chk_logger;
 
+} // end of namespace user_chk
+
 #endif // USER_CHK_LOG_H

src/lib/hooks/library_manager.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -20,6 +20,8 @@
 #include <hooks/library_manager.h>
 #include <hooks/pointer_converter.h>
 #include <hooks/server_hooks.h>
+#include <log/logger_manager.h>
+#include <log/message_initializer.h>
 
 #include <string>
 #include <vector>
@@ -263,6 +265,19 @@ LibraryManager::loadLibrary() {
     // Open the library (which is a check that it exists and is accessible).
     if (openLibrary()) {
 
+        // The hook libraries provide their own log messages and logger
+        // instances. This step is required to register log messages for
+        // the library being loaded in the global dictionary. Ideally, this
+        // should be called after all libraries have been loaded but we're
+        // going to call the version() and load() functions here and these
+        // functions may already contain logging statements.
+        isc::log::MessageInitializer::loadDictionary();
+
+        // The log messages registered by the new hook library may duplicate
+        // some of the existing messages. Log warning for each duplicated
+        // message now.
+        isc::log::LoggerManager::logDuplicatedMessages();
+
         // Library opened OK, see if a version function is present and if so,
         // check what value it returns.
         if (checkVersion()) {

src/lib/hooks/library_manager.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -120,6 +120,10 @@ public:
     /// Open the library and check the version.  If all is OK, load all standard
     /// symbols then call "load" if present.
     ///
+    /// It also calls the @c isc::log::MessageInitializer::loadDictionary, prior
+    /// to invoking the @c version function of the library, to update the global
+    /// logging dictionary with the log messages registered by the loaded library.
+    ///
     /// @return true if the library loaded successfully, false otherwise. In the
     ///         latter case, the library will be unloaded if possible.
     bool loadLibrary();

src/lib/hooks/tests/basic_callout_library.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -34,13 +34,39 @@
 ///   ...where data_1, data_2 and data_3 are the values passed in arguments of
 ///   the same name to the three callouts (data_1 passed to hookpt_one, data_2
 ///   to hookpt_two etc.) and the result is returned in the argument "result".
+///
+/// - The logger instance is created and some log messages are defined. Some
+///   log messages are duplicated purposely, to check that the logger handles
+///   the duplicates correctly.
 
 #include <hooks/hooks.h>
 #include <fstream>
+#include <log/logger.h>
+#include <log/macros.h>
+#include <log/message_initializer.h>
 
 using namespace isc::hooks;
+using namespace isc::log;
 using namespace std;
 
+namespace {
+
+/// @brief Logger used by the library.
+isc::log::Logger logger("bcl");
+
+/// @brief Log messages.
+const char* log_messages[] = {
+    "BCL_LOAD_START", "basic callout load %1",
+    "BCL_LOAD_END", "basic callout load end",
+    "BCL_LOAD_END", "duplicate of basic callout load end",
+    NULL
+};
+
+/// @brief Initializer for log messages.
+const MessageInitializer message_initializer(log_messages);
+
+}
+
 extern "C" {
 
 // Callouts.  All return their result through the "result" argument.
@@ -117,6 +143,8 @@ load(isc::hooks::LibraryHandle&) {
 #ifdef USE_STATIC_LINK
     hooksStaticLinkInit();
 #endif
+    LOG_INFO(logger, "BCL_LOAD_START").arg("argument");
+    LOG_INFO(logger, "BCL_LOAD_END");
     return (0);
 }
 

src/lib/hooks/tests/library_manager_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -21,6 +21,9 @@
 #include <hooks/tests/marker_file.h>
 #include <hooks/tests/test_libraries.h>
 
+#include <log/message_dictionary.h>
+#include <log/message_initializer.h>
+
 #include <gtest/gtest.h>
 
 #include <algorithm>
@@ -32,6 +35,7 @@
 
 using namespace isc;
 using namespace isc::hooks;
+using namespace isc::log;
 using namespace std;
 
 namespace {
@@ -566,4 +570,30 @@ TEST_F(LibraryManagerTest, validateLibraries) {
     EXPECT_TRUE(LibraryManager::validateLibrary(UNLOAD_CALLOUT_LIBRARY));
 }
 
+// Check that log messages are properly registered and unregistered.
+
+TEST_F(LibraryManagerTest, libraryLoggerSetup) {
+    // Load a library with all framework functions.
+    LibraryManager lib_manager(std::string(BASIC_CALLOUT_LIBRARY), 0,
+                               callout_manager_);
+    EXPECT_TRUE(lib_manager.loadLibrary());
+
+    // After loading the library, the global logging dictionary should
+    // contain log messages registerd for this library.
+    const MessageDictionaryPtr& dict = MessageDictionary::globalDictionary();
+    EXPECT_EQ("basic callout load %1", dict->getText("BCL_LOAD_START"));
+    EXPECT_EQ("basic callout load end", dict->getText("BCL_LOAD_END"));
+    // Some of the messages defined by the hook library are duplicates. But,
+    // the loadLibrary function should have logged the duplicates and clear
+    // the duplicates list. By checking that the list of duplicates is empty
+    // we test that the LibraryManager handles the duplicates (logs and
+    // clears them).
+    EXPECT_TRUE(MessageInitializer::getDuplicates().empty());
+
+    // After unloading the library, the messages should be unregistered.
+    EXPECT_TRUE(lib_manager.unloadLibrary());
+    EXPECT_TRUE(dict->getText("BCL_LOAD_START").empty());
+    EXPECT_TRUE(dict->getText("BCL_LOAD_END").empty());
+}
+
 } // Anonymous namespace

src/lib/log/compiler/message.cc

@@ -619,10 +619,10 @@ main(int argc, char* argv[]) {
     }
     catch (const MessageException& e) {
         // Create an error message from the ID and the text
-        MessageDictionary& global = MessageDictionary::globalDictionary();
+        const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
         string text = e.id();
         text += ", ";
-        text += global.getText(e.id());
+        text += global->getText(e.id());
         // Format with arguments
         vector<string> args(e.arguments());
         for (size_t i(0); i < args.size(); ++ i) {

src/lib/log/logger_impl.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011,2014  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011,2014-2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -126,7 +126,7 @@ LoggerImpl::getEffectiveDebugLevel() {
 string*
 LoggerImpl::lookupMessage(const MessageID& ident) {
     return (new string(string(ident) + " " +
-                       MessageDictionary::globalDictionary().getText(ident)));
+                       MessageDictionary::globalDictionary()->getText(ident)));
 }
 
 // Replace the interprocess synchronization object

src/lib/log/logger_manager.cc

@@ -121,26 +121,31 @@ LoggerManager::init(const std::string& root, isc::log::Severity severity,
 
     // Check if there were any duplicate message IDs in the default dictionary
     // and if so, log them.  Log using the logging facility logger.
-    const vector<string>& duplicates = MessageInitializer::getDuplicates();
+    logDuplicatedMessages();
+
+    // Replace any messages with local ones (if given)
+    if (file) {
+        readLocalMessageFile(file);
+    }
+
+    // Ensure that the mutex is constructed and ready at this point.
+    (void) getMutex();
+}
+
+void
+LoggerManager::logDuplicatedMessages() {
+    const list<string>& duplicates = MessageInitializer::getDuplicates();
     if (!duplicates.empty()) {
 
         // There are duplicates present. This list itself may contain
         // duplicates; if so, the message ID is listed as many times as
         // there are duplicates.
-        for (vector<string>::const_iterator i = duplicates.begin();
+        for (list<string>::const_iterator i = duplicates.begin();
              i != duplicates.end(); ++i) {
             LOG_WARN(logger, LOG_DUPLICATE_MESSAGE_ID).arg(*i);
         }
         MessageInitializer::clearDuplicates();
     }
-
-    // Replace any messages with local ones (if given)
-    if (file) {
-        readLocalMessageFile(file);
-    }
-
-    // Ensure that the mutex is constructed and ready at this point.
-    (void) getMutex();
 }
 
 
@@ -150,8 +155,8 @@ LoggerManager::init(const std::string& root, isc::log::Severity severity,
 void
 LoggerManager::readLocalMessageFile(const char* file) {
 
-    MessageDictionary& dictionary = MessageDictionary::globalDictionary();
-    MessageReader reader(&dictionary);
+    const MessageDictionaryPtr& dictionary = MessageDictionary::globalDictionary();
+    MessageReader reader(dictionary.get());
 
     // Turn off use of any lock files. This is because this logger can
     // be used by standalone programs which may not have write access to

src/lib/log/logger_manager.h

@@ -120,6 +120,14 @@ public:
                     int dbglevel = 0, const char* file = NULL,
                     bool buffer = false);
 
+    /// \brief List duplicated log messages.
+    ///
+    /// Lists the duplicated log messages using warning severity. Then, it
+    /// clears the list of duplicated messages. This method is called by the
+    /// \c init method and by the \c isc::hooks::LibraryManager when the new
+    /// hooks library is loaded.
+    static void logDuplicatedMessages();
+
     /// \brief Reset logging
     ///
     /// Resets logging to whatever was set in the call to init(), expect for

src/lib/log/logging.dox

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2014  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -704,6 +704,34 @@ In the case of "stdout", "stderr" and "syslog", they must be written exactly
 as is - no leading or trailing spaces, and in lower-case.</dd>
 </dl>
 
+@subsection logInitializationHooks Hooks Specific Notes
+All hooks libraries should use Kea logging mechanisms. The loggers and the
+library specific log messages are created in the same way as for the core
+Kea modules. The loggers created within the hook library belong to the
+logging hierarchy of the Kea process and their configuration can be
+controlled from the Kea configuration file. If the configuration file doesn't
+contain the specific configuration for the logger used in the library,
+this logger is given the configuration of the root Kea logger.
+
+The hook libraries are loaded dynamically. This requires that the global log
+messages dictionary, holding the mapping of specific log message
+identifiers to the actual messages, is updated to include the messages
+specified in the hook library when the library is loaded. Conversely, the
+messages have to be removed from the dictionary when the library is unloaded.
+
+The new messages are added to the global dictionary using the
+@c isc::log::MessageInitializer::loadDictionary static function. It is
+called by the @c isc::hooks::LibraryManager::loadLibrary for each loaded
+library.
+
+When the library is unloaded, the instance of the
+@c isc::log::MessageInitializer defined in the library is destroyed
+and its destructor removes the messages registered by the destroyed
+instance from the global dictionary.
+
+The hook library itself must not perform any action to register or
+unregister log messages in the global dictionary!
+
 @subsection logInitializationPython Python Initialization
 To initialize the logger in a Python program, the "init" method must be
 called:

src/lib/log/message_dictionary.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -56,6 +56,17 @@ MessageDictionary::replace(const std::string& ident, const std::string& text) {
     return (found);
 }
 
+bool
+MessageDictionary::erase(const std::string& ident, const std::string& text) {
+    Dictionary::iterator mes = dictionary_.find(ident);
+    // Both the ID and the text must match.
+    bool found = (mes != dictionary_.end() && (mes->second == text));
+    if (found) {
+        dictionary_.erase(mes);
+    }
+    return (found);
+}
+
 // Load a set of messages
 
 vector<std::string>
@@ -100,9 +111,9 @@ MessageDictionary::getText(const std::string& ident) const {
 
 // Return global dictionary
 
-MessageDictionary&
+const MessageDictionaryPtr&
 MessageDictionary::globalDictionary() {
-    static MessageDictionary global;
+    static MessageDictionaryPtr global(new MessageDictionary());
     return (global);
 }
 

src/lib/log/message_dictionary.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -21,12 +21,19 @@
 #include <vector>
 
 #include <boost/lexical_cast.hpp>
+#include <boost/shared_ptr.hpp>
 
 #include <log/message_types.h>
 
 namespace isc {
 namespace log {
 
+/// \brief Forward declaration of \c MessageDictionary
+class MessageDictionary;
+
+/// \brief Shared pointer to the \c MessageDictionary.
+typedef boost::shared_ptr<MessageDictionary> MessageDictionaryPtr;
+
 /// \brief Message Dictionary
 ///
 /// The message dictionary is a wrapper around a std::map object, and allows
@@ -96,7 +103,6 @@ public:
         return (replace(boost::lexical_cast<std::string>(ident), text));
     }
 
-
     /// \brief Replace Message
     ///
     /// Alternate signature.
@@ -109,6 +115,25 @@ public:
     virtual bool replace(const std::string& ident, const std::string& text);
 
 
+    /// \brief Removes the specified message from the dictionary.
+    ///
+    /// Checks if both the message identifier and the text match the message
+    /// in the dictionary before removal. If the text doesn't match it is
+    /// an indication that the message which removal is requested is a
+    /// duplicate of another message. This may occur when two Kea modules
+    /// register messages with the same identifier. When one of the modules
+    /// is unloaded and the relevant messages are unregistered, there is a
+    /// need to make sure that the message registered by the other module
+    /// is not accidentally removed. Hence, the additional check for the
+    /// text match is needed.
+    ///
+    /// \param ident Identification of the message to remove.
+    /// \param text Message text
+    ///
+    /// \return true of the message has been removed, false if the message
+    /// couldn't be found.
+    virtual bool erase(const std::string& ident, const std::string& text);
+
     /// \brief Load Dictionary
     ///
     /// Designed to be used during the initialization of programs, this
@@ -126,7 +151,6 @@ public:
     /// empty.
     virtual std::vector<std::string> load(const char* elements[]);
 
-
     /// \brief Get Message Text
     ///
     /// Given an ID, retrieve associated message text.
@@ -178,7 +202,7 @@ public:
     /// Returns a pointer to the singleton global dictionary.
     ///
     /// \return Pointer to global dictionary.
-    static MessageDictionary& globalDictionary();
+    static const MessageDictionaryPtr& globalDictionary();
 
 private:
     Dictionary       dictionary_;   ///< Holds the ID to text lookups

src/lib/log/message_initializer.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -12,10 +12,13 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <cassert>
-#include <cstdlib>
 #include <log/message_dictionary.h>
 #include <log/message_initializer.h>
+#include <boost/array.hpp>
+#include <algorithm>
+#include <cassert>
+#include <cstdlib>
+
 
 
 // As explained in the header file, initialization of the message dictionary
@@ -32,20 +35,19 @@
 
 namespace {
 
-// Declare the array of pointers to value arrays.
-const char** logger_values[isc::log::MessageInitializer::MAX_MESSAGE_ARRAYS];
+/// Type definition for the list of pointers to messages.
+typedef std::list<const char**> LoggerValuesList;
 
-// Declare the index used to access the array.  As this needs to be initialized
-// at first used, it is accessed it via a function.
-size_t& getIndex() {
-    static size_t index = 0;
-    return (index);
+/// Return reference to the list of log messages.
+LoggerValuesList& getNonConstLoggerValues() {
+    static LoggerValuesList logger_values;
+    return (logger_values);
 }
 
 // Return the duplicates singleton version (non-const for local use)
-std::vector<std::string>&
+std::list<std::string>&
 getNonConstDuplicates() {
-    static std::vector<std::string> duplicates;
+    static std::list<std::string> duplicates;
     return (duplicates);
 }
 } // end unnamed namespace
@@ -57,16 +59,43 @@ namespace log {
 // Constructor.  Add the pointer to the message array to the global array.
 // This method will trigger an assertion failure if the array overflows.
 
-MessageInitializer::MessageInitializer(const char* values[]) {
-    assert(getIndex() < MAX_MESSAGE_ARRAYS);
-    logger_values[getIndex()++] = values;
+MessageInitializer::MessageInitializer(const char* values[])
+    : values_(values),
+      global_dictionary_(MessageDictionary::globalDictionary()) {
+    assert(getNonConstLoggerValues().size() < MAX_MESSAGE_ARRAYS);
+    getNonConstLoggerValues().push_back(values);
+}
+
+MessageInitializer::~MessageInitializer() {
+    LoggerValuesList& logger_values = getNonConstLoggerValues();
+
+    // Search for the pointer to pending messages belonging to our instance.
+    LoggerValuesList::iterator my_messages = std::find(logger_values.begin(),
+                                                       logger_values.end(),
+                                                       values_);
+    bool pending = (my_messages != logger_values.end());
+    // Our messages are still pending, so let's remove them from the list
+    // of pending messages.
+    if (pending) {
+        logger_values.erase(my_messages);
+
+    } else {
+        // Our messages are not pending, so they might have been loaded to
+        // the dictionary and/or duplicates.
+        int i = 0;
+        while (values_[i]) {
+            getNonConstDuplicates().remove(values_[i]);
+            global_dictionary_->erase(values_[i], values_[i + 1]);
+            i += 2;
+        }
+    }
 }
 
 // Return the number of arrays registered but not yet loaded.
 
 size_t
 MessageInitializer::getPendingCount() {
-    return (getIndex());
+    return (getNonConstLoggerValues().size());
 }
 
 // Load the messages in the arrays registered in the logger_values array
@@ -74,15 +103,17 @@ MessageInitializer::getPendingCount() {
 
 void
 MessageInitializer::loadDictionary(bool ignore_duplicates) {
-    MessageDictionary& global = MessageDictionary::globalDictionary();
+    const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
+    LoggerValuesList& logger_values = getNonConstLoggerValues(); 
 
-    for (size_t i = 0; i < getIndex(); ++i) {
-        std::vector<std::string> repeats = global.load(logger_values[i]);
+    for (LoggerValuesList::const_iterator values = logger_values.begin();
+         values != logger_values.end(); ++values) {
+        std::vector<std::string> repeats = global->load(*values);
 
         // Append the IDs in the list just loaded (the "repeats") to the
         // global list of duplicate IDs.
         if (!ignore_duplicates && !repeats.empty()) {
-            std::vector<std::string>& duplicates = getNonConstDuplicates();
+            std::list<std::string>& duplicates = getNonConstDuplicates();
             duplicates.insert(duplicates.end(), repeats.begin(),
                               repeats.end());
         }
@@ -91,11 +122,11 @@ MessageInitializer::loadDictionary(bool ignore_duplicates) {
     // ... and mark that the messages have been loaded.  (This avoids a lot
     // of "duplicate message ID" messages in some of the unit tests where the
     // logging initialization code may be called multiple times.)
-    getIndex() = 0;
+    logger_values.clear();
 }
 
 // Return reference to duplicates vector
-const std::vector<std::string>&
+const std::list<std::string>&
 MessageInitializer::getDuplicates() {
     return (getNonConstDuplicates());
 }

src/lib/log/message_initializer.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011,2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -15,10 +15,11 @@
 #ifndef MESSAGEINITIALIZER_H
 #define MESSAGEINITIALIZER_H
 
+#include <log/message_dictionary.h>
+#include <boost/noncopyable.hpp>
 #include <cstdlib>
+#include <list>
 #include <string>
-#include <vector>
-#include <log/message_dictionary.h>
 
 namespace isc {
 namespace log {
@@ -63,10 +64,10 @@ namespace log {
 ///
 /// When messages are added to the dictionary, the are added via the
 /// MessageDictionary::add() method, so any duplicates are stored in the
-/// global dictionary's overflow vector whence they can be retrieved at
+/// global dictionary's overflow lince whence they can be retrieved at
 /// run-time.
 
-class MessageInitializer {
+class MessageInitializer : public boost::noncopyable {
 public:
     /// Maximum number of message arrays that can be initialized in this way
     static const size_t MAX_MESSAGE_ARRAYS = 256;
@@ -82,6 +83,19 @@ public:
     /// loadDictionary() has been called.
     MessageInitializer(const char* values[]);
 
+    /// \brief Destructor
+    ///
+    /// Removes pending messages from the array or loaded messages from the
+    /// global dictionary.
+    ///
+    /// If the messages initialized with the destructed have already been
+    /// loaded to the global dictionary the destructor will remove these
+    /// messages and preserve messages loaded by other instances of the
+    /// \c MessageInitializer. If there are any duplicates, only the instance
+    /// of the duplicated message initialized by the destructed object will
+    /// be removed.
+    ~MessageInitializer();
+
     /// \brief Obtain pending load count
     ///
     /// Returns the number of message arrays that will be loaded by the next
@@ -99,7 +113,7 @@ public:
     ///
     /// \param ignore_duplicates If true, duplicate IDs, and IDs already
     ///        loaded, are ignored instead of stored in the global duplicates
-    ///        vector.
+    ///        list.
     static void loadDictionary(bool ignore_duplicates = false);
 
     /// \brief Return Duplicates
@@ -109,12 +123,27 @@ public:
     ///
     /// \return List of duplicate message IDs when the global dictionary was
     /// loaded.  Note that the duplicates list itself may contain duplicates.
-    static const std::vector<std::string>& getDuplicates();
+    static const std::list<std::string>& getDuplicates();
 
-    /// \brief Clear the static duplicates vector
+    /// \brief Clear the static duplicates list
     ///
-    /// Empties the vector returned by getDuplicates()
+    /// Empties the list returned by getDuplicates()
     static void clearDuplicates();
+
+private:
+
+    /// \brief Holds the pointer to the array of messages.
+    const char** values_;
+
+    /// \brief Holds the pointer to the global dictionary.
+    ///
+    /// The \c MessageInitializer instantiates the global dictionary and
+    /// keeps the reference to it throughout its lifetime as the global
+    /// dictionary is instantiated in the destructor. If the reference is
+    /// not held then it is possible that the global dictionary is destroyed
+    /// before the \c MessageInitializer destructor is called, causing the
+    /// static initialization order fiasco.
+    MessageDictionaryPtr global_dictionary_;
 };
 
 } // namespace log

src/lib/log/tests/logger_manager_unittest.cc

@@ -32,6 +32,7 @@
 #include <log/logger_level.h>
 #include <log/logger_manager.h>
 #include <log/logger_specification.h>
+#include <log/message_initializer.h>
 #include <log/output_option.h>
 
 #include "tempdir.h"
@@ -405,3 +406,27 @@ TEST_F(LoggerManagerTest, checkLayoutPattern) {
     ASSERT_EQ(0, re)
         << "Logged message does not match expected layout pattern";
 }
+
+// Check that after calling the logDuplicatedMessages, the duplicated
+// messages are removed.
+TEST_F(LoggerManagerTest, logDuplicatedMessages) {
+    // Original set should not have duplicates.
+    ASSERT_EQ(0, MessageInitializer::getDuplicates().size());
+
+    // This just defines 1, but we'll add it a number of times.
+    const char* dupe[] = {
+        "DUPE", "dupe",
+        NULL
+    };
+    const MessageInitializer init_message_initializer_1(dupe);
+    const MessageInitializer init_message_initializer_2(dupe);
+
+    MessageInitializer::loadDictionary();
+    // Should have a duplicate now.
+    ASSERT_EQ(1, MessageInitializer::getDuplicates().size());
+
+    // The logDuplicatedMessages, besides logging, should also remove the
+    // duplicates.
+    LoggerManager::logDuplicatedMessages();
+    ASSERT_EQ(0, MessageInitializer::getDuplicates().size());
+}

src/lib/log/tests/message_dictionary_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -107,6 +107,34 @@ TEST_F(MessageDictionaryTest, Replace) {
     EXPECT_EQ(string(""), dictionary.getText(gamma_id));
 }
 
+// Check that removing message works.
+
+TEST_F(MessageDictionaryTest, erase) {
+    MessageDictionary dictionary;
+    ASSERT_NO_THROW(dictionary.erase(alpha_id, alpha_text));
+    ASSERT_EQ(0, dictionary.size());
+
+    // Add a couple of messages.
+    EXPECT_TRUE(dictionary.add(alpha_id, alpha_text));
+    EXPECT_TRUE(dictionary.add(beta_id, beta_text));
+    // There is no sense to continue if messages haven't been added.
+    ASSERT_EQ(2, dictionary.size());
+
+    // Remove one with the existing ID, but non-matching text. It
+    // should not remove any message.
+    EXPECT_FALSE(dictionary.erase(beta_id, alpha_text));
+
+    // Now, remove the message with matching ID and text.
+    EXPECT_TRUE(dictionary.erase(beta_id, beta_text));
+    EXPECT_EQ(1, dictionary.size());
+    // The other entry should still exist.
+    EXPECT_EQ(alpha_text, dictionary.getText(alpha_id));
+
+    // And remove the other message.
+    EXPECT_TRUE(dictionary.erase(alpha_id, alpha_text));
+    EXPECT_EQ(0, dictionary.size());
+}
+
 // Load test
 
 TEST_F(MessageDictionaryTest, LoadTest) {
@@ -180,9 +208,9 @@ TEST_F(MessageDictionaryTest, Lookups) {
 // Check that the global dictionary is a singleton.
 
 TEST_F(MessageDictionaryTest, GlobalTest) {
-    MessageDictionary& global = MessageDictionary::globalDictionary();
-    MessageDictionary& global2 = MessageDictionary::globalDictionary();
-    EXPECT_TRUE(&global2 == &global);
+    const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
+    const MessageDictionaryPtr& global2 = MessageDictionary::globalDictionary();
+    EXPECT_TRUE(global2 == global);
 }
 
 // Check that the global dictionary has detected the duplicate and the
@@ -192,6 +220,6 @@ TEST_F(MessageDictionaryTest, GlobalLoadTest) {
     // There were duplicates but the vector should be cleared in init() now
     ASSERT_EQ(0, MessageInitializer::getDuplicates().size());
 
-    string text = MessageDictionary::globalDictionary().getText("NEWSYM");
+    string text = MessageDictionary::globalDictionary()->getText("NEWSYM");
     EXPECT_EQ(string("new symbol added"), text);
 }

src/lib/log/tests/message_initializer_1_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -15,6 +15,7 @@
 #include <log/message_dictionary.h>
 #include <log/message_initializer.h>
 #include <boost/lexical_cast.hpp>
+#include <boost/scoped_ptr.hpp>
 #include <gtest/gtest.h>
 #include <string>
 
@@ -37,6 +38,21 @@ const char* values2[] = {
     NULL
 };
 
+const char* values3[] = {
+    "GLOBAL7", "global message seven",
+    "GLOBAL8", "global message eight",
+    NULL
+};
+
+const char* values4[] = {
+    "GLOBAL8", "global message eight bis",
+    "GLOBAL9", "global message nine",
+    NULL
+};
+
+/// @brief Scoped pointer to the @c MessageInitializer object.
+typedef boost::scoped_ptr<MessageInitializer> MessageInitializerPtr;
+
 }
 
 // Statically initialize the global dictionary with those messages.  Three sets
@@ -51,14 +67,14 @@ const MessageInitializer init_message_initializer_unittest_2(values2);
 // Check that the global dictionary is initialized with the specified
 // messages.
 
-TEST(MessageInitializerTest1, MessageTest) {
-    MessageDictionary& global = MessageDictionary::globalDictionary();
+TEST(MessageInitializerTest1, messageTest) {
+    const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
 
     // Pointers to the message arrays should have been stored, but none of the
     // messages should yet be in the dictionary.
     for (int i = 1; i <= 6; ++i) {
         string symbol = string("GLOBAL") + boost::lexical_cast<std::string>(i);
-        EXPECT_EQ(string(""), global.getText(symbol));
+        EXPECT_EQ(string(""), global->getText(symbol));
     }
 
     // Load the dictionary - this should clear the message array pending count.
@@ -70,15 +86,121 @@ TEST(MessageInitializerTest1, MessageTest) {
     EXPECT_EQ(0, MessageInitializer::getPendingCount());
 
     // ... and check the messages loaded.
-    EXPECT_EQ(string("global message one"), global.getText("GLOBAL1"));
-    EXPECT_EQ(string("global message two"), global.getText("GLOBAL2"));
-    EXPECT_EQ(string("global message three"), global.getText("GLOBAL3"));
-    EXPECT_EQ(string("global message four"), global.getText("GLOBAL4"));
-    EXPECT_EQ(string("global message five"), global.getText("GLOBAL5"));
-    EXPECT_EQ(string("global message six"), global.getText("GLOBAL6"));
+    EXPECT_EQ(string("global message one"), global->getText("GLOBAL1"));
+    EXPECT_EQ(string("global message two"), global->getText("GLOBAL2"));
+    EXPECT_EQ(string("global message three"), global->getText("GLOBAL3"));
+    EXPECT_EQ(string("global message four"), global->getText("GLOBAL4"));
+    EXPECT_EQ(string("global message five"), global->getText("GLOBAL5"));
+    EXPECT_EQ(string("global message six"), global->getText("GLOBAL6"));
+}
+
+// Check that destroying the MessageInitializer causes the relevant
+// messages to be removed from the dictionary.
+
+TEST(MessageInitializerTest1, dynamicLoadUnload) {
+    // Obtain the instance of the global dictionary.
+    const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
+
+    // Dynamically create the first initializer.
+    MessageInitializerPtr init1(new MessageInitializer(values3));
+    EXPECT_EQ(1, MessageInitializer::getPendingCount());
+
+    // Dynamically create the second initializer.
+    MessageInitializerPtr init2(new MessageInitializer(values4));
+    EXPECT_EQ(2, MessageInitializer::getPendingCount());
+
+    // Load messages from both initializers to the global dictionary.
+    MessageInitializer::loadDictionary();
+    // There should be no pending messages.
+    EXPECT_EQ(0, MessageInitializer::getPendingCount());
+
+    // Make sure that the messages have been loaded.
+    EXPECT_EQ("global message seven", global->getText("GLOBAL7"));
+    EXPECT_EQ("global message eight", global->getText("GLOBAL8"));
+    EXPECT_EQ("global message nine", global->getText("GLOBAL9"));
+
+    // Destroy the first initializer. The first two messages should
+    // be unregistered.
+    init1.reset();
+    EXPECT_TRUE(global->getText("GLOBAL7").empty());
+    EXPECT_TRUE(global->getText("GLOBAL8").empty());
+    EXPECT_EQ("global message nine", global->getText("GLOBAL9"));
+
+    // Destroy the second initializer. Now, all messages should be
+    // unregistered.
+    init2.reset();
+    EXPECT_TRUE(global->getText("GLOBAL7").empty());
+    EXPECT_TRUE(global->getText("GLOBAL8").empty());
+    EXPECT_TRUE(global->getText("GLOBAL9").empty());
+}
+
+// Check that destroying the MessageInitializer removes pending messages.
+
+TEST(MessageInitializerTest1, dynamicUnloadPending) {
+    // Obtain the instance of the global dictionary.
+    const MessageDictionaryPtr& global = MessageDictionary::globalDictionary();
+
+    // Dynamically create the first initializer.
+    MessageInitializerPtr init1(new MessageInitializer(values3));
+    ASSERT_EQ(1, MessageInitializer::getPendingCount());
+
+    // Create second initializer without committing the first set
+    // of messages to the dictionary.
+    MessageInitializerPtr init2(new MessageInitializer(values4));
+    ASSERT_EQ(2, MessageInitializer::getPendingCount());
+
+    // Destroy the first initializer and make sure that the number of
+    // pending message sets drops to 1.
+    init1.reset();
+    ASSERT_EQ(1, MessageInitializer::getPendingCount());
+
+    // Now destroy the second initializer and make sure that there are
+    // no pending messages.
+    init2.reset();
+    ASSERT_EQ(0, MessageInitializer::getPendingCount());
+
+    init1.reset(new MessageInitializer(values3));
+    ASSERT_EQ(1, MessageInitializer::getPendingCount());
+
+    // Load the messages to the dictionary and make sure there are no pending
+    // messages.
+    MessageInitializer::loadDictionary();
+    EXPECT_EQ(0, MessageInitializer::getPendingCount());
+
+    // Create the second initializer. There should be one pending set of
+    // messages.
+    init2.reset(new MessageInitializer(values4));
+    ASSERT_EQ(1, MessageInitializer::getPendingCount());
+
+    // Make sure that the messages defined by the first initializer
+    // are in the dictionary.
+    ASSERT_EQ("global message seven", global->getText("GLOBAL7"));
+    ASSERT_EQ("global message eight", global->getText("GLOBAL8"));
+    ASSERT_TRUE(global->getText("GLOBAL9").empty());
+
+    // Destroy the second initializer. There should be no pending messages
+    // now.
+    init2.reset();
+    ASSERT_EQ(0, MessageInitializer::getPendingCount());
+
+    // Loading the messages should be no-op.
+    MessageInitializer::loadDictionary();
+    ASSERT_EQ(0, MessageInitializer::getPendingCount());
+
+    // Make sure that the messages loaded from the first initializer
+    // are not affected.
+    ASSERT_EQ("global message seven", global->getText("GLOBAL7"));
+    ASSERT_EQ("global message eight", global->getText("GLOBAL8"));
+    ASSERT_TRUE(global->getText("GLOBAL9").empty());
+
+    // And remove them.
+    init1.reset();
+    EXPECT_TRUE(global->getText("GLOBAL7").empty());
+    EXPECT_TRUE(global->getText("GLOBAL8").empty());
+    EXPECT_TRUE(global->getText("GLOBAL9").empty());
 }
 
-TEST(MessageInitializerTest1, Duplicates) {
+TEST(MessageInitializerTest1, duplicates) {
     // Original set should not have dupes
     ASSERT_EQ(0, MessageInitializer::getDuplicates().size());
 

src/lib/log/tests/message_initializer_2_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012,2015  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -31,13 +31,17 @@ const char* values[] = {
 }
 
 TEST(MessageInitializerTest2, MessageLoadTest) {
+    // Create the list where the initializers will be held.
+    std::list<boost::shared_ptr<MessageInitializer> > initializers;
+
     // Load the maximum number of message arrays allowed.  Some arrays may
     // already have been loaded because of static initialization from modules
     // in libraries linked against the test program, hence the reason for the
-    // loop starting from the value returned by getPendingCount() instead of 0.
+    // loop starting from the value returned by getPendingCount() instead of 0
     for (size_t i = MessageInitializer::getPendingCount();
          i < MessageInitializer::MAX_MESSAGE_ARRAYS; ++i) {
-        MessageInitializer initializer1(values);
+        boost::shared_ptr<MessageInitializer> initializer(new MessageInitializer(values));
+        initializers.push_back(initializer);
     }
 
     // Note: Not all systems have EXPECT_DEATH.  As it is a macro we can just
@@ -48,7 +52,7 @@ TEST(MessageInitializerTest2, MessageLoadTest) {
         EXPECT_DEATH({
             isc::util::unittests::dontCreateCoreDumps();
 
-            MessageInitializer initializer2(values);
+            MessageInitializer initializer(values);
           }, ".*");
     }
 #endif