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:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130717
         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:9 stephen]:
 > 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.
 Sorry for the confusion. Here's the explanation what had happened. At
 first I intended to keep the hooks as they were before - register them in
 Dhcpv4Srv constructor (or to be more precise - in AllocEngine constructor
 that is called from Dhcpv4Srv constructor) and call reset() in
 Dhcpv4SrvTest destructor (so once the next test instantiates the Dhcpv4Srv
 class it could be able to register properly). Then while I was working
 through your comments I got convinced by your arguments and rewrote hook
 registration to work as recommended by the hooks guide. Finally, I forgot
 to update this comment.

 Anyway, I understand the difference between Dhcp4SrvTest and Dhcpv4Srv
 classes. The ideal C++ approach would be to undo in destructor whatever
 was done in consutructor. However, since there was no unregisterHook()
 method, I couldn't call reset() in Dhcpv4Srv destructor as that may have
 impacted other components. In any case, that is no longer relevant, as the
 registration is done statically just once and there is no need to use
 reset() anymore.

 > >> 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).
 Great! Thank you.

 I think at the end, this is up to the definition of 'skip' flag. Forget
 any documentation for a moment. What do you think "skip this incoming
 packet" could mean to the server? Anyway, we'll probably get back to this
 once we get some external reviewers.

 > '''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".
 I've updated it to use const Subnet6Selection*. I hope I haven't messed up
 const pointer to variable object and variable pointer to const object this
 time.

 > '''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.
 Sure. Created ticket #3053.

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

 > >> 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.
 Thank you. Patch applied.

 >
 > 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).
 No, that's not the right place. Loading libraries is part of configuration
 (a fancy one, I admit) and it should be done in the same place as every
 other configuration change.

 > * 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.)
 That is the place to load libraries.

 > 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.
 Part of the #2981 should be to extend getCalloutHandle to release existing
 CalloutHandle when called with NULL parameter. You could then call it
 before unloading libraries.

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

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

 Also added the following changes:
 - removed Dhcp6Srv::getServerHooks() and getHooksManager() declarations.
 - updated sendPacket comment (reception->transmission)
 - documented getCalloutHandle()

 I think that addresses all issues. Unless there are new issues spotted, I
 think this is finally ready to merge.

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