Merge #2427

The $ORIGIN handling.

Conflicts:
	src/lib/dns/master_loader.cc
	src/lib/dns/tests/master_loader_unittest.cc

src/lib/dns/master_lexer.cc

@@ -37,7 +37,7 @@ using namespace master_lexer_internal;
 
 struct MasterLexer::MasterLexerImpl {
     MasterLexerImpl() : source_(NULL), token_(MasterToken::NOT_STARTED),
-                        paren_count_(0), last_was_eol_(false),
+                        paren_count_(0), last_was_eol_(true),
                         has_previous_(false),
                         previous_paren_count_(0),
                         previous_was_eol_(false)
@@ -127,6 +127,7 @@ MasterLexer::pushSource(const char* filename, std::string* error) {
 
     impl_->source_ = impl_->sources_.back().get();
     impl_->has_previous_ = false;
+    impl_->last_was_eol_ = true;
     return (true);
 }
 
@@ -135,6 +136,7 @@ MasterLexer::pushSource(std::istream& input) {
     impl_->sources_.push_back(InputSourcePtr(new InputSource(input)));
     impl_->source_ = impl_->sources_.back().get();
     impl_->has_previous_ = false;
+    impl_->last_was_eol_ = true;
 }
 
 void

src/lib/dns/master_lexer.h

@@ -54,7 +54,8 @@ public:
         END_OF_LINE, ///< End of line detected
         END_OF_FILE, ///< End of file detected
         INITIAL_WS,  ///< White spaces at the beginning of a line after an
-                     ///< end of line (if asked for detecting it)
+                     ///< end of line or at the beginning of file (if asked
+                     //   for detecting it)
         NOVALUE_TYPE_MAX = INITIAL_WS, ///< Max integer corresponding to
                                        /// no-value (type only) types.
                                        /// Mainly for internal use.

src/lib/dns/master_loader.cc

@@ -26,10 +26,16 @@
 
 #include <string>
 #include <memory>
+#include <vector>
+#include <boost/algorithm/string/predicate.hpp> // for iequals
+#include <boost/shared_ptr.hpp>
 
 using std::string;
 using std::auto_ptr;
+using std::vector;
+using std::pair;
 using boost::algorithm::iequals;
+using boost::shared_ptr;
 
 namespace isc {
 namespace dns {
@@ -58,6 +64,7 @@ public:
                      MasterLoader::Options options) :
         lexer_(),
         zone_origin_(zone_origin),
+        active_origin_(zone_origin),
         zone_class_(zone_class),
         callbacks_(callbacks),
         add_callback_(add_callback),
@@ -66,6 +73,7 @@ public:
         initialized_(false),
         ok_(true),
         many_errors_((options & MANY_ERRORS) != 0),
+        previous_name_(false),
         complete_(false),
         seen_error_(false),
         warn_rfc1035_ttl_(true)
@@ -82,7 +90,10 @@ public:
                 ok_ = false;
             }
         }
+        // Store the current status, so we can recover it upon popSource
+        include_info_.push_back(IncludeInfo(active_origin_, last_name_));
         initialized_ = true;
+        previous_name_ = false;
     }
 
     void pushStreamSource(std::istream& stream) {
@@ -112,6 +123,16 @@ private:
             return (false);
         }
         lexer_.popSource();
+        // Restore original origin and last seen name
+
+        // We move in tandem, there's an extra item included during the
+        // initialization, so we can never run out of them
+        assert(!include_info_.empty());
+        const IncludeInfo& info(include_info_.back());
+        active_origin_ = info.first;
+        last_name_ = info.second;
+        include_info_.pop_back();
+        previous_name_ = false;
         return (true);
     }
 
@@ -121,24 +142,47 @@ private:
         return (string_token_);
     }
 
