BIND 10 #1807: support NSEC for empty non-terminal in in-memory (2/2)

[BIND 10 Development]

#1807: support NSEC for empty non-terminal in in-memory (2/2)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120515
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  3
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:  in-    |
  memory NSEC                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 This is review for the combined #1806 and #1807 changes.

 - It seems that this part isn't actually tested:
 {{{#!cpp
         if (node_path.getLastComparisonResult().getRelation() ==
             NameComparisonResult::SUPERDOMAIN) { // empty node, so NXRRSET
             LOG_DEBUG(logger, DBG_TRACE_DATA,
 DATASRC_MEM_SUPER_STOP).arg(name);
             // getClosestNSEC returns empty node for non-NSEC zones or if
             // option FIND_DNSSEC is not set, so no need to check here
             return (ResultType(ZoneFinder::NXRRSET, node,
                                getClosestNSEC(node_path, options)));
         }
 }}}
  In fact, if I changed getClosestNSEC() to `ConstRBNodeRRsetPtr()`,
  tests still passed.

 - I suspect the empty wildcard case needs some more work.  Not really
   confirmed but the current implementation seems to use node_path for
   the original (pre wildcard) search, so I suspect getClosestNSEC()
   could return the wrong NSEC.  I guess we should reset the node_path
   and pass it to the second call to find() here:
 {{{#!cpp
             // Now the wildcard should be the best match.
             const Name wildcard(Name("*").concatenate(
                                     node_path.getAbsoluteName()));
             DomainTree::Result result = domains_.find(wildcard, &node);
 }}}

 - If my guess on the previous bullet is correct, we need some more
   complicated test setup for the empty wildcard case so that it would
   fail with the current version but pass with correction.

 - minor point: Do we need to leave this comment?
 {{{#!cpp
             // getClosestNSEC returns empty node for non-NSEC zones or if
             // option FIND_DNSSEC is not set, so no need to check here
 }}}
   This might not be super obvious from the method name (it could read
   as if it always tries to find NSEC), but at least it's clarified in
   the documentation for the method.  And, if we want to comment about
   that here, I wonder why we don't do it for other cases.  BTW, if we
   want to keep this comment somehow, I'd change 'empty node' to 'NULL
   (RRset)'; getClosestNSEC() doesn't return a "node".

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