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      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 > 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.
 The limit of 64 was chosen on the basis that it's taken three years of
 development to get up to 26 messages files so it is unlikely that we will
 significantly increase that number in the remaining life of BIND 10.  I've
 renamed the variable to MAX_MESSAGE_ARRAYS (see below) and upped the value
 to 256 (10 times what we have now) as a compromise between future
 expansion and wasted space.

 > 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.
 Done.  I've also documented the restriction on logger name length (see
 below).

 > Maybe "Mac OS (X) 10.7" instead of just "OS X"?
 I'll use "Mac OS X" (as used in the Wikipedia entry) as I don't think any
 literature places the "X" in brackets.

 > Does MAX_LOGGER_NAME_SIZE have to be public?
 It can be private, but I've left it as public for cases where a logger
 name is constructed and run-time and the logger created dynamically.  It
 allows a check to be made on the name and the caller to take action if the
 name is too long instead of blindly triggering an assertion failure. (It's
 also useful to know this length in the test code.)

 > Why did you use the enum hack for it? I thought we normally simply use a
 static const member variable in such cases.
 I'm not up to date with such modern thinking :-)  Changed.

 > Does the "name" include the terminating nul character?
 It did but when I looked at it again I realised that the name of the
 symbol was incorrect.  MAX_LOGGER_NAME_SIZE is now the maximum length of
 the name excluding the trailing null; the name_ member is declared with a
 length of "MAX_LOGGER_NAME_SIZE + 1".

 > 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.:
 It does no harm, so I've changed it as you suggested.

 > 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.
 Done.

 > This comment doesn't parse
 Isn't it amazing how what you type often bears no relationship to what you
 think you've typed? :-) Corrected.

 > !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.
 Good point, comment added.

 > typo: s/the a/a/?
 Corrected.

 > I don't think this is correct:
 I used the term "external" to mean "outside" and not "external linkage",
 but I can see that this is confusing. (I'm  trying to distinguish between
 a static variable declared inside a function, which is initialised when
 the function is called and one declared outside a function, which is
 initialized before main() is called.)  I've rewritten the comment to
 remove the ambiguity.


 > Why is MAX_LOGGERS an int? I think size_t is better for this purpose:
 Agreed - changed.

 I've changed the name of the constant to MAX_MESSAGE_ARRAYS as better
 describing its purpose and, for the purposes of testing, made it public
 within the !MessageInitializer class.

 > 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
 Done.  As part of this I needed to add a getPendingCount() method and
 split the !MessageInitializer tests into separate programs to avoid one
 test affecting the starting conditions of the next.

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