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-20111207
                  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:5 stephen]:
 > Reviewed commit 36dc8dd6f15a42f401ffa32829ed7c436e529eb3.
 Thank you.

 > '''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).
 Done. In option.h and all derived classes.

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

 > getType()/getData(): as these are just methods just comprising "return
 (xxx_)", they are sufficiently trivial to put the body in the header file.
 Done.
 >
 > '''src/lib/dhcp/option4_addrlst.h'''[[BR]]
 > Typos in constructor headers - "contructor", "Constrcutor".
 Fixed.

 > 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.
 Removed comment about similarity.

 > '''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.
 Nice trick. I didn't know that.

 > 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].)
 Yes, I understand the difference. For some cases (e.g. for loop) I use it
 automatically. Unfortunately, I sometimes forget about this rule in other
 cases.

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

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

 > to_text(): Just a style comment - here addrs_ is iterated over using a
 "for loop".  In pack4(), it is a "while" loop.
 I actually prefer variety. Using different techniques is an opportunity to
 gain (and maintain) experience. Using the same type of loops does not
 improve readability as everyone who is looking into the code is expected
 to be familiar with all loop concepts. It would be possible to say "we
 should use X loop type only and deprecate all others", but that would be
 unwise.

 > '''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?
 No. We are using OutputBuffer that is made of rubber. It will expand. :)
 Seriously speaking, we call writeUint8 that calls ensureAllocated(). This
 method in turn checks if there is enough space available. In case of
 running out of space, extra memory will be allocated.

 > 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().
 No, for couple of reasons. In some cases we want END option to be added
 (assembling v4 packet options field) and in some case we don't (assembling
 sub-options in v4, assembling v6 options). We could add a parameter to
 packOptions() to explicitly command that, but this would add extra
 complexity and sanity checks (e.g. what if someone wants to add END option
 after v6 options?). In fact, the only case when we want END option to be
 added is when assembling v4 packet. That's why we are adding the byte
 there.

 > '''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.
 I assume you meant Option6_AddrLst and Option4_AddrLst.

 Agree. Unfortunately, v4 options use Input/OutputBuffer, while v6 option
 use shared_array<uint8_t>. This unification should be done once #1312 is
 complete. Added a comment to #1312 about merging AddrLst classes.

 Do you think this change should be mentioned in ChangeLog? Currently the
 only text is entry  304 (that was merged already as part of 1228 ticket).

 Please review.

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