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        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I don't see a critical issue in the diff.  I still have some non
 trivial comments.

 '''general'''

 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

 '''MemoryZoneImpl::add()'''

 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.

 '''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.
  - 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?)
  - 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.

 '''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.

 '''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
 }}}
  - related to the question about DNAME under a delegation in the code,
    it would be nice to have a test case for that scenario.

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