BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121115
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpdb                             |           Sub-Project:  DHCP
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 '''dhcp6_srv.cc'''
 > @todo comments in L42 and L92: comments refer to the code without
 support for MySQL_LeaseMgr but the support is now being added so comments
 should be removed.
 Removed.

 > Dhcp6Srv ctor: Dhcp6Srv is using different database, username and
 password than MySQL_LeaseMgr. This will confuse people trying to run unit
 tests with --with-dhcp-mysql...
 Modified.  The parameters to determine what lease manager to use are now
 passed as an argument to the constructor.  The default is the memfile
 lease manager, which is used for tests.

 > Dhcp6Srv ctor: This constructor may now throw a few exceptions:
 isc::!InvalidParameter or isc::!InvalidType emitted by
 LeaseMgrFactory::create. It may also throw a bunch of exceptions from
 !MySqlLeaseMgr ctor: !DbOpenError, !DbOperationError. Whenever error
 occurs earlier in the constructor the shutdown_ member is reset (true)...
 shutdown_ now defaults to true and is only reset to false on successful
 exit from the constructor.  Also, more of the code is now enclosed in a
 try...catch block, the latter of which logs a message make if the code
 fails.

 '''lib/dhcp/Makefile.am'''
 > Your merge brought back the dependency between libdhcp_srv and libdhcp++
 libraries that has been earlier removed with the commit
 1cee186c837d355ef28b00773bbb145e75444b85. This is causing multi-threaded
 build issues that we discussed last week.
 Corrected.

 '''lib/dhcp/lease_mgr.cc'''
 > getParameter: can parameter be returned by reference?
 Possibly, but it is usual to return string parameters by value.  Note that
 this is a non-time-critical operation - the parameters are only accessed
 when the lease manager is created, which is normally once per program.

 '''lib/dhcp/lease_mgr.cc'''
 > !LeaseMgr destructor: since destructor is empty, why not to remove it
 from the implementation and put empty brackets in the header file?
 Done.  The constructor has also been moved to the header as it is simple.

 '''lib/dhcp/lease_mgr.h'''
 > !LeaseMgr destructor: other classes will derive from this class so the
 destructor must be virtual.
 Done - this was the cause of the memory leak reported by valgrind.

 '''lib/dhcp/lease_mgr_factory.h'''
 > !LeaseMgrFactory description: @TODO is not a doxygen tag. You should
 probably replace it with @todo.
 Done

 > Headers inclusion: it appears to me that neither string nor
 exceptions/exceptions.h has to be included. They are pulled indirectly by
 lease_mgr.h.
 Removed.

 > create(): Note: could be replaced with @note in the documentation.
 Replaced.

 > parse(): typo in the @return tag: std::map<>std::string, std::string>
 Corrected.

 '''lib/dhcp/lease_mgr_factory.cc'''
 > According to BIND10 coding style we should first include project
 headers, then library headers and finally system headers.
 Reordered.

 > The headers are included in the reverse order. Also this whole section
 could have been removed as those headers are included indirectly anyway:
 I've modified the order but prefer to leave them in: I think that for a
 .h/.cc file pair, the .cc file should include all headers it needs for
 variable and function declarations, with the exception of those declared
 in the corresponding .h file.  To rely on other .h files for the inclusion
 of a header leaves the software open to something suddently not compiling
 because a #include has been removed from an apparently unrelated header
 file.

 > parse(): Extra space between exclamation mark and dbconfig
 Removed.

 '''lib/dhcp/memfile_lease_mgr.h'''
 > #endif // MEMFILE_LEASE_MGR_HSE4
 >
 > garbage HSE4.
 The result of typing when the focus is elsewhere...


 > There are no tests for !LeaseMgrFactory...
 There are some, but only for the parameter parsing.  A note has been added
 to the !LeaseMgrFactory unit test file to not that the tests for
 create/instance/destroy are carried out implicitly in the tests for the
 various concrete lease manager.s

 > Also please run:
 > :
 > There are many errors there. I did not go through all of them but I
 think some may be related to your work.
 Ones related to this change have been corrected.

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