BIND 10 #2994: Add selected few (3-4) hooks into DHCPv4 server

[BIND 10 Development]

#2994: Add selected few (3-4) hooks into DHCPv4 server
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp4         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130731
           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 stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed commit 049d11fa1b6b08e8ad866686e57726e5906ced1c

 I've updated the .mes files - please pull and review.


 '''!ChangeLog'''[[BR]]
 Don't forget to update with the commit ID after the merge.

 '''doc/devel/mainpage.dox'''[[BR]]
 I suggest taking the "Hooks API" entries out of the maintenance guide.
 The topics "Other DHCPv{4,6} Topics" mention hooks and point to the
 appropriate section. Also, the Hooks API is aimed at the user, not the
 DHCP maintainer.

 It looks as if you've omitted to check in the DHCPv4 hooks document.

 '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 Dhcp4Srv constructor - remove the call to loadLibraries().  It's not
 needed here, as before the configuration is set the hooks framework will
 work as if no libraries are loaded.

 Dhcp4Srv::{receive,send}Packet - return type should be on a separate line
 according to the coding standards.

 Dhcp4Srv::{run,selectSubnet} - remove the getHooksManager() calls - most
 !HooksManager methods have static equivalents.

 Dhcp4Srv::selectSubnet - instead of converting the address to text format
 and check against the string "0.0.0.0", it is likely to be faster to use
 the conversion operator that returns a uint32_t (i.e. "if (relay != 0)
 {").  Alternatively, it might be clearer to declare 'static const
 IOAddress notset("0.0.0.0")' and compare against that.

 Dhcp4Srv::sanityCheck: when checking the hardware address, even single-
 line "if" blocks need braces.

 '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 NakedDhcpv4Srv: minor issue but the headers for the newly-created methods
 (sendPacket, receivePacket etc) seem to use a much short line-length than
 the header for the constructor (or the header for the member variables).

 HooksDhcpv4SrvTest::generateSimpleDiscover: after we have delivered this
 Comcast work and we have a breathing space, we may want to consider
 writing a function that allows the creation of a binary DHCP packet from a
 texttual description - the DNS tests have this (see files in
 /src/lib/dns/tests/testdata).  It should make the creation (and
 readibility) of unit tests easier.

 '''src/lib/dhcpsrv/alloc_engine.cc'''[[BR]]
 createLease4: no need to clear the "skip" flag in the !CalloutHandle -
 callCallouts does that.

 reuseExpiredLease (V4 version): the V6 version of the method gets passed a
 Subnet6Ptr, but this version gets passed a general !SubnetPtr - why the
 difference?

 reuseExpiredLease (V4 version): I would clarify the comment about the
 pointer cast.  Something like:
 {{{
 Convert the general subnet pointer to a pointer to a Subnet4.  Note that
 because we are using boost smart pointers here, we need to do the cast
 using the boost version of dynamic_pointer_cast.
 }}}

 '''src/lib/dhcpsrv/alloc_engine.h'''[[BR]]
 createLease{4,6} header: strictly speaking, lease callouts will only be
 executed if the parameter is passed and there are callouts present on the
 hook.

 '''src/lib/dhcpsrv/tests/alloc_engine_unittest.cc'''[[BR]]
 General: use HooksManager::loadLibraries() instead of
 HooksManager::getHooksManager().loadLibraries().

 lease4_select: Instead of "EXPECT_EQ(callback_fake_allocation_, false)",
 EXPECT_FALSE is briefer and clearer.

 lease4_select: The comment starting "Check if all expected parameters are
 reported" highlights a potential future problem.  Why not solve it now?
 Before the EXPECT statement add the line:
 {{{
 sort(callback_argument_names_.begin(), callback_argument_names_.end());
 }}}
 (...assuming that you've coded the expected_names in alphabetic order.)

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