BIND 10 #2404: MySQL IPv4 lease database access functions

[BIND 10 Development]

#2404: MySQL IPv4 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121129
  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 tomek):

 First part of the review.
 '''src/lib/dhcpsrv/database_backends.dox'''
 1. This file should mention that MySQL is disabled by default and --with-
 dhcp-mysql is needed.

 1. Comment on memfile that nothing is written to external storage
   should emphasise somehow that this is a temporary limitation and
   will eventaully be implemented, but it is currently low priority.
   We may add a comment about 2013 being likely timeframe for this.

 '''src/lib/dhcpsrv/dhcpdb_create.mysql''' - Why are we increasing only
 minor version? There
 are incompatible changes (lease_time => valid_leasetime), so major version
 should be bumped.

 '''src/lib/dhcpsrv/lease_mgr.h'''

 "This is pecified" => "This is specified"

 Comment for Lease6 is truncated. There seem to be missing a line
 {{{/// extensively, direct access rather than through getters/setters is
 warranted.}}}

 Comment for addr_ field in Lease6: It should be commented that it may
 contain prefix in case of IA_PD.

 Comment for iaid field: "Most containers" => "All containers"

 CLIENT_ID_MAX_LEN constant: Please comment that this is arbitrarily chosen
 value as RFC2131 does not specify upper bounds. It just follows 128 byte
 limit from DHCPv6.

 We can possibly add a comment that a minor performance boost may be
 achieved with shortening this field. On a side note, it may be a good
 idea to mark such comments somehow. It will be useful when a customer
 comes to us and says that extra perfomance is needed even if corners
 are cut. Note that it is "when", not "if". Not all performance
 improvements are sane in general case, but it would be useful to have
 pointers to start.

 '''src/lib/dhcpsrv/mysql_lease_mgr.cc'''

 "variables n header" => "variables in header"

 "nonly to satisfy cppcheck" => "only to satisfy cppcheck"

 "structre" => "structure"

 It would be nice to add maximum field lengths in '''createBindForSend'''
 comments, e.g. hwaddr: varbinary(128).

 Please define const int value for bind_[] array size.

 "strictures" => "structures"

 ADDRESS6_TEXT_MAX_LEN is 42 bytes. Comment for address:varchar states that
 we define the field as "couple bytes longer", so we can null terminate it.
 Why couple bytes and not just one? Also, is the MySQL API require null-
 termination? In that case why string length is
 passed as well?

 Why did you remove error checking (bind_[X].error values)? There shouldn't
 be any overflows, but it is better to check it anyway. One example that
 comes to mind is overflow my happen with IDN hostnames and odd UTF-8
 encoding. That encoding may be set by the user and internal MySQL
 conversion is outside of our control. Let's keep it.

 Why MySqlLease6Exchange::getLeaseData() does not set T1 and T2 and uses
 zeros? I haven't reviewed the tests yet, but aren't tests supposed to
 catch that?

 "flushed disk in the background every second or so." comment: I think you
 should remove "or so". I think (haven't checked that, so I may be wrong)
 that innodb_flush_log_at_trx_commit set to 2 means exactly 1 second (with
 the standard granularity of non-real time systems).

 You may add @todo here noting that innodb_flush_log_at_trx_commit
 parameter will be tweakable from bind10 configuration one day.

 Why do we need semicolons in openDatabase() in catch() sections? Is there
 a compiler that complains about empty { } sections?

 MySqlLeaseMgr::addLeaseCommon comment: Please explicitly say that this is
 for Lease4 and Lease6. Also, since this is file local class (no header for
 it), it makes sense to comment its methods using doxygen, including @param
 and @return tags.

 '''getLease4''': I think we should throw exception if the subnet-id field
 does not match, and not silently ignore it. This means database
 corruption.

 I think we should check errors in fields (inbind[0].error should be set to
 something and its value verified).

 getName() method is supposed to return backend type. That is required for
 Kea to report which backend is used. See DHCP6_DB_BACKEND_STARTED log
 message and memfile implementation.

 The review will continue tomorrow.

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