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:  accepted
                       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 stephen):

 Reviewing changes from f60bf56ebed7189ad71d6c4ed4044d97b5e5c7a5
 to 01a1b2f4d210c62fc1d7a9b53e7967057e93d331

 '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 Unless there is some reason not to, suggest you use IPv4 addresses in the
 range 192.0.2.0/24 and domain names such as example.com, as per the coding
 guidelines. (In particular, the hardcoded DNS servers and domain names
 both correspond to real-world entities.)

 Note: as part of any refactoring, we may want to typedef the
 boost::shared_ptr templates for Option and Pkt4/6.

 processDiscover()/processRequest(): is there a need for a blank line
 between the call to copyDefaultField() and appendDefaultOptions()?  Both
 are concerned with initializing the response packet. (Also, perhaps these
 steps deserve a comment?).

 processDiscover() and processRequest() appear to be identical.  Granted
 there is packet-specific code that is not implemented yet, but the common
 functionality should be abstracted into a single method.

 copyDefaultFields(): Can set the size of a vector at initialization time
 with
 {{{
 vector<uint8_t> mac(Pkt4::MAX_CHADDR_LEN);
 }}}
 Even better though, as the next operation is to copy the chaddr field from
 "question" is to initialize it from the question, i.e.
 {{{
 vector<uint8_t> mac(question->getChaddr(),
                     question->getChaddr() + Pkt4::MAX_CHADDR_LEN);
 }}}

 copyDefaultFields()/appendDefaultOptions(): They are OK here for the
 moment.  But I can't help thinking that these logically belong to a
 "Response4" class, derived from Pkt4, that encapsulates a response.  The
 reason being that these are essentially initialization operations of the
 response and are not dependent on the state of the server.  Alternatively,
 if they are part of the Pkt4 class, operations can take place without
 involving the accessor methods. This would have implications in
 copyDefaultFields, where an intermediate vector is created to convey the
 MAC address from the question to the answer: if the operation were part of
 the Pkt4 class, the vector would not be needed.


 '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 processDiscover and processRequest tests: comment above about initializing
 a vector to a specific size applies.

 '''src/lib/dhcp/option.cc'''[[BR]]
 Not part of the change, but I was looking at the getUintX() methods and
 think there is a problem.  If I store a uint32_t value in an option, then
 retrieve it with a uint8_t, I may not get the same value stored:
 * if the value is greater than 255, the answer will be incorrect.
 * if the value is 255 or less, the answer ''may'' be correct, depending on
 whether I am running on a little-endian or big-endian machine.
 I suggest that internally the getUintX methods should retrieve the value
 using the data type of the appropriate size, then convert it to the
 correct type using boost::lexical_cast.

 '''src/lib/dhcp/pkt4.cc'''[[BR]]
 unpack(): typo in a comment "fist use of readVector".

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