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/tests/libdhcp_unittest.cc'''[[BR]]
 > Spaces required around relational and arithmetic operators and
 before/after equal signs.
 Done.

 > packOptions6 test: "expected" array is missing comment for opt5.
 Added.

 > unpackOptions6 test: "packed" array is the same as the "expected" array
 in the previous test.  The declaration could be shared between them.
 Done.

 > '''src/lib/dhcp/tests/option6_addrlst_unittest.cc'''[[BR]]
 > basic test: Comment - sampledata, expected1, expected2 and expected3
 contain much the same data.  Isn't there some way to declare it once and
 create the arrays dynamically (so avoiding the chance of errors if the
 code is changed in the future).  The same goes for the
 text representation of the addresses.
 We could generate this on the fly, but the whole point of having defined
 expected values is to not use any generation mechanisms that may have
 bugs. The code may change in the future, but the format of DHCPv6 options
 will not. Tests are not production code. Test code should favor simplicity
 over efficiency.

 > Is there a reason to use IPV6 addresses other than 2001:db8::/32 for
 tests (as required in the coding guidelines).  If so, the reason should be
 documented.
 There are specific reasons. I added comments explaining them. I also sent
 mail to bind10-dev asking for permission to change coding guidelines.
 2001:db8::/32 class is for documentation only, NOT for testing.

 > All tests.  Is there a need to declare opt1/2/3 as a pointer to
 Option6AddrLst and delete them afterwards?  As they are being initialized
 at creation, why not declare them as objects of type Option6AddrLst?  It
 eliminates the need to delete them at the end of the test method.
 (Alternatively, encapsulate them in a std::auto_ptr to ensure that they
 are deleted even if the method returns prematurely.)
 In tests, I prefer to simplest approach possible. There is nothing that
 could go wrong with new and delete (except running out of memory, but that
 would indicate severe memory leak in code).

 > '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 > Spaces required around relational and arithmetic operators and
 before/after equal signs.
 Done.

 > Do the Option6IA objects need to be created via "new"? Can't they be
 automatically allocated (and so avoid the need for "delete" statements?)
 Using new/delete it the simplest way to test whole lifecycle of an abject
 - creation, use and destruction. Also, object can be destroyed on demand.
 Added extra EXPECT_NO_THROW around delete instructions.

 >
 > '''src/lib/dhcp/tests/option6_iaaddr_unittest.cc'''[[BR]]
 > Spaces required around relational and arithmetic operators and
 before/after equal signs.
 Done.

 > basic test: we are using US conventions in documentation, so the comment
 by simple_buf[23] should read "3,000,000,000" (i.e. commas, not periods)
 Fixed.

 > There should be a test for len().
 Added.

 > '''src/lib/dhcp/tests/option_unittest.cc'''[[BR]]
 > Do the Option objects need to be created via "new"? Can't they be
 automatically allocated (and so avoid the need for "delete" statements?)
 See above.
 >
 > '''src/lib/dhcp/tests/pkt6_unittest.cc'''[[BR]]
 > Is there a reason to use IPV6 addresses other than 2001:db8::/32 for
 tests (as required in the coding guidelines).  If so, the reason should be
 documented.
 > capture1(): wouldn't it be easier to declare the data in a 98-element
 array and then memcpy it to pkt->data?  (As it is, the columns of the set
 of assignments do not line up).

 This code is auto-generated by packet capture. This is a real-life packet.
 Also see my post to bind10-dev about improper statements in coding
 guidelines. The packet capture/generator may be improved as suggested, but
 that is a very low priority task for now. Added TODO in
 src/bin/dhcp6/tests/iface_mgr_unittest.cc.

 > parse_solicit1: as this is a test of unpack(), the test might be better
 named unpack_solicit1.
 Renamed.

 > No tests for len(), packUPD(), unpackUDP(), getType().
 Tests implemented.

 Commit-id d3f306a14de315e82d5e18208ddac116af239131 (Part 7 of review
 changes.

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