BIND 10 #1186: libdhcp implementation - DHCPv6 part

[BIND 10 Development]

#1186: libdhcp implementation - DHCPv6 part
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111011
                  Component:  dhcp   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:  878    |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:5 stephen]:
 > = Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 2 =
 >
 > Would suggest that the typedefs OptionXCollection (instead of
 OptionXLst) be used, as a "Lst" suffix implies a list.
 Renamed.

 > All method declarations - including constructors - should have full
 Doxygen headers.
 Documented.

 > unpack(): given that the option data in the buffer includes a length
 field that is the length of the option data, does unpack() really need the
 parse_len option?
 Yes. Because sometimes only certain size of the buffer needs to be parsed.
 One notable example is when you parse an option that contains suboptions.
 You don't want to parse till end of the buffer, but just a specific
 length. That length is obtained when parsing upper level option, so it
 cannot be derived here. Therefore needs to be passed as a parameter.

 > unpack(): Offsets into the buffer (and buffer sizes) should be size_t
 instead of "unsigned int".
 Agree. This also applies to many other method and code pieces throughout
 the whole libdhcp and dhcp6. Added ticket #1295 for that.

 > toText(): no other toText() method in BIND 10 takes an indentation
 argument.
 That implies that no other code in BIND10 deals with nested options. The
 idea here is that for nested options, each option will be printed indented
 for clearer representation. I understand that it is a new concept, so the
 default value of indent parameter is 0. The usual usage stays the same.

 > addOption(): shared pointer should be passed by reference (to avoid a
 needless copy when passed by value).
 Certainly not. That is where copy or shared pointer is needed. One option
 (e.g. server-id) may be added in many places, yet due to shared pointer
 only one instance will be created.

 > getOption()/delOption() should be renamed getSubOption()/delSubOption().
 I would prefer to keep it as getOption/delOption. This way, regardless of
 the object (a packet, an option or a cache), the same method can be used
 (getOption). A shorter names are easier to type.

 > Related to suboptions: would it be better for the Option class to supply
 an iterator (and associated begin()/end()) methods to allow iterations
 over suboptions contained within an option?  That way, all STL algorithms
 could be used on the suboption collection.
 That is a useful suggestion, but it is not that straightforward. If
 iterator is provided and someone modifies content of said option (e.g.
 adds new suboption) this iterator may become invalid. Having said that,
 getOption(int type) is very convenient. It must stay as it is. Added
 ticket #1296 for iterator work.

 > pack4()/pack6() - see suggestion about two Option classes, one for V4
 and one for V6.
 Added to ticket #1295.

 > What is optionLst_?
 A collection of options. Renamed to options_ and added doxygen comment.

 > Is there any reason why the data members are protected instead of
 private?
 Yes. They are expected to be used in tests.

 > '''src/lib/dhcp/option.cc'''[[BR]]
 > pack6()/pack6(): use of an !OutputBuffer will automatically take care of
 extending the buffer as needed.
 Calling realloc() many times is less optimal than calculating length and
 then doing a single new (Pkt6 approach).

 > pack4(): not clear why data_len_+4 bytes are being copied into the
 output buffer.  The type and length have already been copied and the
 pointer into the buffer advanced. According to the comment in the header
 file, data_len_ is the length of data only, so copying data_len_+4 risks
 copying beyond the allocated data area. (Note that only data_len_ bytes
 are copied in pack6().)
 That was a bug. Fixed. The reason why this wasn't caught is that work on
 v4 is not really started yet. Very small number of v4 functions were
 implemented to see if the design is feasible. Proper implementation and
 testing will commence once this ticket is resolved.

 > len(): need comment as to why the length is increments by the length of
 the contents of optionLst_.  I am assuming that these are suboptions, but
 there is nothing here or in the header file to indicate that.
 Commented how and why specific values are included in len().

 > valid(): the comments here do not relate to the code.
 Removed.

 > valid(): A simpler statement is {{{return ((universe == V4) || (universe
 == V6));}}}.
 >
 > valid(): Under what circumstances (other than wrong universe) can an
 option be invalid?
 This implementation is simple for now, but there is potential for growth
 here. Note that this is a virtual method, so derived classes may do
 additional checks, e.g. length of address list type of options may check
 if their length is divisible by 16. Some checks may be a bit more subtle,
 e.g. even though FQDN is encoded properly it may still be invalid (e.g.
 contain a single label).

 Now, for the base Option class this method is also expected to be
 expanded. We may decide to  have a table with option types and required
 lengths (many options). We may expand this into passing message type and
 then checking if option is allowed to appear in specific message type. The
 same holds true for suboptions. Some options are allowed to appear only
 within specific other options etc.

 This method will definitely grow.

 > addOption(): argument should be passed by reference.
 No, see above comment.

 > getOption()/delOption(): should be spaces around the "!=".
 Done.

 > delOption(): the intended result of the method is that after execution,
 the specified option is not in the collection.  If it wasn't there to
 begin with, after the method call the result is the same, the option is
 not there.  In this case, is it justified to return false?  A second
 question is, if this is an unexpected occurrence, isn't throwing an
 exception the better way of signalling it?
 It's not unexpected. It is faster to try to delete option and check if
 deletion was successful than search for an option and if it's there,
 delete it. Both approaches are equally fast in negative case (no option
 present), but using delOption() is twice as fast in positive case (there's
 option to be deleted). No exceptions necessary.

 > '''src/lib/dhcp/option6_addrlst.h'''[[BR]]
 > In the typedef, suggest that !AddressContainer is easier to read than
 !AddrsContainer. (The latter only saves two characters.)
 Done.

 > In the constructors, the address(es) should be passed as const.
 Done.

 > In the simplified constructor, the single address should be passed by
 reference.
 Done.

 > In the constructor for parsing the received option (and in both pack()
 and unpack()), "buf" should be passed by reference.
 No. This buffer should be kept around for until this option is valid.
 Passing shared pointer be reference is valid only for pure consumers. That
 is clearly not the case here.

 > In setAddress(), the argument should be passed by reference and should
 be const.
 Done.

 > In setAddresses(), the argument should be const.
 Done.

 > '''src/lib/dhcp/option6_addrlst.cc'''
 > Constructor taking a vector: addrs_ can be initialised in the member
 variable initializer list with {{{addrs_(addrs)}}}.
 >
 > Constructor taking a single IOAddress: addrs_ can be initialized in the
 member variable initialiser list with {{{addrs_(1, addrs)}}}.
 That's a nice trick. I didn't know that.

 > pack()/unpack(): use of "magic number" of 16.  Better is to use a named
 constant.
 After couple of minutes of digging around and failing to find already
 defined constant, I added a new one. Once I find where it is already
 defined, I'll replace it.

 > toText(): would suggest that this method just returns the addresses as a
 simple space-separated list.  This will make it easier to use in log
 messages and other places where a list is required.
 toText() is currently used to print out details of incoming and outgoing
 packets. I agree that shorter form is useful as well. If that is common
 practice in BIND10 for functions toText() to return short version, so be
 it. Full text is useful as well. This is generic problem for all classes.
 Added ticket #1297.

 > len(): suggest that the DHCPv6 option header length is defined as a
 constant somewhere.
 Added.

 Commit-id: 2894b07900ec81ed39284c7d7df5660eb8afbfc0 (part 5 of review
 changes).

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