[2831] handle the case where setNamedAddress failed due to short space.

also clarify these in doc.

src/lib/util/memory_segment.h

@@ -51,11 +51,30 @@ public:
     /// \brief Destructor
     virtual ~MemorySegment() {}
 
-    /// \brief Allocate/acquire a segment of memory. The source of the
-    /// memory is dependent on the implementation used.
+    /// \brief Allocate/acquire a segment of memory.
     ///
-    /// Throws <code>std::bad_alloc</code> if the implementation cannot
-    /// allocate the requested storage.
+    /// The source of the memory is dependent on the implementation used.
+    ///
+    /// Depending on the implementation details, it may have to grow the
+    /// internal memory segment (again, in an implementation dependent way)
+    /// to allocate the required size of memory.  In that case the
+    /// implementation must grow the internal segment sufficiently so the
+    /// next call to allocate() for the same size will succeed and throw
+    /// an \c MemorySegmentGrown exception (not really allocating the memory
+    /// yet).
+    ///
+    /// An application that uses this memory segment abstraction to allocate
+    /// memory should expect this exception, and should normally catch it
+    /// at an appropriate layer (which may be immediately after a call to
+    /// \c allocate() or a bit higher layer).  It should interpret the
+    /// exception as any raw address that belongs to the segment may have
+    /// been remapped and must be re-fetched via an already established
+    /// named address using the \c getNamedAddress() method.
+    ///
+    /// \throw std::bad_alloc The implementation cannot allocate the
+    /// requested storage.
+    /// \throw MemorySegmentGrown The memory segment doesn't have sufficient
+    /// space for the requested size and has grown internally.
     ///
     /// \param size The size of the memory requested in bytes.
     /// \return Returns pointer to the memory allocated.
@@ -101,9 +120,29 @@ public:
     /// it's not an API requirement.  It's generally the caller's
     /// responsibility to meet the restriction.
     ///
+    /// There can be an existing association for the name; in that case the
+    /// association will be overridden with the newly given address.
+    ///
+    /// While normally unexpected, it's possible that available space in the
+    /// segment is not sufficient to allocate a space (if not already exist)
+    /// for the specified name in the segment.  In that case, if possible, the
+    /// implementation should try to grow the internal segment and retry
+    /// establishing the association.  The implementation should throw
+    /// std::bad_alloc if even reasonable attempts of retry still fail.
+    ///
+    /// This method should normally return false, but if the internal segment
+    /// had to grow to store the given name, it must return true.  The
+    /// application should interpret it just like the case of
+    /// \c MemorySegmentGrown exception thrown from the \c allocate() method.
+    ///
+    /// \throw std::bad_alloc Allocation of a segment space for the given name
+    /// failed.
+    ///
     /// \param name A C string to be associated with \c addr.
     /// \param addr A memory address returned by a prior call to \c allocate.
-    virtual void setNamedAddress(const char* name, void* addr) = 0;
+    /// \return true if the internal segment has grown to allocate space for
+    /// the name; false otherwise (see above).
+    virtual bool setNamedAddress(const char* name, void* addr) = 0;
 
     /// \brief Return the address in the segment that has the given name.
     ///
@@ -112,6 +151,12 @@ public:
     /// associated by a prior call to \c setNameAddress().  If no address
     /// associated with the given name is found, it returns NULL.
     ///
+    /// 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.
+    ///
     /// \param name A C string of which the segment memory address is to be
     /// returned.
     /// \return The address associated with the name, or NULL if not found.
@@ -123,6 +168,11 @@ public:
     /// a corresponding segment address previously established by
     /// \c setNamedAddress().  If there is no association for the given name
     /// this method returns false; otherwise it returns true.
+    ///
+    /// See \c getNamedAddress() about exception consideration.
+    ///
+    /// \param name A C string of which the segment memory address is to be
+    /// deleted.
     virtual bool clearNamedAddress(const char* name) = 0;
 
     virtual void shrinkToFit() = 0;
@@ -132,3 +182,7 @@ public:
 } // namespace isc
 
 #endif // MEMORY_SEGMENT_H
+
+// Local Variables:
+// mode: c++
+// End:

src/lib/util/memory_segment_local.cc

@@ -60,9 +60,10 @@ MemorySegmentLocal::getNamedAddress(const char* name) {
     return (0);
 }
 
