BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  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 marcin):

 * owner:  marcin => stephen


Comment:

 After the merge of master branch to trac2342 there were a number of files
 modified to resolve conflicts. Thus this recent commit has been reviewed:
 4e7a8f7575704052e70432f1e2508d310e8f6bb0

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

 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. If we are going to release DHCP without concrete
 solution can't we simply require database (etc.) named "kea" instead of
 "keatest" in the MySQL_LeaseMgr unit test? That should be farily trivial
 change. If there is a good reason not to do it, documentation should be
 updated to point out that "kea" database needs to be created (apart from
 "keatest" db) to run dhcp6_srv unit tests. I couldn't find such
 information anywhere. Also, can you create a ticket to reolve this
 inconsistency and put the reference to it in #2342?

 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 shudown_ member is reset (true). Shouldn't
 you catch exceptions emitted by the factory function and set shutdown_?
 Another good reason to catch exceptions would be to log the error message
 just like it is done for other errors occuring in the constructor.
 As a side note: emitted exceptions are not documented in the constructor's
 header (would require a few @throw tags there if you decide not to catch
 exceptions as I suggested).

 '''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 dicussed last week.

 '''lib/dhcp/lease_mgr.cc'''
 getParameter: can parameter be returned by reference?

 '''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?

 '''lib/dhcp/lease_mgr.h'''
 LeaseMgr destructor: other classes will derive from this class so the
 destructor must be virtual.

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

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

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

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


 '''lib/dhcp/lease_mgr_factory.cc'''
 According to BIND10 coding style we should first include project headers,
 then library headers and finally system headers.  The headers are included
 in the reverse order.  Also this whole section could have been removed as
 those headers are included indirectly anyway:
 {{{
 #include <algorithm>
 #include <iostream>
 #include <iterator>
 #include <map>
 #include <sstream>
 #include <string>
 #include <utility>
 }}}

 parse(): Extra space between exclamation mark and ''dbconfig''
 {{{
 if (! dbconfig.empty())
 }}}

 '''lib/dhcp/memfile_lease_mgr.h'''
 {{{
 #endif // MEMFILE_LEASE_MGR_HSE4
 }}}
 garbage ''HSE4''.


 There are no tests for LeaseMgrFactory. I presume that this is because
 this class is tested indirectly but comment in its header file would be
 useful to point this out.

 Also please run:
 {{{
 cd doc
 make devel
 less html/doxygen-errors.log
 }}}
 There are many errors there. I did not go through all of them but I think
 some may be related to your work.

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