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/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.
 True. Removed universe parameter.

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

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

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

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

 > 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.
 Done.
 >
 > unpack(): should be spaces around the relational and arithmetic
 operators.
 Done.

 > 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.
 Both short and full text representation are useful. They will be
 implemented as work on #1297.

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

 > 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.
 See previous responses.
 >
 > setAddress(): argument should be const and passed by reference.
 Done.
 >
 >
 > '''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.
 I'm not sure about that. I think I read some time ago that this type of
 initialization addr_(...) differs from addr_ = ... in constructor code by
 the addr_ being initialized directly, without copy constructor. I read
 that long time ago, so I may be wrong. Changed as suggested.

 > pack(): reference to the magic "16".  This should be a symbolic
 constant.
 Added V6ADDRESS_LEN and V4ADDRESS_LEN to asiolink/io_address.h. There are
 many places throughout the code that may use it.
 >
 > pack(): same comment as above concerning the incrementing of the ptr
 object.
 Done.

 > 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).
 Yes, as pointed out in already existing TODO in Option6IAAddr::len().

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

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

 > All methods must have Doxygen headers.
 Done.

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

 > TODO comment says that member fields should be protected - they should
 be private.
 They should be protected, because they are expected to be accessed from
 tests that sooner or later will have a class derived fro Pkt6.

 > Member variables should be commented using Doxygen constructs.
 They are now.

 > addOption(): argument should be passed by reference.
 See previous responses.

 > Ideally inline comments for member variables should line up.
 Done.

 > It's confusing to mix up member variable and member function
 declarations.
 Reordered. Current order is:
 public methods
 public variables
 protected methods
 protected variable

 > '''src/lib/dhcp/pkt6.cc'''[[BR]]
 > The two constructors could be combined into one if default arguments
 were used.
 No, they couldn't. There are 3 cases needed:
 - define specific trans-id (for replying a message)
 - define trans-id to be zero (a subcase of #1, for reconfigure)
 - define random (for transmitting a message)

 They could be unified into a single constructor, but it would require
 inventing a mechanism to indicate if trans-id should be randomized or not
 (either extra parameter or some special value to passed transid). In some
 cases only certain parameters may be default. Choosing the most reasonable
 parameter order would be difficult. It looks simpler the way it is now.

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

 > len(): uses the same code as used in the Option class.  See above for
 comments.
 Similar, but different. It calculates length of the packet. For UDP it
 happens to be the same. For TCP, there is extra overhead.

 > 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.
 Ok. Removed try and catch.

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

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

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

 Commit-id: 1213572a61fdf5a81e05fa42b4d7e4ed20c99060 (Part 6 of review
 changes)

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