-void
+bool
 MemorySegmentLocal::setNamedAddress(const char* name, void* addr) {
     named_addrs_[name] = addr;
+    return (false);
 }
 
 bool

src/lib/util/memory_segment_local.h

@@ -67,7 +67,7 @@ public:
     virtual bool allMemoryDeallocated() const;
 
     virtual void* getNamedAddress(const char* name);
-    virtual void setNamedAddress(const char* name, void* addr);
+    virtual bool setNamedAddress(const char* name, void* addr);
     virtual bool clearNamedAddress(const char* name);
 
     /// There's nothing this implementation can do for this.
@@ -86,3 +86,7 @@ private:
 } // namespace isc
 
 #endif // MEMORY_SEGMENT_LOCAL_H
+
+// Local Variables:
+// mode: c++
+// End:

src/lib/util/memory_segment_mapped.cc

@@ -46,6 +46,33 @@ struct MemorySegmentMapped::Impl {
                    new managed_mapped_file(open_only, filename.c_str()))
     {}
 
+    // Internal helper to grow the underlying mapped segment.
+    void growSegment() {
+        // We first need to unmap it before calling grow().
+        const size_t prev_size = base_sgmt_->get_size();
+        base_sgmt_.reset();
+
+        const size_t new_size = prev_size * 2;
+        assert(new_size != 0); // assume grow fails before size overflow
+
+        if (!managed_mapped_file::grow(filename_.c_str(),
+                                       new_size - prev_size))
+        {
+            throw std::bad_alloc();
+        }
+
+        try {
+            // Remap the grown file; this should succeed, but it's not 100%
+            // guaranteed.  If it fails we treat it as if we fail to create
+            // the new segment.
+            base_sgmt_.reset(new managed_mapped_file(open_only,
+                                                     filename_.c_str()));
+        } catch (const boost::interprocess::interprocess_exception& ex) {
+            throw std::bad_alloc();
+        }
+    }
+
+    // remember if the segment is opened read-only or not
     const bool read_only_;
 
     // mapped file; remember it in case we need to grow it.
