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

[BIND 10 Development]

#1230: V4 state machine - respond to DISCOVER packets
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  shane
  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 shane):

 I ran through the differences since
 e945e57855e4252349635d4b1f90f11626da7508, because I wasn't sure what I was
 supposed to check exactly.

 Note that I have *not* run the code through "make distcheck" and the like,
 just read it. I'm going to pass it back to Tomek so he can address these
 points.

 dhcp4_srv.cc:

   Do we need the ECHO_SERVER constant and code anymore? Maybe we can
 remove that.

   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. :)

   Pkt4::unpack() should be changed to void, since it no longer returns
 false on error.

   Using:

 {{{
    } catch (const std::exception& e) {
 }}}

   Seems unsafe to me. However since there's a TODO there I suppose it's
 okay for now.

   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.

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

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

   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

 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.

 dhcp6_srv.cc:

   Naming suggestion for assignLease() probably applies to applyLeases(),
 so tryApplyLeases() perhaps...

 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.

 pkt4.cc:

   Should unpack() return true if it is not a DHCP packet? Does this mean
 we're going to handle BOOTP?

 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?

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