Merge branch 'trac2441'

src/lib/datasrc/memory/rdataset.h

@@ -121,12 +121,15 @@ public:
     /// returns a pointer to it.
     ///
     /// If the optional \c old_rdataset parameter is set to non NULL,
-    /// The given \c RdataSet, RRset, RRSIG will be merged in the new
-    /// \c Rdataset object: the new object contain the union set of all
+    /// the given \c RdataSet, RRset, RRSIG will be merged in the new
+    /// \c Rdataset object: the new object will contain the union set of all
     /// RDATA and RRSIGs contained in these.  Obviously \c old_rdataset
-    /// was previously generated for the same RRClass and RRType as those
+    /// must be previously generated for the same RRClass and RRType as those
     /// for the given RRsets; it's the caller's responsibility to ensure
     /// this condition.  If it's not met the result will be undefined.
+    /// No reference to \c old_rdataset is maintained in the newly
+    /// returned \c RdataSet, so if the caller does not need \c
+    /// old_rdataset anymore, it may be freed by the caller.
     ///
     /// In both cases, this method ensures the stored RDATA and RRSIG are
     /// unique.  Any duplicate data (in the sense of the comparison in the

src/lib/datasrc/memory/zone_data_loader.cc

@@ -50,14 +50,15 @@ typedef boost::function<void(isc::dns::ConstRRsetPtr)> LoadCallback;
 // A helper internal class for \c loadZoneData().  make it non-copyable
 // to avoid accidental copy.
 //
-// The current internal implementation expects that both a normal
-// (non RRSIG) RRset and (when signed) its RRSIG are added at once.
-// Also in the current implementation, the input sequence of RRsets
-// are grouped with their owner name (so once a new owner name is encountered,
-// no subsequent RRset has the previous owner name), but the ordering
-// in the same group is not fixed.  So we hold all RRsets of the same
-// owner name in node_rrsets_ and node_rrsigsets_, and add the matching
-// pairs of RRsets to the zone when we see a new owner name.
+// The current internal implementation no longer expects that both a
+// normal (non RRSIG) RRset and (when signed) its RRSIG are added at
+// once, but we do that here anyway to avoid merging RdataSets every
+// single time which can be inefficient.
+//
+// We hold all RRsets of the same owner name in node_rrsets_ and
+// node_rrsigsets_, and add the matching pairs of RRsets to the zone
+// when we see a new owner name. We do this to limit the size of
+// NodeRRsets below. However, RRsets can occur in any order.
 //
 // The caller is responsible for adding the RRsets of the last group
 // in the input sequence by explicitly calling flushNodeRRsets() at the
@@ -86,6 +87,7 @@ private:
 private:
     NodeRRsets node_rrsets_;
     NodeRRsets node_rrsigsets_;
+    std::vector<isc::dns::ConstRRsetPtr> non_consecutive_rrsets_;
     ZoneDataUpdater updater_;
 };
 
