BIND 10 #736: Implement logging configuration

[BIND 10 Development]

#736: Implement logging configuration
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110614
                   Priority:         |            Resolution:
  critical                           |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  logging                            |  Estimated Difficulty:  7.0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => stephen


Comment:

 two commits, one for the minor issues, one for the change to 'output' for
 facility/file/stream, for the other points see below

 Replying to [comment:10 stephen]:
 > Reviewed commit 542a15d604018ea73a5a28f26b30b92fb668e399
 >
 > '''src/bin/cfgmgr/plugins/b10logging.py'''
 > As each logger should have a name, perhaps it would be useful to include
 its name in any error message?
 >

 ack, done

 > The check for "filename" occurs whether or not the destination is a file
 (contrary to what the error message says).
 >
 > There could be a check that if "facility" is present, the output is to
 syslog.
 >

 since there is now just the 'output', these issues are either changed or
 irrelevant now.

 > Question: Although the logging code makes the distinction between
 "file", "console" and "syslog", is there a need for the user-visible
 configuration to do so?  Could we not remove the "console" destination and
 have it that file output to "stdout" or "stderr" routes output to the
 console?  (In fact, how about replacing "filename", "stream" and
 "facility" with "output", and interpret the value of "output" in a
 destination-specific way?)
 >

 Ok. This is what I initally meant with compacting the options, but I had
 chosen to make it a one-to-one mapping first. This prompted me to indeed
 change it. stream, facility and filename are now gone, and replaced by the
 one setting 'output'. Since destination defaults to console, output
 defaults to stdout.

 note that you must delete your b10-config.db or remove the loggers part if
 you have one, this is an incompatible change and we don't have module-
 specific versioning (yet, but even if we did i probably wouldn't up it for
 a non-merged change).

 >
 > '''src/lib/config/ccsession.h'''
 > ModuleCCSession constructor: the documentation for the arguments
 "command_handler" and "start_immediately" is not in the correct place in
 the header.
 >

 moved

 > Is the "handle_logging" argument to the ModuleCCSession constructor a
 temporary construct?  (Under what circumstances will this module not take
 care of logging configuration?)
 >

 Not temporary, but backwards compatible, and 'future-compatible' for
 modules that may not want logging to be handled (if any). Right now auth
 doesn't do logging yet. We can change the default to true, or perhaps
 remove the option not to do it completely though, if there is a preference
 for that.

 >
 > '''src/lib/config/config_data.cc'''
 > findListOrMapSubSpec: Is, strictly speaking, the header correct?  It
 finds the inner-most _spec if the specification is nested.
 >

 Added comment to this effect.

 > findItemInSpecList: (throwing an exception when nothing is found.)
 Presumably not finding the element is an unexpected occurrence?  Does this
 make the handling of optional elements more awkward?  Would returning a
 null pointer be better?
 >

 No, this function should only get specification data. We do not have
 optional specification data, so if there is an identifier that cannot be
 found, either the code calling it or the specification is wrong.

 > find_spec_part: not in the changes made here, but would use of tokens()
 (in util/strutil.{h,cc}) simplify the code?
 >

 Perhaps, there is the slight detail that on the last item it needs to do
 something else than for the ones before that. I'll take a look at that
 (but not now).

 > Comment: again not in the changes here, but some of the functions in the
 file are either in the anonymous namespace or are declared static.  This
 means they do not have unit tests.

 Yep. It also means we won't accidentally be using them outside of this
 scope, which, if i look at your previous comments, may be a very good
 thing :) They are so extremely specific to the one function that calls
 them, I honestly don't know how much value they would be for other parts.
 I do realize that this is not very TDD, but:

 On a more general note, I think we are running against the limits of using
 'direct' data types for both config values and their specifications (the
 ElementPtrs in c++ and the native types in python). This was a cute idea
 IMO, since we can easily pass these around over any channel, but their
 handling is getting annoying. So I'm thinking about making a real class
 hierarchy for these. Anonymously namespaced helper functions should then
 be much less necessary (if at all). I do want to give a bit of design
 thought to this though, and certainly don't want to implement that as a
 side-effect of any other ticket such as this.

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