BIND 10 #2524: Add logging to files in libdhcpsrv

[BIND 10 Development]

#2524: Add logging to files in libdhcpsrv
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  task          |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130103
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:
                                     |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Reviewed commit b07567633570ebffebb13e92ae71d429967a7cb8

 '''lease_mgr_factory.cc'''

 LeaseMgrFactory::parse: Are extra space characters surrounding string()
 added on purpose:
 {{{
 boost::is_any_of( string("\t ") )
 }}}
 ?

 There is no need to initialize the '''access''' object explicitly to an
 empty string as the constructor does that anyway.

 LeaseMgrFactory::redactedAccessString: The space that is intended to
 separate "second and subsequent tokens" (as you stated in the comment)
 will also be inserted before the first token which is not necessary. If it
 is intended because the string is appended to some other string and the
 space is desired then the comment in the function is wrong.

 '''lease_mgr_factory_unittest.cc'''

 redactAccessString: The configuration string is wrong for the second sub-
 case which is supposed to check that the function works with empty
 password. The password is set:
 {{{
 "user=me password=forbidden name=kea type=mysql"
 }}}

 If my interpretation of the coding guidelines is correct we should put the
 argument in the same line as the curly brace -
 http://bind10.isc.org/wiki/CodingGuidelines#OpeningCurlyBracesforFunctions.
 In the code there is:
 {{{
 parameters = LeaseMgrFactory::parse(
     "user=me password=forbidden name=kea type=mysql");
 }}}

 '''src/lib/dhcpsrv/Makefile.am'''

 The comment states: ''Tell Automake that nsas_messages.{cc,h} source files
 are created ...''. Shouldn't that be ''dhcpsrv_messages.{cc,h}''?

 '''src/lib/dhcpsrv/dhcpsrv_log.cc'''

 The date in the copyright header is wrong. Should be '''2012'''

 Can you explain '''NSAS'''?

 '''src/lib/dhcpsrv/dhcpsrv_log.h'''

 The date in the copyright header is wrong. Should be '''2012'''

 '''src/lib/dhcpsrv/hwaddr.h'''

 We don't use underscores in things like:
 {{{
 #ifdef __HWADDR_H
 }}}

 The function header says that the ''six characters is an arbitrary
 length...''. If this is a minimum length (or the exact length) that should
 be picked the function should validate it.
 If any length is allowed, then the comment is misleading to me.

 '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''

 subnet4, subnet6: The checks for NULL pointer like this one:
 {{{
 EXPECT_EQ(Subnet6Ptr(), cfg_mgr.getSubnet6(IOAddress("200::123")));
 }}}

 could be simplified

 {{{
 EXPECT_FALSE(cfg_mgr.getSubnet6(IOAddress("200::123")));
 }}}

 Also, that way we avoid construction of an empty object.

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