BIND 10 #1230: V4 state machine - respond to DISCOVER packets

[BIND 10 Development]

#1230: V4 state machine - respond to DISCOVER packets
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111230
                  Component:  dhcp4  |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:8 shane]:
 > dhcp4_srv.cc:
 >
 >   Do we need the ECHO_SERVER constant and code anymore? Maybe we can
 remove that.
 Removed.
 >   There's a comment to uncomment openSockets4() once a #1238, #992, and
 #1239 are merged... they have been, and the #if 0 removed, so the comment
 can go too. :)
 Done.
 >
 >   Pkt4::unpack() should be changed to void, since it no longer returns
 false on error.
 Done.

 >   There's a comment to uncomment code once ticket #1240 is merged, and
 that has been done. Probably best to simply remove the #if rather than
 changing #if 0 to #if 1.
 Done.

 >   I know everything is very, very early, but maybe
 appendRequestedOptions() should state basically what it should do, which
 is look through the parameter request list of the client (right?). (This
 comment already exists on the DHCPv6 side.)
 Done.

 >   Maybe assignLease() should be called tryAssignLease(), as presumably
 it can fail.
 Done.

 >   I disagree with Stephen about merging processDiscover() and
 processRequest() - they should be separated. I do wonder what our current
 server does if we get a request that has no previous discover though. :-P
 When Stephen was doing review, processDiscover() and processRequest()
 contained the code that is now in appendRequestedOptions(),
 copyDefaultFields(), etc.

 As for what the code does - well, it handles REQUEST as any other decent
 server would do. It assigns a lease, of course! :)

 > dhcp4_srv.hh:
 >
 >   Description of appendRequestedOptions() should not say "(sent out to
 all client)", since there may be some magic heuristics determining which
 clients get enforced options. Probably just leave that parenthetical
 remark out.
 Removed.

 > dhcp6_srv.cc:
 >
 >   Naming suggestion for assignLease() probably applies to applyLeases(),
 so tryApplyLeases() perhaps...
 Renamed.

 > iface_mgr.cc:
 >
 >   Comment in openSockets4() is wrong, should be:
 >
 > {{{
 >     // skip non-IPv4 addresses
 > }}}
 >
 >   There are systems that don't have IP_PKTINFO, so we'll need to revisit
 our basic approach when we move past Linux. Basically we'll need to create
 a separate socket for each interface.
 True. That's the idea behind openSockets4(). It iterates thru all
 interfaces and opens separate socket on each.


 > pkt4.cc:
 >
 >   Should unpack() return true if it is not a DHCP packet? Does this mean
 we're going to handle BOOTP?
 Unpack is now void unpack() and it throws if BOOTP packet is received.
 >
 > pkt4.h:
 >
 >   The comment for check() says that check() should note that it is
 performed after unpack() and that they should be separate, but unpack()
 actually does invoke check(). What's the story here?
 It is going to be separated for hooks. The idea is that hook can "fix" the
 packet *after* it is parsed, but before it is checked. There's real-life
 use case for that: there are networks that have faulty DHCP clients that
 cannot be upgraded. Someone may want to implement hooks that will fix
 requests coming from those clients. another thing is that it will be
 useful for testing purposes to have it separated, so we can unpack message
 with options 1,2,3,4 and not require 53 (message type) to be always
 present etc.

 Added appropriate TODOs as I don't want to rearrange the code now.

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