[2376] Split off the add callback

So the issue callbacks don't depend on other things.

src/lib/datasrc/master_loader_callbacks.cc

@@ -51,25 +51,22 @@ logWarning(const isc::dns::Name& name, const isc::dns::RRClass& rrclass,
 }
 
 isc::dns::MasterLoaderCallbacks
-createMasterLoaderCallbacks(ZoneUpdater& updater, const isc::dns::Name& name,
+createMasterLoaderCallbacks(const isc::dns::Name& name,
                             const isc::dns::RRClass& rrclass, bool* ok)
 {
     return (isc::dns::MasterLoaderCallbacks(boost::bind(&logError, name,
                                                         rrclass, ok, _1, _2,
                                                         _3),
                                             boost::bind(&logWarning, name,
-                                                        rrclass, _1, _2, _3),
-                                            boost::bind(&ZoneUpdater::addRRset,
-                                                        &updater,
-                                                        // The callback provides
-                                                        // a shared pointer, we
-                                                        // need the object. This
-                                                        // bind unpacks the
-                                                        // object.
-                                                        boost::bind(&isc::dns::
-                                                                    RRsetPtr::
-                                                                    operator*,
-                                                                    _1))));
+                                                        rrclass, _1, _2, _3)));
+}
+
+isc::dns::AddRRsetCallback
+createMasterLoaderAddCallback(ZoneUpdater& updater) {
+    return (boost::bind(&ZoneUpdater::addRRset, &updater,
+                        // The callback provides a shared pointer, we
+                        // need the object. This bind unpacks the object.
+                        boost::bind(&isc::dns::RRsetPtr::operator*, _1)));
 }
 
 }

src/lib/datasrc/master_loader_callbacks.h

@@ -22,18 +22,11 @@ namespace datasrc {
 
 class ZoneUpdater;
 
-/// \brief Create callbacks to fill loaded zone into updater.
+/// \brief Create issue callbacks for MasterLoader
 ///
 /// This will create set of callbacks for the MasterLoader that
-/// will fill the loaded rrsets into a zone updater. If any issues
-/// are found, it logs them. If any of the issues are errors and
-/// the ok parameter is non-NULL, it is set to false.
+/// will be used to report any issues found in the zone data.
 ///
-/// \param updater The zone updater used as the destination for the
-///     RRsets. It should be opened in the replace mode and should
-///     be clean (no changes done to it). It is not commited
-///     automatically and it is up to the caller to commit (or not)
-///     when the loading is done.
 /// \param name Name of the zone. Used in logging.
 /// \param rrclass The class of the zone. Used in logging.
 /// \param ok If this is non-NULL and there are any errors during
@@ -41,9 +34,29 @@ class ZoneUpdater;
 /// \return Set of callbacks to be passed to the master loader.
 /// \throw std::bad_alloc when allocation fails.
 isc::dns::MasterLoaderCallbacks
-createMasterLoaderCallbacks(ZoneUpdater& updater, const isc::dns::Name& name,
+createMasterLoaderCallbacks(const isc::dns::Name& name,
                             const isc::dns::RRClass& rrclass, bool* ok);
 
+/// \brief Create a callback for MasterLoader to add RRsets.
+///
+/// This creates a callback that can be used by the MasterLoader to add
+/// loaded RRsets into a zone updater.
+///
+/// The zone updater should be opened in the replace mode no changes should
+/// have been done to it yet (but it is not checked). It is not commited
+/// automatically and it is up to the caller to commit the changes (or not).
+/// It must not be destroyed for the whole time of loading.
+///
+/// The function is mostly straightforward packing of the updater.addRRset
+/// into a boost::function, it is defined explicitly due to small technical
+/// annoyences around boost::bind application, so it can be reused.
+///
+/// \param updater The zone updater to use.
+/// \return The callback to be passed to MasterLoader.
+/// \throw std::bad_alloc when allocation fails.
+isc::dns::AddRRsetCallback
+createMasterLoaderAddCallback(ZoneUpdater& updater);
+
 }
 }
 

