From: Jelte Jansen Date: Fri, 30 Mar 2012 14:54:01 +0000 (+0200) Subject: [1535] address review comments X-Git-Tag: trac2351_base~226^2~116^2~68^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=532aa507e57aaa8c406cd4cc537058de6fcb5a88;p=thirdparty%2Fkea.git [1535] address review comments - removed superfluous toText() calls in out-of-zone exception creation - moved in-memory out-of-zone check to be part of findNode instead of separate check in find() - added out-of-zone check to addAdditional lookup (and don't lookup unless in zone) - removed FIND_DEFAULT parameters in unit tests (they are default values) - change notify_out sqlite test data to have out-of-zone-but-known NS target --- diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc index 85958f3830..c1f8e167e7 100644 --- a/src/lib/datasrc/database.cc +++ b/src/lib/datasrc/database.cc @@ -865,8 +865,7 @@ DatabaseClient::Finder::findInternal(const Name& name, const RRType& type, name.compare(getOrigin()).getRelation(); if (reln != NameComparisonResult::SUBDOMAIN && reln != NameComparisonResult::EQUAL) { - isc_throw(OutOfZoneFind, name.toText() << " not in " << - getOrigin().toText()); + isc_throw(OutOfZoneFind, name.toText() << " not in " << getOrigin()); } // First, go through all superdomains from the origin down, searching for diff --git a/src/lib/datasrc/memory_datasrc.cc b/src/lib/datasrc/memory_datasrc.cc index 17c22df6cf..eb635ef6d4 100644 --- a/src/lib/datasrc/memory_datasrc.cc +++ b/src/lib/datasrc/memory_datasrc.cc @@ -362,8 +362,7 @@ ZoneData::findNode(const Name& name, ZoneFinder::FindOptions options) const { if (result == DomainTree::EXACTMATCH) { return (ResultType(ZoneFinder::SUCCESS, node, state.rrset_, zonecut_flag)); - } - if (result == DomainTree::PARTIALMATCH) { + } else if (result == DomainTree::PARTIALMATCH) { assert(node != NULL); if (state.dname_node_ != NULL) { // DNAME LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_DNAME_FOUND). @@ -408,10 +407,15 @@ ZoneData::findNode(const Name& name, ZoneFinder::FindOptions options) const { FindNodeResult::FIND_WILDCARD | zonecut_flag)); } + // Nothing really matched. + LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_NOT_FOUND).arg(name); + return (ResultType(ZoneFinder::NXDOMAIN, node, state.rrset_)); + } else { + // If the name is neither an exact or partial match, it is + // out of bailiwick, which is considered an error. + isc_throw(OutOfZoneFind, name.toText() << " not in " << + origin_data_->getName()); } - // Nothing really matched. The name may even be out-of-bailiwick. - LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_NOT_FOUND).arg(name); - return (ResultType(ZoneFinder::NXDOMAIN, node, state.rrset_)); } } // unnamed namespace @@ -1202,14 +1206,6 @@ struct InMemoryZoneFinder::InMemoryZoneFinderImpl { LOG_DEBUG(logger, DBG_TRACE_BASIC, DATASRC_MEM_FIND).arg(name). arg(type); - const NameComparisonResult::NameRelation reln = - name.compare(origin_).getRelation(); - if (reln != NameComparisonResult::SUBDOMAIN && - reln != NameComparisonResult::EQUAL) { - isc_throw(OutOfZoneFind, name.toText() << - " not in " << origin_.toText()); - } - // Get the node. All other cases than an exact match are handled // in findNode(). We simply construct a result structure and return. const ZoneData::FindNodeResult node_result = @@ -1459,6 +1455,13 @@ addAdditional(RBNodeRRset* rrset, ZoneData* zone_data, const Name& name = getAdditionalName(rrset->getType(), rdata_iterator->getCurrent()); + // if the name is not in or below this zone, skip it + const NameComparisonResult::NameRelation reln = + name.compare(zone_data->origin_data_->getName()).getRelation(); + if (reln != NameComparisonResult::SUBDOMAIN && + reln != NameComparisonResult::EQUAL) { + continue; + } const ZoneData::FindMutableNodeResult result = zone_data->findNode( name, ZoneFinder::FIND_GLUE_OK); diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc index c30aa5cf37..bf8d2847cd 100644 --- a/src/lib/datasrc/tests/database_unittest.cc +++ b/src/lib/datasrc/tests/database_unittest.cc @@ -1872,34 +1872,32 @@ TYPED_TEST(DatabaseClientTest, find) { } TYPED_TEST(DatabaseClientTest, findOutOfZone) { - // If the query name is out-of-zone it should result in NXDOMAIN + // If the query name is out-of-zone it should result in an exception boost::shared_ptr finder(this->getFinder()); vector target; // Superdomain - EXPECT_THROW(finder->find(Name("org"), this->qtype_, - ZoneFinder::FIND_DEFAULT), OutOfZoneFind); + EXPECT_THROW(finder->find(Name("org"), this->qtype_), OutOfZoneFind); EXPECT_THROW(finder->findAll(Name("org"), target), OutOfZoneFind); // sharing a common ancestor - EXPECT_THROW(finder->find(Name("noexample.org"), this->qtype_, - ZoneFinder::FIND_DEFAULT), OutOfZoneFind); + EXPECT_THROW(finder->find(Name("noexample.org"), this->qtype_), + OutOfZoneFind); EXPECT_THROW(finder->findAll(Name("noexample.org"), target), OutOfZoneFind); // totally unrelated domain, smaller number of labels - EXPECT_THROW(finder->find(Name("com"), this->qtype_, - ZoneFinder::FIND_DEFAULT), OutOfZoneFind); + EXPECT_THROW(finder->find(Name("com"), this->qtype_), OutOfZoneFind); EXPECT_THROW(finder->findAll(Name("com"), target), OutOfZoneFind); // totally unrelated domain, same number of labels - EXPECT_THROW(finder->find(Name("example.com"), this->qtype_, - ZoneFinder::FIND_DEFAULT), OutOfZoneFind); + EXPECT_THROW(finder->find(Name("example.com"), this->qtype_), + OutOfZoneFind); EXPECT_THROW(finder->findAll(Name("example.com"), target), OutOfZoneFind); // totally unrelated domain, larger number of labels - EXPECT_THROW(finder->find(Name("more.example.com"), this->qtype_, - ZoneFinder::FIND_DEFAULT), OutOfZoneFind); + EXPECT_THROW(finder->find(Name("more.example.com"), this->qtype_), + OutOfZoneFind); EXPECT_THROW(finder->findAll(Name("more.example.com"), target), OutOfZoneFind); } @@ -2829,13 +2827,12 @@ TYPED_TEST(DatabaseClientTest, addDeviantRR) { this->expected_rdatas_.clear(); this->expected_rdatas_.push_back("192.0.2.100"); { - // Note: find() rejects out-of-zone query name with NXDOMAIN + // Note: find() rejects out-of-zone query name with an exception // regardless of whether adding the RR succeeded, so this check // actually doesn't confirm it. SCOPED_TRACE("add out-of-zone RR"); EXPECT_THROW(this->updater_->getFinder().find(Name("example.com"), - this->qtype_, - ZoneFinder::FIND_DEFAULT), + this->qtype_), OutOfZoneFind); } } diff --git a/src/lib/datasrc/tests/memory_datasrc_unittest.cc b/src/lib/datasrc/tests/memory_datasrc_unittest.cc index dc1f099ca3..2990dc71b3 100644 --- a/src/lib/datasrc/tests/memory_datasrc_unittest.cc +++ b/src/lib/datasrc/tests/memory_datasrc_unittest.cc @@ -998,8 +998,8 @@ InMemoryZoneFinderTest::findCheck(ZoneFinder::FindResultFlags expected_flags) { // These domains don't exist (and one is out of the zone) findTest(Name("nothere.example.org"), RRType::A(), ZoneFinder::NXDOMAIN, true, ConstRRsetPtr(), expected_flags); - EXPECT_THROW(zone_finder_.find(Name("example.net"), RRType::A(), - ZoneFinder::FIND_DEFAULT), OutOfZoneFind); + EXPECT_THROW(zone_finder_.find(Name("example.net"), RRType::A()), + OutOfZoneFind); } TEST_F(InMemoryZoneFinderTest, find) { @@ -1054,8 +1054,7 @@ InMemoryZoneFinderTest::emptyNodeCheck( // Note: basically we don't expect such a query to be performed (the common // operation is to identify the best matching zone first then perform // search it), but we shouldn't be confused even in the unexpected case. - EXPECT_THROW(zone_finder_.find(Name("org"), RRType::A(), - ZoneFinder::FIND_DEFAULT), + EXPECT_THROW(zone_finder_.find(Name("org"), RRType::A()), OutOfZoneFind); } @@ -1514,16 +1513,12 @@ TEST_F(InMemoryZoneFinderTest, swap) { EXPECT_EQ(RRClass::CH(), finder1.getClass()); EXPECT_EQ(RRClass::IN(), finder2.getClass()); // make sure the zone data is swapped, too - EXPECT_THROW(finder1.find(origin_, RRType::NS(), - ZoneFinder::FIND_DEFAULT), - OutOfZoneFind); + EXPECT_THROW(finder1.find(origin_, RRType::NS()), OutOfZoneFind); findTest(other_origin, RRType::TXT(), ZoneFinder::SUCCESS, false, ConstRRsetPtr(), ZoneFinder::RESULT_DEFAULT, &finder1); findTest(origin_, RRType::NS(), ZoneFinder::SUCCESS, false, ConstRRsetPtr(), ZoneFinder::RESULT_DEFAULT, &finder2); - EXPECT_THROW(finder2.find(other_origin, RRType::TXT(), - ZoneFinder::FIND_DEFAULT), - OutOfZoneFind); + EXPECT_THROW(finder2.find(other_origin, RRType::TXT()), OutOfZoneFind); } TEST_F(InMemoryZoneFinderTest, getFileName) { diff --git a/src/lib/python/isc/datasrc/tests/datasrc_test.py b/src/lib/python/isc/datasrc/tests/datasrc_test.py index f12339b72d..f4ff45585a 100644 --- a/src/lib/python/isc/datasrc/tests/datasrc_test.py +++ b/src/lib/python/isc/datasrc/tests/datasrc_test.py @@ -382,7 +382,7 @@ class DataSrcClient(unittest.TestCase): self.assertRaises(isc.datasrc.OutOfZoneFind, finder.find, isc.dns.Name("www.some.other.domain"), - isc.dns.RRType.A(), finder.FIND_DEFAULT) + isc.dns.RRType.A()) result, rrset, _ = finder.find(isc.dns.Name("www.example.com"), isc.dns.RRType.TXT(), diff --git a/src/lib/python/isc/notify/tests/testdata/test.sqlite3 b/src/lib/python/isc/notify/tests/testdata/test.sqlite3 index 366514b5cb..16060640ce 100644 Binary files a/src/lib/python/isc/notify/tests/testdata/test.sqlite3 and b/src/lib/python/isc/notify/tests/testdata/test.sqlite3 differ