BIND 10 #2995: Add selected few (3-4) hooks into DHCPv6 server

[BIND 10 Development]

#2995: Add selected few (3-4) hooks into DHCPv6 server
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130717
           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 72b06693a049e925d240971d92dd8427d3fa8f73

 I've made some editorial changes to the dhcp6_hooks.dox file - please pull
 before doing any modifications.

 '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
 >> You may want to add the removal of the hook points to the Dhcpv4SrvTest
 constructor as well, in anticipation of the time hooks are added to
 DHCPv4.
 > No. It is added to destructor already. If I added it to contructor, it
 would break down the tests once Dhcpv4Srv gets hooks support.
 Dhcpv4SrvTest constructor is called after Dhcpv4Srv constructor, so adding
 reset() to the latter would effectively get rid of all hook points.
 Is it?  The Dhcpv4SrvTest is a class used by Gtest and instantiated for
 the test.  Unless there is a static declaration of Dhcpv4Srv somewhere,
 the test class will be instantiated before the object being tested.
 However, if the hooks points are statically registered (as they are),
 there is no need to delete them in the test so the whole point is moot.

 >> Question about hooks system: the "skip" flag was so-named because of
 anticpated use. What I hadn't considered was the possibility of dropping
 the packet (hence the overloaded use of the skip flag by the server in
 this situation). On the assumption that it seem illogical to a hook author
 that setting the "skip" flag causes the dropping of the packet
 > For the sake of not holding back the development, I propose to keep the
 code as it is and revisit this topic later after we get some comments from
 external users.
 The code has been left as-is.  In the latest version of the user guide
 (#2982), I've described that setting the flag causes the server to skip
 normal processing (so including the possibility of dropping the packet).



 '''src/bin/dhcp6/dhcp6_hooks.dox'''[[BR]]
 subnet6_select: the documentation mentions that the Subnet6Selection is
 provided as a "const reference".  As the information is copied across to
 the !CallHandle object when setArgument is called and copied from it when
 getArgument is called, I think that what is happening is that the callout
 is getting a copy of the Subnet6Collection, not a reference to it.  If you
 want to pass an object without copying to the callout, and do not want the
 callout to modify it, you need to pass a "pointer to const".

 '''Note:''' In editing this, I added links to the Pkt6 class.  On checking
 them, I noticed that the class does not have a header (hence the Doxygen
 HTML entry does not have a class description).  I also noticed that Pkt4
 does not have a class header.  Can you create a ticket to add ones.


 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 There is no need to call "setSkip(false)" before calling the callouts -
 callCallouts does that.

 (Although I've just noticed that if there are no callouts attached to a
 hook and callCallouts is called, the flag is not cleared - I've raised
 #3050 for that.  This bug does not affect the code here, because the
 server checks that callouts are present first.)

 >> Dhcpv6Srv constructor: the changes in #2980 should mean that there is
 no need to call HooksManager::loadLibraries(). If no libraries are loaded
 when something is called, the framework will set itself up with an empty
 set of libraries (as you have done here).
 > Hmm, I've commented it out and now 3 tests fail:
 > ...
 The reason the tests were failing is that the the callouts registered by
 earlier tests were not cleared.  The call in the Dhcpv6Srv constructor
 cleared them when the object was instantiated at the start of a test.
 I've attached a patch file to the ticket that will clear registered hooks
 at the end of each test.

 As an aside, there is the comment:
 {{{
 /// @todo call loadLibraries() when handling configuration changes
 }}}
 Where is the best place to do this, as I am aiming to do it as part of
 #2981?  The issue with memory allocated within a callout means that it can
 only be done where all structures associated with the previous packet have
 been destroyed.  This appears to be either:

 * In Dhcp6Srv::run(), at the head of the run() loop. (The loading of the
 libraries will only take place if the configuration has changed, not on
 every packet).
 * In configureDhcp6Server().  (This assumes that configureDhcp6Server can
 only be called from the main select/asio loop.  In other words, it assumes
 that Dhcp6Srv::run is at the point where it is waiting for a new packet,
 so everything associated with the previous packet has been destroyed.)

 getCalloutHandle(): we may want something that will allow the deletion of
 the stored pointer.  If the libraries are reloaded, the way the hooks code
 is written will mean that the old libraries will not be unloaded until the
 stored !CalloutHandle is destroyed.  If there is a problem, the deferred
 deletion may make it difficult to debug.


 '''src/lib/dhcpsrv/alloc_engine.cc'''[[BR]]
 reuseExpiredLease(): with the changes to the hooks manager:
 {{{
 HooksManager::getHooksManager()::callCallouts(...)
 }}}
 can be replaced with
 {{{
 HooksManager::callCallouts(...)
 }}}


 '''src/lib/dhcpsrv/cfgmgr.h'''[[BR]]
 White space is incorrect for getSubnets6().

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