BIND 10 #497: CNAME processing / CNAME loop detection (reception)

[BIND 10 Development]

#497: CNAME processing / CNAME loop detection (reception)
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  stephen
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110222
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  3.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => stephen


Comment:

 Replying to [comment:4 stephen]:
 > '''src/lib/asiolink/asiolink.cc'''[[BR]]
 > Should perhaps add a comment to the declaration of cname_count_ in
 !RunningQuery.
 >

 ack, done

 > handleRecursiveAnswer(): in the "switch" statement, would not a
 "default" be better than explicitly listing the remaining codes
 responseClassifier could return?  (There is always a chance that
 !ResponseClassifier is altered to extend the number of codes returned - in
 which case, we probably don't want to abort with a fatal error (as the
 comment suggests) if we exit the switch.)
 >

 I prefer this construction; now if we forget a statement, the compiler
 should error (as it would if we extend the list of return codes, in which
 case i want it added here as well), instead of defaulting to some
 behaviour. The 'fatal' below would trigger if we forget a return statement
 in the cases.

 > Comment: I suggest that comments/suggestions in the comments be
 highlighted with a "TODO:" or a
 > "XXX:" - programming environments highlight these, and they are easy to
 search for.  Else we run the risk of such comments being forgotten.
 >
 > setZoneServersToRoot(): I'm not sure how BOOST_FOREACH could improve
 over the simple loop, other than to pre-increment the iterator instead of
 post-incrementing it.  (Means the iterator code does not have to save a
 copy of the iterator before it was incremented - although it is quite
 possible that the optimiser has optimised that code away.)
 >

 Removed the comment. I expect this method to go away in the near future
 anyway.

 >
 > '''src/lib/asiolink/tests/asiolink_unittest.cc'''[[BR]]
 > forwardClientTimeout(): when creating a !RecursiveQuery, the number of
 retries has been increased to 4, but the comment above it still refers to
 three retries.
 >

 ack, changed to 4 :)

 >
 >
 > '''src/lib/resolve/resolve.h'''[[BR]]
 > Comment: copySection really seems to belong to the isc::dns::Message
 class.  In fact, it does occur to me that if we were to make the section
 numbers bit masks, then we could rename the method copySections() and do
 all the copying in one call by passing in the logical OR of the sections
 we wish to copy.
 >

 Heh, I think i actually removed a different comment like 'should this be
 in lib/dns?'

 Moved to Message class (and slightly modified it, and added specific
 tests)

 I did not do the mask thing. Not sure if we should rely on Sections being,
 er, 'maskable', like that. (we can suggest it though, i just didn't do it
 now)

 > Comment: would "copyResponseMessage()" be a better name than
 "copyAnswerMessage()", as the fact that there is an "answer" section may
 make the name slightly confusing.
 >

 ack, cheanged

 >
 > '''src/lib/resolve/response_classifier.h'''[[BR]]
 > OK, although the "TODO" about code being not being used in this form can
 be removed as the predicted changes have now been made.
 >

 removed

 > Comment: descriptions of new parameters not in same style as the other
 parameter descriptions.
 >

 changed

 > '''!ChangeLog Entries'''[[BR]]
 > These are fine.
 >

 I also added doxygen values for the things Jeremy pointed out in his
 emails yesterday. (most were related, and while i was at it i added
 something for the nsas/zoneentry as well)

 changes in 81c35ef7e5e01da333654443045de3dd2663482c

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