@@ -93,22 +95,20 @@ void
 ZoneDataLoader::addFromLoad(const ConstRRsetPtr& rrset) {
     // If we see a new name, flush the temporary holders, adding the
     // pairs of RRsets and RRSIGs of the previous name to the zone.
-    if ((!node_rrsets_.empty() || !node_rrsigsets_.empty()) &&
+    if ((!node_rrsets_.empty() || !node_rrsigsets_.empty() ||
+         !non_consecutive_rrsets_.empty()) &&
         (getCurrentName() != rrset->getName())) {
         flushNodeRRsets();
     }
 
-    // Store this RRset until it can be added to the zone.  The current
-    // implementation requires RRs of the same RRset should be added at
-    // once, so we check the "duplicate" here.
+    // Store this RRset until it can be added to the zone. If an rrtype
+    // that's already been seen is found, queue it in a different vector
+    // to be merged later.
     const bool is_rrsig = rrset->getType() == RRType::RRSIG();
     NodeRRsets& node_rrsets = is_rrsig ? node_rrsigsets_ : node_rrsets_;
     const RRType& rrtype = is_rrsig ? getCoveredType(rrset) : rrset->getType();
     if (!node_rrsets.insert(NodeRRsetsVal(rrtype, rrset)).second) {
-        isc_throw(ZoneDataUpdater::AddError,
-                  "Duplicate add of the same type of"
-                  << (is_rrsig ? " RRSIG" : "") << " RRset: "
-                  << rrset->getName() << "/" << rrtype);
+        non_consecutive_rrsets_.insert(non_consecutive_rrsets_.begin(), rrset);
     }
 
     if (rrset->getRRsig()) {
@@ -137,8 +137,18 @@ ZoneDataLoader::flushNodeRRsets() {
         updater_.add(ConstRRsetPtr(), val.second);
     }
 
+    // Add any non-consecutive rrsets too.
+    BOOST_FOREACH(ConstRRsetPtr rrset, non_consecutive_rrsets_) {
+        if (rrset->getType() == RRType::RRSIG()) {
+            updater_.add(ConstRRsetPtr(), rrset);
+        } else {
+            updater_.add(rrset, ConstRRsetPtr());
+        }
+    }
+
     node_rrsets_.clear();
     node_rrsigsets_.clear();
+    non_consecutive_rrsets_.clear();
 }
 
 const Name&

src/lib/datasrc/memory/zone_data_updater.cc

@@ -270,8 +270,12 @@ ZoneDataUpdater::addNSEC3(const Name& name, const ConstRRsetPtr rrset,
     ZoneNode* node;
     nsec3_data->insertName(mem_sgmt_, name, &node);
 
-    RdataSet* rdataset = RdataSet::create(mem_sgmt_, encoder_, rrset, rrsig);
-    RdataSet* old_rdataset = node->setData(rdataset);
+    // Create a new RdataSet, merging any existing NSEC3 data for this
+    // name.
+    RdataSet* old_rdataset = node->getData();
+    RdataSet* rdataset = RdataSet::create(mem_sgmt_, encoder_, rrset, rrsig,
+                                          old_rdataset);
+    old_rdataset = node->setData(rdataset);
     if (old_rdataset != NULL) {
         RdataSet::destroy(mem_sgmt_, old_rdataset, rrclass_);
     }
@@ -298,16 +302,34 @@ ZoneDataUpdater::addRdataSet(const Name& name, const RRType& rrtype,
             contextCheck(*rrset, rdataset_head);
         }
 
-        if (RdataSet::find(rdataset_head, rrtype, true) != NULL) {
-            isc_throw(AddError,
-                      "RRset of the type already exists: "
-                      << name << " (type: " << rrtype << ")");
-        }
-
+        // Create a new RdataSet, merging any existing data for this
+        // type.
+        RdataSet* old_rdataset = RdataSet::find(rdataset_head, rrtype, true);
         RdataSet* rdataset_new = RdataSet::create(mem_sgmt_, encoder_,
-                                                  rrset, rrsig);
-        rdataset_new->next = rdataset_head;
-        node->setData(rdataset_new);
+                                                  rrset, rrsig, old_rdataset);
+        if (old_rdataset == NULL) {
+            // There is no existing RdataSet. Prepend the new RdataSet
+            // to the list.
+            rdataset_new->next = rdataset_head;
+            node->setData(rdataset_new);
+        } else {
+            // Replace the old RdataSet in the list with the newly
+            // created one, and destroy the old one.
+            for (RdataSet* cur = rdataset_head, *prev = NULL;
+                 cur != NULL;
+                 prev = cur, cur = cur->getNext()) {
+                if (cur == old_rdataset) {
+                    rdataset_new->next = cur->getNext();
+                    if (prev == NULL) {
+                        node->setData(rdataset_new);
+                    } else {
+                        prev->next = rdataset_new;
+                    }
+                    break;
+                }
+            }
+            RdataSet::destroy(mem_sgmt_, old_rdataset, rrclass_);
+        }
 
         // Ok, we just put it in.
 

src/lib/datasrc/tests/memory/memory_client_unittest.cc

@@ -74,6 +74,7 @@ const char* rrset_data[] = {
 const char* rrset_data_separated[] = {
     "example.org. 3600 IN SOA ns1.example.org. bugs.x.w.example.org. "
     "68 3600 300 3600000 3600",
+    "example.org. 3600 IN NS ns1.example.org.",
     "a.example.org. 3600 IN A 192.168.0.1", // these two belong to the same
     "a.example.org. 3600 IN A 192.168.0.2", // RRset, but are separated.
     NULL
@@ -83,6 +84,7 @@ const char* rrset_data_separated[] = {
 const char* rrset_data_sigseparated[] = {
     "example.org. 3600 IN SOA ns1.example.org. bugs.x.w.example.org. "
     "68 3600 300 3600000 3600",
+    "example.org. 3600 IN NS ns1.example.org.",
     "a.example.org. 3600 IN A 192.168.0.1",
     "a.example.org. 3600 IN RRSIG A 5 3 3600 20150420235959 20051021000000 "
     "40430 example.org. FAKEFAKE",
@@ -261,13 +263,13 @@ TEST_F(MemoryClientTest, loadFromIterator) {
     EXPECT_TRUE(rrset);
     EXPECT_EQ(RRType::NS(), rrset->getType());
 
-    // RRType::MX() RRset
+    // RRType::A() RRset
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
     EXPECT_EQ(RRType::MX(), rrset->getType());
     EXPECT_EQ(1, rrset->getRRsigDataCount()); // this RRset is signed
 
-    // RRType::A() RRset
+    // RRType::MX() RRset
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
     EXPECT_EQ(RRType::A(), rrset->getType());
@@ -279,19 +281,17 @@ TEST_F(MemoryClientTest, loadFromIterator) {
     // Iterating past the end should result in an exception
     EXPECT_THROW(iterator->getNextRRset(), isc::Unexpected);
 
-    // Loading the zone with an iterator separating RRs of the same RRset
-    // will fail because the resulting sequence doesn't meet assumptions of
-    // the (current) in-memory implementation.
-    EXPECT_THROW(client_->load(Name("example.org"),
-                               *MockIterator::makeIterator(
-                                   rrset_data_separated)),
-                 ZoneDataUpdater::AddError);
+    // Loading the zone with an iterator separating RRs of the same
+    // RRset should not fail. It is acceptable to load RRs of the same
+    // type again.
+    client_->load(Name("example.org"),
+                  *MockIterator::makeIterator(
+                      rrset_data_separated));
 
     // Similar to the previous case, but with separated RRSIGs.
-    EXPECT_THROW(client_->load(Name("example.org"),
-                               *MockIterator::makeIterator(
-                                   rrset_data_sigseparated)),
-                 ZoneDataUpdater::AddError);
+    client_->load(Name("example.org"),
+                  *MockIterator::makeIterator(
+                      rrset_data_sigseparated));
 
     // Emulating bogus iterator implementation that passes empty RRSIGs.
     EXPECT_THROW(client_->load(Name("example.org"),
@@ -439,15 +439,41 @@ TEST_F(MemoryClientTest, loadReloadZone) {
 }
 
 TEST_F(MemoryClientTest, loadDuplicateType) {
-    // This should not result in any exceptions:
+    // This should not result in any exceptions (multiple records of the
+    // same name, type are present, one after another in sequence).
     client_->load(Name("example.org"),
                   TEST_DATA_DIR "/example.org-duplicate-type.zone");
 
-    // This should throw:
-    EXPECT_THROW(client_->load(Name("example.org"),
-                               TEST_DATA_DIR
-                               "/example.org-duplicate-type-bad.zone"),
-                 ZoneDataUpdater::AddError);
+    // This should not result in any exceptions (multiple records of the
+    // same name, type are present, but not one after another in
+    // sequence).
+    client_->load(Name("example.org"),
+                  TEST_DATA_DIR
+                  "/example.org-duplicate-type-bad.zone");
+
+    const ZoneData* zone_data =
+        client_->findZoneData(Name("example.org"));
+    EXPECT_NE(static_cast<const ZoneData*>(NULL), zone_data);
+
+    /* Check ns1.example.org */
+    const ZoneTree& tree = zone_data->getZoneTree();
+    const ZoneNode* node;
+    ZoneTree::Result zresult(tree.find(Name("ns1.example.org"), &node));
+    EXPECT_EQ(ZoneTree::EXACTMATCH, zresult);
+
+    const RdataSet* set = node->getData();
+    EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
+    EXPECT_EQ(RRType::AAAA(), set->type);
+
+    set = set->getNext();
+    EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
+    EXPECT_EQ(RRType::A(), set->type);
+    // 192.168.0.1 and 192.168.0.2
+    EXPECT_EQ(2, set->getRdataCount());
+
+    set = set->getNext();
+    EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);
+
     // Teardown checks for memory segment leaks
 }
 

src/lib/datasrc/tests/memory/testdata/example.org-duplicate-type-bad.zone

@@ -1,4 +1,7 @@
-example.org. 3600 IN SOA	ns1.example.org. bugs.x.w.example.org. 77 3600 300 3600000 3600
+example.org. 3600 IN SOA	ns1.example.org. bugs.x.w.example.org. 78 3600 300 3600000 3600
+example.org.			      3600 IN NS	ns1.example.org.
+example.org.			      3600 IN NS	ns2.example.org.
 ns1.example.org.		      3600 IN A	 	192.168.0.1
 ns1.example.org.		      3600 IN AAAA 	::1
+someother.example.org.		      3600 IN AAAA 	::1
 ns1.example.org.		      3600 IN A	 	192.168.0.2

src/lib/datasrc/tests/memory/zone_data_updater_unittest.cc

@@ -111,12 +111,10 @@ TEST_F(ZoneDataUpdaterTest, rrsigOnly) {
     EXPECT_EQ(0, rdset->getRdataCount());
     EXPECT_EQ(1, rdset->getSigRdataCount());
 
-    // The RRSIG covering A prohibits an actual A RRset from being added.
-    // This should be loosened in future version, but we check the current
-    // behavior.
-    EXPECT_THROW(updater_->add(
-                     textToRRset("www.example.org. 3600 IN A 192.0.2.1"),
-                     ConstRRsetPtr()), ZoneDataUpdater::AddError);
+    // The RRSIG covering A must not prohibit an actual A RRset from
+    // being added later.
+    updater_->add(textToRRset("www.example.org. 3600 IN A 192.0.2.1"),
+                  ConstRRsetPtr());
 
     // The special "wildcarding" node mark should be added for the RRSIG-only
     // case, too.