BIND 10 #2512: RR type implementation: CAA

[BIND 10 Development]

#2512: RR type implementation: CAA
-------------------------------------+-------------------------------------
            Reporter:  shane         |                        Owner:  muks
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  libdns++      |                    Milestone:
            Keywords:                |  bind10-1.2-release-freeze
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  5             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => muks


Comment:

 Reviewed commit a6a6cb2e33f807593e6d457bfc822a9c31ef211e.

 I've looked at the differences from commit
 11052a822a6c329a087e5590ac525ab3107926a8 which (going by the date) appears
 to be the commit that Paul reviewed and assessed whether they address
 Paul's comments.

 I've also made some observations that did not appear to be picked up by
 the earlier review.


 '''src/lib/dns/rdata/generic/caa_257.{cc,h}'''
 All the specialised methods - including the trivial getFlags() etc -
 should have Doxygen headers. (These are usually in the .h file, but it's
 not worth changing it now.)

 The documentation for getValue() should note that the reference returned
 is only valid for as long as the relevant CAA object is in existence.

 Suggest declaring "CAA::impl_" as a "boost::scoped_ptr".  Then there is no
 need to have an explicit destructor for the CAA class.  Also, in the CAA
 constructor, there will be no need for the temporary "impl_ptr" variable -
 if the constructor fails, the impl_ destructor will automatically free any
 resources.

 CAA::CAA(!InputBuffer&, size_t): tag_vec is created then immediately
 resized to tag_length.  Suggest that the vector be created with the
 correct size (by passing tag_length as the constructor argument).



 '''src/lib/dns/rdata/generic/detail/char_string.{cc,h}'''
 stringToCharStringData: I would prefix the assertion with a comment. The
 fact that there are three digits following an escape is checked in
 decimalToNumber(), so the only reason an assert() is needed is to catch
 the case where the check is removed from that function. (Also, it took a
 few seconds to work out why the assert was for n >=3 when the following
 line subtracts 2 from n.)


 '''src/lib/dns/tests/rdata_char_string_data_unittest.cc'''
 charStringDataToString test: rather than use an assert(), why not an
 ASSERT_GE? That way if the assertion does fail, the test fails in a
 graceful way rather than the the test program terminate.

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