BIND 10 #2431: support flexible ordering/appearance of TTL, type and class in dns::MasterLoader

[BIND 10 Development]

#2431: support flexible ordering/appearance of TTL, type and class in
dns::MasterLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''master_loader.cc (major)'''

 Overall it seems to do what it's expected to do, but I have several
 suggestions and concerns.

 - I'd like to avoid calling ungetToken() when possible.  It's not
   super cheap for strings.  On a closer look, I found we should be
   able to use the specific-token-type version of `getNextToken` and
   avoid ungetToken() (for valid input).  At least there should be a
   string token for RR type, so, from the candidate of TTL/class
   through RR type, we can safely use this version of `getNextToken`.
 - Secondly, considering named-signzone output (always add explicit
   TTL, and add it first; omit class for some), I think we should check
   TTL first (BIND 9 checks the class first, but I'm not sure why).
   Depending on the overhead of the case the try fails (see below),
   this can affect the overall performance substantially.
 - IMO the code added in this branch makes `loadIncremental` too
   big and unreadable, and should better be delegated to a helper
   method.
 - On a related note, for RR class we only have to do check when
   explicitly specified; because we know what we should use:
   zone_class_.

 I'm attaching a diff implementing these ideas.  Please take a look.
 If you think it makes sense, please incorporate it with adding more
 comments and more cleanups.

 Finally, a possibly controversial point.  As you may have noticed in
 your review on #2429, I'm concerned about exception overhead when the
 try-and-error detection for TTL (and RRclass):

 {{{#!cpp
             try {
                 rrclass.reset(new RRClass(rrparam_token.getString()));
                 rrparam_token = lexer_.getNextToken();
             } catch (const InvalidRRClass&) {
                 // If it's not an rrclass here, continue and try again
                 // after the TTL below.
             }

 ...
                 try {
                     rrclass.reset(new RRClass(rrparam_token.getString()));
                     rrparam_token = lexer_.getNextToken();
                 } catch (const InvalidRRClass&) {
                     // If it's not an rrclass here, use the zone's class.
                     rrclass.reset(new RRClass(zone_class_));
                 }
 }}}

 This is particularly so for RR class as it's often omitted in actual
 zone files (in that case we'll encounter two exceptions in your
 original code).  I'm also concerned about the overhead of `new`ing
 the RRClass for every RR.  One may argue it's premature optimization,
 but if you agree with me, I'd suggest doing something similar to what
 I did in #2431: introduce a "no throw" (and still minimizing
 resource allocation) version of `RRClass` factory:
 {{{
 RRClass*
 RRClass::createFromText(const string& rrclass_txt, RRClass* placeholder);
 }}}
 and maintain a placeholder object within `MasterLoader` so we only
 have to `new` it once in the lifetime of the loader.

 And, in that case, my specific suggestion is to merge the branch
 without this particular optimization (while addressing other issues),
 leave the ticket open, and complete it in the second phase.  (I don't
 like to delay our meta goal for this optimization, but I'm afraid the
 optimization part won't be taken if we defer it to a separate ticket.)

 '''master_loader.cc (minor)'''

 - reference to exceptions in `catch` should be constified.  I've
   noticed this is a common point I'd have to in your code.  Maybe you
   want to include it in your personal (imaginary) check list.  That
   can be even be statically checked by a simple script.  cppcheck also
   generally complains about it, so running cppcheck before asking for
   a review would also be a good practice.  This is trivial and I fixed
   it my self.
 - also, variables are not necessary in `catch` if you don't need them
 {{{#!cpp
             } catch (const InvalidRRClass& e) { // <= no need to define
 'e'
                 // If it's not an rrclass here, continue and try again
                 // after the TTL below.
             }
 }}}

 '''master_loader_unittest.cc'''

 - I'd suggest providing expected error message in `error_cases` like
   the TTL-related cases do.  Invalid input could fail in various ways,
   so if we don't check the actual message tests can still pass even if
   the error is detected in a different way than we actually intend.
 - ""Invalid Rdata" is not really accurate:
 {{{#!cpp
     { "www      A       IN  3600 192.168.2.8", NULL,
       "Invalid Rdata (incorrect order of class, TTL and type)" },
 }}}
   In fact, the "Rdata" is valid here.  If we want to keep the "Invalid
   XXX" phrase, it should be "RR".  But I'd rather simply say
   "Incorrect order of class, TTL and type".  Same for other similar
   cases.
 - `ttlClassTypeOrder`: it repeats a certain amount of copy of the main
   .cc source, and doesn't seem to be good in terms of DRY.  Can't we
   simply refer to the main source and/or be more concise here?
 {{{#!cpp
     // We test the order and lack of TTL, class and type.  Both TTL and
     // class are optional and may occur in any order if they exist. TTL
     // and class come before type which must exist.
     //
     // [<TTL>] [<class>] <type> <RDATA>
     // [<class>] [<TTL>] <type> <RDATA>
 }}}
 - There are some other invalid cases where we should test, like:
 {{{
 ;; omit TTL and class, and missing RDATA
 www A

 ;; other variations of missing token (check if it covers all cases):
 www
 www 3600
 www IN
 www 3600 IN
 www IN 3600
 www 3600 A
 www IN A
 www 3600 IN A
 www IN 3600 A
 }}}

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