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:  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
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:4 stephen]:
 > Note that some changes have been made and should be pulled before
 development continues.
 Thank you.

 > '''doc/devel/mainpage.dox'''[[BR]]
 > '''src/bin/dhcp6/dhcp6_hooks.dox'''[[BR]]
 > I've edited these files and altered the format a bit.  In particular:
 > * The Doxygen tags convention appears to be the same as for method
 names.  I've renamed the tag "hooks-dhcp6" to "hooksDhcp6". (I note that
 the Logging pages don't follow that convention, but an update of them is
 scheduled.)
 > * The description for each hooks has been split, with the setting of the
 "skip" flag put in a separate section. (This may need to change if we
 alter the skip flag - see later in this comment.)
 Thank you. It looks more clear now. I also reverted the order - it is now
 again in the order hook points will be called in packet processing. Also
 corrected skip flag meaning between receive and skip. The difference is
 that skip flag in send hook will not send out the response packet, but the
 server will have this updated state (e.g. allocated a lease).

 > * I've altered the reference to the general Hooks API document in
 anticipating of the merging of #2982.
 >
 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 > 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. That's
 why it has to be added to destructor. BTW Dhcpv4Srv class uses AllocEngine
 that registers lease6_select hook. Obviously it won't be used, but the
 registration takes place nevertheless. That's why reset() in Dhcpv4SrvTest
 is required now.

 > '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 > DHCP6_HOOK_SUBNET6_SELECT_SKIP: the general idea is that when the
 callout instructs the server to skip the next stage of processing, it is
 because the callout has already done that.  So in the case of the skipping
 of subnet selection, it would be because the callout has already selected
 the subnet. (How/why I've no idea - but it would be logical for the
 selected subnet to be returned through the argument list somehow.  And how
 the server translates what is returned into a pointer to the appropriate
 data structure is another issue.)
 I understand the general idea, but I tried to offer as much flexibility as
 possible. The callout could use some extra information (e.g. options added
 by relays, like subscriber-id or docsis3.0 options) to pick a different
 subnet. Obviously, a callout can set subnet6 to Subnet6Ptr(), effectively
 not selecting a subnet. But I thought that using skip flag for that
 purpose would be cleaner and more explicit way of not selecting a subnet
 at all.

 > DHCP6_HOOK_PACKET_RCVD_SKIP: As noted above, the "skip" flag was
 intended to instruct the server to skip the processing associated with the
 hook·  In some cases - such as here - there is no associated processing:
 instead, it makes sense to allow the callout to drop the packet. I suggest
 that the last two sentences of the explanation be replaced with "For this
 particular hook, the setting of the flag by a callout instructs the server
 to drop the packet." (But see question below.)
 But how can you tell a server to skip receiving a packet that it passed to
 you? There will be another callout for that - buffer6_received that is
 called after read from sockets. It will also have skip flag that will
 instruct the server to drop the packet.

 > DHCP6_HOOK_PACKET_SEND_SKIP: suggest a similar change to the wording as
 for DHCP6_HOOK_PACKET_RCVD_SKIP - replace the last two sentences with
 something like "For this particular hook, the setting of the flag by a
 callout instructs the server to drop the response." (But see below.)
 There's a difference between RCVD_SKIP (server not processed anything) and
 SEND_SKIP (server processed the request packet and possibly allocated a
 lease, just the change was not communicated to the client). I used
 your proposed sentence, but kept the existing explanation as well (with a
 small tweak).


 > ----
 > '''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, I think this should be changed.  Although we could require that
 the action be returned to the server in the argument list, an easily-
 accessible flag or value appears to be more convenient (and faster).
 Would it be better to replace {get,set}!SkipFlag() with something like
 {get,set}!ReturnAction() (where the "return action" value is an "int")?
 Appropriate values for the return action indicator would be defined on a
 per-hook basis, although adopting the convention that a value of 0 means
 that the server should continue normal processing will simplify things.
 >
 > So for the above hook points, a return value of 0 means continue and a
 non-zero value is interpreted as "drop the packet" and the send and
 receive points, and "skip the associated processing" at the subnet
 selection point.  (Some hook points may have more than two possible return
 actions (e.g. drop, skip, or continues) which is why it is suggested that
 the the "return action" be an integer value rather than a boolean flag.)
 There is no clear consensus on this one. You seem to prefer int
 returnAction and I prefer bool skip. I asked Marcin and Tom, and Tom has
 mixed feelings. 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. I have updated #3037 with this
 idea.

 > '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 > Methods getServerHooks/getHooksManager.  These won't be needed after
 #2980 is merged. (See end of this comment.)
 >
 > The methods getCalloutHandle(), packetProcess{Start,End} should be
 documented.
 No. They should be removed as they are not used and not implemented. They
 are a leftover from my initial idea that I forgot to remove. They are gone
 now.

 Review will continue on Monday.

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