BIND 10 #1350: V4 packet library - dedicated DHCPv4 options needed

[BIND 10 Development]

#1350: V4 packet library - dedicated DHCPv4 options needed
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111109
                  Component:  dhcp   |            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      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed commit 36dc8dd6f15a42f401ffa32829ed7c436e529eb3.

 '''src/lib/dhcp/option.h'''[[BR]]
 Some comments not related to this change:

 len(): Not related to this change, but len() should return a uint16_t
 instead of "unsigned short".  The former is guaranteed 16 bits long, the
 latter has no guaranteed length (although is almost always 16 bits).

 getData(): Header comment not quite correct: it does not return NULL if
 there is no data, it returns a reference to an empty vector.

 getType()/getData(): as these are just methods just comprising "return
 (xxx_)", they are sufficiently trivial to put the body in the header file.

 '''src/lib/dhcp/option4_addrlst.h'''[[BR]]
 Typos in constructor headers - "contructor", "Constrcutor".

 Constructor for received options (header comment): the constructor is not
 simplar to the previous one: it is explicitly for the case where the
 addresses are presented as a byte array, as in the case when parsing the
 address list in a received packet.

 '''src/lib/dhcp/option4_addrlst.cc'''[[BR]]
 Constructor for received options: all OK, but the use of creating an
 intermediate vector to get the length is not necessary.  The STL function
 [http://www.cplusplus.com/reference/std/iterator/distance distance] will
 do that without the overhead of allocating memory.

 pack4: Using "addr++" instead of "++addr".  Not really an issue here but
 in general, unless a post-increment is really needed, use a pre-increment:
 it can avoid an unnecessary object copy. (A good explanation can be found
 [http://stackoverflow.com/questions/24901/is-there-a-performance-
 difference-between-i-and-i-in-c here].)

 to_text(): Single-line for-loops should include braces.

 to_text(): The current implementation ends up with a trailing space.
 Instead of that, remove the space after the ":" in the line before the
 loop, and append a space before "*addr" instead of after it.

 to_text(): Just a style comment - here addrs_ is iterated over using a
 "for loop".  In pack4(), it is a "while" loop.

 '''src/lib/dhcp/pkt4.cc'''[[BR]]
 Doesn't just adding one byte to the output buffer - as opposed to creating
 an option and using packOptions - run the risk of overflowing the packet?
 It would be better to put this functionality into packOptions: check the
 last option output processed and if not an END option, create one and
 append it to the buffer using pack4().

 '''General'''[[BR]]
 There are sufficient similarities in Option4AddrLst and Option4AddrLst to
 derive them from a common ancestor class.  Assuming that the base class
 holds a  "Universe" member to distinguish between header sizes and address
 families etc., addAddress, setAddress, setAddresses, len, to_text could
 all be put in a base class as they are more or less identical.  In
 addition, if the type were passed as a uint16_t and the DHCPv4 code threw
 an exception if it were greater than 255, some of the constructors have
 common code.

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