From e76b79a1b16a388113c66083f5363b085814cdab Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Mon, 10 Dec 2012 16:50:49 +0100
Subject: [PATCH 01/17] [2428] Check the special handling of $

---
 src/lib/dns/tests/master_loader_unittest.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 74baa3b174..2af578e000 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -223,6 +223,9 @@ 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" },
     { NULL, NULL }
 };
 
@@ -242,7 +245,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. "

From 398a65223150685c4d022dd980d0292e8116afb1 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Mon, 10 Dec 2012 17:09:35 +0100
Subject: [PATCH 02/17] [2428] Generic $DIRECTIVE handler

Decide which directive it is and call corresponding call based on that.
This currently throws NotImplemented for everything except for the
INCLUDE, which will get implemented in the next few commits in this
branch.
---
 src/lib/dns/master_loader.cc | 47 +++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index f556dde6ea..a44fef1f0f 100644
--- a/src/lib/dns/master_loader.cc
+++ b/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,
@@ -94,6 +105,28 @@ public:
 
     bool loadIncremental(size_t count_limit);
 
+    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)) {
+
+        } else if (strncasecmp(directive, "ORIGIN", length)) {
+            // TODO: Implement
+            isc_throw(isc::NotImplemented,
+                      "Origin directive not implemented yet");
+        } else if (strncasecmp(directive, "TTL", length)) {
+            // TODO: Implement
+            isc_throw(isc::NotImplemented,
+                      "TTL directive not implemented yet");
+        } else {
+            isc_throw(InternalException, "Unknown directive '" <<
+                      string(directive, directive + length) << "'");
+        }
+    }
+
 private:
     MasterLexer lexer_;
     const Name zone_origin_;
@@ -144,7 +177,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

From e089de85aec2c705d7fee61f1a51e37aa2510756 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Mon, 10 Dec 2012 17:49:27 +0100
Subject: [PATCH 03/17] [2428] Tests for basic $INCLUDE handling

No special cases, no error handling, no domain name after that.
---
 src/lib/dns/tests/master_loader_unittest.cc | 43 ++++++++++++++++++---
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 2af578e000..ca04b5c053 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/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,36 @@ 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);
+        // Prepare input source that has no other content than just the
+        // include of the real master file.
+        const string include_str = string(*include) + " " + TEST_DATA_SRCDIR +
+            "/example.org\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();
+    }
 }
 
 // Check it works the same when created based on a stream, not filename

From c9bd919bd97e5fd2cf3f50470a29c119bd3b1151 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Mon, 10 Dec 2012 19:14:02 +0100
Subject: [PATCH 04/17] [2428] Implement basic inclusion

---
 src/lib/dns/master_loader.cc                | 27 ++++++++++++++++++---
 src/lib/dns/tests/master_loader_unittest.cc |  6 ++---
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index a44fef1f0f..4457e387d3 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -105,19 +105,38 @@ public:
 
     bool loadIncremental(size_t count_limit);
 
+    void doInclude() {
+        // First, get the filename to include
+        const MasterToken::StringRegion
+            filename(lexer_.getNextToken(MasterLexer::QSTRING).
+                     getStringRegion());
+
+        // TODO: Handle the case where there's Name after the
+        // filename, meaning origin. Once $ORIGIN handling is
+        // done, it should be interconnected somehow.
+
+        // 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.
+        pushSource(filename.beg);
+
+        // TODO: Eat any extra tokens at the end of line (they
+        // should not be here, of course).
+    }
+
     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)) {
-
-        } else if (strncasecmp(directive, "ORIGIN", length)) {
+        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)) {
+        } else if (strncasecmp(directive, "TTL", length) == 0) {
             // TODO: Implement
             isc_throw(isc::NotImplemented,
                       "TTL directive not implemented yet");
diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index ca04b5c053..39740477c0 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -160,15 +160,15 @@ TEST_F(MasterLoaderTest, include) {
         SCOPED_TRACE(*include);
         // Prepare input source that has no other content than just the
         // include of the real master file.
-        const string include_str = string(*include) + " " + TEST_DATA_SRCDIR +
-            "/example.org\n";
+        const string include_str = "$" + string(*include) + " " +
+            TEST_DATA_SRCDIR + "/example.org\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(errors_.empty()) << errors_[0];
         EXPECT_TRUE(warnings_.empty());
 
         checkBasicRRs();

From 1b7273a397602ebd1d04d692f517941624d89839 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Mon, 10 Dec 2012 19:17:59 +0100
Subject: [PATCH 05/17] [2428] Test popping the source

---
 src/lib/dns/tests/master_loader_unittest.cc | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 39740477c0..17b221c43c 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -158,20 +158,21 @@ TEST_F(MasterLoaderTest, include) {
     };
     for (const char** include = includes; *include != NULL; ++include) {
         SCOPED_TRACE(*include);
-        // Prepare input source that has no other content than just the
-        // include of the real master file.
+        // 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\n";
+            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()) << errors_[0];
+        EXPECT_TRUE(errors_.empty());
         EXPECT_TRUE(warnings_.empty());
 
         checkBasicRRs();
+        checkRR("www.example.org", RRType::AAAA(), "2001:db8::1");
     }
 }
 

