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

[BIND 10 Development]

#2427: support $ORIGIN in dns::MasterLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  libdns++      |  closed
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |  complete
         Total Hours:  0             |                 CVSS Scoring:
                                     |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  8.99
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Now responding to some remaining discussion points, mainly for the
 record...

 Replying to [comment:14 vorner]:

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

 Actually, I'd say the use of such a loosely related mutable variables
 is another source of bugs.  At least it makes me the reviewer nervous,
 because I had to take an extra care to check these variables are
 maintained in a consistent way.

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

 I absolutely much prefer the pointer than the bogus value of object +
 yet another state variable that need to be maintained consistently,
 although I know pointers are not a perfect tool.  If we want to use a
 higher level abstraction to represent this concept, btw, we could use
 boost::optional.  That's indeed a C++ version of "maybe".

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

 Admittedly readability is a subjective issue, but the original code
 wasn't bitable to me.  I like the suggested version (not surprising,
 because I wrote it) in that I can understand the overall code logic
 "at a glance".  Due to the large block, I'd need at least one layer of
 additional brain stack to understand the original version.

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

 I admit the extracted method could be improved further (maybe further
 separated).  But the important point is to keep the main method concise.

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

 Hmm, this part didn't seem to suggest that: "It is not copying the
 string, it is only scanning the characters once again in the buffer in
 source." in http://bind10.isc.org/ticket/2377#comment:21

 > I just consider it premature and quite
 > insignificant optimisation, even in case of the copy.

 I don't necessarily object that this might be a premature
 optimization, though.  In any event, my point was if two approaches
 are generally comparable in other points, there's no harm in trying to
 optimize it a bit more.

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

 I don't understand the logic here...the point I mentioned existed in
 your original version, and "it's not there" because I eliminated in my
 proposed version.  But anyway, it's true the point is moot as we
 adopted that version.

 > 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

 No, BIND 9 preserves it.  I actually overlooked this point when I said
 the branch is okay for merge, but I subsequently noticed it and ended
 up fixing it myself in #2380.

 > include. This is what our code does now (but I did add the restoration
 of
 > origin on pop). Do I look wrong?

 I'm not sure what this means, but you can easily confirm the behavior
 of BIND9 by actually loading a zone and making queries.

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