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

 * owner:  stephen => jinmei


Comment:

 >> 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.
 In the original solution, the reference to the external symbol and the
 declaration of the !MessageInitiator were in the header file, so were
 included in files that would be pulled into the link for other reasons.
 Although the compiler's optimiser would have probably removed the external
 reference had it been there alone (as there would be nothing in the module
 that referred to it), passing it as an argument to the !MessageInitiator -
 which the optimiser could not remove as the declaration will cause the
 running of its constructor which is in another module - should mean that
 it would remain.  So the module containing the #include should contain a
 reference to the initialization object, which should cause it to be pulled
 in.

 >> 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?
 I think it's unlikely under Unix, but possible under Windows Visual
 Studio.  The latter contains a "Whole program optimization" switch that
 defers the optimization to link time.  However I would have thought that
 the optimization would detect that !MessageInstantiator refers to
 !MessageInitializer whose constructor invokes !MessageDictionary which is
 referred to by other modules in the program.  It will be interesting to
 find out when we finally port BIND10 to Windows - although I suspect that
 that will be one of our lesser problems :-)


 > 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:
 A case of "more haste, less speed" when I made the changes.  Both points
 are entirely correct, and I've altered the code to use an anonymous
 namespace and a proxy function (and have updated the documentation).

 > I'd make things more cost:
 >
 > //extern const isc::log::MessageID MSG_DUPLNS = "DUPLNS";
 > extern const isc::log::MessageID const MSG_DUPLNS = "DUPLNS";
 Not needed.  MessageID is typedef'ed to "const char*".

 > const isc::log::!MessageInitializer initializer(values);
 Good catch, done.

 > The comment line isn't accurate: There's now no root logger declaration.
 Corrected.

 > 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.
 In logger_support_test.cc I've moved one of them inside main().  This will
 be another check that the logger works at both function and namespace
 level.

 > the call to (isc::log::)init() seems to suggest the name is too generic,
 especially when we
 > omit the namespace:
 > :
 >  While it could sound redundant, I'll be a bit more verbose, e.g.
 initLogger()
 Changed.

 > why does Logger have virtual functions? It doesn't seem to be (intended
 to be/become) a base
 > class.
 For flexibility, e.g. someone may want to intercept or modify logger
 messages (or a subset of them) for some reason or another, which they can
 do by using a derived class.  When we come to doing a programme of
 comprehensive performance improvement, we can remove the virtual keyword
 if there are no derived classes.

 > 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);
 My preference is to keep it as it does perform small bit of of
 documentation and prevents the function call from being excessively long.

 > 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 vsnprint
 > f() 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');
 For the format supplied, 32 is the next power of two above the 20
 characters needed for the string.  The trailing nulls are, I'm afraid, a
 hangover of long years of "C" programming with "strncpy" which doesn't
 guarantee a trailing null - I got into the habit of terminating strings
 with a null, "just in case".

 I've removed the addition of the training nulls.

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