From de08d209347b6639bc28d1847344c80616a6cb87 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Mon, 10 Dec 2012 19:24:34 +0100
Subject: [PATCH 06/17] [2428] Implement popping of sources

At least in the normal case. Error cases must still be handled.
---
 src/lib/dns/master_loader.cc | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index 4457e387d3..a68c67f8da 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -58,6 +58,7 @@ public:
         initialized_(false),
         ok_(true),
         many_errors_((options & MANY_ERRORS) != 0),
+        source_count_(0),
         complete_(false),
         seen_error_(false)
     {}
@@ -90,11 +91,18 @@ public:
             }
         }
         initialized_ = true;
+        ++source_count_;
+    }
+
+    bool popSource() {
+        lexer_.popSource();
+        return (--source_count_ != 0);
     }
 
     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.
@@ -159,6 +167,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
@@ -185,8 +194,13 @@ 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
+                        continue;
+                    }
                 }
                 empty = empty_token.getType() == MasterToken::END_OF_LINE;
             } while (empty);

From 33fb59eeb630ce1fb6055a6a8d26c46951fc9c78 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Tue, 11 Dec 2012 13:45:16 +0100
Subject: [PATCH 07/17] [2428] Do pop even after error

---
 src/lib/dns/master_loader.cc                |  8 +++++---
 src/lib/dns/tests/master_loader_unittest.cc | 22 +++++++++++++++++++++
 src/lib/dns/tests/testdata/Makefile.am      |  1 +
 src/lib/dns/tests/testdata/broken.zone      |  3 +++
 4 files changed, 31 insertions(+), 3 deletions(-)
 create mode 100644 src/lib/dns/tests/testdata/broken.zone

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index a68c67f8da..bf28ea29f5 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -285,9 +285,11 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
                         callbacks_.warning(lexer_.getSourceName(),
                                            lexer_.getSourceLine(),
                                            "Unexpected end ond of file");
-                        // TODO: Try pop in case this is not the only
-                        // source
-                        return (true);
+                        if (!popSource()) {
+                            return (true);
+                        }
+                        // Else: fall through, the popped source is
+                        // at the end of line currently
                     case MasterToken::END_OF_LINE:
                         end = true;
                         break;
diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 17b221c43c..4860badee4 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -158,6 +158,8 @@ TEST_F(MasterLoaderTest, include) {
     };
     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) + " " +
@@ -176,6 +178,26 @@ TEST_F(MasterLoaderTest, include) {
     }
 }
 
+// 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
 TEST_F(MasterLoaderTest, streamConstructor) {
     stringstream zone_stream(prepareZone("", true));
diff --git a/src/lib/dns/tests/testdata/Makefile.am b/src/lib/dns/tests/testdata/Makefile.am
index d312e07955..9494117f8d 100644
--- a/src/lib/dns/tests/testdata/Makefile.am
+++ b/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 $@ $<
diff --git a/src/lib/dns/tests/testdata/broken.zone b/src/lib/dns/tests/testdata/broken.zone
new file mode 100644
index 0000000000..70f4540c1a
--- /dev/null
+++ b/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
\ No newline at end of file

From 45e97b85271f437e9f3897fc78e82cbac8f2368a Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Tue, 11 Dec 2012 15:20:11 +0100
Subject: [PATCH 08/17] [2428] Test and fix error handling in $INCLUDE

---
 src/lib/dns/master_loader.cc                | 6 ++----
 src/lib/dns/tests/master_loader_unittest.cc | 4 ++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index bf28ea29f5..40710c29a7 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -81,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);
@@ -116,7 +114,7 @@ public:
     void doInclude() {
         // First, get the filename to include
         const MasterToken::StringRegion
-            filename(lexer_.getNextToken(MasterLexer::QSTRING).
+            filename(lexer_.getNextToken(MasterToken::QSTRING).
                      getStringRegion());
 
         // TODO: Handle the case where there's Name after the
diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 4860badee4..e79e2f8ca7 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -282,6 +282,10 @@ 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", "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 }
 };
 

From aae73017c63bcb2527bf36a3abb2c436072e3a98 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Tue, 11 Dec 2012 19:49:24 +0100
Subject: [PATCH 09/17] [2428] Handle garbage after the include

