[master] Merge branch 'trac2973'

src/lib/datasrc/memory/zone_table.cc

@@ -83,7 +83,7 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, int)
 }
 
 ZoneTable::AddResult
-ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
+ZoneTable::addZone(util::MemorySegment& mem_sgmt,
                    const Name& zone_name, ZoneData* content)
 {
     LOG_DEBUG(logger, DBG_TRACE_BASIC, DATASRC_MEMORY_MEM_ADD_ZONE).
@@ -94,13 +94,8 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
                   (content ? "empty data" : "NULL") <<
                   " is passed to Zone::addZone");
     }
-    SegmentObjectHolder<ZoneData, RRClass> holder(mem_sgmt, zone_class);
-    holder.set(content);
 
-    const AddResult result =
-        addZoneInternal(mem_sgmt, zone_name, holder.get());
-    holder.release();
-    return (result);
+    return (addZoneInternal(mem_sgmt, zone_name, content));
 }
 
 ZoneTable::AddResult

src/lib/datasrc/memory/zone_table.h

@@ -165,12 +165,22 @@ public:
     ///
     /// This method adds a given zone data to the internal table.
     ///
-    /// This method ensures there'll be no memory leak on exception.
-    /// But addresses allocated from \c mem_sgmt could be relocated if
-    /// \c util::MemorySegmentGrown is thrown; the caller or its upper layer
-    /// must be aware of that possibility and update any such addresses
-    /// accordingly.  On successful return, this method ensures there's no
-    /// address relocation.
+    /// On successful completion (i.e., the method returns without an
+    /// exception), the ownership of \c content will be transferred to
+    /// the \c ZoneTable: the caller should not use the \c content hereafter;
+    /// the \c ZoneTable will be responsible to destroy it when the table
+    /// itself is destroyed.
+    ///
+    /// If this method throws, the caller is responsible to take care of
+    /// the passed \c content, whether to destroy it or use for different
+    /// purposes.  Note that addresses allocated from \c mem_sgmt could be
+    /// relocated if \c util::MemorySegmentGrown is thrown; the caller or its
+    /// upper layer must be aware of that possibility and update any such
+    /// addresses accordingly.  This applies to \c content, as it's expected
+    /// to be created using \c mem_sgmt.
+    ///
+    /// On successful return, this method ensures there's no address
+    /// relocation.
     ///
     /// \throw InvalidParameter content is NULL or empty.
     /// \throw util::MemorySegmentGrown The memory segment has grown, possibly
@@ -181,8 +191,6 @@ public:
     ///     created.  It must be the same segment that was used to create
     ///     the zone table at the time of create().
     /// \param zone_name The name of the zone to be added.
-    /// \param zone_class The RR class of the zone.  It must be the RR class
-    ///     that is supposed to be associated to the zone table.
     /// \param content This one should hold the zone content (the ZoneData).
     ///     The ownership is passed onto the zone table. Must not be null or
     ///     empty. Must correspond to the name and class and must be allocated
@@ -194,7 +202,6 @@ public:
     ///     inside the result unless it's empty; if the zone was previously
     ///     added by \c addEmptyZone(), the data returned is NULL.
     AddResult addZone(util::MemorySegment& mem_sgmt,
-                      dns::RRClass zone_class,
                       const dns::Name& zone_name,
                       ZoneData* content);
 

src/lib/datasrc/memory/zone_writer.cc

