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 pselkirk):

 * owner:  UnAssigned => muks


Comment:

 * The {{{CAA}}} class provides {{{getFlags}}} and {{{getTag}}}, but not
 {{{getValue}}}. If there's a reason for this, it should be documented.

 * In {{{CAAImpl}}}, you say
 {{{#!cpp
     // The first byte of this vector contains the length of the rest of
     // the vector. This byte is actually unused and is skipped when
     // reading the vector.
 }}}
 Maybe what you mean is that {{{detail::charStringToString()}}} ignores the
 length byte (in favor of its own internal length field). But it still
 seems a strange thing to say here, when we do in fact store and render the
 length byte.

 * RFC 6844 describes the presentation format of the value field as
 {{{
    Value:  Is the <character-string> encoding of the value field as
       specified in [RFC1035], Section 5.1.
 }}}
 And RFC 1035 describes escaped characters, multi-line data, and comments
 in the {{{character-string}}} format. So we should probably have test
 cases with escaped characters, multi-line data, and comments.

 * In {{{CAA::constructFromLexer}}}, you have this:
 {{{#!cpp
     if ((token.getType() != MasterToken::END_OF_FILE) &&
         (token.getType() != MasterToken::END_OF_LINE))
     {
         detail::stringToCharString(token.getStringRegion(), value);
     } else {
         // Convert it into a CharString.
         value.push_back(0);
     }
 }}}
 I think the 'else' comment belongs with the 'if' clause, in which case it
 would be documenting the obvious. It's not really clear what you meant
 here.

 * In the constructor from parameters, you do your own conversion of the
 {{{value}}} parameter from string to vector, but you don't do any
 conversion of escaped characters. Why not use {{{stringToCharString}}}
 here?

 * {{{CAA::toWire()}}} has a test for {{{impl_->tag_.empty()}}}, but the
 constructors all throw if {{{tag}}} is empty, so this seems redundant.

 * {{{Rdata_CAA_Test}}} constructor is over-indented.

 * There's a "missing value" test in {{{Rdata_CAA_Test.fields}}} that's
 redundant to {{{Rdata_CAA_Test.createFromText}}}. It's probably okay, but
 you may want to document it as such, as you do in
 {{{Rdata_CAA_Test.badText}}}.

 * {{{Rdata_CAA_Test.compare}}} should perhaps have an equality test, in
 addition to greater-than and less-than.

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