src/lib/datasrc/tests/master_loader_callbacks_test.cc

@@ -63,8 +63,7 @@ class MasterLoaderCallbackTest : public ::testing::Test {
 protected:
     MasterLoaderCallbackTest() :
         ok_(true),
-        callbacks_(createMasterLoaderCallbacks(updater_,
-                                               isc::dns::Name("example.org"),
+        callbacks_(createMasterLoaderCallbacks(isc::dns::Name("example.org"),
                                                isc::dns::RRClass::IN(), &ok_))
     {}
     // Generate a new RRset, put it to the updater and return it.
@@ -87,7 +86,7 @@ protected:
 
 // Check it doesn't crash if we don't provide the OK
 TEST_F(MasterLoaderCallbackTest, noOkProvided) {
-    createMasterLoaderCallbacks(updater_, isc::dns::Name("example.org"),
+    createMasterLoaderCallbacks(isc::dns::Name("example.org"),
                                 isc::dns::RRClass::IN(), NULL).
         error("No source", 1, "No reason");
 }
@@ -113,22 +112,13 @@ TEST_F(MasterLoaderCallbackTest, callbacks) {
 
 // Try adding some RRsets.
 TEST_F(MasterLoaderCallbackTest, addRRset) {
+    isc::dns::AddRRsetCallback
+        callback(createMasterLoaderAddCallback(updater_));
     // Put some of them in.
-    EXPECT_NO_THROW(callbacks_.addRRset(generateRRset()));
-    EXPECT_NO_THROW(callbacks_.addRRset(generateRRset()));
+    EXPECT_NO_THROW(callback(generateRRset()));
+    EXPECT_NO_THROW(callback(generateRRset()));
     // They all get pushed there right away, so there are none in the queue
     EXPECT_TRUE(updater_.expected_rrsets_.empty());
-
-    // Making the status not OK by an error does not stop the RRsets to get
-    // through.
-    callbacks_.error("No source", 1, "Just an error");
-    EXPECT_FALSE(ok_);
-
-    EXPECT_NO_THROW(callbacks_.addRRset(generateRRset()));
-    EXPECT_NO_THROW(callbacks_.addRRset(generateRRset()));
-    // They got through and the OK status didn't get reset
-    EXPECT_TRUE(updater_.expected_rrsets_.empty());
-    EXPECT_FALSE(ok_);
 }
 
 }

src/lib/dns/master_loader_callbacks.h

@@ -25,7 +25,18 @@
 namespace isc {
 namespace dns {
 
-/// \brief Set of callbacks for a loader.
+/// \brief Type of callback to add a RRset.
+///
+/// This type of callback is used by the loader to report another loaded
+/// RRset. The RRset is no longer preserved by the loader and is fully
+/// owned by the callback.
+///
+/// \param RRset The rrset to add. It does not contain the accompanying
+///     RRSIG (if the zone is signed), they are reported with separate
+///     calls to the callback.
+typedef boost::function<void(const RRsetPtr& rrset)> AddRRsetCallback;
+
+/// \brief Set of issue callbacks for a loader.
 ///
 /// This holds a set of callbacks by which a loader (such as MasterLoader)
 /// can report loaded RRsets, errors and other unusual conditions.
@@ -47,33 +58,19 @@ public:
                                  size_t source_line,
                                  const std::string& reason)> IssueCallback;
 
-    /// \brief Type of callback to add a RRset.
-    ///
-    /// This type of callback is used by the loader to report another loaded
-    /// RRset. The RRset is no longer preserved by the loader and is fully
-    /// owned by the callback.
-    ///
-    /// \param RRset The rrset to add. It does not contain the accompanying
-    ///     RRSIG (if the zone is signed), they are reported with separate
-    ///     calls to the callback.
-    typedef boost::function<void(const RRsetPtr& rrset)> AddCallback;
-
     /// \brief Constructor
     ///
     /// Initializes the callbacks.
     ///
     /// \param error The error callback to use.
     /// \param warning The warning callback to use.
-    /// \param add The add callback to use.
     /// \throw isc::InvalidParameter if any of the callbacks is empty.
     MasterLoaderCallbacks(const IssueCallback& error,
-                          const IssueCallback& warning,
-                          const AddCallback& add) :
+                          const IssueCallback& warning) :
         error_(error),
-        warning_(warning),
-        add_(add)
+        warning_(warning)
     {
-        if (error_.empty() || warning_.empty() || add_.empty()) {
+        if (error_.empty() || warning_.empty()) {
             isc_throw(isc::InvalidParameter,
                       "Empty function passed as callback");
         }
@@ -113,16 +110,8 @@ public:
         warning_(source_name, source_line, reason);
     }
 
-    /// \brief Call the add callback.
-    ///
-    /// This is called for each loaded RRset.
-    void addRRset(const RRsetPtr& rrset) {
-        add_(rrset);
-    }
-
 private:
     IssueCallback error_, warning_;
-    AddCallback add_;
 };
 
 }

src/lib/dns/tests/master_loader_callbacks_test.cc

@@ -31,15 +31,13 @@ class MasterLoaderCallbacksTest : public ::testing::Test {
 protected:
     MasterLoaderCallbacksTest() :
         issue_called_(false),
-        add_called_(false),
         rrset_(new RRset(Name("example.org"), RRClass::IN(), RRType::A(),
                          RRTTL(3600))),
         error_(boost::bind(&MasterLoaderCallbacksTest::checkCallback, this,
                            true, _1, _2, _3)),
         warning_(boost::bind(&MasterLoaderCallbacksTest::checkCallback, this,
                              false, _1, _2, _3)),
-        add_(boost::bind(&MasterLoaderCallbacksTest::checkAdd, this, _1)),
-        callbacks_(error_, warning_, add_)
+        callbacks_(error_, warning_)
     {}
 
     void checkCallback(bool error, const string& source, size_t line,
@@ -51,29 +49,21 @@ protected:
         EXPECT_EQ(1, line);
         EXPECT_EQ("reason", reason);
     }
-    void checkAdd(const RRsetPtr& rrset) {
-        add_called_ = true;
-        EXPECT_EQ(rrset_, rrset);
-    }
     bool last_was_error_;
-    bool issue_called_, add_called_;
+    bool issue_called_;
     const RRsetPtr rrset_;
     const MasterLoaderCallbacks::IssueCallback error_, warning_;
-    const MasterLoaderCallbacks::AddCallback add_;
     MasterLoaderCallbacks callbacks_;
 };
 
 // Check the constructor rejects empty callbacks, but accepts non-empty ones
 TEST_F(MasterLoaderCallbacksTest, constructor) {
     EXPECT_THROW(MasterLoaderCallbacks(MasterLoaderCallbacks::IssueCallback(),
-                                       warning_, add_), isc::InvalidParameter);
+                                       warning_), isc::InvalidParameter);
     EXPECT_THROW(MasterLoaderCallbacks(error_,
-                                       MasterLoaderCallbacks::IssueCallback(),
-                                       add_), isc::InvalidParameter);
-    EXPECT_THROW(MasterLoaderCallbacks(error_, warning_,
-                                       MasterLoaderCallbacks::AddCallback()),
+                                       MasterLoaderCallbacks::IssueCallback()),
                  isc::InvalidParameter);
-    EXPECT_NO_THROW(MasterLoaderCallbacks(error_, warning_, add_));
+    EXPECT_NO_THROW(MasterLoaderCallbacks(error_, warning_));
 }
 
 // Call the issue callbacks
@@ -89,12 +79,4 @@ TEST_F(MasterLoaderCallbacksTest, issueCall) {
     EXPECT_TRUE(issue_called_);
 }
 
-// Call the add callback
-TEST_F(MasterLoaderCallbacksTest, addCall) {
-    EXPECT_FALSE(issue_called_);
-    callbacks_.addRRset(rrset_);
-    EXPECT_TRUE(add_called_);
-}
-
-
 }