BIND 10 #1177: NSEC support in new data source

[BIND 10 Development]

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

Comment (by jinmei):

 I'll give the "refactoring" part a review first.

 In general, I support the idea of this refactoring: it seems to be
 clearer to let getRRset(s) be responsible only for a single task.

 I've noted some points where variables can be const.  These should be
 quite trivial and non controversial, so I made the change directly.

 '''getRRsets()'''

 - as a result of replacing getRRset() with this one, most
   of the documentation for the original method was removed, which
   doesn't seem to be good (even though it may not be fully detailed as
   it's private).
 - exception parameter variables aren't used and can be removed, e.g.:
 {{{
         } catch (const InvalidRRType&) {
 }}}
 - I'm not sure if this comment is relevant, but at least NSEC should
   be able to coexist with NS/CNAME (and as commented DS should be okay
   with NS, too):
 {{{
             } else if (cur_type != RRType::RRSIG()) {//RRSIG can live
 anywhere
                 // FIXME: Is something else allowed in the delegation
 point? DS?
                 seen_other = true;
             }
 }}}
 - regarding the previous point, doesn't it make more sense to move
   this type of check outside of this method completely?
 - A map<RRType, RRsetPtr> object is copied twice: when constructing
   FoundRRsets at the end of the method, and when passing the return
   value to the caller.  Unlike the previous type (a shared pointer),
   the copy overhead is bigger.  Should we care about it?  (perhaps
   doesn't matter much, because operations requiring DB access are
   generally slow anyway).

 '''Code in the unnamed namespace'''

 - empty_types is defined as a namespace scope static variable, which
   seems to be dangerous in terms of static initialization fiasco.
   Looking into how it's used, I suspect we actually don't
   (necessarily) need it, especially with taking a risk of introducing
   the fiasco:
 {{{
     //static WantedTypes nsec_types(empty_types + RRType::NSEC());
     static WantedTypes nsec_types(WantedTypes() + RRType::NSEC());
 }}}

 - operator+: I personally don't think this definition is intuitive.
   I'd expect an 'operator+' (with two params) to take the same types
   of arguments and to meet other natural expectations derived from the
   arithmetic '+' operator (such as the commutative law).  Also, while
   admittedly this may purely be a matter of taste, I generally agree
   with the concerns documented in the google's guideline:
   http://google-
 styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overloading

 - Considering these, I'd suggest something like this:
   - Define nsec_types etc via a proxy function:
 {{{
 const set<RRType>&
 NSEC_TYPES() {
     static set<RRType>* types;
     if (types == NULL) {
         auto_ptr<set<RRType> > types_ptr(new set<RRType>());
         types_ptr->insert(RRType::NSEC());
         types = types_ptr.release();
     }
     return (*types);
 }
 }}}
   - For 'final_type' and 'wildcard_type', use the same technique with
    explicit insert():
 {{{
         WantedTypes final_types(FINAL_TYPES());
         final_types.insert(type);
 }}}
   (btw: with this approach you could also combine the definition of
   "final" and "wildcard")

 '''find()'''
 - the changes in the first for loop is not a straightforward
   refactoring of the original code, and it's difficult to be sure if
   it really preserves the original behavior.  For example, is the
   behavior same when DNAME exists and the first 'if' conditions are met?
 - as a more fundamental point, this method is now generally too big
   and complicated, and difficult to follow (although it's not a result
   of this branch).  I suspect we'll need overall cleanup/refacotring
   to improve clarity and readability.  (does #1198 intend to improve
   the situation?)
 - (not actually for this branch but) I'd avoid the construction overhead
   of "star" by defining a proxy function:
 {{{
 const Name&
 STAR_NAME() {
     static Name star("*");
     return (star);
 }
 }}}
 - (ditto) this "TODO" now seems to be doable:
 {{{
                     // TODO: Once the underlying DatabaseAccessor takes
                     // string, do the concatenation on strings, not
                     // Names
 }}}

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