Merge #2428

$INCLUDE handling in the isc::dns::MasterLoader

Conflicts:
	src/lib/dns/master_loader.cc

src/lib/dns/master_lexer.cc

@@ -149,6 +149,11 @@ MasterLexer::popSource() {
     impl_->has_previous_ = false;
 }
 
+size_t
+MasterLexer::getSourceCount() const {
+    return (impl_->sources_.size());
+}
+
 std::string
 MasterLexer::getSourceName() const {
     if (impl_->sources_.empty()) {

src/lib/dns/master_lexer.h

@@ -404,6 +404,11 @@ public:
     /// \throw isc::InvalidOperation Called with no pushed source.
     void popSource();
 
+    /// \brief Get number of sources inside the lexer.
+    ///
+    /// This method never throws.
+    size_t getSourceCount() const;
+
     /// \brief Return the name of the current input source name.
     ///
     /// If it's a file, it will be the C string given at the corresponding

src/lib/dns/master_loader.cc

@@ -22,13 +22,29 @@
 
 #include <string>
 #include <memory>
+#include <boost/algorithm/string/predicate.hpp> // for iequals
 
 using std::string;
 using std::auto_ptr;
+using boost::algorithm::iequals;
 
 namespace isc {
 namespace dns {
 
+namespace {
+
+// An internal exception, used to control the code flow in case of errors.
+// It is thrown during the loading and caught later, not to be propagated
+// outside of the file.
+class InternalException : public isc::Exception {
+public:
+    InternalException(const char* filename, size_t line, const char* what) :
+        Exception(filename, line, what)
+    {}
+};
+
+} // end unnamed namespace
+
 class MasterLoader::MasterLoaderImpl {
 public:
     MasterLoaderImpl(const char* master_file,
@@ -69,9 +85,7 @@ public:
         std::string error;
         if (!lexer_.pushSource(filename.c_str(), &error)) {
             if (initialized_) {
-                // $INCLUDE file
+                isc_throw(InternalException, error.c_str());
-                reportError(lexer_.getSourceName(), lexer_.getSourceLine(),
-                            error);
             } else {
                 // Top-level file
                 reportError("", 0, error);
@@ -81,6 +95,14 @@ public:
         initialized_ = true;
     }
 
+    bool popSource() {
+        if (lexer_.getSourceCount() == 1) {
+            return (false);
+        }
+        lexer_.popSource();
+        return (true);
+    }
+
     void pushStreamSource(std::istream& stream) {
         lexer_.pushSource(stream);
         initialized_ = true;
@@ -94,6 +116,74 @@ public:
 
     bool loadIncremental(size_t count_limit);
 
+    void doInclude() {
+        // First, get the filename to include
+        const string
+            filename(lexer_.getNextToken(MasterToken::QSTRING).getString());
+
+        // There could be an origin (or maybe not). So try looking
+        const MasterToken name_tok(lexer_.getNextToken(MasterToken::QSTRING,
+                                                       true));
+
+        if (name_tok.getType() == MasterToken::QSTRING ||
+            name_tok.getType() == MasterToken::STRING) {
+            // TODO: Handle the origin. Once we complete #2427.
+        } 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.
+            lexer_.ungetToken();
+        }
+
+        pushSource(filename);
+    }
+
+    void handleDirective(const char* directive, size_t length) {
+        if (iequals(directive, "INCLUDE")) {
+            doInclude();
+        } else if (iequals(directive, "ORIGIN")) {
+            // TODO: Implement
+            isc_throw(isc::NotImplemented,
+                      "Origin directive not implemented yet");
+        } else if (iequals(directive, "TTL")) {
+            // TODO: Implement
+            isc_throw(isc::NotImplemented,
+                      "TTL directive not implemented yet");
+        } else {
+            isc_throw(InternalException, "Unknown directive '" <<
+                      string(directive, directive + length) << "'");
+        }
+    }
+
+    void eatUntilEOL(bool reportExtra) {
+        // We want to continue. Try to read until the end of line
+        for (;;) {
+            const MasterToken& token(lexer_.getNextToken());
+            switch (token.getType()) {
+                case MasterToken::END_OF_FILE:
+                    callbacks_.warning(lexer_.getSourceName(),
+                                       lexer_.getSourceLine(),
+                                       "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.
+                    return;
+                case MasterToken::END_OF_LINE:
+                    // Found the end of the line. Good.
+                    return;
+                default:
+                    // Some other type of token.
+                    if (reportExtra) {
+                        reportExtra = false;
+                        reportError(lexer_.getSourceName(),
+                                    lexer_.getSourceLine(),
+                                    "Extra tokens at the end of line");
+                    }
+                    break;
+            }
+        }
+    }
+
 private:
     MasterLexer lexer_;
     const Name zone_origin_;
@@ -133,8 +223,15 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
             do {
                 const MasterToken& empty_token(lexer_.getNextToken());
                 if (empty_token.getType() == MasterToken::END_OF_FILE) {
-                    // TODO: Check if this is the last source, possibly pop
+                    if (!popSource()) {
-                    return (true);
+                        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);
@@ -144,7 +241,19 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
             const MasterToken::StringRegion&
                 name_string(lexer_.getNextToken(MasterToken::QSTRING).
                             getStringRegion());
-            // TODO $ handling
+
+            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 Name name(name_string.beg, name_string.len,
                             &zone_origin_);
             // TODO: Some more flexibility. We don't allow omitting
@@ -199,27 +308,7 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
             // propagated.
             reportError(lexer_.getSourceName(), lexer_.getSourceLine(),
                         e.what());
-            // We want to continue. Try to read until the end of line
+            eatUntilEOL(false);
-            bool end = false;
-            do {
-                const MasterToken& token(lexer_.getNextToken());
-                switch (token.getType()) {
-                    case MasterToken::END_OF_FILE:
-                        callbacks_.warning(lexer_.getSourceName(),
-                                           lexer_.getSourceLine(),
-                                           "File does not end with newline");
-                        // TODO: Try pop in case this is not the only
-                        // source
-                        return (true);
-                    case MasterToken::END_OF_LINE:
-                        end = true;
-                        break;
-                    default:
-                        // Do nothing. This is just to make compiler
-                        // happy
-                        break;
-                }
-            } while (!end);
         }
     }
     // When there was a fatal error and ok is false, we say we are done.

src/lib/dns/tests/master_lexer_unittest.cc

@@ -60,8 +60,10 @@ TEST_F(MasterLexerTest, preOpen) {
 }
 
 TEST_F(MasterLexerTest, pushStream) {
+    EXPECT_EQ(0, lexer.getSourceCount());
     lexer.pushSource(ss);
     EXPECT_EQ(expected_stream_name, lexer.getSourceName());
+    EXPECT_EQ(1, lexer.getSourceCount());
 
     // From the point of view of this test, we only have to check (though
     // indirectly) getSourceLine calls InputSource::getCurrentLine.  It should
@@ -70,18 +72,22 @@ TEST_F(MasterLexerTest, pushStream) {
 
     // By popping it the stack will be empty again.
     lexer.popSource();
+    EXPECT_EQ(0, lexer.getSourceCount());
     checkEmptySource(lexer);
 }
 
 TEST_F(MasterLexerTest, pushFile) {
     // We use zone file (-like) data, but in this test that actually doesn't
     // matter.
+    EXPECT_EQ(0, lexer.getSourceCount());
     EXPECT_TRUE(lexer.pushSource(TEST_DATA_SRCDIR "/masterload.txt"));
+    EXPECT_EQ(1, lexer.getSourceCount());
     EXPECT_EQ(TEST_DATA_SRCDIR "/masterload.txt", lexer.getSourceName());
     EXPECT_EQ(1, lexer.getSourceLine());
 
     lexer.popSource();
     checkEmptySource(lexer);
+    EXPECT_EQ(0, lexer.getSourceCount());
 
     // If we give a non NULL string pointer, its content will be intact
     // if pushSource succeeds.

src/lib/dns/tests/master_loader_unittest.cc

@@ -115,6 +115,14 @@ public:
                   compare(current->getRdataIterator()->getCurrent()));
     }
 
+    void checkBasicRRs() {
+        checkRR("example.org", RRType::SOA(),
+                "ns1.example.org. admin.example.org. "
+                "1234 3600 1800 2419200 7200");
+        checkRR("example.org", RRType::NS(), "ns1.example.org.");
+        checkRR("www.example.org", RRType::A(), "192.0.2.1");
+    }
+
     MasterLoaderCallbacks callbacks_;
     boost::scoped_ptr<MasterLoader> loader_;
     vector<string> errors_;
@@ -135,11 +143,60 @@ TEST_F(MasterLoaderTest, basicLoad) {
     EXPECT_TRUE(errors_.empty());
     EXPECT_TRUE(warnings_.empty());
 
-    checkRR("example.org", RRType::SOA(),
+    checkBasicRRs();
-            "ns1.example.org. admin.example.org. "
+}
-            "1234 3600 1800 2419200 7200");
+
-    checkRR("example.org", RRType::NS(), "ns1.example.org.");
+// Test the $INCLUDE directive
-    checkRR("www.example.org", RRType::A(), "192.0.2.1");
+TEST_F(MasterLoaderTest, include) {
+    // Test various cases of include
+    const char* includes[] = {
+        "$include",
+        "$INCLUDE",
+        "$Include",
+        "$InCluDe",
+        "\"$INCLUDE\"",
+        NULL
+    };
+    for (const char** include = includes; *include != NULL; ++include) {
+        SCOPED_TRACE(*include);
+
+        clear();
+        // Prepare input source that has the include and some more data
+        // below (to see it returns back to the original source).
+        const string include_str = string(*include) + " " +
+            TEST_DATA_SRCDIR + "/example.org\nwww 3600 IN AAAA 2001:db8::1\n";
+        stringstream ss(include_str);
+        setLoader(ss, Name("example.org."), RRClass::IN(),
+                  MasterLoader::MANY_ERRORS);
+
+        loader_->load();
+        EXPECT_TRUE(loader_->loadedSucessfully());
+        EXPECT_TRUE(errors_.empty());
+        EXPECT_TRUE(warnings_.empty());
+
+        checkBasicRRs();
+        checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
+    }
+}
+
+// Test the source is correctly popped even after error
+TEST_F(MasterLoaderTest, popAfterError) {
+    const string include_str = "$include " TEST_DATA_SRCDIR
+        "/broken.zone\nwww 3600 IN AAAA 2001:db8::1\n";
+    stringstream ss(include_str);
+    // We don't test without MANY_ERRORS, we want to see what happens
+    // after the error.
+    setLoader(ss, Name("example.org."), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+
+    loader_->load();
+    EXPECT_FALSE(loader_->loadedSucessfully());
+    EXPECT_EQ(1, errors_.size()); // For the broken RR
+    EXPECT_EQ(1, warnings_.size()); // For missing EOLN
+
+    // The included file doesn't contain anything usable, but the
+    // line after the include should be there.
+    checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
 }
 
 // Check it works the same when created based on a stream, not filename
@@ -223,6 +280,15 @@ struct ErrorCase {
     { "www      3600    IN  \"A\"   192.0.2.1", "Quoted type" },
     { "unbalanced)paren 3600    IN  A   192.0.2.1", "Token error 1" },
     { "www  3600    unbalanced)paren    A   192.0.2.1", "Token error 2" },
+    // 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", "Unknown $ directive" },
+    { "$INCLUD " TEST_DATA_SRCDIR "/example.org", "Include too short" },
+    { "$INCLUDES " TEST_DATA_SRCDIR "/example.org", "Include too long" },
+    { "$INCLUDE", "Missing include path" },
+    { "$INCLUDE /file/not/found", "Include file not found" },
+    { "$INCLUDE /file/not/found and here goes bunch of garbage",
+        "Include file not found and garbage at the end of line" },
     { NULL, NULL }
 };
 
@@ -242,7 +308,7 @@ TEST_F(MasterLoaderTest, brokenZone) {
             EXPECT_FALSE(loader_->loadedSucessfully());
             EXPECT_THROW(loader_->load(), MasterLoaderError);
             EXPECT_FALSE(loader_->loadedSucessfully());
-            EXPECT_EQ(1, errors_.size()) << errors_[0];
+            EXPECT_EQ(1, errors_.size());
             EXPECT_TRUE(warnings_.empty());
 
             checkRR("example.org", RRType::SOA(), "ns1.example.org. "
@@ -273,15 +339,15 @@ TEST_F(MasterLoaderTest, brokenZone) {
         {
             SCOPED_TRACE("Error at EOF");
             // This case is interesting only in the lenient mode.
-            const string zoneEOF(prepareZone(ec->line, false));
             clear();
+            const string zoneEOF(prepareZone(ec->line, false));
             stringstream zone_stream(zoneEOF);
             setLoader(zone_stream, Name("example.org."), RRClass::IN(),
                       MasterLoader::MANY_ERRORS);
             EXPECT_FALSE(loader_->loadedSucessfully());
             EXPECT_NO_THROW(loader_->load());
             EXPECT_FALSE(loader_->loadedSucessfully());
-            EXPECT_EQ(1, errors_.size());
+            EXPECT_EQ(1, errors_.size()) << errors_[0] << "\n" << errors_[1];
             // The unexpected EOF warning
             EXPECT_EQ(1, warnings_.size());
             checkRR("example.org", RRType::SOA(), "ns1.example.org. "
@@ -291,6 +357,28 @@ TEST_F(MasterLoaderTest, brokenZone) {
     }
 }
 
+// Check that a garbage after the include generates an error, but not fatal
+// one (in lenient mode) and we can recover.
+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"
+                             "www 3600 IN AAAA 2001:db8::1\n");
+    stringstream zone_stream(include_str);
+    setLoader(zone_stream, Name("example.org."), RRClass::IN(),
+              MasterLoader::MANY_ERRORS);
+
+    EXPECT_NO_THROW(loader_->load());
+    EXPECT_FALSE(loader_->loadedSucessfully());
+    ASSERT_EQ(1, errors_.size());
+    // It says something about extra tokens at the end
+    EXPECT_NE(string::npos, errors_[0].find("Extra"));
+    EXPECT_TRUE(warnings_.empty());
+    checkBasicRRs();
+    checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
+}
+
 // Test the constructor rejects empty add callback.
 TEST_F(MasterLoaderTest, emptyCallback) {
     EXPECT_THROW(MasterLoader(TEST_DATA_SRCDIR "/example.org",

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

@@ -171,6 +171,7 @@ EXTRA_DIST += tsig_verify4.spec tsig_verify5.spec tsig_verify6.spec
 EXTRA_DIST += tsig_verify7.spec tsig_verify8.spec tsig_verify9.spec
 EXTRA_DIST += tsig_verify10.spec
 EXTRA_DIST += example.org
+EXTRA_DIST += broken.zone
 
 .spec.wire:
 	$(PYTHON) $(top_builddir)/src/lib/util/python/gen_wiredata.py -o $@ $<

src/lib/dns/tests/testdata/broken.zone

@@ -0,0 +1,3 @@
+; This should fail due to broken TTL
+; The file should _NOT_ end with EOLN
+broken.     3600X   IN  A   192.0.2.2 More data