BIND 10 trac1698-test, updated. 4e7ace34dbde7f41d474fc93a0bee27361b9651d [1698-test] Avoid static initialization fiasco for MessageInitializer
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Mar 7 14:07:12 UTC 2012
The branch, trac1698-test has been updated
via 4e7ace34dbde7f41d474fc93a0bee27361b9651d (commit)
via 21a0d06de02275f51776d1177c7b64ababc2489a (commit)
from d7fb4b7244e60a034e21873d5bd32f7148ccd973 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 4e7ace34dbde7f41d474fc93a0bee27361b9651d
Author: Stephen Morris <stephen at isc.org>
Date: Wed Mar 7 14:02:14 2012 +0000
[1698-test] Avoid static initialization fiasco for MessageInitializer
Statically-declared MessageInitializer objects now store the address
of the message array in a pre-defined array of pointers. Loading
the global message dictionary is done after main() is called. This
avoids used of heap storage during static initialization, which
appears to cause problems on some systems.
commit 21a0d06de02275f51776d1177c7b64ababc2489a
Author: Stephen Morris <stephen at isc.org>
Date: Wed Mar 7 13:15:00 2012 +0000
[1698-test] Avoid use of heap storage for logger name
Logger now stores its name in a pre-defined array instead of
using heap storage (through a std::string). This should avoid
static initialization fiasco problems if the Logger is statically
initialized.
-----------------------------------------------------------------------
Summary of changes:
src/lib/log/logger.h | 26 ++++++++--
src/lib/log/logger_manager.cc | 4 ++
src/lib/log/message_initializer.cc | 67 ++++++++++++++++++++++---
src/lib/log/message_initializer.h | 34 ++++++++++----
src/lib/log/tests/logger_example.cc | 2 +-
src/lib/log/tests/logger_manager_unittest.cc | 8 ++--
src/lib/log/tests/logger_unittest.cc | 4 +-
7 files changed, 117 insertions(+), 28 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/log/logger.h b/src/lib/log/logger.h
index 96168c0..de74563 100644
--- a/src/lib/log/logger.h
+++ b/src/lib/log/logger.h
@@ -15,14 +15,17 @@
#ifndef __LOGGER_H
#define __LOGGER_H
+#include <cassert>
#include <cstdlib>
#include <string>
+#include <cstring>
#include <exceptions/exceptions.h>
#include <log/logger_level.h>
#include <log/message_types.h>
#include <log/log_formatter.h>
+
namespace isc {
namespace log {
@@ -99,6 +102,9 @@ public:
class Logger {
public:
+ enum {
+ MAX_LOGGER_NAME_SIZE = 32 ///< Maximum size of logger name
+ };
/// \brief Constructor
///
@@ -107,8 +113,20 @@ public:
/// \param name Name of the logger. If the name is that of the root name,
/// this creates an instance of the root logger; otherwise it creates a
/// child of the root logger.
- Logger(const std::string& name) : loggerptr_(NULL), name_(name)
- {}
+ ///
+ /// \note The name of the logger may be no longer than MAX_LOGGER_NAME_SIZE
+ /// else the program will halt with an assertion failure. This restriction
+ /// allows loggers to be declared statically: the name is stored in a fixed-
+ /// size array to avoid the need to allocate heap storage during program
+ /// initialization (which causes problems on some operating systems).
+ ///
+ /// \note Note also that there is no constructor taking a std::string. This
+ /// minimises the possibility of initializing a static logger with a string,
+ /// so leading to problems mentioned above.
+ Logger(const char* name) : loggerptr_(NULL) {
+ assert(std::strlen(name) < sizeof(name_));
+ std::strcpy(name_, name);
+ }
/// \brief Destructor
virtual ~Logger();
@@ -256,8 +274,8 @@ private:
/// \brief Initialize Underlying Implementation and Set loggerptr_
void initLoggerImpl();
- LoggerImpl* loggerptr_; ///< Pointer to the underlying logger
- std::string name_; ///< Copy of the logger name
+ LoggerImpl* loggerptr_; ///< Pointer to underlying logger
+ char name_[MAX_LOGGER_NAME_SIZE]; ///< Copy of the logger name
};
} // namespace log
diff --git a/src/lib/log/logger_manager.cc b/src/lib/log/logger_manager.cc
index a04fffb..d5dfe7f 100644
--- a/src/lib/log/logger_manager.cc
+++ b/src/lib/log/logger_manager.cc
@@ -95,6 +95,10 @@ void
LoggerManager::init(const std::string& root, isc::log::Severity severity,
int dbglevel, const char* file)
{
+ // Load in the messages declared in the program and registered by
+ // statically-declared MessageInitializer objects.
+ MessageInitializer::loadDictionary();
+
// Save name, severity and debug level for later. No need to save the
// file name as once the local message file is read the messages will
// not be lost.
diff --git a/src/lib/log/message_initializer.cc b/src/lib/log/message_initializer.cc
index 0113497..62037ef 100644
--- a/src/lib/log/message_initializer.cc
+++ b/src/lib/log/message_initializer.cc
@@ -12,25 +12,76 @@
// 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>
+
+// As explained in the header file, initialization of the message dictionary
+// is a two-stage process:
+// 1) In the MessageInitializer constructor, the a pointer to the array of
+// messages is stored in a pre-defined array. Since the MessageInitializers
+// are declared external to a program unit, this takes place before main()
+// is called. As no heap storage is allocated in this process, we should
+// avoid the static initialization fiasco in cases where initialization
+// of system libraries is also carried out at the same time.
+// 2) After main() starts executing, loadDictionary() is called.
+//
+//
+
+namespace {
+
+// Declare the array of pointers to value arrays. At the time of writing
+// (end of BIND 10's third year of development), only 25 such arrays have
+// been defined, so a value of 64 should allow for significant expansion.
+// (Besides, an assertion will be triggered if the array overflows.)
+const int MAX_LOGGERS = 64;
+const char** logger_values[MAX_LOGGERS];
+
+// 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);
+}
+
+}
+
+
namespace isc {
namespace log {
-// Constructor. Just retrieve the global dictionary and load the IDs and
-// associated text into it.
+// 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_LOGGERS);
+ logger_values[getIndex()++] = values;
+}
+
+// Load the messages in the arrays registered in the logger_values array
+// into the global dictionary.
+
+void
+MessageInitializer::loadDictionary() {
MessageDictionary& global = MessageDictionary::globalDictionary();
- 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 (!repeats.empty()) {
- std::vector<std::string>& duplicates = getDuplicates();
- duplicates.insert(duplicates.end(), repeats.begin(), repeats.end());
+ for (size_t i = 0; i < getIndex(); ++i) {
+ std::vector<std::string> repeats = global.load(logger_values[i]);
+
+ // Append the IDs in the list just loaded (the "repeats") to the
+ // global list of duplicate IDs.
+ if (!repeats.empty()) {
+ std::vector<std::string>& duplicates = getDuplicates();
+ duplicates.insert(duplicates.end(), repeats.begin(), repeats.end());
+ }
}
+
+ // ... 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;
}
// Return reference to duplicate array
diff --git a/src/lib/log/message_initializer.h b/src/lib/log/message_initializer.h
index 7516823..f2ff40b 100644
--- a/src/lib/log/message_initializer.h
+++ b/src/lib/log/message_initializer.h
@@ -37,28 +37,44 @@ namespace log {
/// :
/// NULL
/// };
-/// MessageDictionaryHelper xyz(values);
+/// MessageInitializer xyz(values);
///
-/// This will automatically add the message ID/text pairs to the global
-/// dictionary during initialization - all that is required is that the module
-/// containing the definition is included into the final executable.
+/// All that needed is for the module containing the definitions to be
+/// included in the execution unit.
///
-/// Messages are added via the MessageDictionary::add() method, so any
-/// duplicates are stored in the the global dictionary's overflow vector whence
-/// they can be retrieved at run-time.
+/// To avoid static initialization fiasco problems, the initialization is
+/// carried out in two stages:
+/// -# The constructor a pointer to the values array to a pre-defined array
+/// of pointers.
+/// -# During the run-time initialization of the logging system, the static
+/// method loadDictionary() is called to load the message dictionary.
+/// This way, no heap storage is allocated during the static initialization,
+/// something that may give problems on some operating systems.
+///
+/// When messages are added to the dictionary, the are added via the
+/// MessageDictionary::add() method, so any duplicates are stored in the the
+/// global dictionary's overflow vector whence they can be retrieved at
+/// run-time.
class MessageInitializer {
public:
/// \brief Constructor
///
- /// Adds the array of values to the global dictionary, and notes any
- /// duplicates.
+ /// Adds a pointer to the array of messages to the global array of
+ /// pointers to message arrays.
///
/// \param values NULL-terminated array of alternating identifier strings
/// and associated message text.
MessageInitializer(const char* values[]);
+ /// \brief Run-Time Initialization
+ ///
+ /// Loops through the internal array of pointers to message arrays
+ /// and adds the messages to the internal dictionary. This is called
+ /// during run-time initialization.
+ static void loadDictionary();
+
/// \brief Return Duplicates
///
/// When messages are added to the global dictionary, any duplicates are
diff --git a/src/lib/log/tests/logger_example.cc b/src/lib/log/tests/logger_example.cc
index 2170066..d3f08f3 100644
--- a/src/lib/log/tests/logger_example.cc
+++ b/src/lib/log/tests/logger_example.cc
@@ -105,7 +105,7 @@ void usage() {
// messages. Looking at the output determines whether the program worked.
int main(int argc, char** argv) {
- const string ROOT_NAME = "example";
+ const char* ROOT_NAME = "example";
bool sw_found = false; // Set true if switch found
bool c_found = false; // Set true if "-c" found
diff --git a/src/lib/log/tests/logger_manager_unittest.cc b/src/lib/log/tests/logger_manager_unittest.cc
index 9eb85ad..f2713b0 100644
--- a/src/lib/log/tests/logger_manager_unittest.cc
+++ b/src/lib/log/tests/logger_manager_unittest.cc
@@ -201,7 +201,7 @@ TEST_F(LoggerManagerTest, FileLogger) {
// Scope-limit the logger to ensure it is destroyed after the brief
// check. This adds weight to the idea that the logger will not
// keep the file open.
- Logger logger(file_spec.getLoggerName());
+ Logger logger(file_spec.getLoggerName().c_str());
LOG_FATAL(logger, LOG_DUPLICATE_MESSAGE_ID).arg("test");
ids.push_back(LOG_DUPLICATE_MESSAGE_ID);
@@ -223,7 +223,7 @@ TEST_F(LoggerManagerTest, FileLogger) {
manager.process(spec.begin(), spec.end());
// Create a new instance of the logger and log three more messages.
- Logger logger(file_spec.getLoggerName());
+ Logger logger(file_spec.getLoggerName().c_str());
LOG_FATAL(logger, LOG_NO_SUCH_MESSAGE).arg("test");
ids.push_back(LOG_NO_SUCH_MESSAGE);
@@ -275,7 +275,7 @@ TEST_F(LoggerManagerTest, FileSizeRollover) {
// three files as for the log4cplus implementation, the files appear to
// be rolled after the message is logged.
{
- Logger logger(file_spec.getLoggerName());
+ Logger logger(file_spec.getLoggerName().c_str());
LOG_FATAL(logger, LOG_NO_SUCH_MESSAGE).arg(big_arg);
LOG_FATAL(logger, LOG_DUPLICATE_NAMESPACE).arg(big_arg);
}
@@ -295,7 +295,7 @@ TEST_F(LoggerManagerTest, FileSizeRollover) {
// a .3 version does not exist.
manager.process(spec);
{
- Logger logger(file_spec.getLoggerName());
+ Logger logger(file_spec.getLoggerName().c_str());
LOG_FATAL(logger, LOG_NO_MESSAGE_TEXT).arg(big_arg);
}
diff --git a/src/lib/log/tests/logger_unittest.cc b/src/lib/log/tests/logger_unittest.cc
index edca9ce..a4a4e62 100644
--- a/src/lib/log/tests/logger_unittest.cc
+++ b/src/lib/log/tests/logger_unittest.cc
@@ -58,8 +58,8 @@ TEST_F(LoggerTest, Name) {
TEST_F(LoggerTest, GetLogger) {
- const string name1 = "alpha";
- const string name2 = "beta";
+ const char* name1 = "alpha";
+ const char* name2 = "beta";
// Instantiate two loggers that should be the same
Logger logger1(name1);
More information about the bind10-changes
mailing list