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]:
 > '''src/lib/asiolink/io_address.h'''[[BR]]
 > The "data" argument in "from_bytes" should be declared "const uint8_t*"
 as it is binary data.
 Done.

 > 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?
 It is possible, but not very useful. Eventually we will have to call
 inet_ntop() and see if the string is sane.

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

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

 > 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.
 Done as suggested. Nevertheless, the compiled code will be exactly the
 same in both cases. In my opinion previous code was more readable. That is
 not a problem in such simple code, so it doesn't matter much.

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

 > '''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.
 That file is a verbatim import from ISC DHCP. Due to possible code
 sharing, I prefer to not implement any changes, unless necessary. I fixed
 offset, but I will leave defines are they are for now.

 > 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.)
 That would be useful. Having options named exactly the same way as in RFC
 would be nice. Unfortunately, that means that there will be a conflict in
 ISC DHCP. That was the reason to start with D6O_. We need to decide if we
 want to maintain dhcp6.h as a common file or not.

 > 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.
 Will do this during work on DHCPv4 part.

 > 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)?
 I have no idea. This file came from ISC DHCP.

 > '''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.
 That is a work in progress. The idea is to

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

 > '''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.
 InputBuffer is unsuitable here. Hooks may modify incoming packet, so input
 buffer is actually read-write.

 > 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?
 Yes, it will eventually. Option extension framework is not planned for
 phase 1, however.
 >
 > packOptions6(): If "options" is not being changed, can the argument be
 "const" and the iterator a const_iterator?
 Added const in packOptions6 (const options) and unpackOptions6 (const
 buffer).

 > 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.
 Changing type of packet depending on if it is received or sent is not a
 good idea in DHCP. Why changing type of packet container? That would
 double number of required methods (or introduction of a packet hierarchy).

 Also, calling realloc many times is inefficient. In DHCP, size of the
 buffer is known once all options are added to buffer. There's len() method
 that returns number of bytes needed. A single realloc() is sufficient.

 > '''src/lib/dhcp/option.h'''[[BR]]
 > Is there a case for separate Option6 and Option4 classes, possibly
 derived from an abstract base Option class?
 v4 and v6 options are structured completely different. type and length
 fields are of different sizes, there are 1-byte long options in DHCPv4,
 string encoding is sometimes different etc. I can hardly find any
 reasonable case where a method could handle both v4 and v6 options.

 Commit-id:  e4ad0dcba0cd6ab978b826b824f663596349875c
 Rest of the review will continue in the next comment (tomorrow).

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