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:  stephen
                       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 tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:14 stephen]:
 > Reviewed commit 485f7966f583f54cf2694f054de6accea9c19364
 > >> 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.
 It seems that we can get away with const_cast. That is better than
 reinterpret_cast.

 > 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.
 Moved to iface_mgr.cc. Now receive4() and receive6() use the same
 constant.

 >  * 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?
 We could run out of memory. I also expect to have more checks implemented.
 Depending on how we want to approach code hardening, doing simple check
 that early could speed up processing (like dropping unsupported message
 types - seems trivial, just compare first byte).

 >  * 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)
 > }}}
 Added break statement.
 >  * There appears to be no reason why "buf" should be declared static -
 it is not referenced outside this function.
 Performance reasons. We can reuse the same region of heap, rather than
 requiring 1500 to be reserved on stack. I admit that expected benefit of
 such approach is rather slim.

 >
 > > 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.
 I must admit that I never used lambda expressions, so it would be
 interesting to use them. On the other hand, using all new fancy features,
 just because I learned them yesterday is a very bad idea.

 > '''src/lib/dhcp/iface_mgr_linux.cc'''[[BR]]
 > This definitely needs documentation explaining the netlink interface.
 Added links to Wikipedia article that explains netlink framing and also
 reference to RFC3549.
 I think it is not the best use of our resources to document every piece of
 OS-specific interfaces.
 Also, there is #1528 ticket about Linux code refactoring. Introducting too
 many changes in 1540 ticket will only make the merge more difficult.

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

 > >> '''src/lib/dhcp/tests/pkt6_unittest.cc'''
 > >> Thanks for lining up the initialization of data. Now for a spaces
 around the "=" signs?
 Added spaces around equal signs. Also fixed other aesthetic mistakes.
 Equal signs are now
 aligned properly and there are no more leading zeros.

 > >> packUnpack test: should use "static_cast<const uint8_t*>() instead of
 a C-style cast.
 > I think these comments got missed.
 Fixed.
 >
 > '''src/lib/util/range_utilities.h'''[[BR]]
 > I've made minor changes to conform to the style guidelines.
 Thank you.
 >
 > '''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);
 > }}}
 Thank you.

 Code is ready for another round of reviews.

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