BIND 10 #1305: auth NSEC support: some more updates to data source

[BIND 10 Development]

#1305: auth NSEC support: some more updates to data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111025
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:6 stephen]:

 Thanks for the review.

 > '''src/lib/datasrc/database.cc'''[[BR]]
 > Not part of the changes here, but can't the "accessor" argument of
 !DatabaseClient be passed by reference and so save the need to copy the
 shared_ptr when the constructor is called?

 Do you mean passing as DatabaseAccessor& or as
 boost::shared_ptr<DatabaseAccessor>&?
 We couldn't use the former in this case because the caller is a
 datasource-specific factory method and will immediately release the
 ownership of the accessor object.  In the latter case we'll have to
 dereference it to acquire the ownership anyway for the same reason,
 so I suspect the actual benefit of saving the copy is marginal (in
 this specific usage, in particular, I guess the compiler will combine
 the multiple copy constructions involved in the function call and the
 member variable initialization, and the actual benefit would actually
 be zero).

 In general, I'd be careful about passing a shared pointer in the form
 of reference because it could often lead to break the "shared" nature
 of the object.  If the called function handles the passed reference
 carelessly (e.g., keep storing it somewhere in the form of reference)
 it can easily lead to dangling ownership.  But such a discussion would
 be far beyond the scope of this ticket.

 > In the following code:
 > {{{
 > } else if ((options & NO_WILDCARD) != 0) {
 >     // If wildcard check is disabled, terminate the search with
 >     // NXDOMAIN.
 >     if (dnssec_data && !records_found) {
 >         get_cover = true;
 >     }
 > }}}
 > ...there is no need to check that records_found is false here, as this
 block of code is inside an
 > {{{
 > } else if (!records_found) {
 > }}}
 > ... clause and nothing appears to modify records_found since that check.

 Ah, good catch, fixed.  I forgot to leave a note to reviewer about
 this part: there's some code duplication between this case and the end
 of the final 'else' block.  But I didn't try to unify them at this
 point - I thought it made more sense to clean these up in #1198.

 > Also, although the comment doesn't quite reflect the purpose of the
 following code.  Something like:
 > {{{
 > // If wildcard check is disabled, the search will ultimately terminate
 > // with NXDOMAIN. If DNSSEC is enabled, flag that we need to get the
 > // NSEC records to prove this.
 > }}}
 > ... may be better.

 Yeah, as I fixed the previous one I saw the need for revising the
 comment, too, so I did it, but I like your version better.  Updated.

 > '''src/lib/datasrc/tests/database_unittest.cc'''[[BR]]
 > Comment refers to WILDCARD_EMPTY; the actual status is WILDCARD_NXRRSET.

 Good catch, fixed.

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