BIND 10 #1186: libdhcp implementation - DHCPv6 part

[BIND 10 Development]

#1186: libdhcp implementation - DHCPv6 part
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  stephen
                       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 stephen):

 Replying to [comment:8 tomek]:

 >> '''src/lib/dhcp/dhcp6.h'''[[BR]]
 >> Seems to be a mis-alignment of the values for options
 > That file is a verbatim import from ISC DHCP...
 OK, I hadn't realised them.  No changes needed (although leave in the ones
 you have done).

 >> Putting the declarations into a enum also allows...
 > 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.
 Do what you need to do for now.  In the longer-term, we need to decide how
 to handle the compatibility between Kea and DHCPv4.

 >> '''src/lib/dhcp/libdhcp.h'''
 >> 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
 I think the rest of the comment got lost :-).  But we need to look at the
 design to see if "LibDHCP" is really a natural object.


 >> '''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.
 Fair point, but I think encapsulating the input data in some object that
 simplifies the packing/unpacking of data in it is worth some investigation
 at some time in the future.

 >> unpackOptions6(): Should be spaces around relational operators (in
 "while (offset<end)").
 Being pedantic, it's not usual in BIND 10 code to put spaces after an
 opening parenthesis or before a closing one.  (We tend to obey the Google
 guidelines here: http://google-
 styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whitespace)

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