-    void doInclude() {
-        // First, get the filename to include
-        const string
-            filename(lexer_.getNextToken(MasterToken::QSTRING).getString());
+    MasterToken handleInitialToken();
 
-        // There could be an origin (or maybe not). So try looking
-        const MasterToken name_tok(lexer_.getNextToken(MasterToken::QSTRING,
-                                                       true));
+    void doOrigin(bool is_optional) {
+        // Parse and create the new origin. It is relative to the previous
+        // one.
+        const MasterToken&
+            name_tok(lexer_.getNextToken(MasterToken::QSTRING, is_optional));
 
         if (name_tok.getType() == MasterToken::QSTRING ||
             name_tok.getType() == MasterToken::STRING) {
-            // TODO: Handle the origin. Once we complete #2427.
+
+            const MasterToken::StringRegion&
+                name_string(name_tok.getStringRegion());
+            active_origin_ = Name(name_string.beg, name_string.len,
+                                  &active_origin_);
+            if (name_string.len > 0 &&
+                name_string.beg[name_string.len - 1] != '.') {
+                callbacks_.warning(lexer_.getSourceName(),
+                                   lexer_.getSourceLine(),
+                                   "The new origin is relative, did you really"
+                                   " mean " + active_origin_.toText() + "?");
+            }
         } else {
-            // We return the newline there. This is because after we pop
-            // the source, we want to call eatUntilEOL and this would
-            // eat to the next one.
+            // If it is not optional, we must not get anything but
+            // a string token.
+            assert(is_optional);
+
+            // We return the newline there. This is because we want to
+            // behave the same if there is or isn't the name, leaving the
+            // newline there.
             lexer_.ungetToken();
         }
+    }
+
+    void doInclude() {
+        // First, get the filename to include
+        const string
+            filename(lexer_.getNextToken(MasterToken::QSTRING).getString());
+
+        // There optionally can be an origin, that applies before the include.
+        doOrigin(true);
 
         pushSource(filename);
     }
@@ -148,14 +192,13 @@ private:
     // specified before the RR type, it also recognizes and validates
     // them.  explicit_ttl will be set to true if this method finds a
     // valid TTL field.
-    RRType parseRRParams(bool& explicit_ttl) {
+    RRType parseRRParams(bool& explicit_ttl, MasterToken rrparam_token) {
         // Find 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>
-        MasterToken rrparam_token = lexer_.getNextToken(MasterToken::STRING);
 
         // named-signzone outputs TTL first, so try parsing it in order
         // first.
@@ -298,9 +341,8 @@ private:
         if (iequals(directive, "INCLUDE")) {
             doInclude();
         } else if (iequals(directive, "ORIGIN")) {
-            // TODO: Implement
-            isc_throw(isc::NotImplemented,
-                      "Origin directive not implemented yet");
+            doOrigin(false);
+            eatUntilEOL(true);
         } else if (iequals(directive, "TTL")) {
             setDefaultTTL(RRTTL(getString()), false);
             eatUntilEOL(true);
@@ -318,7 +360,7 @@ private:
                 case MasterToken::END_OF_FILE:
                     callbacks_.warning(lexer_.getSourceName(),
                                        lexer_.getSourceLine(),
-                                       "Unexpected end of file");
+                                       "File does not end with newline");
                     // We don't pop here. The End of file will stay there,
                     // and we'll handle it in the next iteration of
                     // loadIncremental properly.
@@ -342,6 +384,9 @@ private:
 private:
     MasterLexer lexer_;
     const Name zone_origin_;
+    Name active_origin_; // The origin used during parsing
+                         // (modifiable by $ORIGIN)
+    shared_ptr<Name> last_name_; // Last seen name (for INITAL_WS handling)
     const RRClass zone_class_;
     MasterLoaderCallbacks callbacks_;
     AddRRCallback add_callback_;
@@ -357,6 +402,13 @@ private:
     bool ok_;                   // Is it OK to continue loading?
     const bool many_errors_;    // Are many errors allowed (or should we abort
                                 // on the first)
+    // Some info about the outer files from which we include.
+    // The first one is current origin, the second is the last seen name
+    // in that file.
+    typedef pair<Name, shared_ptr<Name> > IncludeInfo;
+    vector<IncludeInfo> include_info_;
+    bool previous_name_; // True if there was a previous name in this file
+                         // (false at the beginning or after an $INCLUDE line)
 public:
     bool complete_;             // All work done.
     bool seen_error_;           // Was there at least one error during the
@@ -365,6 +417,91 @@ public:
                                 // from the previous RR is used.
 };
 
+// A helper method of loadIncremental, parsing the first token of a new line.
+// If it looks like an RR, detect its owner name and return a string token for
+// the next field of the RR.
+// Otherwise, return either END_OF_LINE or END_OF_FILE token depending on
+// whether the loader continues to the next line or completes the load,
+// respectively.  Other corner cases including $-directive handling is done
+// here.
+// For unexpected errors, it throws an exception, which will be handled in
+// loadIncremental.
+MasterToken
+MasterLoader::MasterLoaderImpl::handleInitialToken() {
+    const MasterToken& initial_token =
+        lexer_.getNextToken(MasterLexer::QSTRING | MasterLexer::INITIAL_WS);
+
+    // The most likely case is INITIAL_WS, and then string/qstring.  We
+    // handle them first.
+    if (initial_token.getType() == MasterToken::INITIAL_WS) {
+        const MasterToken& next_token = lexer_.getNextToken();
+        if (next_token.getType() == MasterToken::END_OF_LINE) {
+            return (next_token); // blank line
+        } else if (next_token.getType() == MasterToken::END_OF_FILE) {
+            lexer_.ungetToken(); // handle it in the next iteration.
+            eatUntilEOL(true);  // effectively warn about the unexpected EOF.
+            return (MasterToken(MasterToken::END_OF_LINE));
+        }
+
+        // This means the same name as previous.
+        if (last_name_.get() == NULL) {
+            isc_throw(InternalException, "No previous name to use in "
+                      "place of initial whitespace");
+        } else if (!previous_name_) {
+            callbacks_.warning(lexer_.getSourceName(), lexer_.getSourceLine(),
+                               "Owner name omitted around $INCLUDE, the result "
+                               "might not be as expected");
+        }
+        return (next_token);
+    } else if (initial_token.getType() == MasterToken::STRING ||
+               initial_token.getType() == MasterToken::QSTRING) {
+        // If it is name (or directive), handle it.
+        const MasterToken::StringRegion&
+            name_string(initial_token.getStringRegion());
+
+        if (name_string.len > 0 && name_string.beg[0] == '$') {
+            // This should have either thrown (and the error handler
+            // will read up until the end of line) or read until the
+            // end of line.
+
+            // Exclude the $ from the string on this point.
+            handleDirective(name_string.beg + 1, name_string.len - 1);
+            // So, get to the next line, there's nothing more interesting
+            // in this one.
+            return (MasterToken(MasterToken::END_OF_LINE));
+        }
+
+        // This should be an RR, starting with an owner name.  Construct the
+        // name, and some string token should follow.
+        last_name_.reset(new Name(name_string.beg, name_string.len,
+                                  &active_origin_));
+        previous_name_ = true;
+        return (lexer_.getNextToken(MasterToken::STRING));
+    }
+
+    switch (initial_token.getType()) { // handle less common cases
+    case MasterToken::END_OF_FILE:
+        if (!popSource()) {
+            return (initial_token);
+        } else {
+            // We try to read a token from the popped source
+            // So continue to the next line of that source, but first, make
+            // sure the source is at EOL
+            eatUntilEOL(true);
+            return (MasterToken(MasterToken::END_OF_LINE));
+        }
+    case MasterToken::END_OF_LINE:
+        return (initial_token); // empty line
+    case MasterToken::ERROR:
+        // Error token here.
+        isc_throw(InternalException, initial_token.getErrorText());
+    default:
+        // Some other token (what could that be?)
+        isc_throw(InternalException, "Parser got confused (unexpected "
+                  "token " << initial_token.getType() << ")");
+    }
+}
+
 bool
 MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
     if (count_limit == 0) {
@@ -380,61 +517,32 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
     size_t count = 0;
     while (ok_ && count < count_limit) {
         try {
-            // Skip all EOLNs (empty lines) and finish on EOF
-            bool empty = true;
-            do {
-                const MasterToken& empty_token(lexer_.getNextToken());
-                if (empty_token.getType() == MasterToken::END_OF_FILE) {
-                    if (!popSource()) {
-                        return (true);
-                    } else {
-                        // We try to read a token from the popped source
-                        // So retry the loop, but first, make sure the source
-                        // is at EOL
-                        eatUntilEOL(true);
-                        continue;
-                    }
-                }
-                empty = empty_token.getType() == MasterToken::END_OF_LINE;
-            } while (empty);
-            // Return the last token, as it was not empty
-            lexer_.ungetToken();
-
-            const MasterToken::StringRegion&
-                name_string(lexer_.getNextToken(MasterToken::QSTRING).
-                            getStringRegion());
-
-            if (name_string.len > 0 && name_string.beg[0] == '$') {
-                // This should have either thrown (and the error handler
-                // will read up until the end of line) or read until the
-                // end of line.
-
-                // Exclude the $ from the string on this point.
-                handleDirective(name_string.beg + 1, name_string.len - 1);
-                // So, get to the next line, there's nothing more interesting
-                // in this one.
-                continue;
+            const MasterToken next_token = handleInitialToken();
+            if (next_token.getType() == MasterToken::END_OF_FILE) {
+                return (true);  // we are done
+            } else if (next_token.getType() == MasterToken::END_OF_LINE) {
+                continue;       // nothing more to do in this line
             }
+            // We are going to parse an RR, have known the owner name,
+            // and are now seeing the next string token in the rest of the RR.
+            assert(next_token.getType() == MasterToken::STRING);
 
-            const Name name(name_string.beg, name_string.len, &zone_origin_);
-
-            // The parameters
             bool explicit_ttl = false;
-            const RRType rrtype = parseRRParams(explicit_ttl);
+            const RRType rrtype = parseRRParams(explicit_ttl, next_token);
             // TODO: Check if it is SOA, it should be at the origin.
 
             const rdata::RdataPtr rdata =
-                rdata::createRdata(rrtype, zone_class_, lexer_, &zone_origin_,
-                                   options_, callbacks_);
+                rdata::createRdata(rrtype, zone_class_, lexer_,
+                                   &active_origin_, options_, callbacks_);
+
             // In case we get NULL, it means there was error creating
             // the Rdata. The errors should have been reported by
             // callbacks_ already. We need to decide if we want to continue
             // or not.
             if (rdata) {
-                add_callback_(name, zone_class_, rrtype,
+                add_callback_(*last_name_, zone_class_, rrtype,
                               getCurrentTTL(explicit_ttl, rrtype, rdata),
                               rdata);
-
                 // Good, we loaded another one
                 ++count;
             } else {

src/lib/dns/tests/master_lexer_state_unittest.cc

@@ -190,13 +190,16 @@ TEST_F(MasterLexerStateTest, unbalancedParentheses) {
 }
 
 TEST_F(MasterLexerStateTest, startToComment) {
-    // Begin with 'start', skip space, then encounter a comment.  Skip
+    // Begin with 'start', detect space, then encounter a comment.  Skip
     // the rest of the line, and recognize the new line.  Note that the
     // second ';' is simply ignored.
     ss << "  ;a;\n";
     ss << ";a;";           // Likewise, but the comment ends with EOF.
     lexer.pushSource(ss);
 
+    // Initial whitespace (asked for in common_options)
+    EXPECT_EQ(s_null, State::start(lexer, common_options));
+    EXPECT_EQ(Token::INITIAL_WS, s_crlf.getToken(lexer).getType());
     // Comment ending with EOL
     EXPECT_EQ(s_null, State::start(lexer, common_options));
     EXPECT_EQ(Token::END_OF_LINE, s_crlf.getToken(lexer).getType());

src/lib/dns/tests/master_lexer_unittest.cc

@@ -238,10 +238,8 @@ TEST_F(MasterLexerTest, ungetToken) {
 // Check ungetting token without overriding the start method. We also
 // check it works well with changing options between the calls.
 TEST_F(MasterLexerTest, ungetRealOptions) {
-    ss << "\n    \n";
+    ss << "    \n";
     lexer.pushSource(ss);
-    // Skip the first newline
-    EXPECT_EQ(MasterToken::END_OF_LINE, lexer.getNextToken().getType());
 
     // If we call it the usual way, it skips up to the newline and returns
     // it
@@ -254,6 +252,22 @@ TEST_F(MasterLexerTest, ungetRealOptions) {
               lexer.getNextToken(MasterLexer::INITIAL_WS).getType());
 }
 
+// Check the initial whitespace is found even in the first line of included
+// file
+TEST_F(MasterLexerTest, includeAndInitialWS) {
+    ss << "    \n";
+    lexer.pushSource(ss);
+
+    stringstream ss2;
+    ss2 << "    \n";
+
+    EXPECT_EQ(MasterToken::INITIAL_WS,
+              lexer.getNextToken(MasterLexer::INITIAL_WS).getType());
+    lexer.pushSource(ss2);
+    EXPECT_EQ(MasterToken::INITIAL_WS,
+              lexer.getNextToken(MasterLexer::INITIAL_WS).getType());
+}
+
 // Test only one token can be ungotten
 TEST_F(MasterLexerTest, ungetTwice) {
     ss << "\n";

src/lib/dns/tests/master_loader_unittest.cc

@@ -50,6 +50,11 @@ public:
                                &warnings_, _1, _2, _3))
     {}
 
+    void TearDown() {
+        // Check there are no more RRs we didn't expect
+        EXPECT_TRUE(rrsets_.empty());
+    }
+
     /// Concatenate file, line, and reason, and add it to either errors
     /// or warnings
     void callback(vector<string>* target, const std::string& file, size_t line,
@@ -127,6 +132,11 @@ public:
                 "1234 3600 1800 2419200 7200");
         checkRR("example.org", RRType::NS(), "ns1.example.org.");
         checkRR("www.example.org", RRType::A(), "192.0.2.1");
+        checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
+    }
+
+    void checkARR(const string& name) {
+        checkRR(name, RRType::A(), "192.0.2.1");
     }
 
     MasterLoaderCallbacks callbacks_;
@@ -185,6 +195,66 @@ TEST_F(MasterLoaderTest, include) {
     }
 }
 
+// A commonly used helper to check callback message.
+void
+checkCallbackMessage(const string& actual_msg, const string& expected_msg,
+                     size_t expected_line) {
+    // The actual message should begin with the expected message.
+    EXPECT_EQ(0, actual_msg.find(expected_msg)) << "actual message: " <<
+                                                actual_msg << " expected: " <<
+                                                expected_msg;
+
+    // and it should end with "...:<line_num>]"
+    const string line_desc = ":" + lexical_cast<string>(expected_line) + "]";
+    EXPECT_EQ(actual_msg.size() - line_desc.size(),
+              actual_msg.find(line_desc)) << "Expected on line " <<
+        expected_line;
+}
+
+TEST_F(MasterLoaderTest, origin) {
+    // Various forms of the directive
+    const char* origins[] = {
+        "$origin",
+        "$ORIGIN",
+        "$Origin",
+        "$OrigiN",
+        "\"$ORIGIN\"",
+        NULL
+    };
+    for (const char** origin = origins; *origin != NULL; ++origin) {
+        SCOPED_TRACE(*origin);
+
+        clear();
+        const string directive = *origin;
+        const string input =
+            "@  1H  IN  A   192.0.2.1\n" +
+            directive + " sub.example.org.\n"
+            "\"www\"    1H  IN  A   192.0.2.1\n" +
+            // Relative name in the origin
+            directive + " relative\n"
+            "@  1H  IN  A   192.0.2.1\n"
+            // Origin is _not_ used here (absolute name)
+            "noorigin.example.org.  60M IN  A   192.0.2.1\n";
+        stringstream ss(input);
+        setLoader(ss, Name("example.org."), RRClass::IN(),
+                  MasterLoader::MANY_ERRORS);
+
+        loader_->load();
+        EXPECT_TRUE(loader_->loadedSucessfully());
+        EXPECT_TRUE(errors_.empty());
+        // There's a relative origin in it, we warn about that.
+        EXPECT_EQ(1, warnings_.size());
+        checkCallbackMessage(warnings_.at(0),
+                             "The new origin is relative, did you really mean "
+                             "relative.sub.example.org.?", 4);
+
+        checkARR("example.org");
+        checkARR("www.sub.example.org");
+        checkARR("relative.sub.example.org");
+        checkARR("noorigin.example.org");
+    }
+}
+
 // Test the source is correctly popped even after error
 TEST_F(MasterLoaderTest, popAfterError) {
     const string include_str = "$include " TEST_DATA_SRCDIR
@@ -250,6 +320,7 @@ TEST_F(MasterLoaderTest, incrementalLoad) {
     EXPECT_TRUE(warnings_.empty());
 
     checkRR("www.example.org", RRType::A(), "192.0.2.1");
+    checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
 }
 
 // Try loading from file that doesn't exist. There should be single error
@@ -330,12 +401,24 @@ struct ErrorCase {
     // Check the unknown directive. The rest looks like ordinary RR,
     // so we see the $ is actually special.
     { "$UNKNOWN 3600    IN  A   192.0.2.1", NULL, "Unknown $ directive" },
-    { "$INCLUD " TEST_DATA_SRCDIR "/example.org", NULL, "Include too short" },
-    { "$INCLUDES " TEST_DATA_SRCDIR "/example.org", NULL, "Include too long" },
-    { "$INCLUDE", NULL, "Missing include path" },
+    { "$INCLUD " TEST_DATA_SRCDIR "/example.org", "Unknown directive 'INCLUD'",
+        "Include too short" },
+    { "$INCLUDES " TEST_DATA_SRCDIR "/example.org",
+        "Unknown directive 'INCLUDES'", "Include too long" },
+    { "$INCLUDE", "unexpected end of input", "Missing include path" },
+    // The following two error messages are system dependant, omitting
     { "$INCLUDE /file/not/found", NULL, "Include file not found" },
-    { "$INCLUDE /file/not/found and here goes bunch of garbage", NULL,
-        "Include file not found and garbage at the end of line" },
+    { "$INCLUDE /file/not/found example.org. and here goes bunch of garbage",
+        NULL, "Include file not found and garbage at the end of line" },
+    { "$ORIGIN", "unexpected end of input", "Missing origin name" },
+    { "$ORIGIN invalid...name", "duplicate period in invalid...name",
+        "Invalid name for origin" },
+    { "$ORIGIN )brokentoken", "unbalanced parentheses",
+        "Broken token in origin" },
+    { "$ORIGIN example.org. garbage", "Extra tokens at the end of line",
+        "Garbage after origin" },
+    { "$ORIGI name.", "Unknown directive 'ORIGI'", "$ORIGIN too short" },
+    { "$ORIGINAL name.", "Unknown directive 'ORIGINAL'", "$ORIGIN too long" },
     { "$TTL 100 extra-garbage", "Extra tokens at the end of line",
       "$TTL with extra token" },
     { "$TTL", "unexpected end of input", "missing TTL" },
@@ -346,19 +429,6 @@ struct ErrorCase {
     { NULL, NULL, NULL }
 };
 
-// A commonly used helper to check callback message.
-void
-checkCallbackMessage(const string& actual_msg, const string& expected_msg,
-                     size_t expected_line) {
-    // The actual message should begin with the expected message.
-    EXPECT_EQ(0, actual_msg.find(expected_msg)) << "actual message: "
-                                                << actual_msg;
-
-    // and it should end with "...:<line_num>]"
-    const string line_desc = ":" + lexical_cast<string>(expected_line) + "]";
-    EXPECT_EQ(actual_msg.size() - line_desc.size(), actual_msg.find(line_desc));
-}
-
 // Test a broken zone is handled properly. We test several problems,
 // both in strict and lenient mode.
 TEST_F(MasterLoaderTest, brokenZone) {
@@ -433,7 +503,7 @@ TEST_F(MasterLoaderTest, includeWithGarbage) {
     // Include an origin (example.org) because we expect it to be handled
     // soon and we don't want it to break here.
     const string include_str("$INCLUDE " TEST_DATA_SRCDIR
-                             "/example.org example.org bunch of other stuff\n"
+                             "/example.org example.org. bunch of other stuff\n"
                              "www 3600 IN AAAA 2001:db8::1\n");
     stringstream zone_stream(include_str);
     setLoader(zone_stream, Name("example.org."), RRClass::IN(),
@@ -442,6 +512,7 @@ TEST_F(MasterLoaderTest, includeWithGarbage) {
     EXPECT_NO_THROW(loader_->load());
     EXPECT_FALSE(loader_->loadedSucessfully());
     ASSERT_EQ(1, errors_.size());
+    checkCallbackMessage(errors_.at(0), "Extra tokens at the end of line", 1);
     // It says something about extra tokens at the end
     EXPECT_NE(string::npos, errors_[0].find("Extra"));
     EXPECT_TRUE(warnings_.empty());
@@ -449,6 +520,105 @@ TEST_F(MasterLoaderTest, includeWithGarbage) {
     checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
 }
 
+// Check we error about garbage at the end of $ORIGIN line (but the line
+// works).
+TEST_F(MasterLoaderTest, originWithGarbage) {
+    const string origin_str = "$ORIGIN www.example.org. More garbage here\n"
+        "@  1H  IN  A   192.0.2.1\n";
+    stringstream ss(origin_str);
+    setLoader(ss, Name("example.org."), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    EXPECT_NO_THROW(loader_->load());
+    EXPECT_FALSE(loader_->loadedSucessfully());
+    ASSERT_EQ(1, errors_.size());
+    checkCallbackMessage(errors_.at(0), "Extra tokens at the end of line", 1);
+    EXPECT_TRUE(warnings_.empty());
+    checkARR("www.example.org");
+}
+
+// Test we can pass both file to include and the origin to switch
+TEST_F(MasterLoaderTest, includeAndOrigin) {
+    // First, switch origin to something else, so we can check it is
+    // switched back.
+    const string include_string = "$ORIGIN www.example.org.\n"
+        "@  1H  IN  A   192.0.2.1\n"
+        // Then include the file with data and switch origin back
+        "$INCLUDE " TEST_DATA_SRCDIR "/example.org example.org.\n"
+        // Another RR to see the switch survives after we exit include
+        "www    1H  IN  A   192.0.2.1\n";
+    stringstream ss(include_string);
+    setLoader(ss, Name("example.org"), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    // Successfully load the data
+    loader_->load();
+    EXPECT_TRUE(loader_->loadedSucessfully());
+    EXPECT_TRUE(errors_.empty());
+    EXPECT_TRUE(warnings_.empty());
+    // And check it's the correct data
+    checkARR("www.example.org");
+    checkBasicRRs();
+    checkARR("www.example.org");
+}
+
+// Like above, but the origin after include is bogus. The whole line should
+// be rejected.
+TEST_F(MasterLoaderTest, includeAndBadOrigin) {
+    const string include_string =
+        "$INCLUDE " TEST_DATA_SRCDIR "/example.org example..org.\n"
+        // Another RR to see the switch survives after we exit include
+        "www    1H  IN  A   192.0.2.1\n";
+    stringstream ss(include_string);
+    setLoader(ss, Name("example.org"), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    loader_->load();
+    EXPECT_FALSE(loader_->loadedSucessfully());
+    EXPECT_EQ(1, errors_.size());
+    checkCallbackMessage(errors_.at(0), "duplicate period in example..org.",
+                         1);
+    EXPECT_TRUE(warnings_.empty());
+    // And check it's the correct data
+    checkARR("www.example.org");
+}
+
+// Check the origin doesn't get outside of the included file.
+TEST_F(MasterLoaderTest, includeOriginRestore) {
+    const string include_string = "$INCLUDE " TEST_DATA_SRCDIR "/origincheck.txt\n"
+        "@  1H  IN  A   192.0.2.1\n";
+    stringstream ss(include_string);
+    setLoader(ss, Name("example.org"), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    // Successfully load the data
+    loader_->load();
+    EXPECT_TRUE(loader_->loadedSucessfully());
+    EXPECT_TRUE(errors_.empty());
+    EXPECT_TRUE(warnings_.empty());
+    // And check it's the correct data
+    checkARR("www.example.org");
+    checkARR("example.org");
+}
+
+// Check we restore the last name for initial whitespace when returning from
+// include. But we do produce a warning if there's one just ofter the include.
+TEST_F(MasterLoaderTest, includeAndInitialWS) {
+    const string include_string = "xyz  1H  IN  A   192.0.2.1\n"
+        "$INCLUDE " TEST_DATA_SRCDIR "/example.org\n"
+        "   1H  IN  A   192.0.2.1\n";
+    stringstream ss(include_string);
+    setLoader(ss, Name("example.org"), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    // Successfully load the data
+    loader_->load();
+    EXPECT_TRUE(loader_->loadedSucessfully());
+    EXPECT_TRUE(errors_.empty());
+    EXPECT_EQ(1, warnings_.size());
+    checkCallbackMessage(warnings_.at(0),
+                         "Owner name omitted around $INCLUDE, the result might "
+                         "not be as expected", 3);
+    checkARR("xyz.example.org");
+    checkBasicRRs();
+    checkARR("xyz.example.org");
+}
+
 // Test for "$TTL"
 TEST_F(MasterLoaderTest, ttlDirective) {
     stringstream zone_stream;
@@ -598,7 +768,8 @@ TEST_F(MasterLoaderTest, ttlUnknownAndEOF) {
     // RDATA implementation can complain about it, too.  To be independent of
     // its details, we focus on the very last warning.
     EXPECT_FALSE(warnings_.empty());
-    checkCallbackMessage(*warnings_.rbegin(), "Unexpected end of file", 1);
+    checkCallbackMessage(*warnings_.rbegin(), "File does not end with newline",
+                         1);
 }
 
 TEST_F(MasterLoaderTest, ttlOverflow) {
@@ -650,6 +821,9 @@ TEST_F(MasterLoaderTest, loadTwice) {
 
     loader_->load();
     EXPECT_THROW(loader_->load(), isc::InvalidOperation);
+    // Don't check them, they are not interesting, so suppress the error
+    // at TearDown
+    rrsets_.clear();
 }
 
 // Load 0 items should be rejected
@@ -671,10 +845,44 @@ TEST_F(MasterLoaderTest, noEOLN) {
 
     loader_->load();
     EXPECT_TRUE(loader_->loadedSucessfully());
-    EXPECT_TRUE(errors_.empty()) << errors_[0];
+    EXPECT_TRUE(errors_.empty());
     // There should be one warning about the EOLN
     EXPECT_EQ(1, warnings_.size());
     checkRR("example.org", RRType::SOA(), "ns1.example.org. "
             "admin.example.org. 1234 3600 1800 2419200 7200");
 }
+
+// Test it rejects when we don't have the previous name to use in place of
+// initial whitespace
+TEST_F(MasterLoaderTest, noPreviousName) {
+    const string input("    1H  IN  A   192.0.2.1\n");
+    stringstream ss(input);
+    setLoader(ss, Name("example.org."), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    loader_->load();
+    EXPECT_FALSE(loader_->loadedSucessfully());
+    EXPECT_EQ(1, errors_.size());
+    checkCallbackMessage(errors_.at(0), "No previous name to use in place of "
+                         "initial whitespace", 1);
+    EXPECT_TRUE(warnings_.empty());
+}
+
+// Check we warn if the first RR in an included file has omitted name
+TEST_F(MasterLoaderTest, previousInInclude) {
+    const string input("www 1H  IN  A   192.0.2.1\n"
+                       "$INCLUDE " TEST_DATA_SRCDIR "/omitcheck.txt\n");
+    stringstream ss(input);
+    setLoader(ss, Name("example.org"), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+    loader_->load();
+    EXPECT_TRUE(loader_->loadedSucessfully());
+    EXPECT_TRUE(errors_.empty());
+    // There should be one warning about the EOLN
+    EXPECT_EQ(1, warnings_.size());
+    checkCallbackMessage(warnings_.at(0), "Owner name omitted around "
+                         "$INCLUDE, the result might not be as expected", 1);
+    checkARR("www.example.org");
+    checkARR("www.example.org");
+}
+
 }

src/lib/dns/tests/testdata/Makefile.am

@@ -172,6 +172,8 @@ EXTRA_DIST += tsig_verify7.spec tsig_verify8.spec tsig_verify9.spec
 EXTRA_DIST += tsig_verify10.spec
 EXTRA_DIST += example.org
 EXTRA_DIST += broken.zone
+EXTRA_DIST += origincheck.txt
+EXTRA_DIST += omitcheck.txt
 
 .spec.wire:
 	$(PYTHON) $(top_builddir)/src/lib/util/python/gen_wiredata.py -o $@ $<

src/lib/dns/tests/testdata/example.org

@@ -13,3 +13,5 @@ example.org.        3600    IN  SOA ( ; The SOA, split across lines for testing
 
 ; Some empty lines here. They are to make sure the loader can skip them.
 www                 3600    IN  A 192.0.2.1 ; Test a relative name as well.
+                    3600    IN  AAAA    2001:db8::1 ; And initial whitespace hanling
+         ; Here be just some space, no RRs

src/lib/dns/tests/testdata/omitcheck.txt

@@ -0,0 +1 @@
+    1H  IN  A   192.0.2.1

src/lib/dns/tests/testdata/origincheck.txt

@@ -0,0 +1,5 @@
+; We change the origin here. We want to check it is not propagated
+; outside of the included file.
+$ORIGIN www.example.org.
+
+@   1H  IN  A   192.0.2.1