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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:8 jinmei]:
 >
 > - 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)));
 >         }
 > }}}

 Oops, thought I had covered that. Amended.

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

 Ah, doh, I got confused as to what record should've been returned in empty
 wildcard cases. Should be fixed now as well. (ps. if we're going to redo
 masterload in the future, it's probably a good idea to make sure we can
 generate test data easily through the lib as well, the current approach is
 very fragile. It can probably be done better right now, but would be much
 much easier if we used some find of stringToRRSet() function, without full
 zone context, so you can make several apex NSECs, to name something).

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

 Yes, I've added an argument expected_nsec to the helper method, and a more
 complete nsec chain in the test data.

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

 removed.

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