@@ -107,28 +134,7 @@ MemorySegmentMapped::allocate(size_t size) {
     // Grow the mapped segment doubling the size until we have sufficient
     // free memory in the revised segment for the requested size.
     do {
-        // We first need to unmap it before calling grow().
-        const size_t prev_size = impl_->base_sgmt_->get_size();
-        impl_->base_sgmt_.reset();
-
-        const size_t new_size = prev_size * 2;
-        assert(new_size != 0); // assume grow fails before size overflow
-
-        if (!managed_mapped_file::grow(impl_->filename_.c_str(),
-                                       new_size - prev_size))
-        {
-            throw std::bad_alloc();
-        }
-
-        try {
-            // Remap the grown file; this should succeed, but it's not 100%
-            // guaranteed.  If it fails we treat it as if we fail to create
-            // the new segment.
-            impl_->base_sgmt_.reset(
-                new managed_mapped_file(open_only, impl_->filename_.c_str()));
-        } catch (const boost::interprocess::interprocess_exception& ex) {
-            throw std::bad_alloc();
-        }
+        impl_->growSegment();
     } while (impl_->base_sgmt_->get_free_memory() < size);
     isc_throw(MemorySegmentGrown, "mapped memory segment grown, size: "
               << impl_->base_sgmt_->get_size() << ", free size: "
@@ -161,7 +167,7 @@ MemorySegmentMapped::getNamedAddress(const char* name) {
     return (0);
 }
 
-void
+bool
 MemorySegmentMapped::setNamedAddress(const char* name, void* addr) {
     if (impl_->read_only_) {
         isc_throw(InvalidOperation, "setNamedAddress on read-only segment");
@@ -170,9 +176,20 @@ MemorySegmentMapped::setNamedAddress(const char* name, void* addr) {
     if (addr && !impl_->base_sgmt_->belongs_to_segment(addr)) {
         isc_throw(MemorySegmentError, "out of segment address: " << addr);
     }
-    offset_ptr<void>* storage =
-        impl_->base_sgmt_->find_or_construct<offset_ptr<void> >(name)();
-    *storage = addr;
+
+    bool grown = false;
+    while (true) {
+        offset_ptr<void>* storage =
+            impl_->base_sgmt_->find_or_construct<offset_ptr<void> >(
+                name, std::nothrow)();
+        if (storage) {
+            *storage = addr;
+            return (grown);
+        }
+
+        impl_->growSegment();
+        grown = true;
+    }
 }
 
 bool

src/lib/util/memory_segment_mapped.h

@@ -69,7 +69,7 @@ public:
     ///
     /// This implementation detects if \c addr is invalid (see the base class
     /// description) and throws \c MemorySegmentError in that case.
-    virtual void setNamedAddress(const char* name, void* addr);
+    virtual bool setNamedAddress(const char* name, void* addr);
 
     virtual bool clearNamedAddress(const char* name);
 

src/lib/util/tests/memory_segment_common_unittest.cc

@@ -32,7 +32,7 @@ checkSegmentNamedAddress(MemorySegment& segment, bool out_of_segment_ok) {
     void* ptr32 = segment.allocate(sizeof(uint32_t));
     const uint32_t test_val = 42;
     std::memcpy(ptr32, &test_val, sizeof(test_val));
-    segment.setNamedAddress("test address", ptr32);
+    EXPECT_FALSE(segment.setNamedAddress("test address", ptr32));
 
     // we can now get it; the stored value should be intact.
     EXPECT_EQ(ptr32, segment.getNamedAddress("test address"));
@@ -42,7 +42,7 @@ checkSegmentNamedAddress(MemorySegment& segment, bool out_of_segment_ok) {
     void* ptr16 = segment.allocate(sizeof(uint16_t));
     const uint16_t test_val16 = 4200;
     std::memcpy(ptr16, &test_val16, sizeof(test_val16));
-    segment.setNamedAddress("test address", ptr16);
+    EXPECT_FALSE(segment.setNamedAddress("test address", ptr16));
     EXPECT_EQ(ptr16, segment.getNamedAddress("test address"));
     EXPECT_EQ(test_val16, *static_cast<const uint16_t*>(ptr16));
 
@@ -54,7 +54,7 @@ checkSegmentNamedAddress(MemorySegment& segment, bool out_of_segment_ok) {
     EXPECT_FALSE(segment.clearNamedAddress("test address"));
 
     // Setting NULL is okay.
-    segment.setNamedAddress("null address", 0);
+    EXPECT_FALSE(segment.setNamedAddress("null address", 0));
     EXPECT_EQ(static_cast<void*>(0), segment.getNamedAddress("null address"));
 
     // If the underlying implementation performs explicit check against

src/lib/util/tests/memory_segment_mapped_unittest.cc

@@ -184,17 +184,30 @@ TEST_F(MemorySegmentMappedTest, badDeallocate) {
 }
 
 TEST_F(MemorySegmentMappedTest, namedAddress) {
+    // common test cases
     isc::util::test::checkSegmentNamedAddress(*segment_, false);
 
     // Set it again and read it in the read-only mode.
     void* ptr16 = segment_->allocate(sizeof(uint16_t));
     const uint16_t test_val16 = 42000;
     std::memcpy(ptr16, &test_val16, sizeof(test_val16));
-    segment_->setNamedAddress("test address", ptr16);
+    EXPECT_FALSE(segment_->setNamedAddress("test address", ptr16));
     MemorySegmentMapped segment_ro(mapped_file);
     EXPECT_TRUE(segment_ro.getNamedAddress("test address"));
     EXPECT_EQ(test_val16, *static_cast<const uint16_t*>(
                   segment_ro.getNamedAddress("test address")));
+
+    // try to set an unusually long name.  We re-create the file so the
+    // creating the name would cause allocation failure and trigger internal
+    // segment extension.
+    segment_.reset();
+    boost::interprocess::file_mapping::remove(mapped_file);
+    segment_.reset(new MemorySegmentMapped(mapped_file, true, 1024));
+    const std::string long_name(1025, 'x'); // definitely larger than segment
+    // setNamedAddress should return true, indicating segment has grown.
+    EXPECT_TRUE(segment_->setNamedAddress(long_name.c_str(), 0));
+    EXPECT_EQ(static_cast<void*>(0),
+              segment_->getNamedAddress(long_name.c_str()));
 }
 
 TEST_F(MemorySegmentMappedTest, nullDeallocate) {