BIND 10 #534: DNAME support in MemoryZone

[BIND 10 Development]

#534: DNAME support in MemoryZone
-------------------------------------+-------------------------------------
                 Reporter:  vorner   |                Owner:  jinmei
                     Type:  task     |               Status:  reviewing
                 Priority:  major    |            Milestone:  A-Team-
                Component:  data     |  Sprint-20110209
  source                             |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  3.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Replying to [comment:3 jinmei]:
 > I'd like to see RFC references in code that takes particular protocol
 > action (such as where we reject NS+A at non apex names).  Note also
 > that there's a successor internet draft: draft-ietf-dnsext-rfc2672bis-
 dname

 You mean where the NS+DNAME is rejected, right? I don't (and I believe I
 shouldn't) reject A anywhere.

 Anyway, I added some, hopefully they are of some use.

 > this function is now quite big and getting unreadable.  I guess we
 > might want to move the CNAME/DNAME specific checks in the middle of
 > the function to a separate helper method.

 Done

 > '''cutCallback()'''
 >  - why do we have both zonecut_node_ and dname_node_? (FWIW: BIND 9
 >    uses the same single variable for both NS and DNAME).  If the
 >    purpose of having both is to differentiate these two cases in
 >    find():
 > {{{
 >                 if (state.dname_node_ != NULL) {
 >                     // We were traversing a DNAME node (and wanted to go
 >                     // lower below it), so return the DNAME
 >                     return (FindResult(DNAME, state.rrset_));
 >                 }
 >                 if (state.zonecut_node_ != NULL) {
 >                     return (FindResult(DELEGATION, state.rrset_));
 >                 }
 > }}}
 >   we could do this by the RRset's RR type.  See also the next bullet.

 We could, but the code would be more complicated. This seems to be a
 cleaner design anyway (there are two different thinks I want to store,
 therefore I use two different variables, even when at most one of the
 values will be used).

 Anyway, if you are worried about performance reasons here, I'd like to
 point out that the variable is small, is located on the tack (therefore
 probably in cache). And, provided the function is template (find)
 instantiated only at this place, it will be inlined and the existence of
 the variable completely eliminated by data/flow tracing optimisations.

 >  - on a related note, is it okay to only check zonecut_node_ at the
 >    beginning of this function? what if we have an NS based delegation
 >    and a DNAME under the zone cut?  (And what happens if we search
 >    something under a zone cut with glue OK and hit a DNAME?)

 If we have one above another (and don't have the glue OK), then the search
 stops right there and we won't get to the lower one, which is AFAIK
 correct.

 If there are in the same node, then it is the only case allowed, at the
 apex and DNAME has precedence.

 If we have the glue OK mode, I'm not entirely sure, but don't we want to
 ignore the NS delegations completely, therefore getting to the first DNAME
 (under which is nothing) and terminating there? That's why DNAME is
 checked first in the PARTIALMATCH case.

 >  - the comment about the case of 'foundNS' is not really accurate
 >    because with DNAME callback itself can be called at the apex.
 >    I've updated the comment by myself and pushed it.  Please check.

 OK

 > '''find()'''
 >
 > In the following, name comparison may be a bit expensive (considering
 > it's a performance sensitive operation):
 > {{{
 >         // If the node callback is enabled, this may be a zone cut.  If
 it
 >         // has a NS RR, we should return a delegation, but not in the
 apex.
 >         if (node->isCallbackEnabled() && node->getName() != origin_) {
 > }}}
 >    I wonder we might maintain a pointer to the origin node and compare
 >    'node' and that pointer.  Keeping an origin pointer may be useful
 >    for other purposes.

 I added the pointer. But I wouldn't worry about performance there. We just
 searched the tree and compared the name several times. And this one was
 done only in case the node was marked for callback.

 > '''Unittests'''
 >  - Is the added copyright intentional?  (Just checking. I don't have
 >    an opinion on this or I don't know what's the agreed copyright style
 >    between ISC and CZNIC)
 > {{{
 > // Copyright (C) 2011  CZ NIC
 > }}}

 Yes, I ask from time to time about progress in the copyright agreement
 between CZ NIC and ISC and still get the answer that it is not done yet
 and it is waiting for some signatures or so. Therefore this is why I add
 them when I remember, since it is the presently correct way. They should
 be removed one the agreement is signed.

 >  - related to the question about DNAME under a delegation in the code,
 >    it would be nice to have a test case for that scenario.

 Is my understanding that we should not note the NS ones and act based on
 the DNAME in that case? If so, I'll write one.

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