BIND 10 #3084: Changes to MySQL backend/lease tables to store FQDN information for a lease

[BIND 10 Development]

#3084: Changes to MySQL backend/lease tables to store FQDN information for a lease
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130904
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
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 => tmark


Comment:

 Replying to [comment:6 tmark]:
 > Nicely done.  Just as I was writing up the need for tests against too
 long a hostname, I saw them added as the last commit for this ticket.

 Thanks.

 >
 > One minor issue, some cppcheck complaints, and a general question. First
 the minor issue:
 >
 > ---------------------------------------------------
 >
 > In  MySqlLease4Exchange::createBindForSend(const Lease4Ptr& lease)
 >
 > Shouldn't you have done this:
 >
 >     bind_[8].buffer = reinterpret_cast<char *>(const_cast<char*>
 >
 (lease_->hostname_.c_str()));
 >
 > instead of this?
 >
 >     bind_[8].buffer = const_cast<char*>(lease_->hostname_.c_str());
 >

 The c_str() function returns the const char* value. The buffer is non-
 const so I used const_cast to remove constness. I could also copy the
 buffer but I thought it is better
 to use the same pointer for performance reasons.

 With respect to reinterpret_cast. I think it is not necessary here because
 the type of the buffer and the c_str() function's returned type match
 (except constness).

 > --------------------------------------------------------
 >
 > I get the following cppcheck errors:
 >
 > src/lib/dhcpsrv/mysql_lease_mgr.cc:303: check_fail: Member variable
 'MySqlLease4Exchange::hostname_buffer_' is not initialized in the
 constructor. (warning,uninitMemberVar)
 > src/lib/dhcpsrv/mysql_lease_mgr.cc:303: check_fail: Member variable
 'MySqlLease4Exchange::hostname_length_' is not initialized in the
 constructor. (warning,uninitMemberVar)
 > src/lib/dhcpsrv/mysql_lease_mgr.cc:655: check_fail: Member variable
 'MySqlLease6Exchange::hostname_buffer_' is not initialized in the
 constructor. (warning,uninitMemberVar)
 > src/lib/dhcpsrv/mysql_lease_mgr.cc:655: check_fail: Member variable
 'MySqlLease6Exchange::hostname_length_' is not initialized in the
 constructor. (warning,uninitMemberVar)

 Thanks for checking this. I believe cppcheck is right with respect to the
 buffer length initialization. I don't see a compelling reason to
 initialize the table of chars with zeros, other than to satisfy cppcheck.
 Which I did. Note that the buffer is always initialized before it is used
 to update the database and the length of the useful data is provided too.

 >
 >
 > -----------------------------------------------------
 >
 > Now the general question:
 >
 > Are we likely to ever need/want the ability to fetch a lease by
 hostname?
 >

 Good question. But, I don't see a use case for this at the moment. We can
 always get the IP address of the particular client using DNS. That's what
 the DNS is for. Does server on its own need get the lease by hostname
 internally? I don't know.

 >
 >
 > Beyond that everything looks good.

 Thanks for review.

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