@@ -134,9 +134,6 @@ ZoneWriter::install() {
     // zone data or we've allowed load error to create an empty zone.
     assert(impl_->data_holder_.get() || impl_->catch_load_error_);
 
-    // FIXME: This retry is currently untested, as there seems to be no
-    // reasonable way to create a zone table segment with non-local memory
-    // segment. Once there is, we should provide the test.
     while (impl_->state_ != Impl::ZW_INSTALLED) {
         try {
             ZoneTableHeader& header = impl_->segment_.getHeader();
@@ -152,8 +149,7 @@ ZoneWriter::install() {
             const ZoneTable::AddResult result(
                 impl_->data_holder_->get() ?
                 table->addZone(impl_->segment_.getMemorySegment(),
-                               impl_->rrclass_, impl_->origin_,
-                               impl_->data_holder_->get()) :
+                               impl_->origin_, impl_->data_holder_->get()) :
                 table->addEmptyZone(impl_->segment_.getMemorySegment(),
                                     impl_->origin_));
             impl_->data_holder_->set(result.zone_data);

src/lib/datasrc/tests/memory/testdata/Makefile.am

@@ -29,6 +29,7 @@ EXTRA_DIST += example.org-rrsigs.zone
 EXTRA_DIST += example.org-wildcard-dname.zone
 EXTRA_DIST += example.org-wildcard-ns.zone
 EXTRA_DIST += example.org-wildcard-nsec3.zone
+EXTRA_DIST += template.zone
 EXTRA_DIST += rrset-collection.zone
 
 EXTRA_DIST += 2503-test.zone

src/lib/datasrc/tests/memory/testdata/template.zone

@@ -0,0 +1,4 @@
+; a zone file that can be used with any origin.
+
+@ 3600 IN SOA . . 1 0 0 0 0
+@ 3600 IN NS ns1

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

@@ -12,6 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include <config.h>
+
 #include <datasrc/memory/zone_data_loader.h>
 #include <datasrc/memory/rdataset.h>
 #include <datasrc/memory/zone_data.h>

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

@@ -74,7 +74,7 @@ TEST_F(ZoneTableTest, addZone) {
     EXPECT_EQ(0, zone_table->getZoneCount());
 
     // It doesn't accept NULL as zone data
-    EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, zname1, NULL),
+    EXPECT_THROW(zone_table->addZone(mem_sgmt_, zname1, NULL),
                  isc::InvalidParameter);
     EXPECT_EQ(0, zone_table->getZoneCount()); // count is still 0
 
@@ -82,8 +82,7 @@ TEST_F(ZoneTableTest, addZone) {
     SegmentObjectHolder<ZoneData, RRClass> holder_empty(
         mem_sgmt_, zclass_);
     holder_empty.set(ZoneData::create(mem_sgmt_));
-    EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, zname1,
-                                     holder_empty.get()),
+    EXPECT_THROW(zone_table->addZone(mem_sgmt_, zname1, holder_empty.get()),
                  isc::InvalidParameter);
 
     SegmentObjectHolder<ZoneData, RRClass> holder1(
@@ -91,8 +90,7 @@ TEST_F(ZoneTableTest, addZone) {
     holder1.set(ZoneData::create(mem_sgmt_, zname1));
     const ZoneData* data1(holder1.get());
     // Normal successful case.
-    const ZoneTable::AddResult result1(zone_table->addZone(mem_sgmt_, zclass_,
-                                                           zname1,
+    const ZoneTable::AddResult result1(zone_table->addZone(mem_sgmt_, zname1,
                                                            holder1.release()));
     EXPECT_EQ(result::SUCCESS, result1.code);
     EXPECT_EQ(static_cast<const ZoneData*>(NULL), result1.zone_data);
@@ -103,8 +101,7 @@ TEST_F(ZoneTableTest, addZone) {
     // Duplicate add replaces the existing data wit the newly added one.
     SegmentObjectHolder<ZoneData, RRClass> holder2(mem_sgmt_, zclass_);
     holder2.set(ZoneData::create(mem_sgmt_, zname1));
-    const ZoneTable::AddResult result2(zone_table->addZone(mem_sgmt_, zclass_,
-                                                           zname1,
+    const ZoneTable::AddResult result2(zone_table->addZone(mem_sgmt_, zname1,
                                                            holder2.release()));
     EXPECT_EQ(result::EXIST, result2.code);
     // The old one gets out
@@ -119,7 +116,7 @@ TEST_F(ZoneTableTest, addZone) {
         mem_sgmt_, zclass_);
     holder3.set(ZoneData::create(mem_sgmt_, Name("EXAMPLE.COM")));
     // names are compared in a case insensitive manner.
-    const ZoneTable::AddResult result3(zone_table->addZone(mem_sgmt_, zclass_,
+    const ZoneTable::AddResult result3(zone_table->addZone(mem_sgmt_,
                                                            Name("EXAMPLE.COM"),
                                                            holder3.release()));
     EXPECT_EQ(result::EXIST, result3.code);
@@ -129,26 +126,23 @@ TEST_F(ZoneTableTest, addZone) {
         mem_sgmt_, zclass_);
     holder4.set(ZoneData::create(mem_sgmt_, zname2));
     EXPECT_EQ(result::SUCCESS,
-              zone_table->addZone(mem_sgmt_, zclass_, zname2,
-                                  holder4.release()).code);
+              zone_table->addZone(mem_sgmt_, zname2, holder4.release()).code);
     EXPECT_EQ(2, zone_table->getZoneCount());
     SegmentObjectHolder<ZoneData, RRClass> holder5(
         mem_sgmt_, zclass_);
     holder5.set(ZoneData::create(mem_sgmt_, zname3));
     EXPECT_EQ(result::SUCCESS,
-              zone_table->addZone(mem_sgmt_, zclass_, zname3,
-                                  holder5.release()).code);
+              zone_table->addZone(mem_sgmt_, zname3, holder5.release()).code);
     EXPECT_EQ(3, zone_table->getZoneCount());
 
     // Have the memory segment throw an exception in extending the internal
-    // tree.  It still shouldn't cause memory leak (which would be detected
-    // in TearDown()).
+    // tree.  We'll destroy it after that via SegmentObjectHolder.
     SegmentObjectHolder<ZoneData, RRClass> holder6(
         mem_sgmt_, zclass_);
     holder6.set(ZoneData::create(mem_sgmt_, Name("example.org")));
     mem_sgmt_.setThrowCount(1);
-    EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, Name("example.org"),
-                                     holder6.release()),
+    EXPECT_THROW(zone_table->addZone(mem_sgmt_, Name("example.org"),
+                                     holder6.get()),
                  std::bad_alloc);
 }
 
@@ -175,8 +169,7 @@ TEST_F(ZoneTableTest, addEmptyZone) {
     // internal to the ZoneTable implementation.
     SegmentObjectHolder<ZoneData, RRClass> holder2(mem_sgmt_, zclass_);
     holder2.set(ZoneData::create(mem_sgmt_, zname1));
-    const ZoneTable::AddResult result2(zone_table->addZone(mem_sgmt_, zclass_,
-                                                           zname1,
+    const ZoneTable::AddResult result2(zone_table->addZone(mem_sgmt_, zname1,
                                                            holder2.release()));
     EXPECT_EQ(result::EXIST, result2.code);
     EXPECT_EQ(static_cast<const ZoneData*>(NULL), result2.zone_data);
@@ -197,20 +190,18 @@ TEST_F(ZoneTableTest, findZone) {
         mem_sgmt_, zclass_);
     holder1.set(ZoneData::create(mem_sgmt_, zname1));
     ZoneData* zone_data = holder1.get();
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, zname1,
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1,
                                                    holder1.release()).code);
     SegmentObjectHolder<ZoneData, RRClass> holder2(
         mem_sgmt_, zclass_);
     holder2.set(ZoneData::create(mem_sgmt_, zname2));
     EXPECT_EQ(result::SUCCESS,
-              zone_table->addZone(mem_sgmt_, zclass_, zname2,
-                                  holder2.release()).code);
+              zone_table->addZone(mem_sgmt_, zname2, holder2.release()).code);
     SegmentObjectHolder<ZoneData, RRClass> holder3(
         mem_sgmt_, zclass_);
     holder3.set(ZoneData::create(mem_sgmt_, zname3));
     EXPECT_EQ(result::SUCCESS,
-              zone_table->addZone(mem_sgmt_, zclass_, zname3,
-                                  holder3.release()).code);
+              zone_table->addZone(mem_sgmt_, zname3, holder3.release()).code);
 
     const ZoneTable::FindResult find_result1 =
         zone_table->findZone(Name("example.com"));
@@ -234,8 +225,7 @@ TEST_F(ZoneTableTest, findZone) {
     SegmentObjectHolder<ZoneData, RRClass> holder4(
         mem_sgmt_, zclass_);
     holder4.set(ZoneData::create(mem_sgmt_, Name("com")));
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_,
-                                                   Name("com"),
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, Name("com"),
                                                    holder4.release()).code);
     EXPECT_EQ(zone_data,
               zone_table->findZone(Name("www.example.com")).zone_data);

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

@@ -12,10 +12,20 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include <config.h>
+
 #include <datasrc/memory/zone_writer.h>
 #include <datasrc/memory/zone_table_segment_local.h>
 #include <datasrc/memory/zone_data.h>
+#include <datasrc/memory/zone_data_loader.h>
+#include <datasrc/memory/load_action.h>
+#include <datasrc/memory/zone_table.h>
 #include <datasrc/exceptions.h>
+#include <datasrc/result.h>
+
+#include <util/memory_segment_mapped.h>
+
+#include <cc/data.h>
 
 #include <dns/rrclass.h>
 #include <dns/name.h>
@@ -27,8 +37,10 @@
 
 #include <boost/scoped_ptr.hpp>
 #include <boost/bind.hpp>
+#include <boost/format.hpp>
 
 #include <string>
+#include <unistd.h>
 
 using boost::scoped_ptr;
 using boost::bind;
@@ -325,4 +337,79 @@ TEST_F(ZoneWriterTest, autoCleanUp) {
     EXPECT_NO_THROW(writer_->load());
 }
 
+// Used in the manyWrites test, encapsulating loadZoneData() to avoid
+// its signature ambiguity.
+ZoneData*
+loadZoneDataWrapper(isc::util::MemorySegment& segment, const RRClass& rrclass,
+                    const Name& name, const std::string& filename)
+{
+    return (loadZoneData(segment, rrclass, name, filename));
+}
+
+// Check the behavior of creating many small zones.  The main purpose of
+// test is to trigger MemorySegmentGrown exception in ZoneWriter::install.
+// There's no easy (if any) way to cause that reliably as it's highly
+// dependent on details of the underlying boost implementation and probably
+// also on the system behavior, but we'll try some promising scenario (it
+// in fact triggered the intended result at least on one environment).
+TEST_F(ZoneWriterTest, manyWrites) {
+#ifdef USE_SHARED_MEMORY
+    // First, make a fresh mapped file of a small size (so it'll be more likely
+    // to grow in the test.
+    const char* const mapped_file = TEST_DATA_BUILDDIR "/test.mapped";
+    unlink(mapped_file);
+    boost::scoped_ptr<isc::util::MemorySegmentMapped> segment(
+        new isc::util::MemorySegmentMapped(
+            mapped_file, isc::util::MemorySegmentMapped::CREATE_ONLY, 4096));
+    segment.reset();
+
+    // Then prepare a ZoneTableSegment of the 'mapped' type specifying the
+    // file we just created.
+    boost::scoped_ptr<ZoneTableSegment> zt_segment(
+        ZoneTableSegment::create(RRClass::IN(), "mapped"));
+    const isc::data::ConstElementPtr params =
+        isc::data::Element::fromJSON(
+            "{\"mapped-file\": \"" + std::string(mapped_file) + "\"}");
+    zt_segment->reset(ZoneTableSegment::READ_WRITE, params);
+#else
+    // Do the same test for the local segment, although there shouldn't be
+    // anything tricky in that case.
+    boost::scoped_ptr<ZoneTableSegment> zt_segment(
+        ZoneTableSegment::create(RRClass::IN(), "local"));
+#endif
+
+    // Now, create many small zones in the zone table with a ZoneWriter.
+    // We use larger origin names so it'll (hopefully) require the memory
+    // segment to grow while adding the name into the internal table.
+    const size_t zone_count = 10000; // arbitrary choice
+    for (size_t i = 0; i < zone_count; ++i) {
+        const Name origin(
+            boost::str(boost::format("%063u.%063u.%063u.example.org")
+                       % i % i % i));
+        const LoadAction action = boost::bind(loadZoneDataWrapper, _1,
+                                              RRClass::IN(), origin,
+                                              TEST_DATA_DIR
+                                              "/template.zone");
+        ZoneWriter writer(*zt_segment, action, origin, RRClass::IN(), false);
+        writer.load();
+        writer.install();
+        writer.cleanup();
+
+        // Confirm it's been successfully added and can be actually found.
+        const ZoneTable::FindResult result =
+            zt_segment->getHeader().getTable()->findZone(origin);
+        EXPECT_EQ(isc::datasrc::result::SUCCESS, result.code);
+        EXPECT_NE(static_cast<const ZoneData*>(NULL), result.zone_data) <<
+            "unexpected find result: " + origin.toText();
+    }
+
+    // Make sure to close the segment before (possibly) removing the mapped
+    // file.
+    zt_segment.reset();
+
+#ifdef USE_SHARED_MEMORY
+    unlink(mapped_file);
+#endif
+}
+
 }