Merge branch 'trac2850_4'

Conflicts:
	src/lib/datasrc/memory/zone_table.cc
	src/lib/datasrc/memory/zone_writer.cc
	src/lib/datasrc/memory/zone_writer.h

src/lib/datasrc/memory/zone_table.cc

@@ -72,7 +72,8 @@ ZoneTable::create(util::MemorySegment& mem_sgmt, const RRClass& zone_class) {
 }
 
 void
-ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable) {
+ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, int)
+{
     ZoneTableTree::destroy(mem_sgmt, ztable->zones_.get(),
                            boost::bind(deleteZoneData, &mem_sgmt, _1,
                                        ztable->rrclass_));

src/lib/datasrc/memory/zone_table.h

@@ -151,7 +151,10 @@ public:
     /// \param ztable A non NULL pointer to a valid \c ZoneTable object
     /// that was originally created by the \c create() method (the behavior
     /// is undefined if this condition isn't met).
-    static void destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable);
+    /// \param unused Ununsed parameter, provided so it's compatible to
+    /// SegmentObjectHolder.
+    static void destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable,
+                        int unused = 0);
 
     /// \brief Return the number of zones contained in the zone table.
     ///

src/lib/datasrc/memory/zone_table_segment.h

@@ -108,10 +108,9 @@ private:
 /// for the specific memory-implementation behavior.
 ///
 /// Note: At some point in the future, methods such as \c reset(),
-/// \c clear(), \c resetHeader(), \c getHeader(), \c isWritable(),
-/// \c isUsable() may become non-virtual methods. Such a change should
-/// not affect any code that uses this class, but please be aware of
-/// such plans.
+/// \c clear(), \c getHeader(), \c isWritable(), \c isUsable() may
+/// become non-virtual methods. Such a change should not affect any code
+/// that uses this class, but please be aware of such plans.
 class ZoneTableSegment {
 protected:
     /// \brief Protected constructor
@@ -128,6 +127,10 @@ public:
     /// \brief Return a string name for the \c ZoneTableSegment
     /// implementation.
     ///
+    /// Implementations of this method should ensure that the returned
+    /// string is identical to the corresponding string passed to
+    /// \c ZoneTableSegment::create() for that implementation.
+    ///
     /// \throw None This method's implementations must be
     /// exception-free.
     virtual const std::string& getImplType() const = 0;
@@ -334,12 +337,6 @@ public:
     /// Note that after calling \c clear(), this method will return
     /// false until the segment is reset successfully again.
     virtual bool isUsable() const = 0;
-
-    /// \brief Reset the table header address.
-    ///
-    /// This method must recalculate the \c ZoneTableHeader address, so
-    /// that it is valid when requested using the \c getHeader() method.
-    virtual void resetHeader() = 0;
 };
 
 } // namespace memory

src/lib/datasrc/memory/zone_table_segment_local.cc

@@ -60,11 +60,6 @@ ZoneTableSegmentLocal::clear()
               "should not be used.");
 }
 
-void
-ZoneTableSegmentLocal::resetHeader() {
-    // This method does not have to do anything in this implementation.
-}
-
 // After more methods' definitions are added here, it would be a good
 // idea to move getHeader() and getMemorySegment() definitions to the
 // header file.

src/lib/datasrc/memory/zone_table_segment_local.h

@@ -49,10 +49,6 @@ public:
     /// \brief Returns "local" as the implementation type.
     virtual const std::string& getImplType() const;
 
-    /// \brief This method does not have to do anything in this
-    /// implementation. It has an empty definition.
-    virtual void resetHeader();
-
     /// \brief Return the \c ZoneTableHeader for this local zone table
     /// segment.
     virtual ZoneTableHeader& getHeader();

src/lib/datasrc/memory/zone_table_segment_mapped.cc

@@ -13,12 +13,15 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <datasrc/memory/zone_table_segment_mapped.h>
+#include <datasrc/memory/zone_table.h>
+#include <datasrc/memory/segment_object_holder.h>
 
 #include <memory>
 
 using namespace isc::data;
 using namespace isc::dns;
 using namespace isc::util;
+using isc::datasrc::memory::detail::SegmentObjectHolder;
 
 namespace isc {
 namespace datasrc {
@@ -37,8 +40,7 @@ const char* const ZONE_TABLE_HEADER_NAME = "zone_table_header";
 ZoneTableSegmentMapped::ZoneTableSegmentMapped(const RRClass& rrclass) :
     ZoneTableSegment(rrclass),
     impl_type_("mapped"),
-    rrclass_(rrclass),
-    cached_header_(NULL)
+    rrclass_(rrclass)
 {
 }
 
@@ -53,7 +55,7 @@ ZoneTableSegmentMapped::getImplType() const {
 
 bool
 ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
-                                        bool create,
+                                        bool create, bool has_allocations,
                                         std::string& error_msg)
 {
     const MemorySegment::NamedAddressResult result =
@@ -80,6 +82,14 @@ ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
             }
         }
     } else {
+        if ((!create) && has_allocations) {
+            // If we are resetting in READ_WRITE mode, and some memory
+            // was already allocated but there is no checksum name, that
+            // indicates that the segment is corrupted.
+            error_msg = "Existing segment is missing a checksum name";
+            return (false);
+        }
+
         // Allocate space for a checksum (which is saved during close).
         void* checksum = NULL;
         while (!checksum) {
@@ -90,9 +100,7 @@ ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
             }
         }
         *static_cast<size_t*>(checksum) = 0;
