[2850] Make getZoneWriter() non-virtual

This also makes ZoneWriter a concrete class (and removes
ZoneWriterLocal).

src/lib/datasrc/memory/Makefile.am

@@ -25,8 +25,7 @@ libdatasrc_memory_la_SOURCES += zone_table_segment_local.h zone_table_segment_lo
 libdatasrc_memory_la_SOURCES += zone_data_updater.h zone_data_updater.cc
 libdatasrc_memory_la_SOURCES += zone_data_loader.h zone_data_loader.cc
 libdatasrc_memory_la_SOURCES += memory_client.h memory_client.cc
-libdatasrc_memory_la_SOURCES += zone_writer.h
-libdatasrc_memory_la_SOURCES += zone_writer_local.h zone_writer_local.cc
+libdatasrc_memory_la_SOURCES += zone_writer.h zone_writer.cc
 libdatasrc_memory_la_SOURCES += load_action.h
 libdatasrc_memory_la_SOURCES += util_internal.h
 

src/lib/datasrc/memory/zone_table_segment.cc

@@ -14,6 +14,7 @@
 
 #include <datasrc/memory/zone_table_segment.h>
 #include <datasrc/memory/zone_table_segment_local.h>
+#include <datasrc/memory/zone_writer.h>
 
 #include <string>
 
@@ -40,6 +41,14 @@ ZoneTableSegment::destroy(ZoneTableSegment *segment) {
     delete segment;
 }
 
+ZoneWriter*
+ZoneTableSegment::getZoneWriter(const LoadAction& load_action,
+                                const dns::Name& name,
+                                const dns::RRClass& rrclass)
+{
+    return (new ZoneWriter(this, load_action, name, rrclass));
+}
+
 } // namespace memory
 } // namespace datasrc
 } // namespace isc

src/lib/datasrc/memory/zone_table_segment.h

@@ -128,9 +128,9 @@ public:
     /// \param segment The segment to destroy.
     static void destroy(ZoneTableSegment* segment);
 
-    /// \brief Create a zone write corresponding to this segment
+    /// \brief Create a zone writer
     ///
-    /// This creates a new write that can be used to update zones
+    /// This creates a new writer that can be used to update zones
     /// inside this zone table segment.
     ///
     /// \param loadAction Callback to provide the actual data.
@@ -139,9 +139,9 @@ public:
     /// \return New instance of a zone writer. The ownership is passed
     ///     onto the caller and the caller needs to \c delete it when
     ///     it's done with the writer.
-    virtual ZoneWriter* getZoneWriter(const LoadAction& load_action,
-                                      const dns::Name& origin,
-                                      const dns::RRClass& rrclass) = 0;
+    ZoneWriter* getZoneWriter(const LoadAction& load_action,
+                              const dns::Name& origin,
+                              const dns::RRClass& rrclass);
 };
 
 } // namespace memory

src/lib/datasrc/memory/zone_table_segment_local.cc

@@ -13,7 +13,6 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <datasrc/memory/zone_table_segment_local.h>
-#include "zone_writer_local.h"
 
 using namespace isc::dns;
 using namespace isc::util;
@@ -56,14 +55,6 @@ ZoneTableSegmentLocal::getMemorySegment() {
      return (mem_sgmt_);
 }
 
-ZoneWriter*
-ZoneTableSegmentLocal::getZoneWriter(const LoadAction& load_action,
-                                     const dns::Name& name,
-                                     const dns::RRClass& rrclass)
-{
-    return (new ZoneWriterLocal(this, load_action, name, rrclass));
-}
-
 } // namespace memory
 } // namespace datasrc
 } // namespace isc

src/lib/datasrc/memory/zone_table_segment_local.h

@@ -53,10 +53,6 @@ public:
     /// implementation (a MemorySegmentLocal instance).
     virtual isc::util::MemorySegment& getMemorySegment();
 
-    /// \brief Concrete implementation of ZoneTableSegment::getZoneWriter
-    virtual ZoneWriter* getZoneWriter(const LoadAction& load_action,
-                                      const dns::Name& origin,
-                                      const dns::RRClass& rrclass);
 private:
     isc::util::MemorySegmentLocal mem_sgmt_;
     ZoneTableHeader header_;