---
 src/lib/dns/master_loader.cc                | 69 ++++++++++++---------
 src/lib/dns/tests/master_loader_unittest.cc | 26 +++++++-
 2 files changed, 62 insertions(+), 33 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index 40710c29a7..e73ccee615 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -93,8 +93,11 @@ public:
     }
 
     bool popSource() {
+        if (--source_count_ == 0) {
+            return (false);
+        }
         lexer_.popSource();
-        return (--source_count_ != 0);
+        return (true);
     }
 
     void pushStreamSource(std::istream& stream) {
@@ -117,17 +120,12 @@ public:
             filename(lexer_.getNextToken(MasterToken::QSTRING).
                      getStringRegion());
 
-        // TODO: Handle the case where there's Name after the
-        // filename, meaning origin. Once $ORIGIN handling is
-        // done, it should be interconnected somehow.
+        // TODO: Handle Origin
 
         // 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.
         pushSource(filename.beg);
-
-        // TODO: Eat any extra tokens at the end of line (they
-        // should not be here, of course).
     }
 
     void handleDirective(const char* directive, size_t length) {
@@ -152,6 +150,35 @@ public:
         }
     }
 
+    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_;
@@ -196,7 +223,9 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) {
                         return (true);
                     } else {
                         // We try to read a token from the popped source
-                        // So retry the loop
+                        // So retry the loop, but first, make sure the source
+                        // is at EOL
+                        eatUntilEOL(true);
                         continue;
                     }
                 }
@@ -274,29 +303,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(),
-                                           "Unexpected end ond of file");
-                        if (!popSource()) {
-                            return (true);
-                        }
-                        // Else: fall through, the popped source is
-                        // at the end of line currently
-                    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.
diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index e79e2f8ca7..73188d9cf8 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -336,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. "
@@ -354,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",

From 320f960c727b5785a1869fc2e9898fea6b9e14fd Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Tue, 11 Dec 2012 19:54:12 +0100
Subject: [PATCH 10/17] [2428] Placeholder for origin handling

