[2429] Merge remote-tracking branch 'origin/trac2428' into trac2429

resolving conflicts in:
	src/lib/dns/master_loader.cc

src/lib/dns/master_loader.cc

@@ -22,6 +22,7 @@
 
 #include <string>
 #include <memory>
+#include <strings.h>
 
 using std::string;
 using std::auto_ptr;
@@ -29,6 +30,16 @@ using std::auto_ptr;
 namespace isc {
 namespace dns {
 
+// 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)
+    {}
+};
+
 class MasterLoader::MasterLoaderImpl {
 public:
     MasterLoaderImpl(const char* master_file,
@@ -47,6 +58,7 @@ public:
         initialized_(false),
         ok_(true),
         many_errors_((options & MANY_ERRORS) != 0),
+        source_count_(0),
         complete_(false),
         seen_error_(false)
     {}
@@ -69,9 +81,7 @@ public:
         std::string error;
         if (!lexer_.pushSource(filename.c_str(), &error)) {
             if (initialized_) {
-                // $INCLUDE file
-                reportError(lexer_.getSourceName(), lexer_.getSourceLine(),
-                            error);
+                isc_throw(InternalException, error.c_str());
             } else {
                 // Top-level file
                 reportError("", 0, error);
@@ -79,11 +89,21 @@ public:
             }
         }
         initialized_ = true;
+        ++source_count_;
+    }
+
+    bool popSource() {
+        if (--source_count_ == 0) {
+            return (false);
+        }
+        lexer_.popSource();
+        return (true);
     }
 
     void pushStreamSource(std::istream& stream) {
         lexer_.pushSource(stream);
         initialized_ = true;
+        ++source_count_;
     }
 
     // Get a string token. Handle it as error if it is not string.
@@ -94,6 +114,85 @@ public:
 
     bool loadIncremental(size_t count_limit);
 
+    void doInclude() {
+        // First, get the filename to include
+        const MasterToken::StringRegion
+            filename_tok(lexer_.getNextToken(MasterToken::QSTRING).
+                         getStringRegion());
+
+        // Push the filename. We abuse the fact that filename
+        // may not contain '\0' anywhere in it, so we can
+        // freely use the filename.beg directly.
+        string filename(filename_tok.beg);
+
+        // 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) {
+        // We use strncasecmp, because there seems to be no reasonable
+        // way to compare strings case-insensitive in C++
+
+        // Warning: The order of compared strings does matter. The length
+        // parameter applies to the first one only.
+        if (strncasecmp(directive, "INCLUDE", length) == 0) {
+            doInclude();
+        } else if (strncasecmp(directive, "ORIGIN", length) == 0) {
+            // TODO: Implement
+            isc_throw(isc::NotImplemented,
+                      "Origin directive not implemented yet");
+        } else if (strncasecmp(directive, "TTL", length) == 0) {
+            // 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(),
+                                       "Unexpected end ond of file");
+                    // 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_;
@@ -107,6 +206,7 @@ private:
     bool ok_;                   // Is it OK to continue loading?
     const bool many_errors_;    // Are many errors allowed (or should we abort
                                 // on the first)
+    size_t source_count_;       // How many sources are currently pushed.
 public:
     bool complete_;             // All work done.
     bool seen_error_;           // Was there at least one error during the
@@ -133,8 +233,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
-                    return (true);
+                    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);
@@ -144,7 +251,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 +318,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
-            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);
+            eatUntilEOL(false);
         }
     }
     // When there was a fatal error and ok is false, we say we are done.

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,59 @@ TEST_F(MasterLoaderTest, basicLoad) {
     EXPECT_TRUE(errors_.empty());
     EXPECT_TRUE(warnings_.empty());
 
-    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");
+    checkBasicRRs();
+}
+
+// Test the $INCLUDE directive
+TEST_F(MasterLoaderTest, include) {
+    // Test various cases of include
+    const char* includes[] = {
+        "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 +279,13 @@ 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" },
+    { "$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 +305,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 +336,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 +354,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