BIND 10 #2749: kill io_utilities.h or make it safe

[BIND 10 Development]

#2749: kill io_utilities.h or make it safe
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  stephen
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20131015
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => stephen


Comment:

 Hi Stephen

 Here are my replies:

 Replying to [comment:14 stephen]:
 > Reviews commit c7aeb4bec30cf6373e77216058174d6d35850dde
 >
 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > generateServerID(): the construct "srvid.size() - n" is used to ensure
 > that the buffer is large enough for the writeUintNN() calls.  However,
 > as the preceding line declares the !OptionBuffer with a size of at
 > least 8, I think this construct - although general - is unnecessary.
 > I suggest preceding the three write statements with a comment along
 > the lines of "We know that srvid is at least 8 bytes in length,
 > so...".  This makes the code marginally faster.

 This has been updated.

 > (Note that the assumption that there is enough space in a buffer is
 > made in unpackOptions(), where an explicit length is passed to the
 > readUint16() calls. In that case, the assumption is made in the
 > "while" condition in that the readUint16() functions won't be executed
 > if there is less than four bytes remaining.)

 A comment has been added in that `while` loop.

 > '''src/lib/asiodns/tests/io_fetch_unittest.cc'''[[BR]]
 > Although "sizeof" is a keyword, the general usage elsewhere in BIND 10
 > appears to be treat it as a function.  So the "standard" appears to
 > write it as "sizeof(xxx)". not "sizeof (xxxgdif )".  The embedded
 > space should be removed.

 Updated.

 > '''src/lib/asiolink/tests/tcp_socket_unittest.cc'''[[BR]]
 > See comment for io_fetch_unittest.cc

 Updated.

 > !SequenceTest: to be really generic, TCPCallback::MIN_SIZE could be
 > used as the size argument for writeUint16(), as that is the size to
 > which the data buffer returned by server_cb.data() is set.

 Nod. I thought this would be at least 2 bytes in length, so I put in 2
 there. But the buffer size is better.

 > '''src/lib/asiolink/tests/udp_socket_unittest.cc'''[[BR]]
 > See comment for io_fetch_unittest.cc

 Updated.

 > '''src/lib/dhcp/option.cc'''[[BR]]
 > As readUintNN() now checks the size and throws the !OutOfRange
 > exception if the size is invalid, the equivalent checks in
 > Option::getUintNN() can be removed.

 The appropriate methods were updated.

 > Although not strictly part of this change, in the setUintNN()
 > functions the argument to the data_.resize() function call could be a
 > "sizeof(datatype)" instead of an explicit literal.

 This has also been fixed.

 > '''src/lib/dhcp/option4_addrlst.cc'''[[BR]]
 > '''src/lib/dhcp/option6_ia.cc'''[[BR]]
 > '''src/lib/dhcp/option6_iaaddr.cc'''[[BR]]
 > '''src/lib/dhcp/option6_iaprefix.cc'''[[BR]]
 > '''src/lib/dhcp/option_int.h'''[[BR]]
 > '''src/lib/dhcp/option_int_array.h'''[[BR]]
 > '''src/lib/dhcp/option_vendor.cc'''[[BR]]
 > I'm not sure that use of "distance" is strictly correct here.  The
 > readUintNN() calls require as argument the number of bytes in the
 > buffer, but "distance" returns the number of elements (which may not
 > be equal to the number of bytes) between two pointers. Use of
 > "distance" relies on !OptionBufferConstIterator being a pointer to a
 > byte. However, as most of the code here in these files make that
 > (valid) assumption, I don't think it is worth changing.

 For this point, I didn't make any changes to the code.

 > '''src/lib/resolve/tests/recursive_query_unittest_2.cc'''[[BR]]
 > '''src/lib/resolve/tests/recursive_query_unittest_3.cc'''[[BR]]
 > '''src/lib/util/io_utilities.h'''[[BR]]
 > '''src/lib/util/tests/io_utilities_unittest.cc'''[[BR]]
 > See comment for io_fetch_unittest.cc

 Updated.

-- 
Ticket URL: <https://bind10.isc.org/ticket/2749#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development