BIND 10 #2983: Add remaining (15+) Hooks into the DHCPv4 Server

[BIND 10 Development]

#2983: Add remaining (15+) Hooks into the DHCPv4 Server
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp4         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130904
           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 marcin):

 * owner:  marcin => tomek


Comment:

 Reviewed commit 8abbdb589f6205c51f461541778f40dd355db60b

 '''!ChangeLog'''
 This is ok.

 src/bin/dhcp4/dhcp4_hooks.dox
 pkt4_receive description: Suggest that the first sentence is modified:

 ''this callout is executed when an incoming DHCPv4 packet is received and
 its content '''has been''' parsed''
 When using present tense it is confusing if the hook is called before or
 after parsing the message.

 pkt4_receive:
 ''By the time this hook is reached, that information has '''been'''
 already parsed...''

 Suggest that this:
 ''data_ contains the incoming packet as raw buffer. By the time this hook
 is reached, that information has already parsed and is available though
 other fields in the Pkt4 object. For this reason, it doesn't make sense to
 modify it.''

 is rephrased:
 ''By the time this hook is reached, the contents of the data_ field has
 been already parsed and stored in other fields. Therefore, the
 modification in the data_ field has no effect.''
 Or something like this....

 Also, this may sound better:
 ''The callout should '''sanity check all modifications''' as the server
 will use that data as is with no further checking.''

 lease4_select:
 ''a value of true '''indicates''' that the lease won't be inserted into
 the database (DISCOVER), a value of false '''indicates''' that it will
 (REQUEST).''

 ''It doesn't make sense to modify it at this time.'', I wonder if there
 are an (negative) implications of modifying it?

 ''this callout is executed when server's response is about to be
 '''sent''' back''

 ''object that contains the packet, with source and destination addresses
 '''set''', interface over which it will be '''sent and a''' list of all
 options and relay information ''

 ''All fields of the Pkt4 object can be modified at this time, except
 '''bufferOut_''' ''. The highlighted name does not conform to the naming
 convention for variables. Is it a typo in the Developer's Guide or in the
 code?

 buffer4_send:
 '' this callout is executed when server's response is about to be
 '''sent''' back to the client''

 '' object that contains the packet, with source and destination addresses
 ''''set''', interface over which it will be '''sent and''' list of all
 options and relay information''

 ''The raw on-wire form is already prepared in '''bufferOut_''' '', again
 something is wrong with this name.


 '''src/bin/dhcp4/dhcp4_messages.mes'''
 DHCP4_HOOK_LEASE4_RELEASE_SKIP: typo  in
 ''If client requested release of '''multiple''' leases (by sending
 multiple IA options), the server will '''retain''' this particular lease
 and will proceed with other renewals as usual.''

 '''DHCP4_PACKET_DROP_NO_TYPE dropped packet received on interface %1: does
 not have msg-type option'''
 It has to be rephrased because I read it as: ''There was a dropped packet
 on the interface X and then it turned out that this packet didn't have
 msg-type option'', while actually it is that ''The packet has been dropped
 and the reason for it was that it hadn't had the msg-type option''.

 Also, typo in: ''THis is a debug message''

 src/bin/dhcp4/dhcp4_srv.cc
 General comment: I suggest that you stick to 80 lines. I haven't seen this
 documented anywhere that we can do more than 80. Even if there was an
 informal agreement that we can, there are a lot of cases when you actually
 could do 80 lines by wrapping the arguments of the function invocation. I
 am using emacs on my laptop and I often have two windows open side-by-
 side. The 80 line text is displayed just fine. Anything wide gets wrapped
 and makes the code unreadable until I close the other buffer.

 Shouldn't !''Hooks!'' be named using lower case letters?
 {{{
 Dhcp4Hooks hooks;
 }}}

 With the recent changes the Dhcpv4Srv::run function is now over 250 lines
 long. It makes it very hard to read. Therefore I suggest that the parts of
 this function which process the certain callouts are enclosed in the
 private functions. These functions could return the boolean flags
 indicating whether to skip the next step or not. I guess it could even be
 a sole private function taking parameters.

 line 355: should be ''Execute all callouts registered for
 '''packet4_send''' ''

 line 394: the same issue

 line 525: the same issue

 process Discover and processRequest: Why is this? Why is this commented?
 And why it should be uncommented?
 {{{
     /// @todo Uncomment this
     // sanityCheck(request, MANDATORY);

 }}}

 line 922: packet6_send

 src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

 Tests look good to me. The only thing is to rename the test cases so as,
 instead of:
 !''simple_buffer4_receive!'' you'll have !''simpleBuffer4Receive!''.


 src/lib/dhcp/pkt4.h

 bufferOut_ should be renamed to buffer_out_.

 Also the description should start with capital letter.

 data_
 The description should start with capital letter.


 src/lib/dhcpsrv/alloc_engine.cc

 renewLease4: It should be ''Execute all callouts registered for
 '''packet4_send ''' ''

 The rollback which is at the end of the function (for memfile) will not be
 valid anymore when I finish the ticket number #3083. With this ticket, I
 am making a change in memfile to return a copy of the lease, so as the
 change to the returned structure doesn't affect the lease in the lease
 container. I suggest that you add a todo to this ticket saying that the
 rollback is removed once 3083 is merged.

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