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

 Replying to [comment:4 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.)
 DNS server was hardcoded to 8.8.8.8. That is an actual DNS server provided
 by Google. Its use may be somewhat suboptimal, but that was one parameter
 that should work "out-of-box". Reverted it to 192.0.2.1. Changed other
 parameters to 192.0.2.0/24.

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

 > 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?).
 No. Removed and added appendRequestedOptions(). This method is a simple
 for now, but it will be greatly expanded in the future. It not just
 assumes that client requested DNS addresses and domain name.

 > 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.
 They are for now, with the exception of a different message type. They
 will be different soon, though. As both are 5 lines long, I plan to keep
 them separated. It does not make much sense to unify them, just to have
 them split in two months.

 > 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);
 > }}}
 Done.

 > 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.
 That is a good approach in theory. It is reasonable from object oriented
 programming, but it is very inconvenient from practical perspective. I
 fell into this trap with dibbler. It would be great if there were a single
 place where message is constructed and options are added. It is especially
 useful during early stage, when code is not stable and evolves quickly.
 "We didn't send option X? Need to find out why" is a very frequent
 scenario.


 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 > processDiscover and processRequest tests: comment above about
 initializing a vector to a specific size applies.
 Done.
 >
 > '''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.
 No. That would be wrong. This is really a convenient way to not have
 Option_uint8, Option_uint16 and Option_uint32 (or a template that produces
 them). If an option stores integer value on 32 bits, you really should
 read it as 32 bits.

 I agree that this could be misused to read only part of the option (e.g.
 option is 32 bits, but someone cares only for first 16 bits, so uses
 getUint16). To forbid such behavior, perhaps the best way would be to
 change size checks from less than sizeof(uintX) to not equal
 sizeof(uintX)?

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

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