From: Michal 'vorner' Vaner Date: Fri, 7 Dec 2012 16:10:27 +0000 (+0100) Subject: [2377] Report error on !MANY_ERRORS as exception X-Git-Tag: bind10-1.0.0-beta-release~24^2~38 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3a2ccbf78e869817a411d735e28425713c220ffa;p=thirdparty%2Fkea.git [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 c4c5bc9964..28e7065c5f 100644 --- 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 574ad69d1e..281a7fdca8 100644 --- 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 2156acdec6..1fe9a02e58 100644 --- 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. "