BIND 10 #1540: dhcp code refactor: Pkt6 and Options for DHCPv6

[BIND 10 Development]

#1540: dhcp code refactor: Pkt6 and Options for DHCPv6
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20120305
                  Component:  dhcp   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 > The only way to cast that to struct in_pktinfo* is to use
 reinterpret_cast, which is only slightly better than pure C cast. I would
 be less readable, though. I left C style cast for now. Let me know if you
 prefer reinterpret_cast instead.
 I would prefer reinterpret_cast, but Jinmei is opposed.  We had a
 discussion about just this point on #1593 recently.  We ended up using a
 separate function to perform the conversion, inside which there was a
 construct of the form
 {{{
 static_cast<desired_type*>(static_cast<void*>(pointer));
 }}}
 For consistency, I think we should do the casting here using a similar
 function.

 > Also, can the Linux-specific part be put into a separate function
 (possibly in a separate file)?
 Taking as an example IfaceMgr::send(), instead of:
 {{{
 #if defined(OS_LINUX)
 :
 #endif
 }}}
 call something like
 {{{
 os_setup_interface(m, control_buf, control_buf_len_, pkt);
 }}}
 os_setup_interface is in a separate file and it is this file that
 betterholds the operating-specific stuff. (In this way for example, there
 would be no need to reference the Linux-specific CMSG stuff within the
 general functions.)

 > My sense of good taste (or lack of thereof) must have changed recently.
 I don't consider calling 4 methods
 (getAddress().to_v6().to_bytes().data()) to get an actual value of address
 from address object ugly anymore.
 It's called getting old.  You develop a taste for the sophisticated things
 in life - good food, fine wine, and the use of four methods to get an
 address. :-)

 '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 run(): change is OK, but suggest either:
 {{{
 Pkt4Ptr query = IfaceMgr::instance::receive4(); // Client's message
 Pkt4Ptr rsp; // Server's response
 }}}
 (this was done in Dhcp6Srv::run()) or
 {{{
 // Client's query and server's response
 Pkt4Ptr query = IfaceMgr::instance::receive4();
 Pkt4Ptr rsp;
 }}}
 (As it is, "client's message" appears to apply to both statements.)

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 setServerID(): the comment about the DUID being generated and stored is
 fine.  I think we should create a separate ticket to abstract the DUID
 into a separate class.  That way we can extend the code to add other DUID
 types without affecting operation of the server.

 setServerID(): (Comment) "all those conditions".  Do you mean "All the
 following checks"?  (Also, being pedantic, please start comments with
 capital letters.  It is easier to read them they follow the rules of
 English.)

 setServerID(): (Comment) "mac at least 6 bytes".  Should be "MAC: at least
 6 bytes".  Also, it would be more descriptive to put that comment into a
 header with the declaration of "const size_t MIN_MAC_LEN = 6".  (I note
 that there is already MAX_MAC_LEN symbol.)

 setServerID(): with regards to checking the array for all zeroes, this
 would seem to be of general usefulness and could be abstracted into a
 separate function (perhaps in a separate "util/range_utilities.h").  To
 avoid the need to allocate memory for a comparison array, I would suggest
 something like:
 {{{
 template <typename Iterator>
 bool
 isRangeZero(Iterator begin, Iterator end) {
     return (std::find_if(begin, end,
                          std::bind1st(std::not_equal_to<int>(), 0))
             == end);
 }
 }}}

 setServerID(): The fillling in of random numbers should be done in a
 separate function.  It will make it easier to update. (I'm not sure than
 rand() has particularly good randomisation properties on all operating
 systems.)  Also, if you are declaring a "standard", again it would be best
 in a separate function.


 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 ''"This test will start failing after 2030.  Hopefully we will be on
 BIND12 by then."''[[BR]]
 Optimist!

 If the isRangeZero() function above is written (and separately tested), it
 can be used in this test.

 '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 stubdetectIfaces(): call to "if_nametoindex" has spurious blank space
 inside the parentheses.

 OpenSocketX(): May want to consider defining the port as "const uint16_t"
 (and in the header file as well).  It doesn't change meaning, but it is a
 guarantee that "port" is not modified inside the method.

 send(): as "const_cast" is unusual, a description why there is a need to
 write to what is considered to be an unmodifiable buffer is needed.

 send(): "int result = sendmsg(..." is incorrectly indented.

 receive6(): I must have missed this last time round, but:
  * Declare the objects closer to where they are used.
  * Why is "v" not initialized to zero before being used?
  * Use "static_cast<void*>()" instead of "(void*)".

 '''src/lib/dhcp/iface_mgr.h'''[[BR]]
 getIFaces() actually returns a reference to the container collection
 stored inside !IfaceMgr.  The documentation should indicate that the
 reference only valid for as long as the !IfaceMgr object is valid.

 '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 unpackOptions6(): should be spaces around binary operators.

 !OptionFactoryRegister(): typo "consumed", not "consumer".

 '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 packOptions4/6 tests: EXPECT_THROW should have no spaces either side of
 the parentheses.

 '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 suboptions_unpack tests: In the "ASSERT_EQ", the expected value should be
 the first parameter.

 '''src/lib/dhcp/tests/option_unittest.cc'''BR]]
 v4basic_test: there is no need to check if "opt" is defined before calling
 "delete".

 '''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
 Thanks for lining up the initialization of data.  Now for a spaces around
 the "=" signs?

 packUnpack test: should use "static_cast<const uint8_t*>() instead of a
 C-style cast.

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