BIND 10 #2427: support $ORIGIN in dns::MasterLoader

[BIND 10 Development]

#2427: support $ORIGIN 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:  3             |                 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'''

 - I'd like to avoid using a valid value to mean "not really
   initialized":
 {{{#!cpp
         last_name_(Name::ROOT_NAME()), // Initialize with something,
                                        // we don't care
 }}}
   This will trigger a bug that we see a magical use of root owner name
   through some extension that introduces a case where last_name_ could
   be used before initial explicit set.  I'd suggest something like
   this:
 {{{#!cpp
     scoped_ptr<Name> last_name_;
     ...
     void setLastName(const Name& name) {
         if (!last_name_) {
             last_name_.reset(new Name(name));
         } else {
             *last_name_ = name;
         }
     }
 }}}
   This way, bugs like the one I mentioned above will be detected more
   explicitly and sooner.  This approach will also eliminate the need
   for `seen_name_` (if I understand it correctly).
 - BIND 9 rejects quoted string for $ORIGIN but our current code
   accepts it:
 {{{#!cpp
         // There could be an origin (or maybe not). So try looking
         const MasterToken
 name_tok(lexer_.getNextToken(MasterToken::QSTRING,
                                                        true));
 }}}
   I don't know the rationale of the inconsistency in BIND 9 (asked on
   jabber but no response as of this writing), though.  I quickly had a
   jabber chat with Evan on this, and the tentative conclusion is it's
   an oversight of BIND 9 that allowing qstring for owner name in the
   first place, but it's probably better to keep the behavior if it
   doesn't hurt anyone.  Regarding inconsistency, we seem to tend to
   agree that it's better to be consistent on this, and so allow
   qstring for $ORIGIN value, too.  In the end, we don't have to change
   the implementation, but I suggest documenting the discussion
   explicitly, somewhere.
 - doInclude and doOrigin: it seems implementation-dependent whether to
   allow relative name for $ORIGIN; BIND 9 allows it, NSD doesn't, and
   AFAICS RFC is silent (check yourself).  If it's
   implementation-dependent we should be compatible with BIND 9, so the
   code is okay, but I'd leave a comment about it.
 - Related to the previous point, since there's some non trivial point,
   I'd consider unifying the two cases for ORIGIN even if the code
   block is small.  It seems possible by parameterizing the `eol` param
   of getNextToken().
 - doInclude: name_ok can be a reference:
 {{{#!cpp
         const MasterToken
 name_tok(lexer_.getNextToken(MasterToken::QSTRING,
                                                        true));
 }}}
   (but wouldn't be a big deal as $INCLUDE should be a minor case)

 '''master_loader.cc, loadIncremental()'''

 Now, let's resume the deferred discussion of #2377.  First, I don't
 particularly see it readable, although I'm not saying the BIND 9
 version is more readable.  The `do` loop and the following `if` clause
 are pretty long, greatly reducing the browsability of the main method.
 The double loop due to `do` also makes me use my brain harder to
 figure out what's going on.

 Secondly, we still needed to do ungetToken() (and re-get it) for the
 main scenario.  In case you still misunderstand it, I'm repeating:
 this actually involves redundant data copy.  The effect of this on the
 overall loading overhead may not be significant, but if other points
 are largely comparable, I'd prefer the one that doesn't have this
 overhead.

 There's one other, relatively minor issue in the branch version:
 The use of `MasterToken::NO_TOKEN_PRODUCED`:
 {{{#!cpp
             MasterToken initial_token(MasterToken::NO_TOKEN_PRODUCED);
 }}}
 This is against what the doc says:
 {{{#!cpp
         NO_TOKEN_PRODUCED, ///< No token was produced. This means
 programmer
                            /// error and should never get out of the
 lexer.
 }}}
 We could retroactively distort the description of the value just for
 justifying this particular use, but, in general, such an ad hoc
 semantics change is risky because there can be other part of the code
 that relies on that original design decision.  Even if we don't have
 such an issue for this value, I generally don't like to rely on a
 magic placeholder value just like I explained for last_name_ above.

 I have a counter proposal addressing all of these (or at least
 everything except readability, which, after all, is in the eye of the
 beholder and may vary).  I'm attaching a diff of the proposal.  It
 extracts the handling of the initial token into a separate helper
 method, keeping the main thread concise, avoid using ungetToken() for
 major scenario, and avoid the use of `MasterToken::NO_TOKEN_PRODUCED`.
 It also makes all use of `MasterToken` const.  And, each case for the
 token is completely independent, we can even delegate a part of the
 method if we want further refactoring for even better readability.

 As for readability, I personally think the proposed version is
 measurably more readable, but I admit different people may have
 different impression for this kind of matter.  But at least I'm quite
 sure both versions are largely comparable in readability, and, if so,
 I still much prefer the proposed version because it addresses the
 specific issues I listed above.

 If you think the proposed version makes sense, please make any
 necessary cleanup, etc, and apply it; if you still like the original
 version, we can discuss it, but I'd at least strongly request
 extracting this code logic into a separate method.

 '''master_loader_unittest.cc'''

 - I'd test cases where getNextToken() due to errors, like this one:
 {{{
 $ORIGIN unbalanced)paren
 }}}
 - This case doesn't seem to be tested:
 {{{#!cpp
             } else if (initial_token.getType() == MasterToken::INITIAL_WS)
 {
                 // This means the same name as previous.
                 if (!seen_name_) {
                     isc_throw(InternalException, "No previous name to use
 in "
                               "place of initial whitespace");
                 }
 }}}
 - This case doesn't seem to be tested:
 {{{#!cpp
             } else if (initial_token.getType() == MasterToken::ERROR) {
                 // Error token here.
                 isc_throw(InternalException,
 initial_token.getErrorText());
             } else {
 }}}
 - Does it test trailing garbage after $ORIGIN case?  Also for $INCLUDE +
 origin.
 - Does it test name is bogus for $INCLUDE + origin?
 - One of my comments on #2431 applies: "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."  (But in the case of this
   branch you'd need to rebase it on #2429.)

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