From: JINMEI Tatuya Date: Wed, 8 Aug 2012 23:26:28 +0000 (-0700) Subject: [2100] improved exception safety. X-Git-Tag: trac2351_base~115^2~3^2~11 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3e433eb3ea7937dec94549d04053b808f8f3869e;p=thirdparty%2Fkea.git [2100] improved exception safety. --- diff --git a/src/lib/datasrc/memory/tests/zone_table_unittest.cc b/src/lib/datasrc/memory/tests/zone_table_unittest.cc index 03c6b92dfb..73f53c3225 100644 --- a/src/lib/datasrc/memory/tests/zone_table_unittest.cc +++ b/src/lib/datasrc/memory/tests/zone_table_unittest.cc @@ -25,11 +25,34 @@ #include +#include // for bad_alloc + using namespace isc::dns; using namespace isc::datasrc; using namespace isc::datasrc::memory; namespace { +// Memory segment specified for tests. It normally behaves like a "local" +// memory segment. If "throw count" is set to non 0 via setThrowCount(), +// it continues the normal behavior up to the specified number of calls to +// allocate(), and throws an exception at the next call. +class TestMemorySegment : public isc::util::MemorySegmentLocal { +public: + TestMemorySegment() : throw_count_(0) {} + virtual void* allocate(size_t size) { + if (throw_count_ > 0) { + if (--throw_count_ == 0) { + throw std::bad_alloc(); + } + } + return (isc::util::MemorySegmentLocal::allocate(size)); + } + void setThrowCount(size_t count) { throw_count_ = count; } + +private: + size_t throw_count_; +}; + class ZoneTableTest : public ::testing::Test { protected: ZoneTableTest() : zname1(Name("example.com")), @@ -45,13 +68,23 @@ protected: void TearDown() { ZoneTable::destroy(mem_sgmt_, zone_table); zone_table = NULL; - EXPECT_TRUE(mem_sgmt_.allMemoryDeallocated()); + EXPECT_TRUE(mem_sgmt_.allMemoryDeallocated()); // catch any leak here. } const Name zname1, zname2, zname3; - isc::util::MemorySegmentLocal mem_sgmt_; + TestMemorySegment mem_sgmt_; ZoneTable* zone_table; }; +TEST_F(ZoneTableTest, create) { + // Test about creating a zone table. Normal case covers through other + // tests. We only check exception safety by letting the test memory + // segment throw. + mem_sgmt_.setThrowCount(2); + ZoneTable* table; + EXPECT_THROW(table = ZoneTable::create(mem_sgmt_), std::bad_alloc); + // This shouldn't cause memory leak (that would be caught in TearDown()). +} + TEST_F(ZoneTableTest, addZone) { EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1)); EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zname1)); @@ -61,6 +94,13 @@ TEST_F(ZoneTableTest, addZone) { // Add some more different ones. Should just succeed. EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2)); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3)); + + // 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()). + mem_sgmt_.setThrowCount(2); + EXPECT_THROW(zone_table->addZone(mem_sgmt_, Name("example.org")), + std::bad_alloc); } TEST_F(ZoneTableTest, DISABLED_removeZone) { diff --git a/src/lib/datasrc/memory/zone_data.h b/src/lib/datasrc/memory/zone_data.h index 382fb2faaa..61406ff68c 100644 --- a/src/lib/datasrc/memory/zone_data.h +++ b/src/lib/datasrc/memory/zone_data.h @@ -22,10 +22,22 @@ namespace isc { namespace datasrc { namespace memory { class ZoneData { -public: +private: ZoneData(const dns::Name& zone_name) : zone_name_(zone_name) {} +public: + static ZoneData* create(util::MemorySegment& mem_sgmt, + const dns::Name& zone_name) + { + void* p = mem_sgmt.allocate(sizeof(ZoneData)); + ZoneData* zone_data = new(p) ZoneData(zone_name); + return (zone_data); + } + static void destroy(util::MemorySegment& mem_sgmt, ZoneData* zone_data) { + zone_data->~ZoneData(); + mem_sgmt.deallocate(zone_data, sizeof(ZoneData)); + } private: const dns::Name zone_name_; diff --git a/src/lib/datasrc/memory/zone_table.cc b/src/lib/datasrc/memory/zone_table.cc index 1f1554435c..c291b6ef9f 100644 --- a/src/lib/datasrc/memory/zone_table.cc +++ b/src/lib/datasrc/memory/zone_table.cc @@ -28,35 +28,38 @@ using namespace isc::dns; namespace isc { namespace datasrc { namespace memory { - -ZoneTable::ZoneTable(util::MemorySegment& mem_sgmt) : - zones_(ZoneTableTree::create(mem_sgmt)) -{} - namespace { -// A simple holder to make resource allocation for ZoneTable exception safe. -// It works like std::auto_ptr but much more simplified. -struct ZoneTableHolder { - ZoneTableHolder(util::MemorySegment& mem_sgmt, void* ptr) : - mem_sgmt_(mem_sgmt), ptr_(ptr) +// A simple holder to create and use some objects in this implementation +// in an exception safe manner. It works like std::auto_ptr but much +// more simplified. +template +class Holder { +public: + Holder(util::MemorySegment& mem_sgmt, T* obj) : + mem_sgmt_(mem_sgmt), obj_(obj) {} - ~ZoneTableHolder() { - if (ptr_ != NULL) { - mem_sgmt_.deallocate(ptr_, sizeof(ZoneTable)); + ~Holder() { + if (obj_ != NULL) { + T::destroy(mem_sgmt_, obj_); } } - void* get() { return (ptr_); } - void release() { ptr_ = NULL; } + T* get() { return (obj_); } + T* release() { + T* ret = obj_; + obj_ = NULL; + return (ret); + } private: util::MemorySegment& mem_sgmt_; - void* ptr_; + T* obj_; }; } ZoneTable* ZoneTable::create(util::MemorySegment& mem_sgmt) { - ZoneTableHolder holder(mem_sgmt, mem_sgmt.allocate(sizeof(ZoneTable))); - ZoneTable* zone_table = new(holder.get()) ZoneTable(mem_sgmt); + Holder holder(mem_sgmt, ZoneTableTree::create(mem_sgmt)); + void* p = mem_sgmt.allocate(sizeof(ZoneTable)); + ZoneTable* zone_table = new(p) ZoneTable(holder.get()); holder.release(); return (zone_table); } @@ -69,13 +72,13 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable) { result::Result ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name) { - // Create a new ZoneData instance first. If it already exists the new - // one will soon be destroyed, but we want to make sure if this allocation - // fails the tree won't be changed to provide as strong guarantee as - // possible. In practice, we generally expect the caller tries to add - // a zone only when it's a new one, so this should be a minor concern. - void* p = mem_sgmt.allocate(sizeof(ZoneData)); // XXX safety - ZoneData* zone_data = new(p) ZoneData(zone_name); + // Create a new ZoneData instance first. If the specified name already + // exists in the table, the new data will soon be destroyed, but we want + // to make sure if this allocation fails the tree won't be changed to + // provide as strong guarantee as possible. In practice, we generally + // expect the caller tries to add a zone only when it's a new one, so + // this should be a minor concern. + Holder holder(mem_sgmt, ZoneData::create(mem_sgmt, zone_name)); // Get the node where we put the zone ZoneTableNode* node(NULL); @@ -93,10 +96,9 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name) { // Is it empty? We either just created it or it might be nonterminal if (node->isEmpty()) { - node->setData(mem_sgmt, zone_data); + node->setData(mem_sgmt, holder.release()); return (result::SUCCESS); } else { // There's something there already - mem_sgmt.deallocate(p, sizeof(ZoneData)); return (result::EXIST); } } diff --git a/src/lib/datasrc/memory/zone_table.h b/src/lib/datasrc/memory/zone_table.h index 7db5cd2e41..ed28f43d92 100644 --- a/src/lib/datasrc/memory/zone_table.h +++ b/src/lib/datasrc/memory/zone_table.h @@ -45,6 +45,20 @@ class ZoneData; /// For more descriptions about its struct and interfaces, please refer to the /// corresponding struct and interfaces of \c MemoryDataSrc. class ZoneTable { +private: + struct ZoneDataDeleter { + ZoneDataDeleter() {} + void operator()(util::MemorySegment& mem_sgmt, + ZoneData* zone_data) const + { + mem_sgmt.deallocate(zone_data, sizeof(ZoneData)); + } + }; + + // Type aliases to make it shorter + typedef DomainTree ZoneTableTree; + typedef DomainTreeNode ZoneTableNode; + public: struct FindResult { FindResult(result::Result param_code, const ZoneFinderPtr param_zone) : @@ -72,7 +86,8 @@ private: /// This constructor internally involves resource allocation, and if /// it fails, a corresponding standard exception will be thrown. /// It never throws an exception otherwise. - ZoneTable(util::MemorySegment& mem_sgmt); + ZoneTable(ZoneTableTree* zones) : zones_(zones) + {} //@} public: @@ -143,19 +158,6 @@ public: FindResult findZone(const isc::dns::Name& name) const; private: - struct ZoneDataDeleter { - ZoneDataDeleter() {} - void operator()(util::MemorySegment& mem_sgmt, - ZoneData* zone_data) const - { - mem_sgmt.deallocate(zone_data, sizeof(ZoneData)); - } - }; - - // Type aliases to make it shorter - typedef DomainTree ZoneTableTree; - typedef DomainTreeNode ZoneTableNode; - ZoneTableTree* zones_; }; }