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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 stephen]:
 > 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:

 Thanks for the review.

 Sorry for the confusion and inconvenience.  I should have created a
 separate ticket for the second subtask from the beginning (I took that
 approach for the verify tasks).

 == 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).

 Okay, I've added some more generic comments (not specific to the
 subject of this ticket, but the message class still generally misses
 documentation and tests, it would be a good idea to improve it
 gradually).

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

 I'd rather keep hiding it in the unnamed namespace.  First, since
 the existence of !MessageImpl is still publicly visible (even though
 the class definition is hidden), it's less safer in terms of
 encapsulating a totally private thing.  Second, !RenderSection doesn't
 need any internal information about !MessageImpl (or !Message) and
 doesn't have to be a part of it.  I basically prefer keeping a class
 as concise as possible even for a private pimpl class.

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

 Okay, done.

 > 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!

 Hmm, on thinking about it more, I've removed the assert.  I gave more
 detailed reason in the commit log (commit 28c50de).

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

 Yeah I know.  This is part of long standing hidden agenda.  I've been
 trying to improve it gradually, but it makes more sense to create a
 dedicated task.  Will do.

 == 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.)

 Applied the suggestion.  Actually, in some cases I intentionally use
 hardcoded magic numbers in tests so that we can detect it when the
 testee is accidentally modified in an unintended way.  But in this
 case such consideration doesn't seem to be crucial, so I modified it
 to centralize the hardcode constant.

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