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

 Replying to [comment:9 muks]:

 Regarding the additional optimization, let's defer it until we
 complete the other part of the loadzone-ng tasks.  I think we are at a
 borderline of making it happen, and should focus on the very related
 tasks for that feature.

 This re-review is changes up to 757d91f.  It generally looks okay
 and ready for merge (when #2429 is ready).  I have two additional
 editorial comments.

 - I suggest adding some simple description of parseRRParams like this:
 {{{#!cpp
     // A helper method for loadIncremental.  It parses part of an RR
     // until it finds the RR type field.  If TTL or RR class is specified
     // before the RR type, it also recognize and validate them.
     // explicit_ttl will be set to true if this method finds a valid TTL
     // field.
 }}}
 - I suggest using `InternalException` instead of `BadValue` and remove
   the comments.
 {{{#!cpp
             // It doesn't really matter much what type of exception
             // we throw, we catch it just below.
             isc_throw(isc::BadValue, "Class mismatch: " << *rrclass <<
                       "vs. " << zone_class_);
 }}}
   `InternalException` seems to be introduced for this exact purpose,
   but I guess at the time of the use of `BadValue` it wasn't there.

 > The updated code looks fine to me. I've added comments. There was a
 syntax error in one of the statements, so I have to ask, did write all
 this directly in the browser? If so, that's remarkable.

 No, (and it's not possible because it's an attachment).  I thought I
 checked it compiled and passed tests, but maybe I didn't fully check
 it.

 > > '''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.
 >
 > From the parser's point of view, it is invalid Rdata as everything after
 the type (`A`) is passed to the `IN/A` constructor. But I've updated the
 message anyway as the suggested message is just as good.

 Ah, okay.  Checking the actual error message is actually helpful to
 understand that, too.

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