[trac550] refactored the callback interface (with a minor API change)

for the next step of wildcard work, extended the notion of callback to
generic "node flags".  callback specific interfaces were replaced with
flag manipulation methods.

src/lib/datasrc/memory_datasrc.cc

@@ -248,10 +248,10 @@ struct MemoryZone::MemoryZoneImpl {
             // indicating the need for callback in find().
             if (rrset->getType() == RRType::NS() &&
                 rrset->getName() != origin_) {
-                node->enableCallback();
+                node->setFlag(DomainNode::FLAG_CALLBACK);
             // If it is DNAME, we have a callback as well here
             } else if (rrset->getType() == RRType::DNAME()) {
-                node->enableCallback();
+                node->setFlag(DomainNode::FLAG_CALLBACK);
             }
 
             return (result::SUCCESS);
@@ -407,7 +407,7 @@ struct MemoryZone::MemoryZoneImpl {
 
         // If the node callback is enabled, this may be a zone cut.  If it
         // has a NS RR, we should return a delegation, but not in the apex.
-        if (node->isCallbackEnabled() && node != origin_data_) {
+        if (node->getFlag(DomainNode::FLAG_CALLBACK) && node != origin_data_) {
             found = node->getData()->find(RRType::NS());
             if (found != node->getData()->end()) {
                 return (FindResult(DELEGATION, found->second));

src/lib/datasrc/rbtree.h

@@ -23,6 +23,8 @@
 ///     issue, the design and interface are not fixed, and RBTree isn't ready
 ///     to be used as a base data structure by other modules.
 
+#include <exceptions/exceptions.h>
+
 #include <dns/name.h>
 #include <boost/utility.hpp>
 #include <boost/shared_ptr.hpp>
@@ -108,6 +110,29 @@ public:
     /// \brief Alias for shared pointer to the data.
     typedef boost::shared_ptr<T> NodeDataPtr;
 
+    /// Node flags.
+    ///
+    /// Each flag value defines a non default property for a specific node.
+    /// These are defined as bitmask type values for the convenience of
+    /// internal implementation, but applications are expected to use
+    /// each flag separately via the enum definitions.
+    ///
+    /// All (settable) flags are off by default; they must be explicitly
+    /// set to on by the \c setFlag() method.
+    enum Flags {
+        FLAG_CALLBACK = 1, ///< Callback enabled. See \ref callback
+        FLAG_USER1 = 0x8000000U ///< Application specific flag
+    };
+private:
+    // Some flag values are expected to be used for internal purposes
+    // (e.g., representing the node color) in future versions, so we
+    // limit the settable flags via the \c setFlag() method to those
+    // explicitly defined in \c Flags.  This constant represents all
+    // such flags.
+    static const uint32_t SETTABLE_FLAGS = (FLAG_CALLBACK | FLAG_USER1);
+
+public:
+
     /// \brief Destructor
     ///
     /// It might seem strange that constructors are private and destructor
@@ -150,6 +175,52 @@ public:
     void setData(const NodeDataPtr& data) { data_ = data; }
     //@}
 
+    /// \name Node flag manipulation methods
+    //@{
+    /// Get the status of a node flag.
+    ///
+    /// This method returns whether the given node flag is set (enabled)
+    /// on the node.  The \c flag parameter is expected to be one of the
+    /// defined \c Flags constants.  For simplicity, the method interface
+    /// does not prohibit passing an undefined flag or combined flags, but
+    /// the return value in such a case will be meaningless for the caller
+    /// (an application would have to use an ugly cast for such an unintended
+    /// form of call, which will hopefully avoid accidental misuse).
+    ///
+    /// \exception None
+    /// \param flag The flag to be tested.
+    /// \return \c true if the \c flag is set; \c false otherwise.
+    bool getFlag(Flags flag) const {
+        return ((flags_ & flag) != 0);
+    }
+
+    /// Set or clear a node flag.
+    ///
+    /// This method changes the status of the specified node flag to either
+    /// "on" (enabled) or "off" (disabled).  The new status is specified by
+    /// the \c on parameter.
+    /// Like the \c getFlag() method, \c flag is expected to be one of the
+    /// defined \c Flags constants.  If an undefined or unsettable flag is
+    /// specified, \c isc::InvalidParameter exception will be thrown.
+    ///
+    /// \exception isc::InvalidParameter Unsettable flag is specified
+    /// \exception None otherwise
+    /// \param flag The node flag to be changed.
+    /// \on If \c true, set the flag to on; otherwise set it to off.
+    void setFlag(Flags flag, bool on = true) {
+        if ((flag & ~SETTABLE_FLAGS) != 0) {
+            isc_throw(isc::InvalidParameter,
+                      "Unsettable RBTree flag is being set");
+        }
+        if (on) {
+            flags_ |= flag;
+        } else {
+            flags_ &= ~flag;
+        }
+    }
+    //@}
+
+private:
     /// \name Callback related methods
     ///
     /// See the description of \c RBTree<T>::find() about callbacks.
@@ -157,16 +228,8 @@ public:
     /// These methods never throw an exception.
     //@{
     /// Return if callback is enabled at the node.
-    bool isCallbackEnabled() const { return (callback_required_); }
-
-    /// Enable callback at the node.
-    void enableCallback() { callback_required_ = true; }
-
-    /// Disable callback at the node.
-    void disableCallback() { callback_required_ = false; }
     //@}
 
-
 private:
     /// \brief Define rbnode color
     enum RBNodeColor {BLACK, RED};
@@ -204,7 +267,7 @@ private:
     /// RBTree::find().
     ///
     /// \todo It might be needed to put it into more general attributes field.
-    bool callback_required_;
+    uint32_t flags_;
 };
 
 
@@ -218,7 +281,7 @@ RBNode<T>::RBNode() :
     // dummy name, the value doesn't matter:
     name_(isc::dns::Name::ROOT_NAME()),
     down_(this),
-    callback_required_(false)
+    flags_(0)
 {
 }
 
@@ -230,7 +293,7 @@ RBNode<T>::RBNode(const isc::dns::Name& name) :
     color_(RED),
     name_(name),
     down_(NULL_NODE()),
-    callback_required_(false)
+    flags_(0)
 {
 }
 
@@ -375,7 +438,7 @@ public:
     ///
     /// This version of find calls the callback whenever traversing (on the
     /// way from root down the tree) a marked node on the way down through the
-    /// domain namespace (see RBNode::enableCallback and related functions).
+    /// domain namespace (see \c RBNode::setFlag and related functions).
     ///
     /// If you return true from the callback, the search is stopped and a
     /// PARTIALMATCH is returned with the given node. Note that this node
@@ -645,7 +708,8 @@ RBTree<T,returnEmptyNode>::findHelper(const isc::dns::Name& target_name,
                 if (returnEmptyNode || !node->isEmpty()) {
                     ret = RBTree<T,returnEmptyNode>::PARTIALMATCH;
                     *target = node;
-                    if (callback != NULL && node->callback_required_) {
+                    if (callback != NULL &&
+                        node->getFlag(RBNode<T>::FLAG_CALLBACK)) {
                         if ((callback)(*node, callback_arg)) {
                             break;
                         }
@@ -759,7 +823,7 @@ RBTree<T,S>::nodeFission(RBNode<T>& node, const isc::dns::Name& base_name) {
     // consistent behavior (i.e., a weak form of strong exception guarantee)
     // even if code after the call to this function throws an exception.
     std::swap(node.data_, down_node->data_);
-    std::swap(node.callback_required_, down_node->callback_required_);
+    std::swap(node.flags_, down_node->flags_);
     down_node->down_ = node.down_;
     node.down_ = down_node.get();
     // root node of sub tree, the initial color is BLACK

src/lib/datasrc/tests/rbtree_unittest.cc

@@ -15,6 +15,8 @@
 
 #include <gtest/gtest.h>
 
+#include <exceptions/exceptions.h>
+
 #include <dns/name.h>
 #include <dns/rrclass.h>
 #include <dns/rrset.h>
@@ -245,6 +247,30 @@ TEST_F(RBTreeTest, findName) {
     EXPECT_EQ(Name("q"), rbtnode->getName());
 }
 
+TEST_F(RBTreeTest, flags) {
+    EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("flags.example"),
+                                                  &rbtnode));
+
+    // by default, flags are all off
+    EXPECT_FALSE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+
+    // set operation, by default it enables the flag
+    rbtnode->setFlag(RBNode<int>::FLAG_CALLBACK);
+    EXPECT_TRUE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+
+    // try disable the flag explicitly
+    rbtnode->setFlag(RBNode<int>::FLAG_CALLBACK, false);
+    EXPECT_FALSE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+
+    // try enable the flag explicitly
+    rbtnode->setFlag(RBNode<int>::FLAG_CALLBACK, true);
+    EXPECT_TRUE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+
+    // setting an unknown flag will trigger an exception
+    EXPECT_THROW(rbtnode->setFlag(static_cast<RBNode<int>::Flags>(2), true),
+                 isc::InvalidParameter);
+}
+
 bool
 testCallback(const RBNode<int>&, bool* callack_checker) {
     *callack_checker = true;
@@ -256,16 +282,16 @@ TEST_F(RBTreeTest, callback) {
     EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("callback.example"),
                                                   &rbtnode));
     rbtnode->setData(RBNode<int>::NodeDataPtr(new int(1)));
-    EXPECT_FALSE(rbtnode->isCallbackEnabled());
+    EXPECT_FALSE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
 
     // enable/re-disable callback
-    rbtnode->enableCallback();
-    EXPECT_TRUE(rbtnode->isCallbackEnabled());
-    rbtnode->disableCallback();
-    EXPECT_FALSE(rbtnode->isCallbackEnabled());
+    rbtnode->setFlag(RBNode<int>::FLAG_CALLBACK);
+    EXPECT_TRUE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+    rbtnode->setFlag(RBNode<int>::FLAG_CALLBACK, false);
+    EXPECT_FALSE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
 
     // enable again for subsequent tests
-    rbtnode->enableCallback();
+    rbtnode->setFlag(RBNode<int>::FLAG_CALLBACK);
     // add more levels below and above the callback node for partial match.
     RBNode<int>* subrbtnode;
     EXPECT_EQ(RBTree<int>::SUCCESS, rbtree.insert(Name("sub.callback.example"),
@@ -279,9 +305,9 @@ TEST_F(RBTreeTest, callback) {
     // it.
     EXPECT_EQ(RBTree<int>::EXACTMATCH, rbtree.find(Name("callback.example"),
                                                    &rbtnode));
-    EXPECT_TRUE(rbtnode->isCallbackEnabled());
-    EXPECT_FALSE(subrbtnode->isCallbackEnabled());
-    EXPECT_FALSE(parentrbtnode->isCallbackEnabled());
+    EXPECT_TRUE(rbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+    EXPECT_FALSE(subrbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
+    EXPECT_FALSE(parentrbtnode->getFlag(RBNode<int>::FLAG_CALLBACK));
 
     // check if the callback is called from find()
     bool callback_called = false;
@@ -292,7 +318,7 @@ TEST_F(RBTreeTest, callback) {
 
     // enable callback at the parent node, but it doesn't have data so
     // the callback shouldn't be called.
-    parentrbtnode->enableCallback();
+    parentrbtnode->setFlag(RBNode<int>::FLAG_CALLBACK);
     callback_called = false;
     EXPECT_EQ(RBTree<int>::EXACTMATCH,
               rbtree.find(Name("callback.example"), &crbtnode,