-        const bool grew = segment.setNamedAddress(ZONE_TABLE_CHECKSUM_NAME,
-                                                  checksum);
-        assert(!grew);
+        segment.setNamedAddress(ZONE_TABLE_CHECKSUM_NAME, checksum);
     }
 
     return (true);
@@ -100,14 +108,14 @@ ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
 
 bool
 ZoneTableSegmentMapped::processHeader(MemorySegmentMapped& segment,
-                                      bool create,
+                                      bool create, bool has_allocations,
                                       std::string& error_msg)
 {
     const MemorySegment::NamedAddressResult result =
         segment.getNamedAddress(ZONE_TABLE_HEADER_NAME);
     if (result.first) {
         if (create) {
-            // There must be no previously saved checksum.
+            // There must be no previously saved header.
             error_msg = "There is already a saved ZoneTableHeader in the "
                  "segment opened in create mode";
             return (false);
@@ -115,25 +123,24 @@ ZoneTableSegmentMapped::processHeader(MemorySegmentMapped& segment,
             assert(result.second);
         }
     } else {
-        void* ptr = NULL;
-        while (!ptr) {
-            try {
-                ptr = segment.allocate(sizeof(ZoneTableHeader));
-            } catch (const MemorySegmentGrown&) {
-                // Do nothing and try again.
-            }
+        if ((!create) && has_allocations) {
+            // If we are resetting in READ_WRITE mode, and some memory
+            // was already allocated but there is no header name, that
+            // indicates that the segment is corrupted.
+            error_msg = "Existing segment is missing a ZoneTableHeader name";
+            return (false);
         }
-        try {
-            ZoneTableHeader* new_header = new(ptr)
-                ZoneTableHeader(ZoneTable::create(segment, rrclass_));
-            const bool grew = segment.setNamedAddress(ZONE_TABLE_HEADER_NAME,
-                                                      new_header);
-            assert(!grew);
-        } catch (const MemorySegmentGrown&) {
-            // This is extremely unlikely and we just throw a fatal
-            // exception here without attempting to recover.
-
-            throw std::bad_alloc();
+
+        while (true) {
+            try {
+                SegmentObjectHolder<ZoneTable, int> zt_holder(segment, 0);
+                zt_holder.set(ZoneTable::create(segment, rrclass_));
+                void* ptr = segment.allocate(sizeof(ZoneTableHeader));
+                ZoneTableHeader* new_header = new(ptr)
+                    ZoneTableHeader(zt_holder.release());
+                segment.setNamedAddress(ZONE_TABLE_HEADER_NAME, new_header);
+                break;
+            } catch (const MemorySegmentGrown&) {}
         }
     }
 
@@ -152,9 +159,13 @@ ZoneTableSegmentMapped::openReadWrite(const std::string& filename,
     std::auto_ptr<MemorySegmentMapped> segment
         (new MemorySegmentMapped(filename, mode));
 
+    // This flag is used inside processCheckSum() and processHeader(),
+    // and must be initialized before we make any further allocations.
+    const bool has_allocations = !segment->allMemoryDeallocated();
+
     std::string error_msg;
-    if ((!processChecksum(*segment, create, error_msg)) ||
-        (!processHeader(*segment, create, error_msg))) {
+    if ((!processChecksum(*segment, create, has_allocations, error_msg)) ||
+        (!processHeader(*segment, create, has_allocations, error_msg))) {
          if (mem_sgmt_) {
               isc_throw(ResetFailed,
                         "Error in resetting zone table segment to use "
@@ -268,7 +279,7 @@ ZoneTableSegmentMapped::reset(MemorySegmentOpenMode mode,
         break;
 
     default:
-        isc_throw(isc::InvalidOperation,
+        isc_throw(isc::InvalidParameter,
                   "Invalid MemorySegmentOpenMode passed to reset()");
     }
 
@@ -276,9 +287,11 @@ ZoneTableSegmentMapped::reset(MemorySegmentOpenMode mode,
     current_mode_ = mode;
     mem_sgmt_.reset(segment.release());
 
-    // Given what we setup above, resetHeader() must not throw at this
-    // point. If it does, all bets are off.
-    resetHeader();
+    if (!isWritable()) {
+        // Given what we setup above, the following must not throw at
+        // this point. If it does, all bets are off.
+        cached_ro_header_ = getHeaderHelper<ZoneTableHeader>(true);
+    }
 }
 
 void
@@ -310,15 +323,18 @@ ZoneTableSegmentMapped::clear() {
     }
 }
 
-void
-ZoneTableSegmentMapped::resetHeader() {
-    // We cannot perform resetHeader() lazily during getHeader() as
-    // getHeader() has to work on const objects too. So we do it here
-    // now.
-
+template<typename T>
+T*
+ZoneTableSegmentMapped::getHeaderHelper(bool initial) const {
     if (!isUsable()) {
         isc_throw(isc::InvalidOperation,
-                  "resetHeader() called without calling reset() first");
+                  "getHeader() called without calling reset() first");
+    }
+
+    if (!isWritable() && !initial) {
+        // The header address would not have changed since reset() for
+        // READ_ONLY segments.
+        return (cached_ro_header_);
     }
 
     const MemorySegment::NamedAddressResult result =
@@ -329,29 +345,19 @@ ZoneTableSegmentMapped::resetHeader() {
                   "getHeader()");
     }
 
-    cached_header_ = static_cast<ZoneTableHeader*>(result.second);
-}
-
-template<typename T>
-T*
-ZoneTableSegmentMapped::getHeaderHelper() const {
-    if (!isUsable()) {
-        isc_throw(isc::InvalidOperation,
-                  "getHeader() called without calling reset() first");
-    }
-
-    assert(cached_header_);
-    return (cached_header_);
+    T* header = static_cast<ZoneTableHeader*>(result.second);
+    assert(header);
+    return (header);
 }
 
 ZoneTableHeader&
 ZoneTableSegmentMapped::getHeader() {
-    return (*getHeaderHelper<ZoneTableHeader>());
+    return (*getHeaderHelper<ZoneTableHeader>(false));
 }
 
 const ZoneTableHeader&
 ZoneTableSegmentMapped::getHeader() const {
-    return (*getHeaderHelper<const ZoneTableHeader>());
+    return (*getHeaderHelper<const ZoneTableHeader>(false));
 }
 
 MemorySegment&
@@ -373,8 +379,8 @@ bool
 ZoneTableSegmentMapped::isWritable() const {
     if (!isUsable()) {
         // If reset() was never performed for this segment, or if the
-        // most recent reset() had failed, then the segment is not
-        // writable.
+        // most recent reset() had failed, or if the segment had been
+        // cleared, then the segment is not writable.
         return (false);
     }
 

src/lib/datasrc/memory/zone_table_segment_mapped.h

@@ -50,12 +50,6 @@ public:
     /// \brief Returns "mapped" as the implementation type.
     virtual const std::string& getImplType() const;
 
-    /// \brief Resets the \c ZoneTableHeader address from the named
-    /// address in the mapped file. This method should be called once
-    /// before calls to \c getHeader() if the mapped \c MemorySegment
-    /// has grown.
-    virtual void resetHeader();
-
     /// \brief Return the \c ZoneTableHeader for this mapped zone table
     /// segment.
     ///
@@ -122,15 +116,15 @@ private:
     void sync();
 
     bool processChecksum(isc::util::MemorySegmentMapped& segment, bool create,
-                         std::string& error_msg);
+                         bool has_allocations, std::string& error_msg);
     bool processHeader(isc::util::MemorySegmentMapped& segment, bool create,
-                       std::string& error_msg);
+                       bool has_allocations, std::string& error_msg);
 
     isc::util::MemorySegmentMapped* openReadWrite(const std::string& filename,
                                                   bool create);
     isc::util::MemorySegmentMapped* openReadOnly(const std::string& filename);
 
-    template<typename T> T* getHeaderHelper() const;
+    template<typename T> T* getHeaderHelper(bool initial) const;
 
 private:
     std::string impl_type_;
@@ -140,7 +134,7 @@ private:
     // Internally holds a MemorySegmentMapped. This is NULL on
     // construction, and is set by the \c reset() method.
     boost::scoped_ptr<isc::util::MemorySegmentMapped> mem_sgmt_;
-    ZoneTableHeader* cached_header_;
+    ZoneTableHeader* cached_ro_header_;
 };
 
 } // namespace memory

src/lib/datasrc/memory/zone_writer.cc

@@ -15,6 +15,11 @@
 #include <datasrc/memory/zone_writer.h>
 #include <datasrc/memory/zone_data.h>
 #include <datasrc/memory/zone_table_segment.h>
+#include <datasrc/memory/segment_object_holder.h>
+
+#include <boost/scoped_ptr.hpp>
+
+#include <dns/rrclass.h>
 
 #include <datasrc/exceptions.h>
 
@@ -26,87 +31,133 @@ namespace isc {
 namespace datasrc {
 namespace memory {
 
+ZoneTableSegment&
+checkZoneTableSegment(ZoneTableSegment& segment) {
+    if (!segment.isWritable()) {
+        isc_throw(isc::InvalidOperation,
+                  "Attempt to construct ZoneWriter for a read-only segment");
+    }
+    return (segment);
+}
+
+struct ZoneWriter::Impl {
+    Impl(ZoneTableSegment& segment, const LoadAction& load_action,
+         const dns::Name& origin, const dns::RRClass& rrclass,
+         bool throw_on_load_error) :
+        // We validate segment first so we can use it to initialize
+        // data_holder_ safely.
+        segment_(checkZoneTableSegment(segment)),
+        load_action_(load_action),
+        origin_(origin),
+        rrclass_(rrclass),
+        state_(ZW_UNUSED),
+        catch_load_error_(throw_on_load_error)
+    {
+        while (true) {
+            try {
+                data_holder_.reset(
+                    new ZoneDataHolder(segment.getMemorySegment(), rrclass_));
+                break;
+            } catch (const isc::util::MemorySegmentGrown&) {}
+        }
+    }
+
+    ZoneTableSegment& segment_;
+    const LoadAction load_action_;
+    const dns::Name origin_;
+    const dns::RRClass rrclass_;
+    enum State {
+        ZW_UNUSED,
+        ZW_LOADED,
+        ZW_INSTALLED,
+        ZW_CLEANED
+    };
+    State state_;
+    const bool catch_load_error_;
+    typedef detail::SegmentObjectHolder<ZoneData, dns::RRClass> ZoneDataHolder;
+    boost::scoped_ptr<ZoneDataHolder> data_holder_;
+};
+
 ZoneWriter::ZoneWriter(ZoneTableSegment& segment,
                        const LoadAction& load_action,
                        const dns::Name& origin,
                        const dns::RRClass& rrclass,
                        bool throw_on_load_error) :
-    segment_(segment),
-    load_action_(load_action),
-    origin_(origin),
-    rrclass_(rrclass),
-    zone_data_(NULL),
-    state_(ZW_UNUSED),
-    catch_load_error_(throw_on_load_error)
+    impl_(new Impl(segment, load_action, origin, rrclass, throw_on_load_error))
 {
-    if (!segment.isWritable()) {
-        isc_throw(isc::InvalidOperation,
-                  "Attempt to construct ZoneWriter for a read-only segment");
-    }
 }
 
 ZoneWriter::~ZoneWriter() {
     // Clean up everything there might be left if someone forgot, just
     // in case.
     cleanup();
+    delete impl_;
 }
 
 void
 ZoneWriter::load(std::string* error_msg) {
-    if (state_ != ZW_UNUSED) {
+    if (impl_->state_ != Impl::ZW_UNUSED) {
         isc_throw(isc::InvalidOperation, "Trying to load twice");
     }
 
     try {
-        zone_data_ = load_action_(segment_.getMemorySegment());
-        segment_.resetHeader();
+        ZoneData* zone_data =
+            impl_->load_action_(impl_->segment_.getMemorySegment());
 
-        if (!zone_data_) {
-            // Bug inside load_action_.
+        if (!zone_data) {
+            // Bug inside impl_->load_action_.
             isc_throw(isc::InvalidOperation,
                       "No data returned from load action");
         }
+
+        impl_->data_holder_->set(zone_data);
+
     } catch (const ZoneLoaderException& ex) {
-        if (!catch_load_error_) {
+        if (!impl_->catch_load_error_) {
             throw;
         }
         if (error_msg) {
             *error_msg = ex.what();
         }
-        segment_.resetHeader();
     }
 
-    state_ = ZW_LOADED;
+    impl_->state_ = Impl::ZW_LOADED;
 }
 
 void
 ZoneWriter::install() {
-    if (state_ != ZW_LOADED) {
+    if (impl_->state_ != Impl::ZW_LOADED) {
         isc_throw(isc::InvalidOperation, "No data to install");
     }
 
     // Check the internal integrity assumption: we should have non NULL
     // zone data or we've allowed load error to create an empty zone.
-    assert(zone_data_ || catch_load_error_);
+    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 (state_ != ZW_INSTALLED) {
+    while (impl_->state_ != Impl::ZW_INSTALLED) {
         try {
-            ZoneTable* table(segment_.getHeader().getTable());
-            if (table == NULL) {
+            ZoneTable* table(impl_->segment_.getHeader().getTable());
+            if (!table) {
                 isc_throw(isc::Unexpected, "No zone table present");
             }
+            // We still need to hold the zone data until we return from
+            // addZone in case it throws, but we then need to immediately
+            // release it as the ownership is transferred to the zone table.
+            // we release this by (re)set it to the old data; that way we can
+            // use the holder for the final cleanup.
             const ZoneTable::AddResult result(
-                zone_data_ ? table->addZone(segment_.getMemorySegment(),
-                                            rrclass_, origin_, zone_data_) :
-                table->addEmptyZone(segment_.getMemorySegment(), origin_));
-            state_ = ZW_INSTALLED;
-            zone_data_ = result.zone_data;
+                impl_->data_holder_->get() ?
+                table->addZone(impl_->segment_.getMemorySegment(),
+                               impl_->rrclass_, impl_->origin_,
+                               impl_->data_holder_->get()) :
+                table->addEmptyZone(impl_->segment_.getMemorySegment(),
+                                    impl_->origin_));
+            impl_->data_holder_->set(result.zone_data);
+            impl_->state_ = Impl::ZW_INSTALLED;
         } catch (const isc::util::MemorySegmentGrown&) {}
-
-        segment_.resetHeader();
     }
 }
 
@@ -114,10 +165,11 @@ void
 ZoneWriter::cleanup() {
     // We eat the data (if any) now.
 
-    if (zone_data_ != NULL) {
-        ZoneData::destroy(segment_.getMemorySegment(), zone_data_, rrclass_);
-        zone_data_ = NULL;
-        state_ = ZW_CLEANED;
+    ZoneData* zone_data = impl_->data_holder_->release();
+    if (zone_data) {
+        ZoneData::destroy(impl_->segment_.getMemorySegment(), zone_data,
+                          impl_->rrclass_);
+        impl_->state_ = Impl::ZW_CLEANED;
     }
 }
 

src/lib/datasrc/memory/zone_writer.h

@@ -15,15 +15,16 @@
 #ifndef MEM_ZONE_WRITER_H
 #define MEM_ZONE_WRITER_H
 
-#include <datasrc/memory/zone_table_segment.h>
 #include <datasrc/memory/load_action.h>
 
-#include <dns/rrclass.h>
-#include <dns/name.h>
+#include <boost/noncopyable.hpp>
+
+#include <dns/dns_fwd.h>
 
 namespace isc {
 namespace datasrc {
 namespace memory {
+class ZoneTableSegment;
 
 /// \brief Does an update to a zone.
 ///
@@ -38,7 +39,7 @@ namespace memory {
 /// This class provides strong exception guarantee for each public
 /// method. That is, when any of the methods throws, the entire state
 /// stays the same as before the call.
-class ZoneWriter {
+class ZoneWriter : boost::noncopyable {
 public:
     /// \brief Constructor
     ///
@@ -127,19 +128,10 @@ public:
     void cleanup();
 
 private:
-    ZoneTableSegment& segment_;
-    const LoadAction load_action_;
-    const dns::Name origin_;
-    const dns::RRClass rrclass_;
-    ZoneData* zone_data_;
-    enum State {
-        ZW_UNUSED,
-        ZW_LOADED,
-        ZW_INSTALLED,
-        ZW_CLEANED
-    };
-    State state_;
-    const bool catch_load_error_;
+    // We hide details as this class will be used by various applications
+    // and we use some internal data structures in the implementation.
+    struct Impl;
+    Impl* impl_;
 };
 
 }

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

@@ -267,6 +267,14 @@ TEST_F(ZoneTableSegmentMappedTest, reset) {
     EXPECT_FALSE(ztable_segment_->isUsable());
     EXPECT_FALSE(ztable_segment_->isWritable());
 
+    // If a Python binding passes an invalid integer as the mode,
+    // reset() should reject it.
+    EXPECT_THROW({
+        ztable_segment_->reset
+            (static_cast<ZoneTableSegment::MemorySegmentOpenMode>(1234),
+             config_params_);
+    }, isc::InvalidParameter);
+
     // READ_WRITE mode must create the mapped file if it doesn't exist
     // (and must not result in an exception).
     ztable_segment_->reset(ZoneTableSegment::READ_WRITE, config_params_);
@@ -555,36 +563,4 @@ TEST_F(ZoneTableSegmentMappedTest, resetCreateOverCorruptedFile) {
     EXPECT_FALSE(verifyData(ztable_segment_->getMemorySegment()));
 }
 
-TEST_F(ZoneTableSegmentMappedTest, resetHeaderUninitialized) {
-    // This should throw as we haven't called reset() yet.
-    EXPECT_THROW(ztable_segment_->resetHeader(), isc::InvalidOperation);
-}
-
-TEST_F(ZoneTableSegmentMappedTest, resetHeader) {
-    // First, open an underlying mapped file in read+write mode (doesn't
-    // exist yet)
-    ztable_segment_->reset(ZoneTableSegment::READ_WRITE, config_params_);
-
-    // Check if a valid ZoneTable is found.
-    {
-        const ZoneTableHeader& header = ztable_segment_->getHeader();
-        const ZoneTable* table = header.getTable();
-        EXPECT_EQ(0, table->getZoneCount());
-    }
-
-    // Grow the segment by allocating something large.
-    EXPECT_THROW(ztable_segment_->getMemorySegment().allocate(1<<16),
-                 MemorySegmentGrown);
-
-    // Reset the header address. This should not throw now.
-    EXPECT_NO_THROW(ztable_segment_->resetHeader());
-
-    // Check if a valid ZoneTable is found.
-    {
-        const ZoneTableHeader& header = ztable_segment_->getHeader();
-        const ZoneTable* table = header.getTable();
-        EXPECT_EQ(0, table->getZoneCount());
-    }
-}
-
 } // anonymous namespace

src/lib/datasrc/tests/memory/zone_table_segment_mock.h

@@ -56,11 +56,6 @@ public:
         isc_throw(isc::NotImplemented, "clear() is not implemented");
     }
 
-    virtual void resetHeader() {
-        // This method does not have to do anything in this
-        // implementation.
-    }
-
     virtual ZoneTableHeader& getHeader() {
         return (header_);
     }

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

@@ -88,19 +88,6 @@ TEST_F(ZoneTableSegmentTest, getHeader) {
     // const version.
     testGetHeader<const ZoneTableSegment, const ZoneTableHeader,
                   const ZoneTable>(ztable_segment_);
-
-    // This is a nop for local segments.
-    ztable_segment_->resetHeader();
-
-    // The following still behave as before after resetHeader().
-
-    // non-const version.
-    testGetHeader<ZoneTableSegment, ZoneTableHeader, ZoneTable>
-        (ztable_segment_);
-
-    // const version.
-    testGetHeader<const ZoneTableSegment, const ZoneTableHeader,
-                  const ZoneTable>(ztable_segment_);
 }
 
 TEST_F(ZoneTableSegmentTest, getMemorySegment) {

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

@@ -42,25 +42,10 @@ namespace {
 
 class TestException {};
 
-class ZoneTableSegmentHelper : public ZoneTableSegmentMock {
-public:
-    ZoneTableSegmentHelper(const isc::dns::RRClass& rrclass,
-                           isc::util::MemorySegment& mem_sgmt) :
-        ZoneTableSegmentMock(rrclass, mem_sgmt),
-        reset_header_called_(false)
-    {}
-
-    virtual void resetHeader() {
-        reset_header_called_ = true;
-    }
-
-    bool reset_header_called_;
-};
-
 class ZoneWriterTest : public ::testing::Test {
 protected:
     ZoneWriterTest() :
-        segment_(new ZoneTableSegmentHelper(RRClass::IN(), mem_sgmt_)),
+        segment_(new ZoneTableSegmentMock(RRClass::IN(), mem_sgmt_)),
         writer_(new
             ZoneWriter(*segment_,
                        bind(&ZoneWriterTest::loadAction, this, _1),
@@ -76,7 +61,7 @@ protected:
         writer_.reset();
     }
     MemorySegmentMock mem_sgmt_;
-    scoped_ptr<ZoneTableSegmentHelper> segment_;
+    scoped_ptr<ZoneTableSegmentMock> segment_;
     scoped_ptr<ZoneWriter> writer_;
     bool load_called_;
     bool load_throw_;
@@ -148,18 +133,14 @@ TEST_F(ZoneWriterTest, constructForReadOnlySegment) {
 TEST_F(ZoneWriterTest, correctCall) {
     // Nothing called before we call it
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(segment_->reset_header_called_);
 
     // Just the load gets called now
     EXPECT_NO_THROW(writer_->load());
     EXPECT_TRUE(load_called_);
-    EXPECT_TRUE(segment_->reset_header_called_);
     load_called_ = false;
-    segment_->reset_header_called_ = false;
 
     EXPECT_NO_THROW(writer_->install());
     EXPECT_FALSE(load_called_);
-    EXPECT_TRUE(segment_->reset_header_called_);
 
     // We don't check explicitly how this works, but call it to free memory. If
     // everything is freed should be checked inside the TearDown.
@@ -170,19 +151,15 @@ TEST_F(ZoneWriterTest, loadTwice) {
     // Load it the first time
     EXPECT_NO_THROW(writer_->load());
     EXPECT_TRUE(load_called_);
-    EXPECT_TRUE(segment_->reset_header_called_);
     load_called_ = false;
-    segment_->reset_header_called_ = false;
 
     // The second time, it should not be possible
     EXPECT_THROW(writer_->load(), isc::InvalidOperation);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(segment_->reset_header_called_);
 
     // The object should not be damaged, try installing and clearing now
     EXPECT_NO_THROW(writer_->install());
     EXPECT_FALSE(load_called_);
-    EXPECT_TRUE(segment_->reset_header_called_);
 
     // We don't check explicitly how this works, but call it to free memory. If
     // everything is freed should be checked inside the TearDown.
@@ -197,18 +174,15 @@ TEST_F(ZoneWriterTest, loadLater) {
     EXPECT_NO_THROW(writer_->install());
     // Reset so we see nothing is called now
     load_called_ = false;
-    segment_->reset_header_called_ = false;
 
     EXPECT_THROW(writer_->load(), isc::InvalidOperation);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(segment_->reset_header_called_);
 
     // Cleanup and try loading again. Still shouldn't work.
     EXPECT_NO_THROW(writer_->cleanup());
 
     EXPECT_THROW(writer_->load(), isc::InvalidOperation);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(segment_->reset_header_called_);
 }
 
 // Try calling install at various bad times

src/lib/util/memory_segment.h

@@ -157,7 +157,7 @@ public:
     /// \return Returns <code>true</code> if all allocated memory (including
     /// names associated by memory addresses by \c setNamedAddress()) was
     /// deallocated, <code>false</code> otherwise.
-    virtual bool allMemoryDeallocated() = 0;
+    virtual bool allMemoryDeallocated() const = 0;
 
     /// \brief Associate specified address in the segment with a given name.
     ///
@@ -171,6 +171,10 @@ public:
     /// corresponding address by that name (in such cases the real address
     /// may be different between these two processes).
     ///
+    /// Some names are reserved for internal use by this class. If such
+    /// a name is passed to this method, an \c isc::InvalidParameter
+    /// exception will be thrown. See \c validateName() method for details.
+    ///
     /// \c addr must be 0 (NULL) or an address that belongs to this segment.
     /// The latter case means it must be the return value of a previous call
     /// to \c allocate().  The actual implementation is encouraged to detect
@@ -214,7 +218,8 @@ public:
     ///
     /// \throw std::bad_alloc Allocation of a segment space for the given name
     /// failed.
-    /// \throw InvalidParameter name is NULL.
+    /// \throw InvalidParameter name is NULL, empty ("") or begins with
+    /// an underscore ('_').
     /// \throw MemorySegmentError Failure of implementation specific
     /// validation.
     ///
@@ -226,10 +231,7 @@ public:
         // This public method implements common validation.  The actual
         // work specific to the derived segment is delegated to the
         // corresponding protected method.
-        if (!name) {
-            isc_throw(InvalidParameter,
-                      "NULL name is given to setNamedAddress");
-        }
+        validateName(name);
         return (setNamedAddressImpl(name, addr));
     }
 
@@ -243,13 +245,18 @@ public:
     /// associated by a prior call to \c setNameAddress().  If no address
     /// associated with the given name is found, it returns NULL.
     ///
+    /// Some names are reserved for internal use by this class. If such
+    /// a name is passed to this method, an \c isc::InvalidParameter
+    /// exception will be thrown. See \c validateName() method for details.
+    ///
     /// This method should generally be considered exception free, but there
     /// can be a small chance it throws, depending on the internal
     /// implementation (e.g., if it converts the name to std::string), so the
     /// API doesn't guarantee that property.  In general, if this method
     /// throws it should be considered a fatal condition.
     ///
-    /// \throw InvalidParameter name is NULL.
+    /// \throw InvalidParameter name is NULL, empty ("") or begins with
+    /// an underscore ('_').
     ///
     /// \param name A C string of which the segment memory address is to be
     /// returned.  Must not be NULL.
@@ -260,10 +267,7 @@ public:
         // This public method implements common validation.  The actual
         // work specific to the derived segment is delegated to the
         // corresponding protected method.
-        if (!name) {
-            isc_throw(InvalidParameter,
-                      "NULL name is given to getNamedAddress");
-        }
+        validateName(name);
         return (getNamedAddressImpl(name));
     }
 
@@ -274,9 +278,14 @@ public:
     /// \c setNamedAddress().  If there is no association for the given name
     /// this method returns false; otherwise it returns true.
     ///
+    /// Some names are reserved for internal use by this class. If such
+    /// a name is passed to this method, an \c isc::InvalidParameter
+    /// exception will be thrown. See \c validateName() method for details.
+    ///
     /// See \c getNamedAddress() about exception consideration.
     ///
-    /// \throw InvalidParameter name is NULL.
+    /// \throw InvalidParameter name is NULL, empty ("") or begins with
+    /// an underscore ('_').
     /// \throw MemorySegmentError Failure of implementation specific
     /// validation.
     ///
@@ -286,11 +295,29 @@ public:
         // This public method implements common validation.  The actual
         // work specific to the derived segment is delegated to the
         // corresponding protected method.
+        validateName(name);
+        return (clearNamedAddressImpl(name));
+    }
+
+private:
+    /// \brief Validate the passed name.
+    ///
+    /// This method validates the passed name (for name/address pairs)
+    /// and throws \c InvalidParameter if the name fails
+    /// validation. Otherwise, it does nothing.
+    ///
+    /// \throw InvalidParameter name is NULL, empty ("") or begins with
+    /// an underscore ('_').
+    static void validateName(const char* name) {
         if (!name) {
+            isc_throw(InvalidParameter, "NULL is invalid for a name.");
+        } else if (*name == '\0') {
+            isc_throw(InvalidParameter, "Empty names are invalid.");
+        } else if (*name == '_') {
             isc_throw(InvalidParameter,
-                      "NULL name is given to clearNamedAddress");
+                      "Names beginning with '_' are reserved for "
+                      "internal use only.");
         }
-        return (clearNamedAddressImpl(name));
     }
 
 protected:

src/lib/util/memory_segment_local.cc

@@ -47,7 +47,7 @@ MemorySegmentLocal::deallocate(void* ptr, size_t size) {
 }
 
 bool
-MemorySegmentLocal::allMemoryDeallocated() {
+MemorySegmentLocal::allMemoryDeallocated() const {
     return (allocated_size_ == 0 && named_addrs_.empty());
 }
 

src/lib/util/memory_segment_local.h

@@ -64,7 +64,7 @@ public:
     ///
     /// \return Returns <code>true</code> if all allocated memory was
     /// deallocated, <code>false</code> otherwise.
-    virtual bool allMemoryDeallocated();
+    virtual bool allMemoryDeallocated() const;
 
     /// \brief Local segment version of getNamedAddress.
     ///

src/lib/util/memory_segment_mapped.cc

@@ -137,7 +137,7 @@ struct MemorySegmentMapped::Impl {
         reserveMemory();
     }
 
-    void reserveMemory() {
+    void reserveMemory(bool no_grow = false) {
         if (!read_only_) {
             // Reserve a named address for use during
             // setNamedAddress(). Though this will almost always succeed
@@ -153,6 +153,7 @@ struct MemorySegmentMapped::Impl {
                 if (reserved_storage) {
                     break;
                 }
+                assert(!no_grow);
 
                 growSegment();
             }
@@ -324,12 +325,20 @@ MemorySegmentMapped::deallocate(void* ptr, size_t) {
 }
 
 bool
-MemorySegmentMapped::allMemoryDeallocated() {
-    impl_->freeReservedMemory();
-    const bool result = impl_->base_sgmt_->all_memory_deallocated();
-    impl_->reserveMemory();
-
-    return (result);
+MemorySegmentMapped::allMemoryDeallocated() const {
+    // This method is not technically const, but it reserves the
+    // const-ness property. In case of exceptions, we abort here. (See
+    // ticket #2850 for additional commentary.)
+    try {
+        impl_->freeReservedMemory();
+        const bool result = impl_->base_sgmt_->all_memory_deallocated();
+        // reserveMemory() should succeed now as the memory was already
+        // allocated, so we set no_grow to true.
+        impl_->reserveMemory(true);
+        return (result);
+    } catch (...) {
+        abort();
+    }
 }
 
 MemorySegment::NamedAddressResult

src/lib/util/memory_segment_mapped.h

@@ -182,7 +182,7 @@ public:
     /// read-only mode; in that case MemorySegmentError will be thrown.
     virtual void deallocate(void* ptr, size_t size);
 
-    virtual bool allMemoryDeallocated();
+    virtual bool allMemoryDeallocated() const;
 
     /// \brief Mapped segment version of setNamedAddress.
     ///

src/lib/util/random/random_number_generator.h

@@ -60,9 +60,7 @@ public:
     ///
     /// \param min The minimum number in the range
     /// \param max The maximum number in the range
-    /// \param seed A seed for the RNG. If 0 is passed, the current time
-    /// is used.
-    UniformRandomIntegerGenerator(int min, int max, unsigned int seed = 0):
+    UniformRandomIntegerGenerator(int min, int max):
         min_(std::min(min, max)), max_(std::max(min, max)),
         dist_(min_, max_), generator_(rng_, dist_)
     {
@@ -75,10 +73,7 @@ public:
         }
 
         // Init with the current time
-        if (seed == 0) {
-            seed = time(NULL);
-        }
-        rng_.seed(seed);
+        rng_.seed(time(NULL));
     }
 
     /// \brief Generate uniformly distributed integer

src/lib/util/tests/memory_segment_common_unittest.cc

@@ -41,6 +41,18 @@ checkSegmentNamedAddress(MemorySegment& segment, bool out_of_segment_ok) {
 
     // NULL name isn't allowed.
     EXPECT_THROW(segment.setNamedAddress(NULL, ptr32), InvalidParameter);
+    EXPECT_THROW(segment.getNamedAddress(NULL), InvalidParameter);
+    EXPECT_THROW(segment.clearNamedAddress(NULL), InvalidParameter);
+
+    // Empty names are not allowed.
+    EXPECT_THROW(segment.setNamedAddress("", ptr32), InvalidParameter);
+    EXPECT_THROW(segment.getNamedAddress(""), InvalidParameter);
+    EXPECT_THROW(segment.clearNamedAddress(""), InvalidParameter);
+
+    // Names beginning with _ are not allowed.
+    EXPECT_THROW(segment.setNamedAddress("_foo", ptr32), InvalidParameter);
+    EXPECT_THROW(segment.getNamedAddress("_foo"), InvalidParameter);
+    EXPECT_THROW(segment.clearNamedAddress("_foo"), InvalidParameter);
 
     // we can now get it; the stored value should be intact.
     MemorySegment::NamedAddressResult result =

src/lib/util/tests/random_number_generator_unittest.cc

@@ -20,10 +20,7 @@
 #include <boost/shared_ptr.hpp>
 
 #include <iostream>
-#include <climits>
 
-#include <sys/types.h>
-#include <unistd.h>
 
 namespace isc {
 namespace util {
@@ -87,21 +84,6 @@ TEST_F(UniformRandomIntegerGeneratorTest, IntegerRange) {
     ASSERT_EQ(it - numbers.begin(), max() - min() + 1);
 }
 
-TEST_F(UniformRandomIntegerGeneratorTest, withSeed) {
-    // Test that two generators with the same seed return the same
-    // sequence.
-    UniformRandomIntegerGenerator gen1(0, INT_MAX, getpid());
-    vector<int> numbers;
-    for (int i = 0; i < 1024; ++i) {
-        numbers.push_back(gen1());
-    }
-
-    UniformRandomIntegerGenerator gen2(0, INT_MAX, getpid());
-    for (int i = 0; i < 1024; ++i) {
-        EXPECT_EQ(numbers[i], gen2());
-    }
-}
-
 /// \brief Test Fixture Class for weighted random number generator
 class WeightedRandomIntegerGeneratorTest : public ::testing::Test {
 public: