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:         |  DHCP-20120319
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed commit 485f7966f583f54cf2694f054de6accea9c19364

 I've pushed some small changes.

 '''src/bin/dhcp4/dhcp6_srv.cc'''[[BR]]
 Dhcp6Srv::setServerID() - should use the isRangeZero() function added as
 part of this set of changes.

 '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 Minor formatting changes have been made.

 openSockets4(): Does this not leak sockets?  "sock" is a local variable
 and appears to be discarded after receiving the value returned by
 openSocket(); it is not closed.

 (Perhaps we should consider a "!SocketPtr" object - a shared_ptr to a
 socket that would close the socket when the last pointer reference it was
 destroyed?)

 openSockets6(): The logic here is very similar to openSockets4 and the
 common code could be abstracted into a separate method.

 >> IfaceMgr::send() - (V4 version) a number of the local variables could
 be declared closer to where they are first used: in the case of "cmsg" and
 "result", the declaration could be combined with initialization.
 > Done.
 Did you check that change in? "struct msghdr" and "struct iovec v" are
 still defined at the head of the function.

 BTW, the same considerations about locality of declaration apply to the V6
 version.


 >> IfaceMgr::send() - (V4 version) although the compiler apparently
 accepts it for the purposes of calculating a sizeof(), it feels
 uncomfortable to dereference pktinfo before initializing it. Instead of
 "sizeof(*pktinfo)", "sizeof(in6_pktinfo)" seems safer. Or swap this
 statement with the one before it?
 > Done.
 The V4 stuff has been moved to the Linux-specific code, but the V6 version
 of send() still has this problem.

 >> IfaceMgr::send() - (V6 version) Use a "static_int<char*>" to cast the
 data for v.iov_base instead of the C-style cast.
 > Let me know if you prefer reinterpret_cast instead.
 I do, along with a comment as to why a reinterpret_cast (as opposed to a
 static_cast) is necessary.


 Issues remaining with IfaceMgr::receive6():[[BR]]
  * "result" should be defined at point of first use.
  * Should RCVBUFSIZE be a global constant? It sets the maximum receive
 buffer size and may be used elsewhere.
  * Given that "pkt" is being created using a constructor that just copies
 "result" bytes from "buf" into internal storage, is there any reason to
 suspect that the creation of a Pkt6 could fail?
  * ifindex should be declared close to point of first use.
  * found_pktinfo should be "bool"
  * In the "while (cmsg != NULL)" loop, it appears that the the CMSG blocks
 form a linked list and you are searching for the last one that matches the
 criteria of the "if" test.  The comment implies that there is only one; if
 this is the case, add a "break" out of the loop when the first block is
 found; alternatively extend the loop termination condition to:
 {{{
 while ((cmsg != NULL) && !found_pktinfo)
 }}}
  * There appears to be no reason why "buf" should be declared static - it
 is not referenced outside this function.

 > I think we could also start using lambda expressions
 True - I think we should investigate boost::lambda.  However, in this case
 the function is descriptive of what it does and usage is clear, so
 isRangeZero() is useful in this case.

 '''src/lib/dhcp/iface_mgr_linux.cc'''[[BR]]
 This definitely needs documentation explaining the netlink interface.

 >> '''src/lib/dhcp/tests/option_unittest.cc'''
 >> v4basic_test: there is no need to check if "opt" is defined before
 calling "delete".
 >>
 >> '''src/lib/dhcp/tests/pkt6_unittest.cc'''
 >> 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.
 I think these comments got missed.

 '''src/lib/util/range_utilities.h'''[[BR]]
 I've made minor changes to conform to the style guidelines.

 '''src/lib/util/tests/buffer_unittest.cc'''[[BR]]
 OK, but made one small change to get it to compile on Linux:
 {{{
 EXPECT_EQ(vec1, vec2);
 }}}
 was changed to
 {{{
 EXPECT_TRUE(vec1 == vec2);
 }}}

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