src/lib/datasrc/memory/zone_writer_local.cc

@@ -12,9 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "zone_writer_local.h"
+#include "zone_writer.h"
 #include "zone_data.h"
-#include "zone_table_segment_local.h"
 
 #include <memory>
 
@@ -24,10 +23,10 @@ namespace isc {
 namespace datasrc {
 namespace memory {
 
-ZoneWriterLocal::ZoneWriterLocal(ZoneTableSegmentLocal* segment,
-                                 const LoadAction& load_action,
-                                 const dns::Name& origin,
-                                 const dns::RRClass& rrclass) :
+ZoneWriter::ZoneWriter(ZoneTableSegment* segment,
+                       const LoadAction& load_action,
+                       const dns::Name& origin,
+                       const dns::RRClass& rrclass) :
     segment_(segment),
     load_action_(load_action),
     origin_(origin),
@@ -36,14 +35,14 @@ ZoneWriterLocal::ZoneWriterLocal(ZoneTableSegmentLocal* segment,
     state_(ZW_UNUSED)
 {}
 
-ZoneWriterLocal::~ZoneWriterLocal() {
+ZoneWriter::~ZoneWriter() {
     // Clean up everything there might be left if someone forgot, just
     // in case.
     cleanup();
 }
 
 void
-ZoneWriterLocal::load() {
+ZoneWriter::load() {
     if (state_ != ZW_UNUSED) {
         isc_throw(isc::InvalidOperation, "Trying to load twice");
     }
@@ -59,7 +58,7 @@ ZoneWriterLocal::load() {
 }
 
 void
-ZoneWriterLocal::install() {
+ZoneWriter::install() {
     if (state_ != ZW_LOADED) {
         isc_throw(isc::InvalidOperation, "No data to install");
     }
@@ -78,7 +77,7 @@ ZoneWriterLocal::install() {
 }
 
 void
-ZoneWriterLocal::cleanup() {
+ZoneWriter::cleanup() {
     // We eat the data (if any) now.
 
     if (zone_data_ != NULL) {

src/lib/datasrc/memory/zone_writer.h

@@ -15,30 +15,43 @@
 #ifndef MEM_ZONE_WRITER_H
 #define MEM_ZONE_WRITER_H
 
+#include "zone_table_segment.h"
 #include "load_action.h"
 
+#include <dns/rrclass.h>
+#include <dns/name.h>
+
 namespace isc {
 namespace datasrc {
 namespace memory {
 
 /// \brief Does an update to a zone.
 ///
-/// This abstract base class represents the work of a reload of a zone.
-/// The work is divided into three stages -- load(), install() and cleanup().
-/// They should be called in this order for the effect to take place.
+/// This represents the work of a reload of a zone.  The work is divided
+/// into three stages -- load(), install() and cleanup().  They should
+/// be called in this order for the effect to take place.
 ///
 /// We divide them so the update of zone data can be done asynchronously,
 /// in a different thread. The install() operation is the only one that needs
 /// to be done in a critical section.
 ///
-/// Each derived class implementation must provide the strong exception
-/// guarantee for each public method. That is, when any of the methods
-/// throws, the entire state should stay the same as before the call
-/// (how to achieve that may be implementation dependant).
+/// 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 {
 public:
-    /// \brief Virtual destructor.
-    virtual ~ZoneWriter() {};
+    /// \brief Constructor
+    ///
+    /// \param segment The zone table segment to store the zone into.
+    /// \param load_action The callback used to load data.
+    /// \param install_action The callback used to install the loaded zone.
+    /// \param rrclass The class of the zone.
+    ZoneWriter(ZoneTableSegment* segment,
+               const LoadAction& load_action, const dns::Name& name,
+               const dns::RRClass& rrclass);
+
+    /// \brief Destructor.
+    ~ZoneWriter();
 
     /// \brief Get the zone data into memory.
     ///
@@ -56,7 +69,7 @@ public:
     /// \note After successful load(), you have to call cleanup() some time
     ///     later.
     /// \throw isc::InvalidOperation if called second time.
-    virtual void load() = 0;
+    void load();
 
     /// \brief Put the changes to effect.
     ///
@@ -68,12 +81,12 @@ public:
     /// The operation is expected to be fast and is meant to be used inside
     /// a critical section.
     ///
-    /// This may throw in rare cases, depending on the concrete implementation.
-    /// If it throws, you still need to call cleanup().
+    /// This may throw in rare cases.  If it throws, you still need to
+    /// call cleanup().
     ///
     /// \throw isc::InvalidOperation if called without previous load() or for
     ///     the second time or cleanup() was called already.
-    virtual void install() = 0;
+    void install();
 
     /// \brief Clean up resources.
     ///
@@ -82,7 +95,21 @@ public:
     /// successful, or the one replaced in install().
     ///
     /// Generally, this should never throw.
-    virtual void cleanup() = 0;
+    void cleanup();
+
+private:
+    ZoneTableSegment* segment_;
+    LoadAction load_action_;
+    dns::Name origin_;
+    dns::RRClass rrclass_;
+    ZoneData* zone_data_;
+    enum State {
+        ZW_UNUSED,
+        ZW_LOADED,
+        ZW_INSTALLED,
+        ZW_CLEANED
+    };
+    State state_;
 };
 
 }

src/lib/datasrc/memory/zone_writer_local.h

@@ -1,95 +0,0 @@
-// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
-// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
-// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
-// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-// PERFORMANCE OF THIS SOFTWARE.
-
-#ifndef MEM_ZONE_WRITER_LOCAL_H
-#define MEM_ZONE_WRITER_LOCAL_H
-
-#include "zone_writer.h"
-
-#include <dns/rrclass.h>
-#include <dns/name.h>
-
-namespace isc {
-namespace datasrc {
-namespace memory {
-
-class ZoneData;
-class ZoneTableSegmentLocal;
-
-/// \brief Writer implementation which loads data locally.
-///
-/// This implementation prepares a clean zone data and lets one callback
-/// to fill it and another to install it somewhere. The class does mostly
-/// nothing (and delegates the work to the callbacks), just stores little bit
-/// of state between the calls.
-class ZoneWriterLocal : public ZoneWriter {
-public:
-    /// \brief Constructor
-    ///
-    /// \param segment The zone table segment to store the zone into.
-    /// \param load_action The callback used to load data.
-    /// \param install_action The callback used to install the loaded zone.
-    /// \param rrclass The class of the zone.
-    ZoneWriterLocal(ZoneTableSegmentLocal* segment,
-                    const LoadAction& load_action, const dns::Name& name,
-                    const dns::RRClass& rrclass);
-
-    /// \brief Destructor
-    ~ZoneWriterLocal();
-
-    /// \brief Loads the data.
-    ///
-    /// This calls the load_action (passed to constructor) and stores the
-    /// data for future use.
-    ///
-    /// \throw isc::InvalidOperation if it is called the second time in
-    ///     lifetime of the object.
-    /// \throw Whatever the load_action throws, it is propagated up.
-    virtual void load();
-
-    /// \brief Installs the zone.
-    ///
-    /// It modifies the zone table accessible through the segment (passed to
-    /// constructor).
-    ///
-    /// \throw isc::InvalidOperation if it is called the second time in
-    ///     lifetime of the object or if load() was not called previously or if
-    ///     cleanup() was already called.
-    virtual void install();
-
-    /// \brief Clean up memory.
-    ///
-    /// Cleans up the memory used by load()ed zone if not yet installed, or
-    /// the old zone replaced by install().
-    virtual void cleanup();
-private:
-    ZoneTableSegmentLocal* segment_;
-    LoadAction load_action_;
-    dns::Name origin_;
-    dns::RRClass rrclass_;
-    ZoneData* zone_data_;
-    enum State {
-        ZW_UNUSED,
-        ZW_LOADED,
-        ZW_INSTALLED,
-        ZW_CLEANED
-    };
-    State state_;
-};
-
-}
-}
-}
-
-#endif

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

@@ -18,7 +18,7 @@
 #include <datasrc/memory/zone_table_segment.h>
 #include <datasrc/memory/zone_table.h>
 #include <datasrc/memory/zone_data.h>
-#include <datasrc/memory/zone_writer_local.h>
+#include <datasrc/memory/zone_writer.h>
 
 namespace isc {
 namespace datasrc {
@@ -57,51 +57,12 @@ public:
                                       const dns::Name& name,
                                       const dns::RRClass& rrclass)
     {
-        return (new Writer(this, load_action, name, rrclass));
+        return (new ZoneWriter(this, load_action, name, rrclass));
     }
 
 private:
     isc::util::MemorySegment& mem_sgmt_;
     ZoneTableHeader header_;
-
-    // A writer for this segment. The implementation is similar
-    // to ZoneWriterLocal, but all the error handling is stripped
-    // for simplicity. Also, we do everything inside the
-    // install(), for the same reason. We just need something
-    // inside the tests, not a full-blown implementation
-    // for background loading.
-    class Writer : public ZoneWriter {
-    public:
-        Writer(ZoneTableSegmentTest* segment, const LoadAction& load_action,
-               const dns::Name& name, const dns::RRClass& rrclass) :
-            segment_(segment),
-            load_action_(load_action),
-            name_(name),
-            rrclass_(rrclass)
-        {}
-
-        void load() {}
-
-        void install() {
-            ZoneTable* table(segment_->getHeader().getTable());
-            const ZoneTable::AddResult
-                result(table->addZone(segment_->getMemorySegment(), rrclass_,
-                                      name_,
-                                      load_action_(segment_->
-                                                   getMemorySegment())));
-            if (result.zone_data != NULL) {
-                ZoneData::destroy(segment_->getMemorySegment(),
-                                  result.zone_data, rrclass_);
-            }
-        }
-
-        virtual void cleanup() {}
-    private:
-        ZoneTableSegmentTest* segment_;
-        LoadAction load_action_;
-        dns::Name name_;
-        dns::RRClass rrclass_;
-    };
 };
 
 } // namespace test

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

@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <datasrc/memory/zone_writer_local.h>
+#include <datasrc/memory/zone_writer.h>
 #include <datasrc/memory/zone_table_segment_local.h>
 #include <util/memory_segment_local.h>
 
@@ -92,9 +92,6 @@ TEST_F(ZoneTableSegmentTest, getZoneWriter) {
                                               RRClass::IN()));
     // We have to get something
     EXPECT_NE(static_cast<void*>(NULL), writer.get());
-    // And for now, it should be the local writer
-    EXPECT_NE(static_cast<void*>(NULL),
-              dynamic_cast<ZoneWriterLocal*>(writer.get()));
 }
 
 } // anonymous namespace

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

@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <datasrc/memory/zone_writer_local.h>
+#include <datasrc/memory/zone_writer.h>
 #include <datasrc/memory/zone_table_segment_local.h>
 #include <datasrc/memory/zone_data.h>
 
@@ -34,15 +34,14 @@ namespace {
 
 class TestException {};
 
-class ZoneWriterLocalTest : public ::testing::Test {
+class ZoneWriterTest : public ::testing::Test {
 public:
-    ZoneWriterLocalTest() :
+    ZoneWriterTest() :
         segment_(ZoneTableSegment::create(RRClass::IN(), "local")),
         writer_(new
-            ZoneWriterLocal(dynamic_cast<ZoneTableSegmentLocal*>(segment_.
-                                                                 get()),
-                            bind(&ZoneWriterLocalTest::loadAction, this, _1),
-                            Name("example.org"), RRClass::IN())),
+            ZoneWriter(segment_.get(),
+                       bind(&ZoneWriterTest::loadAction, this, _1),
+                       Name("example.org"), RRClass::IN())),
         load_called_(false),
         load_throw_(false),
         load_null_(false),
@@ -54,7 +53,7 @@ public:
     }
 protected:
     scoped_ptr<ZoneTableSegment> segment_;
-    scoped_ptr<ZoneWriterLocal> writer_;
+    scoped_ptr<ZoneWriter> writer_;
     bool load_called_;
     bool load_throw_;
     bool load_null_;
@@ -88,7 +87,7 @@ private:
 
 // We call it the way we are supposed to, check every callback is called in the
 // right moment.
-TEST_F(ZoneWriterLocalTest, correctCall) {
+TEST_F(ZoneWriterTest, correctCall) {
     // Nothing called before we call it
     EXPECT_FALSE(load_called_);
 
@@ -105,7 +104,7 @@ TEST_F(ZoneWriterLocalTest, correctCall) {
     EXPECT_NO_THROW(writer_->cleanup());
 }
 
-TEST_F(ZoneWriterLocalTest, loadTwice) {
+TEST_F(ZoneWriterTest, loadTwice) {
     // Load it the first time
     EXPECT_NO_THROW(writer_->load());
     EXPECT_TRUE(load_called_);
@@ -126,7 +125,7 @@ TEST_F(ZoneWriterLocalTest, loadTwice) {
 
 // Try loading after call to install and call to cleanup. Both is
 // forbidden.
-TEST_F(ZoneWriterLocalTest, loadLater) {
+TEST_F(ZoneWriterTest, loadLater) {
     // Load first, so we can install
     EXPECT_NO_THROW(writer_->load());
     EXPECT_NO_THROW(writer_->install());
@@ -144,7 +143,7 @@ TEST_F(ZoneWriterLocalTest, loadLater) {
 }
 
 // Try calling install at various bad times
-TEST_F(ZoneWriterLocalTest, invalidInstall) {
+TEST_F(ZoneWriterTest, invalidInstall) {
     // Nothing loaded yet
     EXPECT_THROW(writer_->install(), isc::InvalidOperation);
     EXPECT_FALSE(load_called_);
@@ -161,7 +160,7 @@ TEST_F(ZoneWriterLocalTest, invalidInstall) {
 // We check we can clean without installing first and nothing bad
 // happens. We also misuse the testcase to check we can't install
 // after cleanup.
-TEST_F(ZoneWriterLocalTest, cleanWithoutInstall) {
+TEST_F(ZoneWriterTest, cleanWithoutInstall) {
     EXPECT_NO_THROW(writer_->load());
     EXPECT_NO_THROW(writer_->cleanup());
 
@@ -172,7 +171,7 @@ TEST_F(ZoneWriterLocalTest, cleanWithoutInstall) {
 }
 
 // Test the case when load callback throws
-TEST_F(ZoneWriterLocalTest, loadThrows) {
+TEST_F(ZoneWriterTest, loadThrows) {
     load_throw_ = true;
     EXPECT_THROW(writer_->load(), TestException);
 
@@ -186,7 +185,7 @@ TEST_F(ZoneWriterLocalTest, loadThrows) {
 
 // Check the strong exception guarantee - if it throws, nothing happened
 // to the content.
-TEST_F(ZoneWriterLocalTest, retry) {
+TEST_F(ZoneWriterTest, retry) {
     // First attempt fails due to some exception.
     load_throw_ = true;
     EXPECT_THROW(writer_->load(), TestException);
@@ -214,7 +213,7 @@ TEST_F(ZoneWriterLocalTest, retry) {
 }
 
 // Check the writer defends itsefl when load action returns NULL
-TEST_F(ZoneWriterLocalTest, loadNull) {
+TEST_F(ZoneWriterTest, loadNull) {
     load_null_ = true;
     EXPECT_THROW(writer_->load(), isc::InvalidOperation);
 
@@ -226,7 +225,7 @@ TEST_F(ZoneWriterLocalTest, loadNull) {
 }
 
 // Check the object cleans up in case we forget it.
-TEST_F(ZoneWriterLocalTest, autoCleanUp) {
+TEST_F(ZoneWriterTest, autoCleanUp) {
     // Load data and forget about it. It should get released
     // when the writer itself is destroyed.
     EXPECT_NO_THROW(writer_->load());