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
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:8 jinmei]:
 > - I'd like to avoid using a valid value to mean "not really
 >   initialized":

 Well, it doesn't mean so (or, it is never detected that way). It was
 paired
 with the `seen_name_` variable to see if it was initialized.

 The pointer is not really that great either, but if you like it more, I
 don't
 really mind. But haskell's „Maybe Name“ would be better here.

 > - 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.

 +1 for consistency. It just never crossed my mind that a name would once
 be
 quoted and once unquoted ‒ that's why I didn't really check thoroughly
 what
 kind of string it takes, I just knew it takes a quoted one in case of
 domain
 names of RRs. Though I had the impression the name parsing and handling
 was
 somewhat shared between these cases.

 > - 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.

 The conclusion in the mailing list seems to be to allow it. It was
 unclear,
 though, if to warn in that case or not. What do you think?

 > '''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.

 I agree with the low browsability due to the length, but I don't think it
 would
 be particularly unreadable, at least for me ‒ the parts of the code have
 strictly separated goals.

 Your suggestion doesn't make it significantly worse for me (though I think
 the
 method is separated into two in quite arbitrary point and that the
 auxiliary
 method does more than one thing), and I don't have a specific proposal for
 a
 concrete better-readable solution, so I'm adopting it to move forward. I
 guess
 a parser code is never pretty so we'll have to accept some amount of
 ugliness.

 > 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.

 No, I don't misunderstand it. I just consider it premature and quite
 insignificant optimisation, even in case of the copy.

 > 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.
 > }}}

 Well, if the token was ever used, it would be programmer error. But this
 part
 of code is not there any more, so it doesn't matter.

 > - Does it test trailing garbage after $ORIGIN case?  Also for $INCLUDE +
 origin.

 Yes.

 > - Does it test name is bogus for $INCLUDE + origin?

 Added.

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

 I didn't rebase it, because this one was on top of newer master and
 therefore
 newer version of #2428, I didn't know if I used anything from there. So I
 simply merged #2429, resolving some conflicts (while there were quite
 some,
 they were mostly straightforward to solve, so whoever merges #2429 to
 master
 would probably do the same).

 Replying to [comment:10 jinmei]:
 > Another important point: if I understand it correctly, this
 > implementation doesn't handle this (of RFC1035) correctly:
 >
 > {{{
 > have a comment.  Note that a $INCLUDE entry never changes the relative
 > origin of the parent file, regardless of changes to the relative origin
 > made within the included file.
 > }}}

 Hmm, I updated it. Though, looking to bind9, it seems the case of $INCLUDE
 with
 two parameters do change it, because it first applies the name and then
 the
 include. This is what our code does now (but I did add the restoration of
 origin on pop). Do I look wrong?

 > It could also have been noticed from BIND 9's implementation.  This is
 > another example of why we should learn from existing practice, rather
 > than easily dismissing it and thinking we can do it better ourselves
 > from the scratch.

 Actually, I did study the code of bind9 and I was looking just for this
 and I
 didn't find any handling of the origin near the $INCLUDE (except for the
 second
 parameter). It did surprise me a little, but I followed the impression it
 doesn't do so.

 Replying to [comment:11 jinmei]:
 > Which name should be used for the AAAA RR?  RFC1035 doesn't seem to
 > be clear on this point; BIND 9 uses foo.example.com; NSD uses
 > bar.example.com.  In my understanding, your current implementation
 > behaves like NSD.

 I just did it, as it was easily handled together with restoring the
 origin.
 Well, except for the question to the mailing list, which I'm going to do
 now.

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