BIND 10 #1579: Update database ZoneFinder::find() for negative cases of NSEC3-signed zones

[BIND 10 Development]

#1579: Update database ZoneFinder::find() for negative cases of NSEC3-signed zones
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  haikuo
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by haikuo):

 Replying to [comment:20 jinmei]:
 > Replying to [comment:19 haikuo]:
 > > after our discussion, I adjust my codes. please review it.
 >
 > First off, please basically respond to each itemized point one by
 > one.  I cannot be sure whether you addressed all points or part of
 > them, and whether you agreed or disagreed with me about each case (you
 > don't necessarily have to agree, of course, but if you didn't say
 > anything I cannot tell).  In any case, please remember that such a
 > one-line response to itemized specific comments would be generally not
 > acceptable.
 okay, no problem. I can do it next time. and responding to each itemized
 point
 is good for all of us.
 > Secondly, in fact, it doesn't address one of my main concerns: "If we
 > know it's an NSEC3-signed zone, I wouldn't bother to look for NSEC
 > because it's a waste. I'd introduce a helper method or something to
 > unify such cases in an efficient way".  And, findInternal() is now
 > terribly unreadable and non understandable.  If you cannot be sure
 > about my concern, could I propose revised code rather than trying to
 > explain my intent further?

 For findInternal() function, I know I input lot of "if else",and it
 reduced
 the code readability, I will check my codes,and hope I can find a good way
 to
 resolve it.
 If you want to revise my code, please create a new branch,like
 trac1579-review,
 and I will review it. I have not understand your bind10 deeply by now, and
 maybe I
 misunderstood some of high level goals. but I think I need a process to
 understand
 the architecture totally. If this ticket is urgent ticket, you can do it
 and I of course
 agree that.
 I think your concern is some redundant codes could be executed in find()
 function,and
 those rebundant codes could be a waste for some queries.I will check it.
 I know the find() function is computing intensive one,and the performance
 is
 critical for bind10.

 > Third, I've still seen some editorial issues.  I've directly updated
 > the branch and committed the changes.  Some are clearly described in
 > the coding guideline page, but you still seem to not be able to follow
 > them (although, to be fair, it's not only you, and I myself sometimes
 > make such errors).  I'd suggest you reread the guideline page.  For
 > the rationale of specific changes, please see the commit logs.
 >
 as we discuss in jabber, I wrote codes as the rules of guidelines.
 but some style problem is not mentioned in the guidelines. So I suggest
 to append some more detail regulation in the guidelines. It may be good
 for our new colleague in the future.
 > Finally, the added test cases look basically okay, although I'd
 > suggest testing some strange cases like the zone having both NSEC and
 > NSEC3PARAM.  I suspect that the current implementation doesn't follow
 > this API spec (the last sentence):
 > {{{#!c++
 >     /// First, it tells the application whether the zone is signed with
 >     /// NSEC or NSEC3 via the \c isNSEC(3)Signed() method.  Any sanely
 signed
 >     /// zone should be signed with either (and only one) of these two
 types
 >     /// of RRs; however, the application should expect that the zone
 could
 >     /// be broken and these methods could both return false.  But this
 method
 >     /// should ensure that not both of these methods return true.
 > }}}
 I agree to add some case to test the strange cases.

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