[2377] Report error on !MANY_ERRORS as exception

In this case, the user should be aware of the error as much as possible.
In case of MANY_ERRORS, we can't do this, exception is able to handle
just one error.

diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc
index c4c5bc996415704bfa17ad4936c17f79bc186fd4..28e7065c5fe3b76168a7fd17cb5f4dfe3726ba93 100644 (file)
--- a/src/lib/dns/master_loader.cc
+++ b/src/lib/dns/master_loader.cc
@@ -45,11 +45,24 @@ public:
         complete_(false)
     {}
 
+    void reportError(const std::string& filename, size_t line,
+                     const std::string& reason)
+    {
+        callbacks_.error(filename, line, reason);
+        if (!(options_ & MANY_ERRORS)) {
+            // In case we don't have the lenient mode, every error is fatal
+            // and we throw
+            ok_ = false;
+            complete_ = true;
+            isc_throw(MasterLoaderError, reason.c_str());
+        }
+    }
+
     void pushSource(const std::string& filename) {
         std::string error;
         if (!lexer_.pushSource(filename.c_str(), &error)) {
+            reportError("", 0, error);
             ok_ = false;
-            callbacks_.error("", 0, error);
         }
         initialized_ = true;
     }
@@ -125,36 +138,41 @@ public:
                     // Good, we loaded another one
                     ++count;
                 } else if ((options_ & MANY_ERRORS) == 0) {
-                    return (true);
+                    ok_ = false;
+                    complete_ = true;
+                    // We don't have the exact error here, but it was reported
+                    // by the error callback.
+                    isc_throw(MasterLoaderError, "Invalid RR data");
                 }
+            } catch (const MasterLoaderError&) {
+                // This is a hack. We exclude the MasterLoaderError from the
+                // below case. Once we restrict the below to some smaller
+                // exception, we should remove this.
+                throw;
             } catch (const isc::Exception& e) {
-                // TODO: Do we want to list expected exceptions here instead?
-                callbacks_.error(lexer_.getSourceName(),
-                                 lexer_.getSourceLine(),
-                                 e.what());
-                if ((options_ & MANY_ERRORS) != 0) {
-                    // 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:
-                                // 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);
-                } else {
-                    // We abort on first error. We are therefore done.
-                    return (true);
-                }
+                // TODO: Once we do #2518, catch only the DNSTextError here,
+                // not isc::Exception. The rest should be just simply
+                // 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:
+                            // 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.

diff --git a/src/lib/dns/master_loader.h b/src/lib/dns/master_loader.h
index 574ad69d1e79c1e221651c84257fcdb3daf00517..281a7fdca8857241db6452dd7a32d56938c965ad 100644 (file)
--- a/src/lib/dns/master_loader.h
+++ b/src/lib/dns/master_loader.h
@@ -25,6 +25,15 @@ namespace dns {
 class Name;
 class RRClass;
 
+/// \brief Error while loading by MasterLoader without specifying the
+///     MANY_ERRORS option.
+class MasterLoaderError : public isc::Exception {
+public:
+    MasterLoaderError(const char* file, size_t line, const char* what) :
+        Exception(file, line, what)
+    {}
+};
+
 /// \brief A class able to load DNS master files
 ///
 /// This class is able to produce a stream of RRs from a master file.
@@ -100,6 +109,9 @@ public:
     ///     It returns true if the loading is done.
     /// \throw isc::InvalidOperation when called after loading was done
     ///     already.
+    /// \throw MasterLoaderError when there's an error in the input master
+    ///     file and the MANY_ERRORS is not specified. It never throws this
+    ///     in case MANY_ERRORS is specified.
     bool loadIncremental(size_t count_limit);
 
     /// \brief Load everything
@@ -107,6 +119,9 @@ public:
     /// This simply calls loadIncremental until the loading is done.
     /// \throw isc::InvalidOperation when called after loading was done
     ///     already.
+    /// \throw MasterLoaderError when there's an error in the input master
+    ///     file and the MANY_ERRORS is not specified. It never throws this
+    ///     in case MANY_ERRORS is specified.
     void load() {
         while (!loadIncremental(1000)) { // 1000 = arbitrary largish number
             // Body intentionally left blank

diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc
index 2156acdec61bf4efc6e1dee2c378c03bd95a13e5..1fe9a02e58e2be9c862d8a66d4eb1beb04dc5f53 100644 (file)
--- a/src/lib/dns/tests/master_loader_unittest.cc
+++ b/src/lib/dns/tests/master_loader_unittest.cc
@@ -225,8 +225,8 @@ TEST_F(MasterLoaderTest, brokenZone) {
             stringstream zone_stream(zone);
             setLoader(zone_stream, Name("example.org."), RRClass::IN(),
                       MasterLoader::DEFAULT);
-            loader_->load();
-            EXPECT_EQ(1, errors_.size());
+            EXPECT_THROW(loader_->load(), MasterLoaderError);
+            EXPECT_EQ(1, errors_.size()) << errors_[0];
             EXPECT_TRUE(warnings_.empty());
 
             checkRR("example.org", RRType::SOA(), "ns1.example.org. "
@@ -242,7 +242,7 @@ TEST_F(MasterLoaderTest, brokenZone) {
             stringstream zone_stream(zone);
             setLoader(zone_stream, Name("example.org."), RRClass::IN(),
                       MasterLoader::MANY_ERRORS);
-            loader_->load();
+            EXPECT_NO_THROW(loader_->load());
             EXPECT_EQ(1, errors_.size());
             EXPECT_TRUE(warnings_.empty());
             checkRR("example.org", RRType::SOA(), "ns1.example.org. "