BIND 10 #871: TSIG signing, part 2

[BIND 10 Development]

#871: TSIG signing, part 2
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:         |            Resolution:
  critical                           |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  DNSPacket API                      |  Estimated Difficulty:  2.0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 I've got a bit confused with the various branches and what commits to
 review/not reviews (and the git inheritance tree is also complicated).  To
 summarise: at fd45c40 (the commit that was the subject of review on ticket
 #812), trac812 was branched:

 * one child was merged into master (starting at commit cc788af) has a set
 of commits with log messages starting [trac812next].  The final commit in
 this branch was 28143be.
 * the other branch (with commits labelled ![trac812]) addresses changes
 made in the review of #812.  This was subsequently merged into master.

 The two branches (trac812next and master) were merged at commit 364be72;
 at this point, branch trac871 starts.

 Accordingly, the review is divided into two parts:

 * Review of [trac812next] changes - differences between cc788af and
 28143be.
 * Review of ![trac871] - differences between 364be72 and the latest
 version (31a6f34).

 == trac812next ==

 '''src/lib/dns/message.{cc, h}'''[[BR]]
 Although it's only been moved within the .cc file, some documentation
 about the !RenderSection struct would be useful (the two terse one-line
 comments don't really give a lot of information).

 Comment: a matter of taste, but as !RenderSection is now only used within
 MessageImpl::toWire() (and !MessageImpl is defined within the .cc file),
 it could be declared as part of !MessageImpl instead of being inside an
 anonymous namespace.

 Suggest that the "TBD" comments in MessageImpl::toWire() be renamed "TODO"
 in common with most "to do" comments.

 It took me a moment to realise why the {{{assert(arcount != 0)}}} is there
 and what condition could trigger it.  The condition is very unlikely I
 agree, but congratulations on foreseeing the possibility!


 '''src/lib/dns/tests/testdata/gen-wiredata.py.in'''[[BR]]
 There appears to be very little documentation on this program or on the
 format of its input files - yet a lot of tests appear to be based on it.
 I would suggest that a task be added to write such documentation.  Having
 said that, the changes appear to be OK.



 == trac871 ==

 '''src/lib/dns/tests/tsigrecord_unittest.cc'''[[BR]]
 Comment: Given that the "getLength" test has verified that
 TSIGRecord::getLength() works and gives the correct size, in the
 "recordTooLongToWire" test instead of setting the render's length limit to
 a hard-coded 84, it could be set to "test_record.getLength() - 1" instead.
 (Don't bother changing it unless you anticipate future changes - it was
 only made because it was noticed that a change to the TSIG record length
 resulted in two changes in the file.)

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