BIND 10 #1239: Receiving data over V4

[BIND 10 Development]

#1239: Receiving data over V4
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111221
                  Component:  dhcp4  |            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      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:5 stephen]:
 > Reviewing changes from 1ee6d8e7f892d929321b32e5577eb3a3be65a15e (Merge
 of 1238) to 5be5e6a639a6a1c74761cae55a97f1fa46de5c6d
 >
 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > No need to change this, as it is in a temporary debug output statement,
 but should use the static_cast<int>() construct to convert to an int.
 (Although as Pkt4::getType() returns a uint8_8, is there a need to
 convert?  Can't the ostream "<<" operator output uint8_t data?)
 Yes, it can, but uint8_t is really unsigned char, so this prints out
 mostly unreadable characters with ASCII code 1 to 9. I want them printed
 as integer values.

 >
 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 > Dhcpv4SrvTest() constructor - unlink() should be indented by one more
 space.
 Fixed.
 >
 > Dhcpv4SrvTest() constructor - "|" and ">" operators should have spaces
 round them.
 Done.

 > Dhcpv4SrvTest() constructor - should the code be reading "127.0.0.1"
 instead of "::1" for an IPV4 test?
 My initial idea was to keep it as IPv6 addresses, so openSockets4()
 wouldn't open any sockets (to avoid problems with running tests as non-
 root). However, I now modified the code to follow dhcp6. There is
 constructor parameter that specifies port on which sockets should be
 opened. Tests now open sockets (on port 10067) and pass, when run as
 regular user.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 > Dhcpv6SrvTest() constructor - "|" and ">" operators should have spaces
 round them.
 Done.

 > There appears to be some commonality between the v4 and v6 tests that we
 should perhaps explore at some later time.
 My gut feeling is that the code will diverge. There will be new parameters
 that are IP-family specific.

 > '''src/lib/dhcp/iface_mgr.{cc,h}'''[[BR]]
 > Not reviewed - these appear to be earlier version of code reviewed in
 another ticket.
 >
 > '''src/lib/dhcp/option.cc'''
 > getUint16() and getUint32(): Rather than do the bit manipulation here,
 there are functions available in src/lib/util/io_utilities.h.  Using these
 may make the code clearer.
 Now that you mentioned those methods, also tests for getUintX() would be
 useful. I think I'm getting addicted to TDD. :)

 Implemented OptionTest.getUintX. Once the test code was working, I
 refactored getUintX() to use code from io_utilities.h. Note that Jinmei
 was against using io_utilities code and said that the code should go away.

 > '''src/lib/dhcp/option.h'''[[BR]]
 > getUintXX() methods require Doxygen headers.
 Documented.

 > '''src/lib/dhcp/pkt4.cc'''[[BR]]
 > pack(): Need spaces around ">".
 I assume you meant "unpack()". Fixed that. There is no ">" in
 Pkt4::pack().

 > '''src/lib/dns/benchmarks/Makefile'''[[BR]]
 > For some reason, this file appears to have been added to git in this
 branch - it should be removed.
 For some reason I must have added this Makefile. I should be more careful.
 Removed.

 Changes checked in. Please review.

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