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

[BIND 10 Development]

#2995: Add selected few (3-4) hoots 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:4 stephen]:
 > Reviewed commit commit c6b3ce6d16f92b80db77487900fb57a7a67b3203
 Review continues. See comment:5 and comment:6 for previous parts.

 > Dhcpv6Srv::run(): "Callouts decided to skip the next processing step".
 The comment is incorrect: as indicated the comment (above) for
 DHCP6_HOOK_SUBNET6_SELECT_SKIP, callouts can skip a processing step when
 it makes sense to do so. In other cases - such as here - "skip" makes no
 sense, but "drop" does.
 I decided to keep the "skip" flag for now. If we decide later to implement
 int returnAction, we will do so at a later date.

 > Dhcpv6Srv::run(): "No need to clear arguments".  This is true, but the
 "deleteAllArguments()" call should be negligible overhead if no arguments
 are stored.  It might be more defensive to clear them regardless. (On the
 other hand, callouts should only reference documented arguments.  If they
 access something undocumented - such as an argument from a previous call
 that has not been deleted - we can't be held responsible if a changes to
 BIND 10 causes their code to fail.)
 deleteAllArguments() is now called. Also the comment about being first
 callout removed. We will soon implement buffer6_receive, which will be
 called before pkt6_receive, so the comment would be incorrect, but we
 would probably forget to update it.

 > Dhcpv6Srv::run(): Would the names of the arguments to the callouts be
 more descriptive if named "query" and "response" rather than "pkt6"?
 That way you can't reuse the same method for both pkt6_send and
 pkt6_receive. Anyway, I've changed the code as you suggested.

 > Dhcp6Srv general: "!CalloutHandlePtr" has been defined and can be used
 instead of boost::shared_ptr<!CalloutHandle>.
 I hope I spotted all the instances. Let me know if I missed anything.

 > Dhcp6Srv::selectSubnet(): this has the same comment as the "run()"
 method - "This is the first callout so not need to clear any arguments".
 Both statements can't be true.
 Copy and paste error. Fixed.

 > Dhcp6Srv::assignIA_NA(): there is a call to get the !CalloutHandle here
 although the return value is not used anywhere.
 Look closer. :) It is passed to allocateAddress6(). This is the last place
 where we have access to packet. Allocation engine operates on addresses,
 subnets and leases it knows nothing about packets.

 >
 > Dhcpv6Srv::getCalloutHandle(): just a comment here that this scheme is
 only valid providing a packet is fully processed (from being received to
 the response being sent (or the query dropped)).  This is the case for the
 server, but comments in the method should make the restrictions clear.
 Explanatory comment added.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 > This file is now 2,500 lines long - at some time we should break it down
 into smaller files.
 Yes, but not within this ticket. It is already too complex - 2 large
 merges from another branch. Please create a separate refactoring ticket if
 you want to pursue this matter further.

 > "Hooks" test: serverHooks::getIndex() will throw a !NoSuchHook exception
 if the hook is not registered, so these tests should really be
 EXPECT_NO_THROW tests.
 It doesn't make much difference as exception thrown will fail the test
 anyway. But I modified the code as you suggested.

 > HooksDhcpv6SrvTest class: methods should be documented, including
 arguments.
 It is now documented.

 > HooksDhcpv6SrvTest class: once #2980 is merged, there will be no need to
 set the library index in the constructor.
 I can't find specific code that sets library index. I must have removed it
 when tweaking code for the second merge.

 > Note: once #2980 is merged, we should think of creating a shared library
 containing a variant of a HooksDhcpv6SrvTest object and repeat the hooks
 tests here using that library.
 We may, but I thought that will be part of the tests that QA is supposed
 to be doing.

 > '''src/lib/dhcpsrv/alloc_engine.{cc,h}'''[[BR]]
 > See above for discussion of hook registration questions.
 Done.

 > AllocEngine::reuseExpiredLease(): there is no need to reset the "skip"
 flag - that is automatically set "false" in HooksManager::callCallouts()
 before the first callout is called.
 Removed.

 > AllocEngine::reuseExpiredLease(): (comment before callCallouts()) is
 this the first callout as well?
 Another copy and paste error. Fixed.
 >
 > AllocEngine::reuseExpiredLease(): the callout called is for
 lease6_select, yet this method is described as reusing an expired lease
 (and the comment before the callouts talks about lease6_ia being added).
 Reusing an expired lease seems to be an event that merits its own distinct
 callout.

 No. I consider the fact that we are reusing expired lease a very internal
 things. For all intents and purposes, we are allocating a lease for
 client. The fact that we reuse a lease is our internal optimization and
 honestly speaking a workaround for lack of housekeeping process.

 If you really think that we should expose that information, then we can
 perhaps add a flag that states whether this is a new or reused lease.

 > AllocEngine::createLease6(): the callout called in this method has the
 same hook index as that in reuseExpiredLease() (as well as the same
 comments, including "this is the first callout".  I suspect a cut-and-
 paste error somewhere, probably in reuseExpiredLease().
 No. That is by design. These are two points where server allocates leases.
 It is very internal matter. For users it is sufficient to know that this
 lease going to be used. The previous history is irrelevant.


 > '''src/lib/dhcpsrv/cfgmgr.{cc,h}'''[[BR]]
 > getSubnets6(): for getter and setter methods that get/return a member of
 the class (and that are not virtual), putting the single line of the code
 in the header allows the compiler to inline the call and save the overhead
 of a call/return.
 Done. Added explicit inline.


 > '''src/lib/dhcpsrv/dhcpsrv_log.h'''[[BR]]
 > DHCPSRV_HOOKS should be DHCPSRV_DBG_HOOKS to be consistent with other
 debug levels.
 Done.

 >
 > '''src/lib/dhcpsrv/dhcpsrv_messages.mes'''[[BR]]
 > DHCPSRV_HOOK_LEASE_IA_ADD_SKIP:[[BR]]
 > s/callout assigned on lease6_ia.../callout installed on the
 lease6_ia.../[[BR]]
 > s/Server will not.../The server will not.../
 Done.


 > '''src/lib/dhcpsrv/tests/alloc_engine_unittest.cc'''[[BR]]
 > General: The EXPECT_EQ methods should have the expected value as the
 first argument and the value under test as the second (Google test
 messages are optimised for this order).
 Done, except one case where false being first causes the code to not
 compile.

 > various: the calls to loadLibraries will not be required after #2980 is
 merged.
 Ok. We will clean this up in the next ticket.

 Ok, the code is back with you. Thanks for thorough review.

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