BIND 10 #2512: RR type implementation: CAA

[BIND 10 Development]

#2512: RR type implementation: CAA
-------------------------------------+-------------------------------------
            Reporter:  shane         |                        Owner:
                Type:  enhancement   |  UnAssigned
            Priority:  high          |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  bind10-1.2-release-freeze
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 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 => UnAssigned


Comment:

 Replying to [comment:7 pselkirk]:
 > * The {{{CAA}}} class provides {{{getFlags}}} and {{{getTag}}}, but
 > * not {{{getValue}}}. If there's a reason for this, it should be
 > * documented.

 `getValue()` and a unittest for it have been added.

 > * 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.

 The code which the above describes was correctly written (with an unused
 length byte in the vector to simulate a `CharString`), but it seems this
 was confusing to a reader.

 So I've now introduced a `CharStringData` type that does away with the
 length byte prefix and the 255-byte limit (from `<character-string>` as
 it does not apply to RR types like CAA). With this, the code should be
 easier to follow.

 Tests have been added for escaped characters, etc.

 > * 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?

 This has been updated, and tests have been added for the
 constructor-from-params that use escaping, etc.

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

 This has been changed to an assertion now.

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

 This has been fixed.

 > * 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}}}.

 Such a comment has been added.

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

 There is one in the branch in the `.createFromWire` test, but I've
 copied it into the `.compare` test now with a comment.

 Putting back to review.

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