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:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130731
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 049d11fa1b6b08e8ad866686e57726e5906ced1c
 >
 > I've updated the .mes files - please pull and review.
 Thank you. Your changes look fine.

 >
 > '''!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.
 Ok, removed. The DHCPv4 and DHCPv6 hooks are listed in

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

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

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

 > 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.
 Although it was me who added the uint32_t operator, I don't like it at
 all. It is too error prone, e.g. logging using arg(addr); will compile ok,
 but the actual code will try to log uint32_t. That will look ugly for v4
 and will throw for v6. I plan to remove the uint32_t conversion operator
 from IOAddress one day. For that reason I chose to implement your second
 proposal - static const IOAddress.

 > Dhcp4Srv::sanityCheck: when checking the hardware address, even single-
 line "if" blocks need braces.
 Added.
 >
 > '''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).
 Fixed new comments and couple existing ones. On a related note, BIND10
 coding guidelines should be updated. We had an agreement that the code
 width should be limited to 80 if possible, but in some cases it may be
 wider, up to 100 characters. That was only agreed in bind10 chatroom, but
 the guidelines were never updated.

 > 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.
 Agree. That one was painful. I also have a code in Dibbler that take
 hexstring from Wireshark, so I can copy paste a real packet into C++ test
 code. It is easy to do. I'm willing to donate that code. We may also
 consider developing a support for libpcap in our tests. That would be a
 good robustness test. I have a collection of various (sometimes odd)
 DHCPv6 packets. It would be useful to throw them at our implementation and
 see what happens.

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

 > reuseExpiredLease (V4 version): the V6 version of the method gets passed
 a Subnet6Ptr, but this version gets passed a general !SubnetPtr - why the
 difference?
 I tried to unify (SubnetPtr) the alloc engine code as far as I could, but
 hit a problem that would require doing dynamic_pointer_cast. After that I
 reverted back to Subnet4Ptr and Subnet6Ptr, but apparently I missed some
 spots. I think now that we have the code up and running, we may try again
 to use SubnetPtr more. Anyway, ultimately the library user needs to get a
 specific (Subnet4Ptr or Subnet6Ptr) pointer, so he won't have to do any
 dynamic_casts.

 > 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.
 > }}}
 Done as suggested.
 >
 > '''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.
 Comment updated.

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

 >
 > 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.)
 Added sorting routines.

 I also checked in the missing dhcp4_hooks.dox file.

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