From: JINMEI Tatuya Date: Tue, 12 Mar 2013 03:27:09 +0000 (-0700) Subject: [2831] handle the case where setNamedAddress failed due to short space. X-Git-Tag: bind10-1.2.0beta1-release~457^2~113 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a2ea0fb9c996d174936122809275cc464cd2e17c;p=thirdparty%2Fkea.git [2831] handle the case where setNamedAddress failed due to short space. also clarify these in doc. --- diff --git a/src/lib/util/memory_segment.h b/src/lib/util/memory_segment.h index e683c839e1..d224c6997c 100644 --- a/src/lib/util/memory_segment.h +++ b/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 std::bad_alloc 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: diff --git a/src/lib/util/memory_segment_local.cc b/src/lib/util/memory_segment_local.cc index 787c9fae7e..1727a3d736 100644 --- a/src/lib/util/memory_segment_local.cc +++ b/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 diff --git a/src/lib/util/memory_segment_local.h b/src/lib/util/memory_segment_local.h index c056c6097e..6367c77afd 100644 --- a/src/lib/util/memory_segment_local.h +++ b/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: diff --git a/src/lib/util/memory_segment_mapped.cc b/src/lib/util/memory_segment_mapped.cc index 9a685f8c3c..38637df67a 100644 --- a/src/lib/util/memory_segment_mapped.cc +++ b/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* storage = - impl_->base_sgmt_->find_or_construct >(name)(); - *storage = addr; + + bool grown = false; + while (true) { + offset_ptr* storage = + impl_->base_sgmt_->find_or_construct >( + name, std::nothrow)(); + if (storage) { + *storage = addr; + return (grown); + } + + impl_->growSegment(); + grown = true; + } } bool diff --git a/src/lib/util/memory_segment_mapped.h b/src/lib/util/memory_segment_mapped.h index a029a0e34a..1de8bbc5ed 100644 --- a/src/lib/util/memory_segment_mapped.h +++ b/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); diff --git a/src/lib/util/tests/memory_segment_common_unittest.cc b/src/lib/util/tests/memory_segment_common_unittest.cc index d68ce8bc66..2ffdb7c678 100644 --- a/src/lib/util/tests/memory_segment_common_unittest.cc +++ b/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(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(0), segment.getNamedAddress("null address")); // If the underlying implementation performs explicit check against diff --git a/src/lib/util/tests/memory_segment_mapped_unittest.cc b/src/lib/util/tests/memory_segment_mapped_unittest.cc index 80e4250ae1..9cec26490a 100644 --- a/src/lib/util/tests/memory_segment_mapped_unittest.cc +++ b/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( 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(0), + segment_->getNamedAddress(long_name.c_str())); } TEST_F(MemorySegmentMappedTest, nullDeallocate) {