BIND 10 #1698: log initialization causes real fiasco for MacOS 10.7

[BIND 10 Development]

#1698: log initialization causes real fiasco for MacOS 10.7
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120320
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  Core
  logging                            |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''general'''

 The proposed solution basically seems to be reasonable.  But I'd
 suggest increasing MAX_LOGGERS to even much larger value because it's
 a hard limit.  Maybe something like 1024, and the required memory
 footprint is still just 8KB for 64-bit machines.  I'd also suggest
 this parameter can be modified at least at compile time.  And I'd
 suggest documenting this restriction (and possible way of extension)
 in the header file.

 '''changelog'''

 Maybe "Mac OS (X) 10.7" instead of just "OS X"?

 '''logger.h'''

 - Does MAX_LOGGER_NAME_SIZE have to be public?

 - Why did you use the enum hack for it?  I thought we normally simply
   use a static const member variable in such cases.

 - Ideally we should do a death test to trigger the assertion in the
   `Logger` constructor.  At least I'd add a test for the possible
   longest log name.

 - This note sounds a bit ambiguous:
 {{{#!c++
     /// \note The name of the logger may be no longer than
 MAX_LOGGER_NAME_SIZE
 }}}
   Does the "name" include the terminating nul character?  According to
   the condition for assert, it does, and then this statement is
   correct, but I guess this statement would normally be interpreted as
   "assert(strlen(name) <= MAX_LOGGER_NAME_SIZE)".  To be super clear
   I'd rephrase it to: "The name of the logger (including the
   terminating NUL character) may be no longer than
   MAX_LOGGER_NAME_SIZE".

 - While this strcpy is actually safe, I'd be even redundantly paranoid
   when we deal with this type of low-level dangerous API, e.g.:
 {{{#!c++
     Logger(const char* name) : loggerptr_(NULL) {
         assert(std::strlen(name) < sizeof(name_));
         std::strncpy(name_, name, sizeof(name_));
         // strncpy ensures the following condition as long as name is
 valid:
         assert(name_[sizeof(name_) - 1] == '\0');
     }
 }}}

 '''message_initializer.h'''

 - This comment doesn't parse:
 {{{#!c++
 /// -# The constructor a pointer to the values array to a pre-defined
 array
 ///    of pointers.
 }}}
   "The constructor *adds* a pointer..."?

 - `MessageInitializer` constructor: If I understand it correctly, the
   caller must ensure that `values` array be valid at least until
   loadDictionary() is called (so for example it cannot be a temporary
   array defined within some function).  In practice a misuse is
   probably unlikely to happen, but I think we should note that
   explicitly.

 '''message_initializer.cc'''

 - typo: s/the a/a/?
 {{{#!c++
 // 1) In the MessageInitializer constructor, the a pointer to the array of
 }}}
 - nit: I don't thinks this is correct:
 {{{#!c++
 //    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
 }}}
   This is not because `MessageInitializer` objects have external
   linkage (btw they actually don't even have external linkage because
   they are defined as const), but because they are initialized at a
   namespace level (i.e., "statically" in a translation unit).

 - Why is MAX_LOGGERS an int?  I think size_t is better for this
   purpose:
 {{{#!c++
 const int MAX_LOGGERS = 64;
 }}}

 - Like MAX_LOGGER_NAME_SIZE, I'd add a test with a max number of
   initializers, and ideally a death test when they exceed the limit

 '''misc'''

 I made a few minor editorial changes directly to the branch.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1698#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development