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      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 = Review of commit f56892b514cedb0538c37db808ec543d7a4fcfaf - part 2 =


 '''src/lib/asiolink/io_address.h'''[[BR]]
 The "data" argument in "from_bytes" should be declared "const uint8_t*" as
 it is binary data.

 There is the possibility that in use, a data buffer of the wrong length
 will be passed through.  Is it feasible/useful to also pass through a
 buffer length, and have the method check that as well?

 '''src/lib/asiolink/io_address.cc'''[[BR]]
 IOAddress::from_bytes(): the buffer addr_str does not need to be static.
 Also, it can be declared closer to the point of first use, immediately
 before the call to inet_ntop().

 IOAddress::from_bytes(): addr_str is declared as length INET6_ADDRSTRLEN.
 In fact, as it will hold either an IPv4 address string or an IPv6 address
 string, the length should really be max(INET6_ADDRSTRLEN,
 INET_ADDRSTRLEN).  True, the IPv6 constant is larger, but I would add a:

 {{{BOOST_STATIC_ASSERT(INET6_ADDRSTRLEN >= INET_ADDRSTRLEN)}}}

 ...to the code to emphasise the dual nature of the buffer.

 IOAddress::from_bytes(): missing space before INET6_ADDRSTRLEN in call to
 inet_ntop().

 IOAddress::from_bytes(): minor semantic point - I suggest that the second
 "if" be an "else if", to emphasise that this is part of the "if" block
 checking for the validity of the arguments.

 '''src/lib/dhcp/Makefile.am'''[[BR]]
 Why is "-Wall" added to the C++ flags varianble. Also, the comment about
 turning off -Werror does not appear to be relevant here.

 You should not need to include any LOG4CPLUS symbols in the lipdhcp_la_xxx
 symbol definitions; those should be hidden by the logging library.


 '''src/lib/dhcp/dhcp6.h'''[[BR]]
 Seems to be a mis-alignment of the values for options codes 6, 13, 17, 27,
 39, 43 and 45; the values should be vertically aligned (looks neater).

 #define is generally discouraged in C++, with "const" declarations being
 preferred.  In this case, as there is a relationship between the D6O
 values - they are DHCPv6 options - using an "enum" would seem to be
 better.  Each values can be implicitly converted to an "int", although the
 main advantage is that some form of type checking can be employed where
 the value is passed as an argument.

 Putting the declarations into a enum also allows the omission of the
 prefixes (e.g.
 {{{
 #define STATUS_Success 0
 }}}
 ... becomes ...
 {{{
 enum DHCPv6Status {
    Success = 0,
         :
 };
 }}}
 (... although I would suggest retaining a prefix for DHCPv6 options but
 changing it from D6O_ to OPTION_, so as to match up with the entries in
 the IANA registry.)

 Suggest the declarations of DHCPv4 and DHCPv6 symbols are put into
 separate namespaces.  In this way the prefix to the symbol names can be
 omitted, with the namespace being used to disambiguate options with
 similar names.

 Suggest changing the D6O prefix to OPTION_ to indicate a V6 option (this
 is used in the IANA registry).  Also, perhaps STATUS6_ and MSG6_ to
 indicate V6 status codes and message types?

 Should perhaps indicate in which RFC options 2 through 20 are defined.
 Also, what is a "paa-option" (option 40)?


 '''src/lib/dhcp/libdhcp.h'''[[BR]]
 The idea of a "LibDHCP" class is not really object oriented - it seems to
 imply a general collection of loosely-related functions, which is more of
 a C paradigm.  What we have in this class seems more to be an
 !OptionCollection object - an object capable of disassembling and
 assembling

 Related to the above, there is are packOptions6() and unpackOptions6()
 methods and an !OptionFactoryRegister() method that requires an indication
 of the "universe" in which it operates.  All of these seem to be arguing
 for two classes, one for v6 and one for v4, each with pack, unpack and
 factoryRegister methods.

 No version() method is required - versioning of all BIND 10 libraries will
 be taken care of in a general manner.

 '''src/lib/dhcp/libdhcp.cc'''[[BR]]
 unpackOptions6(): Using the !InputBuffer class in "util" allows items of
 data to be read from a buffer in 16-bit words.

 unpackOptions6(): Should be spaces around relational operators (in "while
 (offset<end)").

 unpackOptions6(): The "while" loop just checks for offset < end.  The
 extraction of the type and length fields require that four bytes be
 present.  The check should be "offset + 4 <= end" (to allow for an option
 with no data).

 unpackOptions6(): Presumably unpack6() will ultimately use the factories
 registered in this class?

 packOptions6(): If "options" is not being changed, can the argument be
 "const" and the iterator a const_iterator?

 packOptions6(): Using an !OutputBuffer from the util directory (instead of
 the shared array of char) will allow for automatic extension of the buffer
 as the options are added.

 '''src/lib/dhcp/option.h'''[[BR]]
 Is there a case for separate Option6 and Option4 classes, possibly derived
 from an abstract base Option class?

 Would suggest that the typedefs OptionXCollection (instead of OptionXLst)
 be used, as a "Lst" suffix implies a list.

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

 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?

 unpack(): Offsets into the buffer (and buffer sizes) should be size_t
 instead of "unsigned int".

 toText(): no other toText() method in BIND 10 takes an indentation
 argument.

 addOption(): shared pointer should be passed by reference (to avoid a
 needless copy when passed by value).

 getOption()/delOption() should be renamed getSubOption()/delSubOption().

 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.

 pack4()/pack6() - see suggestion about two Option classes, one for V4 and
 one for V6.

 What is optionLst_?

 Is there any reason why the data members are protected instead of private?

 '''src/lib/dhcp/option.cc'''[[BR]]
 pack6()/pack6(): use of an !OutputBuffer will automatically take care of
 extending the buffer as needed.

 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().)

 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.

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

 valid(): A simpler statement is {{{return ((universe == V4) || (universe
 == V6));}}}.

 valid(): Under what circumstances (other than wrong universe) can an
 option be invalid?

 addOption(): argument should be passed by reference.

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

 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?


 '''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.)

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

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

 In the constructor for parsing the received option (and in both pack() and
 unpack()), "buf" should be passed by reference.

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

 In setAddresses(), the argument should be const.

 '''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)}}}.

 pack()/unpack(): use of "magic number" of 16.  Better is to use a named
 constant.

 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.

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


 '''src/lib/dhcp/option6_ia.h'''[[BR]]
 Not sure why Option6IA constructor needs a "Universe" parameter.  Doesn't
 this option only exist in V6, in which case there is no need for the
 Option6IA constructor to have it as an argument; when calling the base
 constructor, it could pass V6 across explicitly.

 Although overridden methods do not need a full Doxygen header, methods
 declared in this class - even trivial ones - should have such
 documentation.

 Arguments in argument lists should be on the same line where possible (up
 to a line length of 80).

 Constructor/pack()/unpack(): buf should be passed by reference to avoid a
 needless copy of the shared_array object.



 '''src/lib/dhcp/option6_ia.cc'''[[BR]]
 pack(): should use !OutputBuffer to take account of field extension.

 pack(): there should be a comment (and possibly an assert()) to ensure
 that len() returns a value >= 16.

 pack(): when incrementing the pointer, something like "ptr +=
 sizeof(uint32_t)" is more descriptive that "ptr += 4" when coming
 immediately after the insertion of a uint32_t.

 unpack(): should be spaces around the relational and arithmetic operators.

 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.  If there is a need to
 identify the type of this option, then there should be a method that
 returns a class describing the option type, this class having its own
 toText() method.


 '''src/lib/dhcp/option6_iaaddr.h'''[[BR]]
 Although overridden methods do not need a full Doxygen header, methods
 declared in this header - even trivial ones - should have such
 documentation.

 Arguments in argument lists should be on the same line where possible (up
 to a line length of 80).

 Constructor/pack()/unpack(): buf should be passed by reference to avoid a
 needless copy of the shared_array object.

 setAddress(): argument should be const and passed by reference.


 '''src/lib/dhcp/option6_iaaddr.cc'''[[BR]]
 Second constructor: when initializing addr_, just pass the string "::" as
 the argument.  As it is, an anonymous IOAddress object is being created
 then addr_ is being initialized via the copy constructor.

 pack(): reference to the magic "16".  This should be a symbolic constant.

 pack(): same comment as above concerning the incrementing of the ptr
 object.

 Note: the part of pack() that adds the type and length to the header is
 common to both Option6IA and Option6IAAddr; presumably it applies to all
 V6 options.  This suggests that it should be a method in the base class
 that can be called by all derived classes.  (It would also mean that the
 type_ field can remain private to the base class.)

 Similarly, the only difference between Option6IA::len() and
 Option6IAAddr::len() is the size of the header; the rest of the code in
 the method is concerned with summing the length of the suboptions.  This
 suggests that the code for summing suboption lengths could be put in the
 base Option class.  Better though would be to define Option6Lst as a
 separate class and provide a len() method within it (as it is returning an
 attribute of the object).

 '''src/lib/dhcp/pkt6.h'''[[BR]]
 Public enums should not end with an underscore. (Trailing underscores are
 used in private member variables.)

 Its is not BIND 10 convention to indent the class definition within a
 namespace.

 All methods must have Doxygen headers.

 Inconsistent definition of default values in the Pkt6 constructor and
 setProto (UDP v Pkt6::UDP)

 TODO comment says that member fields should be protected - they should be
 private.

 Member variables should be commented using Doxygen constructs.

 addOption(): argument should be passed by reference.

 Ideally inline comments for member variables should line up.

 It's confusing to mix up member variable and member function declarations.

 '''src/lib/dhcp/pkt6.cc'''[[BR]]
 The two constructors could be combined into one if default arguments were
 used.

 All member variables that are basic types (ints etc.) should be
 initialized in the constructor, else their value is not defined.

 len(): uses the same code as used in the Option class.  See above for
 comments.

 packUDP(): if the system fails to allocate memory, this is fairly fatal.
 It seems logical to allow the exception to propagate instead of returning
 a value as there is little than can be done.

 unpackUDP(): data_ is declared as a shared_array of char, yet the unpack
 method casts individual bytes to unsigned char.  Suggest declaring data_
 as a shared_array of uint8_t.

 getType(): this is sufficiently trivial that it could be defined in the
 header file.

 As there are no virtual functions in the class, there is no need to
 declare and define a destructor - the default one provided by the compiler
 will be sufficient.


 '''src/lib/dhcp/tests/libdhcp_unittest.cc'''[[BR]]
 Spaces required around relational and arithmetic operators and
 before/after equal signs.

 packOptions6 test: "expected" array is missing comment for opt5.

 unpackOptions6 test: "packed" array is the same as the "expected" array in
 the previous test.  The declaration could be shared between them.


 '''src/lib/dhcp/tests/option6_addrlst_unittest.cc'''[[BR]]
 basic test: Comment - sampledata, expected1, expected2 and expected3
 contain much the same data.  Isn't there some way to declare it once and
 create the arrays dynamically (so avoiding the chance of errors if the
 code is changed in the future).  The same goes for the text representation
 of the addresses.

 Is there a reason to use IPV6 addresses other than 2001:db8::/32 for tests
 (as required in the coding guidelines).  If so, the reason should be
 documented.

 All tests.  Is there a need to declare opt1/2/3 as a pointer to
 Option6AddrLst and delete them afterwards?  As they are being initialized
 at creation, why not declare them as objects of type Option6AddrLst?  It
 eliminates the need to delete them at the end of the test method.
 (Alternatively, encapsulate them in a std::auto_ptr to ensure that they
 are deleted even if the method returns prematurely.)

 '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 Spaces required around relational and arithmetic operators and
 before/after equal signs.

 Do the Option6IA objects need to be created via "new"? Can't they be
 automatically allocated (and so avoid the need for "delete" statements?)

 '''src/lib/dhcp/tests/option6_iaaddr_unittest.cc'''[[BR]]
 Spaces required around relational and arithmetic operators and
 before/after equal signs.

 basic test: we are using US conventions in documentation, so the comment
 by simple_buf[23] should read "3,000,000,000" (i.e. commas, not periods)

 There should be a test for len().

 '''src/lib/dhcp/tests/option_unittest.cc'''[[BR]]
 Do the Option objects need to be created via "new"? Can't they be
 automatically allocated (and so avoid the need for "delete" statements?)

 '''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
 Is there a reason to use IPV6 addresses other than 2001:db8::/32 for tests
 (as required in the coding guidelines).  If so, the reason should be
 documented.

 capture1(): wouldn't it be easier to declare the data in a 98-element
 array and then memcpy it to pkt->data?  (As it is, the columns of the set
 of assignments do not line up).

 parse_solicit1: as this is a test of unpack(), the test might be better
 named unpack_solicit1.

 No tests for len(), packUPD(), unpackUDP(), getType().

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