From: Marcin Siodelski Date: Tue, 14 Apr 2015 11:50:19 +0000 (+0200) Subject: [3198] Fixed broken logging in the hooks libraries. X-Git-Tag: trac3812_base~2^2~6 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=a85c59af8b3a79c3157060eed78984e38faedb60;p=thirdparty%2Fkea.git [3198] Fixed broken logging in the hooks libraries. - when library is loaded, message dictionary is updated - when library is unloaded, the messages are unregistered - MessageInitializer prevents static init. fiasco with global dictionary - enabled logging in user_chk --- diff --git a/src/hooks/dhcp/user_chk/.gitignore b/src/hooks/dhcp/user_chk/.gitignore new file mode 100644 index 0000000000..76bf339e04 --- /dev/null +++ b/src/hooks/dhcp/user_chk/.gitignore @@ -0,0 +1,3 @@ +/user_chk_messages.cc +/user_chk_messages.h +/s-messages diff --git a/src/hooks/dhcp/user_chk/Makefile.am b/src/hooks/dhcp/user_chk/Makefile.am index afc2b83797..e3ae56562f 100644 --- a/src/hooks/dhcp/user_chk/Makefile.am +++ b/src/hooks/dhcp/user_chk/Makefile.am @@ -10,14 +10,11 @@ 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). @@ -28,8 +25,7 @@ BUILT_SOURCES = 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 +38,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) diff --git a/src/hooks/dhcp/user_chk/load_unload.cc b/src/hooks/dhcp/user_chk/load_unload.cc index 526ff7c948..4e02cbcafc 100644 --- a/src/hooks/dhcp/user_chk/load_unload.cc +++ b/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 +#include #include #include @@ -69,6 +70,13 @@ extern "C" { /// /// @return Returns 0 upon success, non-zero upon failure. int load(LibraryHandle&) { + /// @todo The following log message includes a dummy parameter for us + /// to check if parameters are logged properly. This parameter will have + /// to be removed. + int test_value = 123; + LOG_INFO(user_chk_logger, USER_CHK_HOOK_LOAD) + .arg(test_value); + // non-zero indicates an error. int ret_val = 0; try { diff --git a/src/hooks/dhcp/user_chk/user_chk_log.h b/src/hooks/dhcp/user_chk/user_chk_log.h index 43fb3640b0..d188b5cd77 100644 --- a/src/hooks/dhcp/user_chk/user_chk_log.h +++ b/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 #include +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 diff --git a/src/hooks/dhcp/user_chk/user_chk_messages.mes b/src/hooks/dhcp/user_chk/user_chk_messages.mes index 0b11a1c964..34708c34b2 100644 --- a/src/hooks/dhcp/user_chk/user_chk_messages.mes +++ b/src/hooks/dhcp/user_chk/user_chk_messages.mes @@ -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 @@ -12,6 +12,9 @@ # OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR # PERFORMANCE OF THIS SOFTWARE. +% USER_CHK_HOOK_LOAD load function called: %1 +This message is logged by the hook library on entry to the load function. + % USER_CHK_HOOK_LOAD_ERROR DHCP UserCheckHook could not be loaded: %1 This is an error message issued when the DHCP UserCheckHook could not be loaded. The exact cause should be explained in the log message. User subnet selection diff --git a/src/lib/hooks/library_manager.cc b/src/lib/hooks/library_manager.cc index 7d60fbdc54..7be52624d8 100644 --- a/src/lib/hooks/library_manager.cc +++ b/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 #include #include +#include +#include #include #include @@ -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()) { diff --git a/src/lib/hooks/library_manager.h b/src/lib/hooks/library_manager.h index 1ea4f322ca..6f70091c71 100644 --- a/src/lib/hooks/library_manager.h +++ b/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(); diff --git a/src/lib/hooks/tests/basic_callout_library.cc b/src/lib/hooks/tests/basic_callout_library.cc index 622ad42b33..88d7f0e2fa 100644 --- a/src/lib/hooks/tests/basic_callout_library.cc +++ b/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 #include +#include +#include +#include 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); } diff --git a/src/lib/hooks/tests/library_manager_unittest.cc b/src/lib/hooks/tests/library_manager_unittest.cc index 86e86e6aa4..3be4f58966 100644 --- a/src/lib/hooks/tests/library_manager_unittest.cc +++ b/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 #include +#include +#include + #include #include @@ -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 diff --git a/src/lib/log/compiler/message.cc b/src/lib/log/compiler/message.cc index 239882f97c..6d7a150ad3 100644 --- a/src/lib/log/compiler/message.cc +++ b/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 args(e.arguments()); for (size_t i(0); i < args.size(); ++ i) { diff --git a/src/lib/log/logger_impl.cc b/src/lib/log/logger_impl.cc index 58793b0e00..0905e98122 100644 --- a/src/lib/log/logger_impl.cc +++ b/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 diff --git a/src/lib/log/logger_manager.cc b/src/lib/log/logger_manager.cc index bc7aff0e29..9b62b8f8db 100644 --- a/src/lib/log/logger_manager.cc +++ b/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& 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& 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::const_iterator i = duplicates.begin(); + for (list::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 diff --git a/src/lib/log/logger_manager.h b/src/lib/log/logger_manager.h index 0c49757677..4e4a67e8ee 100644 --- a/src/lib/log/logger_manager.h +++ b/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 diff --git a/src/lib/log/message_dictionary.cc b/src/lib/log/message_dictionary.cc index 577042bc6b..d2f6159d1e 100644 --- a/src/lib/log/message_dictionary.cc +++ b/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) { + const_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 @@ -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); } diff --git a/src/lib/log/message_dictionary.h b/src/lib/log/message_dictionary.h index 0dfc6306c9..5c9ba3ae10 100644 --- a/src/lib/log/message_dictionary.h +++ b/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 #include +#include #include namespace isc { namespace log { +/// \brief Forward declaration of \c MessageDictionary +class MessageDictionary; + +/// \brief Shared pointer to the \c MessageDictionary. +typedef boost::shared_ptr 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(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 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 diff --git a/src/lib/log/message_initializer.cc b/src/lib/log/message_initializer.cc index 327704759b..6c39911c8e 100644 --- a/src/lib/log/message_initializer.cc +++ b/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 -#include #include #include +#include +#include +#include +#include + // As explained in the header file, initialization of the message dictionary @@ -32,20 +35,15 @@ namespace { -// Declare the array of pointers to value arrays. -const char** logger_values[isc::log::MessageInitializer::MAX_MESSAGE_ARRAYS]; - -// 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); -} +/// Type definition for the list of pointers to messages. +typedef std::list LoggerValuesList; +// Declare the list of pointers to messages. +LoggerValuesList logger_values; // Return the duplicates singleton version (non-const for local use) -std::vector& +std::list& getNonConstDuplicates() { - static std::vector duplicates; + static std::list duplicates; return (duplicates); } } // end unnamed namespace @@ -57,16 +55,41 @@ 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(logger_values.size() < MAX_MESSAGE_ARRAYS); + logger_values.push_back(values); +} + +MessageInitializer::~MessageInitializer() { + // 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 (logger_values.size()); } // Load the messages in the arrays registered in the logger_values array @@ -74,15 +97,16 @@ MessageInitializer::getPendingCount() { void MessageInitializer::loadDictionary(bool ignore_duplicates) { - MessageDictionary& global = MessageDictionary::globalDictionary(); + const MessageDictionaryPtr& global = MessageDictionary::globalDictionary(); - for (size_t i = 0; i < getIndex(); ++i) { - std::vector repeats = global.load(logger_values[i]); + for (LoggerValuesList::const_iterator values = logger_values.begin(); + values != logger_values.end(); ++values) { + std::vector 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& duplicates = getNonConstDuplicates(); + std::list& duplicates = getNonConstDuplicates(); duplicates.insert(duplicates.end(), repeats.begin(), repeats.end()); } @@ -91,11 +115,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& +const std::list& MessageInitializer::getDuplicates() { return (getNonConstDuplicates()); } diff --git a/src/lib/log/message_initializer.h b/src/lib/log/message_initializer.h index ae67484600..a742ea238a 100644 --- a/src/lib/log/message_initializer.h +++ b/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 +#include #include +#include #include -#include -#include 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& getDuplicates(); + static const std::list& 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 diff --git a/src/lib/log/tests/logger_manager_unittest.cc b/src/lib/log/tests/logger_manager_unittest.cc index c3d3579e53..e3ca27290e 100644 --- a/src/lib/log/tests/logger_manager_unittest.cc +++ b/src/lib/log/tests/logger_manager_unittest.cc @@ -32,6 +32,7 @@ #include #include #include +#include #include #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()); +} diff --git a/src/lib/log/tests/message_dictionary_unittest.cc b/src/lib/log/tests/message_dictionary_unittest.cc index 065641a649..33e7234648 100644 --- a/src/lib/log/tests/message_dictionary_unittest.cc +++ b/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); } diff --git a/src/lib/log/tests/message_initializer_1_unittest.cc b/src/lib/log/tests/message_initializer_1_unittest.cc index 761231d072..7171a88495 100644 --- a/src/lib/log/tests/message_initializer_1_unittest.cc +++ b/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 #include #include +#include #include #include @@ -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 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(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 unregistsred. + 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()); diff --git a/src/lib/log/tests/message_initializer_2_unittest.cc b/src/lib/log/tests/message_initializer_2_unittest.cc index 8cc78fade1..f12ec0191c 100644 --- a/src/lib/log/tests/message_initializer_2_unittest.cc +++ b/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 > 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 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