BIND 10 #1964: regressions due to --enable-logger-checks

[BIND 10 Development]

#1964: regressions due to --enable-logger-checks
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120515
                   Priority:  very   |            Resolution:
  high                               |             Sensitive:  0
                  Component:         |           Sub-Project:  Core
  logging                            |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  UnAssigned => jinmei


Comment:

 Hi jinmei

 Replying to [ticket:1964 jinmei]:
 > This feature added in #1892 caused at least a couple of troubles.
 > The code also seems to have other potential issues.
 >
 > First, it breaks compile when the check is disabled due to the unused
 > function:
 >
 http://git.bind10.isc.org/~tester/builder/BIND10-systest/20120508204501-MacOS/logs/build.out
 >
 > Second, a related unit test fails at least on some environments
 > including mine:
 > {{{
 > [ RUN      ] FormatterTest.mismatchedPlaceholders
 > log_formatter_unittest.cc:117: Failure
 > Death test: { isc::testutils::dontCreateCoreDumps();
 Formatter(isc::log::INFO, s("Too many arguments in %1 %2"),
 this).arg("only one"); }
 >     Result: illegal return in test statement.
 >  Error msg:
 > [  FAILED  ] FormatterTest.mismatchedPlaceholders (4474 ms)
 > }}}
 >

 The way we use log strings is such that we can only detect broken cases
 (after all `.arg()` calls are processed) in the destructor. Before that,
 every formatter can have a valid format string still.
 Throwing from the destructor is known, but that caused a abort in our
 environments which was checked in the unit tests. It seems that this is
 not always the case.


 > Third, the way dontCreateCoreDumps() is implemented is broken:
 > - we must not use an unnamed name space in a publicly shared header
 >   file.

 It used a static function for it initially, and the unnamed namespace was
 committed during the review. It's less than clean, but putting it in
 testutils brought a circular library dependency issue (see comments in
 #1892).

 > - liblog is a quite primitive library while testutils are generally
 >   considered utilities for a more advanced modules such as data source
 >   or the auth server.  It's probably better to this set up in
 >   lib/util/unittests

 *nod*. I didn't see `lib/util/unittests`, after finding testutils.

 > Combining these, I'd suggest declaring this function in a normal named
 > namespace in lib/util as a non inline function and defining it in a
 > separate .cc file.  It will also work as a solution to the
 > unused-function problem.

 I've checked the code changes you have made and they look fine.

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