BIND 10 #558: Disable log4cxx dependency in logging API

[BIND 10 Development]

#558: Disable log4cxx dependency in logging API
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:  major    |            Milestone:  R-Team-
                Component:  logging  |  Sprint-20110222
                 Keywords:           |           Resolution:
Estimated Number of Hours:  2.0      |            Sensitive:  0
                Billable?:  1        |  Add Hours to Ticket:  0
                Internal?:  0        |          Total Hours:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:14 stephen]:

 Comments about the main topic first:

 {{{
 namespace {
 const isc::log::!MessageInstantiator initiator(
     &isc::log::messagedef_cc_Tue_Feb__8_18_01_54_2011);
 }
 }}}

 > If there is no reference to "initiator" in the code, what causes the
 linker to pull in this file? (But see below.)
 >

 The same question applies to your original solution, and that's
 exactly why I asked the question in my previous comment.  This
 approach is effective only if the call to the constructor with the
 reference to the mesagedef_... symbol isn't removed.  I didn't
 figure out how exactly this one worked.  I guess it was because it
 happened in a different translation unit, but I was not sure about
 that, and I was not sure whether it's always guaranteed.

 > It also solves the problem of the referencing of the !MessageInitializer
 - that can be in the same file as the definitions of the the message
 symbols.  Since the file will be included at link time to resolve the
 message symbol references, the !MessageInitializer object will also be
 included.

 I'm still not sure if it's always guaranteed.  Isn't it possible that
 the linker detect we actually don't need "initializer" and removes the
 corresponding code as optimization?

 The rest of the comments are something I happened to notice.  It's not
 comprehensive review, but I didn't think I was requested for it.

 '''logging performance'''
 >> do we have performance consideration? Logger::xxx() methods seem to
 involve expensive string
 >> comparison and many object copies and may have substantial performance
 impact.
 > Isn't this premature optimisation? :-)

 Actually, I didn't worry about performance at this stage.  I simply
 wondered whether there was any consideration was given, which could be
 "this version is intended to provide necessary stuff for the purpose
 of year2 deliverable and doesn't care about performance yet".  As long
 as we understand the current (possible) limitation about performance
 and have a plan to revisit it later, I'm okay with that.

 '''root_logger_name.cc'''
  - regarding root_name_: to be pedantic, namespace level static is
    deprecated in C++.  I would use an unnamed namespace (but read the
    next bullet first).  I'd also remove the trailing _ from the
    variable name as it's not a member variable of a class.
  - the use of namespace level static object (root_name_) opens up the
    possibility of initialization fiasco again, even though it's much
    less likely to happen (consider, e.g. the case where another static
    object calls getRootLoggerName() in its constructor).  One
    compromise would be to note the restriction in the documentation
    (such as get/setRootLoggerName() shouldn't be used inside a
    constructor of a class).  We could also use a proxy function like:
 {{{
 namespace {
 std::string&
 getRootLoggerNameInternal() {
     static std::string root_name;
     return (root_name);
 }
 }
 }}}
   which would probably be better.
  - getRootLoggerName is not documented (in the .h)

 '''messagedef.cc'''

 I'd make things more cost:

 {{{
 //extern const isc::log::MessageID MSG_DUPLNS = "DUPLNS";
 extern const isc::log::MessageID const MSG_DUPLNS = "DUPLNS";
 ...
 ///isc::log::MessageInitializer initializer(values);
 const isc::log::MessageInitializer initializer(values);
 }}}

 '''logger_support_test.cc'''
  - The comment line isn't accurate: There's now no root logger
 declaration.
 {{{
 // Declare root logger and a loggers to use an example.
 Logger logger_ex("example");
 Logger logger_dlm("dlm");
 }}}
  - also, does logger_xxx have to be in the namespace level?  if not,
    I'd move them inside main().  it would generally be better to use
    narrower scope.
  - the call to (isc::log::)init() seems to suggest the name is too
    generic, especially when we omit the namespace:
 {{{
     // Update the logging parameters
     init("alpha", severity, dbglevel, localfile);
 }}}
    While it could sound redundant, I'll be a bit more verbose, e.g.
    initLogger()

 '''logger.{h,cc}'''
  - why does Logger have virtual functions?  It doesn't seem to be
    (intended to be/become) a base class.
  - about the macro: if the concern is to have a duplicate pattern in
    derived classes, we could use a template method pattern (or C++
    template), but that would be beyond the subject of this ticket.

 '''LoggerImpl::output() in logger_impl.cc'''
  - maybe a matter of taste, but I'd avoid using a temporary variable
    "format":
 {{{
     //const string format =
 MessageDictionary::globalDictionary().getText(ident);
     //vsnprintf(message, sizeof(message), format.c_str(), ap);
     vsnprintf(message, sizeof(message),
 MessageDictionary::globalDictionary().getText(ident).c_str(), ap);
 }}}
  - in general, I don't like to override the end of a buffer with a
    nul, possibly changing the meaning of the string:
 {{{
     message[sizeof(message) - 1] = '\0';    // Guarantee trailing NULL
 ...
     chr_time[sizeof(chr_time) - 1] = '\0';  // Guarantee a trailing NULL
 }}}
    The best is to use a sufficient size of buffer so that there should
    be no need for overriding.  At least it seems we should be able to
    do that for the latter case (until year 10000).  And, in that
    sense, using a magic number of "32" doesn't seem to be good.
    Besides, since both vsnprintf() and strftime() should guarantee nul
    termination, the above code is actually redundant, too.  If we
    worry about buggy implementation of such library functions, I'd
    rather use an assertion:
 {{{
     message[sizeof(message) - 1] = '\0'
     vsnprintf(message, sizeof(message), format.c_str(), ap);
     assert(message[sizeof(message) - 1] == '\0');
 }}}

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