BIND 10 #1584: auth::Query NSEC3 support: Wildcard answer case
BIND 10 Development
do-not-reply at isc.org
Thu Feb 16 23:15:10 UTC 2012
#1584: auth::Query NSEC3 support: Wildcard answer case
-------------------------------------+-------------------------------------
Reporter: | Owner: haikuo
jinmei | Status: accepted
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 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")
- 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)
- 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.
- 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:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list