[2850] ZoneWriter updates

src/lib/datasrc/memory/zone_writer.cc

@@ -15,6 +15,9 @@
 #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 <dns/rrclass.h>
 
 #include <memory>
 
@@ -24,65 +27,100 @@ 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) :
+        // 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),
+        data_holder_(segment.getMemorySegment(), rrclass_)
+    {}
+
+    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_;
+    detail::SegmentObjectHolder<ZoneData, dns::RRClass> data_holder_;
+};
+
 ZoneWriter::ZoneWriter(ZoneTableSegment& segment,
                        const LoadAction& load_action,
                        const dns::Name& origin,
                        const dns::RRClass& rrclass) :
-    segment_(segment),
-    load_action_(load_action),
-    origin_(origin),
-    rrclass_(rrclass),
-    zone_data_(NULL),
-    state_(ZW_UNUSED)
+    impl_(new Impl(segment, load_action, origin, rrclass))
 {
-    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() {
-    if (state_ != ZW_UNUSED) {
+    if (impl_->state_ != Impl::ZW_UNUSED) {
         isc_throw(isc::InvalidOperation, "Trying to load twice");
     }
 
-    zone_data_ = load_action_(segment_.getMemorySegment());
-    if (!zone_data_) {
+    ZoneData* zone_data =
+        impl_->load_action_(impl_->segment_.getMemorySegment());
+    if (!zone_data) {
         // Bug inside load_action_.
         isc_throw(isc::InvalidOperation, "No data returned from load action");
     }
+    impl_->data_holder_.set(zone_data);
 
-    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");
     }
 
     // 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");
             }
-            const ZoneTable::AddResult result(table->addZone(
-                                                  segment_.getMemorySegment(),
-                                                  rrclass_, origin_,
-                                                  zone_data_));
-            state_ = ZW_INSTALLED;
-            zone_data_ = result.zone_data;
+            // 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(
+                table->addZone(impl_->segment_.getMemorySegment(),
+                               impl_->rrclass_, impl_->origin_,
+                               impl_->data_holder_.get()));
+            impl_->data_holder_.set(result.zone_data);
+            impl_->state_ = Impl::ZW_INSTALLED;
         } catch (const isc::util::MemorySegmentGrown&) {}
     }
 }
@@ -91,10 +129,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
     ///
@@ -100,18 +101,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_;
+    // 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_;
 };
 
 }