The $INCLUDE can contain a name meaning origin. We don't handle origin
yet, that's #2427, but we at least mark the place where it should be
linked.
---
 src/lib/dns/master_loader.cc | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index e73ccee615..1f7b7d67d1 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -117,15 +117,29 @@ public:
     void doInclude() {
         // First, get the filename to include
         const MasterToken::StringRegion
-            filename(lexer_.getNextToken(MasterToken::QSTRING).
-                     getStringRegion());
-
-        // TODO: Handle Origin
+            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.
-        pushSource(filename.beg);
+        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) {

From 041637fac582e9b2fd003b557e1d616ecbb0db9b Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Wed, 12 Dec 2012 10:33:33 +0100
Subject: [PATCH 11/17] [2428] Check quoted "$INCLUDE" is accepted too

---
 src/lib/dns/tests/master_loader_unittest.cc | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 73188d9cf8..70375573ef 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -150,10 +150,11 @@ TEST_F(MasterLoaderTest, basicLoad) {
 TEST_F(MasterLoaderTest, include) {
     // Test various cases of include
     const char* includes[] = {
-        "include",
-        "INCLUDE",
-        "Include",
-        "InCluDe",
+        "$include",
+        "$INCLUDE",
+        "$Include",
+        "$InCluDe",
+        "\"$INCLUDE\"",
         NULL
     };
     for (const char** include = includes; *include != NULL; ++include) {
@@ -162,7 +163,7 @@ TEST_F(MasterLoaderTest, 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) + " " +
+        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(),

From 3fda721ea7863c9e61728c6e2b7f499705ca9abf Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Wed, 12 Dec 2012 10:35:43 +0100
Subject: [PATCH 12/17] [2428] Code simplification

We don't need that intermediate variable
---
 src/lib/dns/master_loader.cc | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index 1f7b7d67d1..da12013b54 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -116,14 +116,8 @@ public:
 
     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);
+        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,

From 443cabc1e4a305f19b39bcbdb66c0bde010c93ae Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Wed, 12 Dec 2012 10:43:57 +0100
Subject: [PATCH 13/17] [2428] Use iequals instead of strncasecmp

It has nicer interface and should be slightly less peculiar.
---
 src/lib/dns/master_loader.cc | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index da12013b54..f31d9bfc09 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -22,10 +22,11 @@
 
 #include <string>
 #include <memory>
-#include <strings.h>
+#include <boost/algorithm/string/predicate.hpp> // for iequals
 
 using std::string;
 using std::auto_ptr;
+using boost::algorithm::iequals;
 
 namespace isc {
 namespace dns {
@@ -137,18 +138,13 @@ public:
     }
 
     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) {
+        if (iequals(directive, "INCLUDE")) {
             doInclude();
-        } else if (strncasecmp(directive, "ORIGIN", length) == 0) {
+        } else if (iequals(directive, "ORIGIN")) {
             // TODO: Implement
             isc_throw(isc::NotImplemented,
                       "Origin directive not implemented yet");
-        } else if (strncasecmp(directive, "TTL", length) == 0) {
+        } else if (iequals(directive, "TTL")) {
             // TODO: Implement
             isc_throw(isc::NotImplemented,
                       "TTL directive not implemented yet");

From 58664e2ecc10b71b6ce0126ccf8c5f336f7cd380 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Thu, 13 Dec 2012 12:09:17 +0100
Subject: [PATCH 14/17] [2428] Put internal exception to anonymous namespace

---
 src/lib/dns/master_loader.cc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index f31d9bfc09..9a7f1ee3d9 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -31,6 +31,8 @@ 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.
@@ -41,6 +43,8 @@ public:
     {}
 };
 
+}
+
 class MasterLoader::MasterLoaderImpl {
 public:
     MasterLoaderImpl(const char* master_file,

From bb1f38e03fb01fb164a58d91bd96bc9fcf4c07df Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Thu, 13 Dec 2012 12:12:37 +0100
Subject: [PATCH 15/17] [2428] Test two more error cases

Shorter and longer version of INCLUDE.
---
 src/lib/dns/tests/master_loader_unittest.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 70375573ef..e690943766 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -283,6 +283,8 @@ 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", "Unknown $ directive" },
+    { "$INCLUD " TEST_DATA_SRCDIR "/example.org", "Include too short" },
+    { "$INCLUDES " TEST_DATA_SRCDIR "/example.org", "Include too short" },
     { "$INCLUDE", "Missing include path" },
     { "$INCLUDE /file/not/found", "Include file not found" },
     { "$INCLUDE /file/not/found and here goes bunch of garbage",

From 588ff1eef555b3362954296388e93cf8729f267b Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Thu, 13 Dec 2012 12:32:38 +0100
Subject: [PATCH 16/17] [2428] Interface to get count of sources in lexer

And use it in the master loader, instead of keeping it separately there.
---
 src/lib/dns/master_lexer.cc                | 5 +++++
 src/lib/dns/master_lexer.h                 | 5 +++++
 src/lib/dns/master_loader.cc               | 6 +-----
 src/lib/dns/tests/master_lexer_unittest.cc | 6 ++++++
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/lib/dns/master_lexer.cc b/src/lib/dns/master_lexer.cc
index 4593b48b5f..e192c72298 100644
--- a/src/lib/dns/master_lexer.cc
+++ b/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()) {
diff --git a/src/lib/dns/master_lexer.h b/src/lib/dns/master_lexer.h
index 35586feb0f..76147c8029 100644
--- a/src/lib/dns/master_lexer.h
+++ b/src/lib/dns/master_lexer.h
@@ -405,6 +405,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
diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index 9a7f1ee3d9..b3474648fd 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -63,7 +63,6 @@ public:
         initialized_(false),
         ok_(true),
         many_errors_((options & MANY_ERRORS) != 0),
-        source_count_(0),
         complete_(false),
         seen_error_(false)
     {}
@@ -94,11 +93,10 @@ public:
             }
         }
         initialized_ = true;
-        ++source_count_;
     }
 
     bool popSource() {
-        if (--source_count_ == 0) {
+        if (lexer_.getSourceCount() == 1) {
             return (false);
         }
         lexer_.popSource();
@@ -108,7 +106,6 @@ public:
     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.
@@ -200,7 +197,6 @@ 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
diff --git a/src/lib/dns/tests/master_lexer_unittest.cc b/src/lib/dns/tests/master_lexer_unittest.cc
index b751da84f3..8c21bcc3fa 100644
--- a/src/lib/dns/tests/master_lexer_unittest.cc
+++ b/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.

From ea9d025cbcd9a318a2946c1a7f00283885ff381f Mon Sep 17 00:00:00 2001
From: Jelte Jansen <jelte@isc.org>
Date: Thu, 13 Dec 2012 21:20:57 +0100
Subject: [PATCH 17/17] [2428] Some minor cleanups

---
 src/lib/dns/master_loader.cc                | 2 +-
 src/lib/dns/tests/master_loader_unittest.cc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index b3474648fd..e4b662c1e0 100644
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -43,7 +43,7 @@ public:
     {}
 };
 
-}
+} // end unnamed namespace
 
 class MasterLoader::MasterLoaderImpl {
 public:
diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index e690943766..4f40a98932 100644
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -284,7 +284,7 @@ struct ErrorCase {
     // 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 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",