BIND 10 #2524: Add logging to files in libdhcpsrv

[BIND 10 Development]

#2524: Add logging to files in libdhcpsrv
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:
                Type:  task          |  marcin
            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 stephen):

 * owner:  stephen => marcin


Comment:

 > '''lease_mgr_factory.cc'''
 > LeaseMgrFactory::parse: Are extra space characters surrounding string()
 added on purpose:
 No - removed.

 > There is no need to initialize the access object explicitly to an empty
 string as the constructor does that anyway.
 True, but explicitly initialising it to an empty string clearly indicates
 what the value is before it enters the loop.

 > 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.
 The first time through the loop, "access" is empty.  Hence
 "!access.empty()" is false, and the space is not appended.  On the second
 and subsequent passes, "access" contains one or more "keyword=value"
 tokens. so the intermediate space is added.


 > '''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...
 Fixed.

 > 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");
 Moved so that the "=" and everything after it is no a new line.  (The idea
 is to avoid breaking up the argument string.)

 > '''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}?
 Yes - corrected.

 > '''src/lib/dhcpsrv/dhcpsrv_log.cc'''
 > '''src/lib/dhcpsrv/dhcpsrv_log.h'''
 > The date in the copyright header is wrong. Should be 2012
 Fixed.

 > Can you explain NSAS?
 !NameServer Address Store: I copied the files from src/lib/nsas and edited
 them.

 > '''src/lib/dhcpsrv/hwaddr.h'''
 > We don't use underscores in things like:
 > #ifdef !__HWADDR_H
 Fixed (I was using an old template).

 > 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.
 The comment was a hangover from an earlier version, where only the first
 six characters of the converted hardware address were returned.  This
 version of the function converts the entire hardware address, so the
 comment is redundant and has been removed.

 > '''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.
 True - these tests were not part of the changes of this ticket, but they
 have been changed.

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