BIND 10 #558: Disable log4cxx dependency in logging API

[BIND 10 Development]

#558: Disable log4cxx dependency in logging API
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jelte
                     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:12 stephen]:

 > > Apart from this, the code looks ok, btw :) (though perhaps Jinmei
 wants to have a quick
 > > look at the changes for the init problem)
 > I'll pass it to Jinmei for his comments.

 Okay, I've taken a closer look at the code, focusing on:
  - the initialization fiasco for RootLoggerName
  - initialization of the default messages (messagedef.h)

 As for RootLoggerName, the original problem seems to be solved.  But I
 now wonder whether it has to be a class.  Unlike the Logger class or
 messages, this must be explicitly initialized in the application (such
 as b10-xxx) in some way, right?  And since this is done in the
 application (which should have main()), we can control the
 initialization timing.  So, in practice, it should be sufficient to
 provide a set of free functions like "setRootLoggerName()" (which
 would do the same thing as the RootLoggerName constructor.  The
 function based interface is more restrictive, but the behavior will be
 more intuitive and provide more predictable behavior, which I would
 rather prefer in this case.

 As for the changes to messagedef.h, I have several questions.

  1. is it really guaranteed that the symbol for the MessageInitializer
     object remains?  It's statically detectable that
     instantiate_messagedef_xxx is never used and its construction
     is actually no-op, so I could imagine some aggressive linker
     optimizes the resulting code by stripping instantiate_messagedef_xxx,
     and then stripping messagedef_xxx.  Or is there any standard that
     precludes this scenario?

  2. Even if this approach is guaranteed to work, I don't see the need
     for defining the static variable in the header file (which is in
     general not a good practice because a copy of that object will be
     created in all translation units that include this header file).
     My experiment indicates it should be sufficient if we create
     another .cc that has the following code:
 {{{
 namespace {
 const isc::log::MessageInstantiator initiator(
     &isc::log::messagedef_cc_Tue_Feb__8_18_01_54_2011);
 }
 }}}

   3. not directly related to the main topic, but I'm concerned about
      the static MessageID variables defined in messagedef.h (such as
      MSG_DUPLNS), too.  Each translation unit that includes
      messagedef.h  will have a copy of MSG_xxx variables, and,
      depending on the compiler/linker optimization there may be copies
      of the defined strings (such as "DUPLNS").  Is there any reason
      that we cannot declare these MSG_xxx as extern in messagedef.h
      and define only a single instance of each of them in
      messagedef.cc?  In my quick experience it seems to be possible.
      This way we can ensure each MSG_xxx symbol is unique within the
      program, and we can possibly compare these IDs by addresses,
      which should be much faster than comparison as strings.

 Other random comments, mostly off topic in this context but I happened
 to notice in reviewing the relevant part:

  - I've made some trivial editorial/style fixes directly on the branch
    and pushed the changes.
  - I don't (yet) fully understand the expected usage, but is each app
    (e.g. b10-auth) and (some of) BIND 10 library (e.g. libdns++) is
    expected to create its own message definition and register it via
    MessageInitializer?  If so, are these specific definitions expected
    to override the default MSG_xxx defined in lib/log/messagedef.h or
    expected to be different sets of IDs?  If it's the latter, should
    we worry about the current default definition (DUPLNS, etc) can
    cause conflict?
  - It seems the return value of RootLoggerName::getName() could be
    'const string&', and it would be a bit (or much, depending on the
    std::string implementation) faster.
  - same comment applies to MessageDictionary::getText()
  - do we have performance consideration? Logger::xxx() methods seem to
    involve expensive string comparison and many object copies and may
    have substantial performance impact.
  - why does MessageDictionary::globalDictionary() return a pointer
    instead of a reference?  In general when we can ensure it can never
    be a NULL pointer (and in this case we can) it would be better to
    use a reference, because then we don't have to worry about one
    corner case where a null pointer is returned by some accident or
    bug.
  - in logger.cc, I'd try to avoid relying on macro.  At least it seems
    to be achieved easily for MESSAGE_SIZE (which could be a const
    size_t in an unnamed namespace).  I see some difficulty in
    FORMAT_MESSAGE(), but Logger::xxx methods already have some
    duplicate, so I'd rather push the va_xxx stuff in each method and
    avoid relying on macro.
  - minor editorial nit: some of the copyright years may be incorrect.
    Shouldn't all these be 2011?

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