BIND 10 #595: Refactor function MessageEntry::getRRsetTrustLevel()

[BIND 10 Development]

#595: Refactor function MessageEntry::getRRsetTrustLevel()
-------------------------------------+-------------------------------------
                 Reporter:           |                Owner:  jelte
  zhanglikun                         |               Status:  reviewing
                     Type:  defect   |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110308
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  0.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by zhanglikun):

 * owner:  zhanglikun => jelte


Comment:

 Replying to [comment:3 jelte]:
 >
 > in getRRsetTrustLevel:
 >
 >   !QuestionIterator quest_iter = message.beginQuestion();
 >   const Name& query_name = (*quest_iter)->getName();
 >
 > i'd check whether we really do have a question section here (i guess
 assert is good enough);
 > assert(quest_iter != message.endQuestion());
 > between these lines

 done

 >
 >
 > in getDeepestDNAMEOwner(): this assumes that if there are multiple
 DNAMEs, they are ordered from shallowest to deepest. Is that a safe
 assumption? (i don't suspect so. btw if is *is* so, you wouldn't need to
 compare but could simply take the last dname).

 I has renamed the function to getDNAMEChainStarter() and add some more
 comments to it. it doesn't assume multiple dnames are in order, instead it
 has to find the starter of the dname chain by iterating all the records in
 answer section.

 >
 > answerHasNonAuthRecord(): the return value of this seems slightly
 different than the name or the comment above it would suggest; if i only
 look at the name or the comment, I would suspect it to also return true if
 the answer section contains out-of-zone data, or data below a delegation
 point. (not that i think this can happen; our responseclassifier would
 have flagged that already i think, but it might be worth explaining what
 it is exactly that it checks in the comment).
 >
 > Taken out of its context, it would also give (i think) the wrong result
 if you called it with an answer section that only contains 1 cname record.
 Again, this shouldn't happen with the way it is called right now, but to
 prevent future changes from breaking it, i'd add more explanation to the
 function :)

 I have removed the function and merged it to function
 getRRsetTrustLevel(), since I find it's really hard to find a proper name
 for it, and it make the code hard to read for the answer section with only
 1 cname record.

 how about now?

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