BIND 10 #1584: auth::Query NSEC3 support: Wildcard answer case

[BIND 10 Development]

#1584: auth::Query NSEC3 support: Wildcard answer case
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  haikuo
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120221
                  Component:         |            Resolution:
  b10-auth                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  NSEC3  |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by haikuo):

 Replying to [comment:9 jinmei]:
 > As we briefly discussed in the daily call, I reviewed the latest
 > snapshot of the trac1584 branch anyway.  I completed the review, and
 > actually even addressed my own comments myself.  I didn't want to
 > steal the branch owned by someone else, but in this particular case
 > we rely on the completion of this branch for including the major
 > feature in the next release, and apparently we are running out of our
 > time.  So I hope it's acceptable for you.
 >
 > (Most of) my own updates are made in a separate branch (to minimize
 > possible conflicts), trac1584review.  I've created the branch from a
 > recent master, and merged the latest available version of trac1584 (at
 > commit c9b1b85).

 > Now, if this "takeover" is acceptable for you, could you review the
 > changes I made in the trac1584review?  If you don't think you have
 > enough time to complete the review within this week, please ask
 > someone else for taking over it.
 >
 > Comments on the original trac1584 branch (and explanation on my own
 > changes) follow:
 >
 > '''general'''
 >
 > - There are many violations of the coding guidelines: indentation, too
 >   long lines, (although not explicitly documented) redundant white
 >   space characters at end-of-lines or in blank lines, inconsistent
 >   space style like "if (foo){" (we add space between ')' and '{').
 >   If you have not done so, please read the guideline carefully.  For
 >   indentation, maybe you want to adjust your editor configuration (you
 >   seem to insert 8-space for a "tab")


 from my side, I want to say I would like to tidy up codes after the codes
 is done
 and before change it to "review" status. If we have the rules that we must
 tidy codes before
 commit to repository, I will do it every time.


 > - conventionally we add "[ticket number]" at the beginning of commit
 >   logs.  Please follow that convention.  You can do that, e.g., by
 >   adding this to .git/hooks/prepare-commit-msg (this is what I'm
 >   doing):
 > {{{
 > #!/bin/sh
 >
 > BRANCH=`git branch | sed -ne 's/\* trac\(.*\)/\1/p'`
 > if test -z $BRANCH; then
 >       BRANCH=master
 > fi
 > /bin/echo -n "[$BRANCH] " > "$1.msg"
 > cat "$1" >> "$1.msg"
 > cat "$1.msg" > "$1"
 > }}}
 > - As I pointed out in the first comment message, we adopt test driven
 >   development.  So, basically, it cannot happen you only commit main
 >   code without tests.  In the case of this branch, the first change
 >   would be something like commit 31ae358.  We then confirm it fails
 >   with the current implementation, then extend the main code.
 >   Apparently your work flow is opposite.  Please make sure write tests
 >   first next time.  If you are not familiar with TDD, please ask us or
 >   someone in CNNIC who have worked on BIND10 before.
 >
 > '''query.cc'''
 >
 > - the check for isWildcard() in addWildcardProof() is redundant (I
 >   fixed it)
     do you fix it in your "trac1584review" branch? I have't seen anything
 about it.
     and do you develop the unit test in "trac1584review" branch?
 > - on further look, I noticed we actually didn't have to construct the
 >   next closer name to the wildcard.  By definition it must be equal to
 >   the next closer of the closest encloser for the qname.  So I
 >   simplified the code by removing the unnecessary stuff.



     do you make sure you push your codes to the "trac1584review" branch? I
 did see
     anything about that.


 > - otherwise the code generally looked correct, but as I did in #1583,
 >   I had Message::addRRset() handle the unexpected case where
 >   next_proof is NULL.  That simplifies the code for the normal case.

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