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      |
-------------------------------------+-------------------------------------

Comment (by marcin):

 Reviewed commit f25544eeedb66a4f5bb0e0c41387e2e58555daa9

 '''database_backends.dox'''
 Typo here: ''A lease manager is instantiated through '''a the'''
 LeaseMgrFactory....''
 (''a'' or ''the'' should be left)

 Typo here: ... '''is is''' controlled by the argument.

 Typo here: ...there are certain pre-requisites for '''scuccessfully'''
 running the unit tests.

 Awkward formatting when enumerating pre-requisites for setting up
 different (currently MySQL only) databases. In particular,  ''These are
 database-specific:'' with colon which makes me expect some form of bullets
 or at least indentation before ''MYSQL'' and other databases which may be
 added in the future. Why not to add sub-sections like '''Pre-requisites
 for MYSQL''' etc. and replace !'' These are database-specific: !'' with
 !'' Database-specific setup for running unit tests are given in the
 following sub-sections. !'' ?

 Consider adding references/links to the MySQL documentation on how to
 create MySQL users and schema:
 - http://dev.mysql.com/doc/refman/5.5/en/adding-users.html
 - http://dev.mysql.com/doc/refman/5.0/en/create-database.html

 Also, suggest that whenever you write '''keatest''' in a sentence it is
 taken between quotation marks. This will make it clear for a reader that
 it is user name, password or data base name rather than two separate
 words: '''kea''' and '''test''' without accidentally deleted space between
 them.

 '''lease_mgr.h'''
 updateLease6: This function is pure and thus it doesn't throw any
 exception so I suggest that @exception tags are removed from the
 documentation. Also, since we may have multiple lease managers derived
 from LeaseMgr base class, listed exceptions may be not relevant to them
 (e.g. DbOperationError).

 updateLease6: Although not mandated in style guidelines, I suggest that
 @exception tags are replaced with @throw tags for two reasons:
 - @throw appears to be more common in BIND10 code
 - @throw is simply shorter

 This applies to the following files as well:
 - mysql_lease_mgr.h
 - lease_mgr_factory.h

 '''mysql_lease_mgr.cc'''
 Minor: Included headers: according to BIND10 coding style, if possible we
 should be first including project headers, then library headers and lastly
 system headers.

 MySqlLease6Exchange doxygen documentation: There is extra new line between
 doxygen description of this class and line starting with ''Note - ...''.
 In such case the note is not incorporated into the doxygen description of
 the class. Is this intentional? I think should be. If you agree, please
 also consider using @note tag for the note.

 MySqlLease6Exchange constructor documentation: Thanks for putting the note
 why you perform memset on addr6_buffer_ etc. However when I read it in the
 html form I get confused what !''other initializations!'' it is referring
 to. I suggest that you clarify this comment so it can be understood
 without a need to read the code.

 getLease6: It is not necessary to do complex cast and usually removing
 "constness" is bad idea too. Can't you just initialize duid_vector by
 value:
 {{{
 vector<uint8_t> duid_vector = duid.getDuid();
 ...
 inbind[0].buffer = &duid_vector[0];
 }}}
 This may have some performance implications but is safer and simpler.

 getLease6: In the try-catch statement you catch '''isc::BadValue'''
 exception (which may be emitted by getLeaseData()) and free some allocated
 resources if this exception is caught. However it appears to me that
 getLeaseData() may throw other exceptions too. That is for example:
 bad_alloc if new operator fails here:
 {{{
 result->duid_.reset(new DUID(duid_buffer_, duid_length_));
 }}}
 Also DUID's constructor emits '''isc::OutOfRange''' when DUID has invalid
 length. These exceptions will not be caught and
 mysql_stmt_free_result(...) will not be called in getLease6 making it
 prone to resource leaks.

 '''mysql_lease_mgr.h'''
 updateLease6 doxygen: invalid parameter name '''lease4''' (instead of
 '''lease6'''). Also similar issue in @brief description.

 deleteLease6 doxygen: same as above


 '''mysql_lease_mgr_unittest.cc'''
 MysqlLeaseMgrTest: suggest that you try-catch around this:
 {{{
 LeaseMgrFactory::create(validConnectionString());
 }}}
 and output some more information about the reason of failure just like you
 do in the same file in line 416. Otherwise, when MySQL is not configured,
 constructor will emit exception which will be logged similar to this:
 {{{
 unknown file: Failure
 C++ exception with description "Access denied for user ..."
 }}}
 which doesn't mention that it was a problem to access database.

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