[svn] commit: r2223 - in /branches/trac192/src/lib/datasrc: cache.cc cache.h data_source.cc tests/cache_unittest.cc

BIND 10 source code commits bind10-changes at lists.isc.org
Wed Jun 23 03:28:56 UTC 2010


Author: each
Date: Wed Jun 23 03:28:56 2010
New Revision: 2223

Log:
Addressing review comments -- refactored HotCache to use pimpl,
move CacheNode and CacheEntry into cache.cc.  HotCache::retrieve()
is changed to return a boolean indicating whether there was a cache
hit, with rrset and flags returned via reference in the parameter
list.  Also fixed some comments and made HotCache noncopyable.

Modified:
    branches/trac192/src/lib/datasrc/cache.cc
    branches/trac192/src/lib/datasrc/cache.h
    branches/trac192/src/lib/datasrc/data_source.cc
    branches/trac192/src/lib/datasrc/tests/cache_unittest.cc

Modified: branches/trac192/src/lib/datasrc/cache.cc
==============================================================================
--- branches/trac192/src/lib/datasrc/cache.cc (original)
+++ branches/trac192/src/lib/datasrc/cache.cc Wed Jun 23 03:28:56 2010
@@ -22,6 +22,154 @@
 namespace isc {
 namespace datasrc {
 
+/// \brief A \c CacheEntry object contains a pointer to an RRset,
+/// and the query-response flags that were returned when the RRset
+/// was originally looked up in the low-level data source.
+class CacheEntry {
+public:
+    RRsetPtr rrset;
+    uint32_t flags;
+};
+
+typedef boost::shared_ptr<CacheEntry> CacheEntryPtr;
+
+/// \brief A \c CacheNode is a node in the \c HotCache.
+class CacheNode {
+public:
+    /// \name Constructors and Destructor
+    //@{
+    /// \brief Constructor for positive cache entry.
+    ///
+    /// \param rrset The \c RRset to cache.
+    /// \param flags The query response flags returned from the low-level
+    /// data source when this \c RRset was looked up.
+    /// \param interval How long the cache node is to be considered valid.
+    CacheNode(const RRsetPtr rrset, uint32_t flags, time_t interval);
+
+    /// \brief Constructor for negative cache entry.
+    ///
+    /// \param name Query name
+    /// \param rrclass Query class
+    /// \param rrtype Query type
+    /// \param flags Query response flags returned from the low-level
+    /// data source, indicating why this lookup failed (name not found,
+    /// type not found, etc).
+    /// \param interval How long the cache node is to be considered valid.
+    CacheNode(const Name& name,
+              const RRClass& rrclass,
+              const RRType& rrtype,
+              uint32_t flags,
+              time_t interval);
+
+    // \brief Destructor
+    ~CacheNode();
+    //@}
+
+    /// \name Getter and Setter Methods
+    //@{
+    /// \brief Returns a pointer to the cached RRset (or an empty
+    /// RRsetPtr for negative cache entries).
+
+    /// \return \c RRsetPtr
+    RRsetPtr getRRset() const { return (entry_->rrset); }
+
+    /// \brief Returns the query response flags associated with the data.
+    ///
+    /// \return \c uint32_t
+    uint32_t getFlags() const { return (entry_->flags); }
+
+    /// \brief Is this record still valid?
+    ///
+    /// \return True if the expiration time has not yet passed,
+    /// or false if it has.
+    bool isValid() const;
+    //@}
+
+    // Links to next and previous nodes in the LRU list.
+    CacheNodePtr prev;
+    CacheNodePtr next;
+
+    // The standard query tuple: qname/qclass/qtype.
+    // Used as the search key for cache nodes.
+    const Name qname;
+    const RRClass qclass;
+    const RRType qtype;
+
+private:
+    // The cached RRset data
+    CacheEntryPtr entry_;
+
+    // When this record should be discarded
+    time_t expiry_;
+};
+
+/// \brief A \c QTuple is a simple object containing the standard query
+/// tuple: {name, class, type}.
+///
+/// Its purpose is to act as the key data format for the \c std::map
+/// in the \c HotCache class.
+///
+/// (Possible TODO item: using this in other parts of libdatasrc as well.)
+class QTuple {
+public:
+    QTuple(Name n, RRClass c, RRType t);
+
+    // A comparison operator is needed for this class to function
+    // as an index to std::map.
+    bool operator <(const QTuple& rhs) const {
+        return (qclass < rhs.qclass ||
+                (qclass == rhs.qclass &&
+                 (qtype < rhs.qtype ||
+                  (qtype == rhs.qtype && (qname < rhs.qname)))));
+    }
+
+    Name qname;
+    RRClass qclass;
+    RRType qtype;
+};
+
+/// This class abstracts the implementation details for the HotCache.
+///
+/// Each node inserted into the cache is placed at the head of a
+/// doubly-linked list.  Whenever that node is retrieved from the cache,
+/// it is again moved to the head of the list.  When the configured
+/// number of slots in the cache has been exceeded, the least recently
+/// used nodes will be removed from the tail of the list.
+///
+/// A pointer to each cache node is also placed in a \c std::map object,
+/// keyed by \c QTuple.  This allows retrieval of data in (usually)
+/// logarithmic time.  (Possible TODO item: replace this with a hash
+/// table instead.)
+class HotCacheImpl {
+public:
+    HotCacheImpl(int slots);
+    ~HotCacheImpl();
+
+    // Pointers to the head and tail of the LRU list.
+    CacheNodePtr lru_head_;
+    CacheNodePtr lru_tail_;
+
+    // Number of available slots in the LRU list.  (If zero,
+    // then the list is unbounded; otherwise, items are removed
+    // from the tail of the list whenever it grows past slots_.)
+    int slots_;
+
+    // The number of items currently in the list.
+    int count_;
+
+    // Map from query tuple to cache node pointer, allowing faster retrieval
+    std::map<QTuple, CacheNodePtr> map_;
+
+    // Move a node to the front of the LRU queue.
+    void promote(CacheNodePtr node);
+
+    // Remove a node from the cache.
+    void remove(ConstCacheNodePtr node);
+
+    // Insert a node into the cache (called by both cache() and ncache())
+    void insert(CacheNodePtr node);
+};
+
 // CacheNode constructors
 CacheNode::CacheNode(const RRsetPtr rrset, const uint32_t flags,
                      const time_t interval) :
@@ -68,11 +216,85 @@
 // QTuple constructor
 QTuple::QTuple(Name n, RRClass c, RRType t) : qname(n), qclass(c), qtype(t) {} 
 
+// HotCache constructor
+HotCache::HotCache(const int slots) {
+    impl_ = new HotCacheImpl(slots);
+}
+
 // HotCache destructor
 HotCache::~HotCache() {
+    delete impl_;
+}
+
+void
+HotCache::cache(RRsetPtr rrset, const uint32_t flags, const time_t interval) {
+    impl_->insert(CacheNodePtr(new CacheNode(rrset, flags, interval)));
+}
+
+void
+HotCache::ncache(const Name& name, const RRClass &rrclass,
+                 const RRType& rrtype, const uint32_t flags,
+                 const time_t interval)
+{
+    if (rrtype == RRType::ANY() || rrclass == RRClass::ANY()) {
+        return;
+    }
+
+    impl_->insert(CacheNodePtr(new CacheNode(name, rrclass, rrtype,
+                                             flags, interval)));
+}
+
+bool
+HotCache::retrieve(Name n, RRClass c, RRType t,
+                   RRsetPtr& rrset, uint32_t& flags)
+{
+    std::map<QTuple, CacheNodePtr>::const_iterator iter;
+    iter = impl_->map_.find(QTuple(n, c, t));
+    if (iter == impl_->map_.end()) {
+        return (false);
+    }
+
+    CacheNodePtr node = iter->second;
+
+    if (node->isValid()) {
+        impl_->promote(node);
+        rrset = node->getRRset();
+        flags = node->getFlags();
+        return (true);
+    }
+
+    impl_->remove(node);
+    return (false);
+}
+
+void
+HotCache::setSlots(const int slots) {
+    impl_->slots_ = slots;
+
+    while (impl_->slots_ != 0 && impl_->count_ > impl_->slots_ &&
+           impl_->lru_tail_)
+    {
+        impl_->remove(impl_->lru_tail_);
+    }
+}
+
+int
+HotCache::getSlots() const {
+    return (impl_->slots_);
+}
+
+int
+HotCache::getCount() const {
+    return (impl_->count_);
+}
+
+// HotCacheImpl constructor
+HotCacheImpl::HotCacheImpl(const int slots) : slots_(slots), count_(0) {}
+
+// HotCacheImpl destructor
+HotCacheImpl::~HotCacheImpl() {
     CacheNodePtr node;
-
-    for (node = lru_head_; node = node->next; node != CacheNodePtr()) {
+    for (node = lru_head_; node != CacheNodePtr(); node = node->next) {
         lru_head_ = node->next;
         if (lru_head_) {
             lru_head_->prev = CacheNodePtr();
@@ -82,24 +304,17 @@
     lru_head_ = lru_tail_ = CacheNodePtr();
 }
 
-ConstCacheNodePtr
-HotCache::retrieve(Name n, RRClass c, RRType t) {
-    CacheNodePtr node = map_[QTuple(n, c, t)];
-    if (!node) {
-        return (CacheNodePtr());
-    }
-
-    if (node->isValid()) {
-        promote(node);
-        return (node);
-    } else {
-        remove(node);
-        return (CacheNodePtr());
-    }
-}
-
 inline void
-HotCache::insert(const CacheNodePtr node) {
+HotCacheImpl::insert(const CacheNodePtr node) {
+    std::map<QTuple, CacheNodePtr>::const_iterator iter;
+    iter = map_.find(QTuple(node->qname, node->qclass, node->qtype));
+    if (iter != map_.end()) {
+        CacheNodePtr old = iter->second;
+        if (old && old->isValid()) {
+            remove(old);
+        }
+    }
+
     if (lru_head_) {
         node->next = lru_head_;
         lru_head_->prev = node;
@@ -113,60 +328,14 @@
     map_[QTuple(node->qname, node->qclass, node->qtype)] = node;
     ++count_;
 
-    while (slots_ != 0 && count_ > slots_ && lru_tail_) {
+    if (slots_ != 0 && count_ > slots_ && lru_tail_) {
         remove(lru_tail_);
-    }
-}
-
-void
-HotCache::cache(RRsetPtr rrset, const uint32_t flags, const time_t interval) {
-    ConstCacheNodePtr node = retrieve(rrset->getName(), rrset->getClass(), 
-                                      rrset->getType());
-    if (node) {
-        remove(node);
-    }
-
-    insert(CacheNodePtr(new CacheNode(rrset, flags, interval)));
-}
-
-void
-HotCache::ncache(const Name& name, const RRClass &rrclass,
-                 const RRType& rrtype, const uint32_t flags,
-                 const time_t interval)
-{
-    if (rrtype == RRType::ANY() || rrclass == RRClass::ANY()) {
-        return;
-    }
-
-    ConstCacheNodePtr node = retrieve(name, rrclass, rrtype);
-    if (node) {
-        remove(node);
-    }
-
-    insert(CacheNodePtr(new CacheNode(name, rrclass, rrtype, flags, interval)));
-}
-
-void
-HotCache::setSlots(const int slots) {
-    slots_ = slots;
-
-    while (slots_ != 0 && count_ > slots_ && lru_tail_) {
-        remove(lru_tail_);
-    }
-}
-
-int
-HotCache::getSlots() const {
-    return (slots_);
-}
-
-int
-HotCache::getCount() const {
-    return (count_);
-}
-
-void
-HotCache::promote(CacheNodePtr node) {
+        assert(count_ == slots_);
+    }
+}
+
+void
+HotCacheImpl::promote(CacheNodePtr node) {
     if (!node || node == lru_head_) {
         return;
     }
@@ -191,7 +360,7 @@
 }
 
 void
-HotCache::remove(ConstCacheNodePtr node) {
+HotCacheImpl::remove(ConstCacheNodePtr node) {
     if (!node) {
         return;
     }

Modified: branches/trac192/src/lib/datasrc/cache.h
==============================================================================
--- branches/trac192/src/lib/datasrc/cache.h (original)
+++ branches/trac192/src/lib/datasrc/cache.h Wed Jun 23 03:28:56 2010
@@ -34,109 +34,8 @@
 class CacheNode;
 typedef boost::shared_ptr<CacheNode> CacheNodePtr;
 typedef boost::shared_ptr<const CacheNode> ConstCacheNodePtr;
-class CacheEntry;
-typedef boost::shared_ptr<CacheEntry> CacheEntryPtr;
 
-/// \brief A \c CacheEntry object contains a pointer to an RRset,
-/// and the query-response flags that were returned when the RRset
-/// was originally looked up in the low-level data source.
-class CacheEntry {
-public:
-    isc::dns::RRsetPtr rrset;
-    uint32_t flags;
-};
-
-/// \brief A \c CacheNode is a node in the \c HotCache.
-class CacheNode {
-public:
-    /// \name Constructors and Destructor
-    //@{
-    /// \brief Constructor for positive cache entry.
-    ///
-    /// \param rrset The \c RRset to cache.
-    /// \param flags The query response flags returned from the low-level
-    /// data source when this \c RRset was looked up.
-    /// \param interval How long the cache node is to be considered valid.
-    CacheNode(const isc::dns::RRsetPtr rrset, uint32_t flags, time_t interval);
-
-    /// \brief Constructor for negative cache entry.
-    ///
-    /// \param name Query name
-    /// \param rrclass Query class
-    /// \param rrtype Query type
-    /// \param flags Query response flags returned from the low-level
-    /// data source, indicating why this lookup failed (name not found,
-    /// type not found, etc).
-    /// \param interval How long the cache node is to be considered valid.
-    CacheNode(const isc::dns::Name& name,
-              const isc::dns::RRClass& rrclass,
-              const isc::dns::RRType& rrtype,
-              uint32_t flags,
-              time_t interval);
-
-    // \brief Destructor
-    ~CacheNode();
-    //@}
-
-    /// \name Getter and Setter Methods
-    //@{
-    /// \brief Returns a pointer to the cached RRset (or an empty
-    /// RRsetPtr for negative cache entries).
-
-    /// \return \c RRsetPtr
-    isc::dns::RRsetPtr getRRset() const { return (entry_->rrset); }
-
-    /// \brief Returns the query response flags associated with the data.
-    ///
-    /// \return \c uint32_t
-    uint32_t getFlags() const { return (entry_->flags); }
-
-    /// \brief Is this record still valid?
-    ///
-    /// \return True if the expiration time has not yet passed,
-    /// or false if it has.
-    bool isValid() const;
-    //@}
-
-    // Links to next and previous nodes in the LRU list.
-    CacheNodePtr prev;
-    CacheNodePtr next;
-
-    // The standard query tuple: qname/qclass/qtype.
-    // Used as the search key for cache nodes.
-    const isc::dns::Name qname;
-    const isc::dns::RRClass qclass;
-    const isc::dns::RRType qtype;
-
-private:
-    // The cached RRset data
-    CacheEntryPtr entry_;
-
-    // When the record should be discarded
-    time_t expiry_;
-};
-
-/// \brief A \c QTuple is a simple object containing the standard query
-/// tuple: {name, class, type}.
-///
-/// Its purpose is to act as the key data format for the \c std::map
-/// in the \c HotCache class.
-///
-/// (Possible TODO item: using this in other parts of libdatasrc as well.)
-class QTuple {
-public:
-    QTuple(isc::dns::Name n, isc::dns::RRClass c, isc::dns::RRType t);
-    bool operator <(const QTuple& rhs) const {
-        return (qclass < rhs.qclass ||
-                (qclass == rhs.qclass &&
-                 (qtype < rhs.qtype ||
-                  (qtype == rhs.qtype && (qname < rhs.qname)))));
-    }
-
-    isc::dns::Name qname;
-    isc::dns::RRClass qclass;
-    isc::dns::RRType qtype;
-};
+class HotCacheImpl;
 
 /// \brief A \c HotCache is a hot-spot cache for one or more data sources.
 ///
@@ -152,27 +51,32 @@
 /// has expired, it will be looked up in the underlying data source.
 /// If found there, it will be cached with a lifespan currently defaulting
 /// to 30 seconds.
-///
-/// Each node inserted into the cache is placed at the head of a
-/// doubly-linked list; whenever that node is retrieved from the cache,
-/// it is again moved to the head of the list.  When the configured
-/// number of slots in the cache has been exceeded, the least recently
-/// used nodes will be removed from the tail of the list.
-///
-/// A pointer to each cache node is also placed in a \c std::map object,
-/// keyed by \c QTuple.  This allows retrieval of data in (usually)
-/// logarithmic time.  (Possible TODO item: replace this with a hash
-/// table instead.)
 class HotCache {
+private:
+    /// \name Static definitions
+    //@{
+    /// \brief Default validity period for cache entries
+    static const int INTERVAL_ = 30;
+
+    /// \brief Default number of slots in cache
+    static const int SLOTS_ = 0;
+    //@}
+
+    /// \name Constructors, Assignment Operator and Destructor.
+    ///
+    /// Note: The copy constructor and the assignment operator are intentionally
+    /// defined as private.
+    //@{
+    HotCache(const HotCache& source);
+    HotCache& operator=(const HotCache& source);
+
 public:
-    /// \name Constructor and Destructor
-    //@{
-    /// \brief Constructor
+    /// \brief Constructor for HotCache
     ///
-    /// \param slots The number of slots.  If zero, the cache is unlimited.
-    HotCache(const int slots = 0) : slots_(slots), count_(0) {}
+    /// \param slots The number of slots available in the cache.
+    HotCache(const int slots = SLOTS_);
 
-    // \brief Destructor
+    /// \brief Destructor for HotCache
     ~HotCache();
     //@}
 
@@ -187,7 +91,7 @@
     /// defaulting to 30 seconds.
     void cache(isc::dns::RRsetPtr rrset,
                uint32_t flags,
-               time_t interval = 30);
+               time_t interval = INTERVAL_);
 
     /// \brief Enter a negative cache entry.
     ///
@@ -195,7 +99,7 @@
     /// cache, so instead a null \c RRsetPtr will be stored.  Since the
     /// name, class, and type cannot be retrieved from an \c RRset, they
     /// must be specified in the parameters.
-    ///
+    /// 
     /// \param name Query name
     /// \param rrclass Query class
     /// \param rrtype Query type
@@ -204,22 +108,38 @@
     /// type not found, etc).
     /// \param interval How long the cache node is to be considered valid;
     /// defaulting to 30 seconds.
+    ///
+    /// Note: 'rrclass' and 'rrtype' must refer to a specific class and
+    /// type; it is not meaningful to cache type or class ANY.  Currently,
+    /// this condition is silently ignored.
     void ncache(const isc::dns::Name& name,
                 const isc::dns::RRClass& rrclass,
                 const isc::dns::RRType& rrtype,
                 uint32_t flags,
-                time_t interval = 30);
+                time_t interval = INTERVAL_);
 
-    /// \brief Retrieve a record from the cache
+    /// \brief Retrieve (and promote) a record from the cache
+    ///
+    /// Retrieves a record from the cache matching the given 
+    /// query-tuple.  Returns true if one is found.  If it is a
+    /// posiitve cache entry, then 'rrset' is set to the cached
+    /// RRset.  For both positive and negative cache entries, 'flags'
+    /// is set to the query response flags.  The cache entry is 
+    /// then promoted to the head of the LRU queue.  (NOTE: Because
+    /// of this, "retrieve" cannot be implemented as a const method.)
     ///
     /// \param name The query name
     /// \param rrclass The query class
     /// \param rrtype The query type
+    /// \param rrset Returns the RRset found, if any, to the caller
+    /// \param flags Returns the flags, if any, to the caller
     ///
-    /// \return \c CacheNodePtr, empty if no data was found in the cache.
-    ConstCacheNodePtr retrieve(isc::dns::Name qname,
-                               isc::dns::RRClass qclass,
-                               isc::dns::RRType qtype);
+    /// \return \c bool, true if data was found in the cache, false if not.
+    bool retrieve(isc::dns::Name qname,
+                  isc::dns::RRClass qclass,
+                  isc::dns::RRType qtype,
+                  isc::dns::RRsetPtr& rrset,
+                  uint32_t& flags);
     //@}
 
 
@@ -228,10 +148,9 @@
     /// \brief Sets the number of slots in the cache.
     ///
     /// If slots is set to zero, the cache can grow without any imposed
-    /// limit, up to the number of RRsets in the data source.  If slots
-    /// is set to a lower number than previous (but not to zero), and
-    /// the number of nodes currently in the cache exceeds the new
-    /// value of slots, then the oldest records in the LRU queue will
+    /// limit.  If slots is set to a lower number than previous (but not to
+    /// zero), and the number of nodes currently in the cache exceeds the
+    /// new value of slots, then the oldest records in the cache will
     /// be removed immediately.
     void setSlots(int slots);
 
@@ -246,26 +165,8 @@
     //@}
 
 private:
-    // Pointers to the head and tail of the LRU list.
-    CacheNodePtr lru_head_;
-    CacheNodePtr lru_tail_;
-
-    int slots_;
-    int count_;
-
-    // Map from query tuple to cache node pointer, allowing faster retrieval
-    std::map<QTuple, CacheNodePtr> map_;
-
-    // Move a node to the front of the LRU queue.
-    void promote(CacheNodePtr node);
-
-    // Remove a node from the cache.
-    void remove(ConstCacheNodePtr node);
-
-    // Insert a node into the cache (called by both cache() and ncache())
-    void insert(CacheNodePtr node);
+    HotCacheImpl* impl_;
 };
-
 
 }
 }

Modified: branches/trac192/src/lib/datasrc/data_source.cc
==============================================================================
--- branches/trac192/src/lib/datasrc/data_source.cc (original)
+++ branches/trac192/src/lib/datasrc/data_source.cc Wed Jun 23 03:28:56 2010
@@ -146,8 +146,8 @@
     RRsetPtr rrset;
     ConstCacheNodePtr cnode;
     int count = 0;
-    uint32_t flags = 0;
-    bool found = false;
+    uint32_t flags = 0, cflags = 0;
+    bool hit = false, found = false;
 
     // First, we check the hot cache for the requested information.
     switch (task.op) {
@@ -158,91 +158,66 @@
             break;
         }
 
-        cnode  = cache.retrieve(task.qname, task.qclass, task.qtype);
-        if (!cnode) {
+        hit = cache.retrieve(task.qname, task.qclass, task.qtype, rrset, flags);
+        if (!hit) {
             break;
         }
 
-        rrset = cnode->getRRset();
-        flags = cnode->getFlags();
-
         if (rrset) {
-            rrsets.addRRset(cnode->getRRset());
-            ++count;
-        } else {
-            ++count;
-        }
-
-        if (count == 1) {
-            task.flags = flags;
-            target.append(rrsets);
-
-            return (DataSrc::SUCCESS);
-        }
-        break;
+            rrsets.addRRset(rrset);
+        }
+
+        task.flags = flags;
+        target.append(rrsets);
+        return (DataSrc::SUCCESS);
 
     case QueryTask::AUTH_QUERY:         // Find exact RRset or CNAME
         if (task.qtype == RRType::ANY() || task.qclass == RRClass::ANY()) {
             break;
         }
 
-        cnode = cache.retrieve(task.qname, task.qclass, task.qtype);
-        if (!cnode || !cnode->getRRset() != 0) {
-            ConstCacheNodePtr cnamenode = cache.retrieve(task.qname,
-                                                         task.qclass,
-                                                         RRType::CNAME());
-            if (cnamenode && cnamenode->getRRset()) {
-                cnode = cnamenode;
-            }
-        }
-        if (!cnode) {
+        hit = cache.retrieve(task.qname, task.qclass, task.qtype, rrset, flags);
+        if (!hit || !rrset) {
+            hit = cache.retrieve(task.qname, task.qclass, RRType::CNAME(),
+                                 rrset, flags);
+        }
+
+        if (!hit || !rrset) {
             break;
         }
-
-        rrset = cnode->getRRset();
-        flags = cnode->getFlags();
 
         if (rrset) {
             rrsets.addRRset(rrset);
-            task.flags = flags;
-            ++count;
-        } else {
-            ++count;
-        }
-
-        if (count == 1) {
-            target.append(rrsets);
-            task.flags = flags;
-            return (DataSrc::SUCCESS);
-        }
-        break;
+        }
+
+        task.flags = flags;
+        target.append(rrsets);
+        return (DataSrc::SUCCESS);
 
     case QueryTask::GLUE_QUERY:         // Find addresses
     case QueryTask::NOGLUE_QUERY:
         // (XXX: need to figure out how to deal with noglue case)
-        cnode = cache.retrieve(task.qname, task.qclass, RRType::A());
-        if (cnode) {
-            rrset = cnode->getRRset();
-            flags = cnode->getFlags();
+        flags = 0;
+
+        hit = cache.retrieve(task.qname, task.qclass, RRType::A(),
+                             rrset, cflags);
+        if (hit) {
+            flags |= cflags;
+            ++count;
             if (rrset) {
                 rrsets.addRRset(rrset);
                 found = true;
-                ++count;
-            } else {
-                ++count;
-            }
-        }
-
-        cnode = cache.retrieve(task.qname, task.qclass, RRType::AAAA());
-        if (cnode) {
-            rrset = cnode->getRRset();
-            flags |= cnode->getFlags();
+            }
+        }
+
+        hit = cache.retrieve(task.qname, task.qclass, RRType::AAAA(),
+                             rrset, flags);
+        if (hit) {
+            flags |= cflags;
+            ++count;
             if (rrset) {
                 rrsets.addRRset(rrset);
                 found = true;
-                ++count;
-            } else {
-                ++count;
             }
         }
 
@@ -258,42 +233,38 @@
 
 
     case QueryTask::REF_QUERY:          // Find NS, DS and/or DNAME
-        cnode = cache.retrieve(task.qname, task.qclass, RRType::NS());
-        if (cnode) {
-            rrset = cnode->getRRset();
-            flags = cnode->getFlags();
+        flags = 0;
+
+        hit = cache.retrieve(task.qname, task.qclass, RRType::NS(),
+                             rrset, cflags);
+        if (hit) {
+            flags |= cflags;
+            ++count;
             if (rrset) {
                 rrsets.addRRset(rrset);
                 found = true;
-                ++count;
-            } else {
-                ++count;
-            }
-        }
-
-        cnode = cache.retrieve(task.qname, task.qclass, RRType::DS());
-        if (cnode) {
-            rrset = cnode->getRRset();
-            flags |= cnode->getFlags();
+            }
+        }
+
+        hit = cache.retrieve(task.qname, task.qclass, RRType::DS(),
+                             rrset, flags);
+        if (hit) {
+            flags |= cflags;
+            ++count;
             if (rrset) {
                 rrsets.addRRset(rrset);
                 found = true;
-                ++count;
-            } else {
-                ++count;
-            }
-        }
-
-        cnode = cache.retrieve(task.qname, task.qclass, RRType::DNAME());
-        if (cnode) {
-            rrset = cnode->getRRset();
-            flags |= cnode->getFlags();
+            }
+        }
+
+        hit = cache.retrieve(task.qname, task.qclass, RRType::DNAME(),
+                             rrset, flags);
+        if (hit) {
+            flags |= cflags;
+            ++count;
             if (rrset) {
                 rrsets.addRRset(rrset);
                 found = true;
-                ++count;
-            } else {
-                ++count;
             }
         }
 
@@ -302,8 +273,8 @@
                 flags &= ~DataSrc::TYPE_NOT_FOUND;
                 flags &= DataSrc::REFERRAL;
             }
+            target.append(rrsets);
             task.flags = flags;
-            target.append(rrsets);
             return (DataSrc::SUCCESS);
         } 
         break;

Modified: branches/trac192/src/lib/datasrc/tests/cache_unittest.cc
==============================================================================
--- branches/trac192/src/lib/datasrc/tests/cache_unittest.cc (original)
+++ branches/trac192/src/lib/datasrc/tests/cache_unittest.cc Wed Jun 23 03:28:56 2010
@@ -78,85 +78,101 @@
 
     EXPECT_EQ(4, cache.getCount());
 
-    RRsetPtr r = cache.retrieve(Name("foo"), RRClass::IN(),
-                                RRType::AAAA())->getRRset();
+    RRsetPtr r;
+    uint32_t f;
+    bool hit = cache.retrieve(Name("foo"), RRClass::IN(),
+                                RRType::AAAA(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(aaaa, r);
 }
 
 TEST_F(CacheTest, retrieveOK) {
-    RRsetPtr r;
+    bool hit;
+    RRsetPtr r;
+    uint32_t f;
 
     // Test repeatedly to ensure that all records remain accessible
     // even after being promoted to the top of the cache
-    r = cache.retrieve(test_name, RRClass::IN(), RRType::A())->getRRset();
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_name, r->getName());
     EXPECT_EQ(RRClass::IN(), r->getClass());
     EXPECT_EQ(RRType::A(), r->getType());
 
-    r = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS())->getRRset();
+    hit = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_nsname, r->getName());
     EXPECT_EQ(RRClass::IN(), r->getClass());
     EXPECT_EQ(RRType::NS(), r->getType());
 
-    r = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT())->getRRset();
+    hit = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_ch, r->getName());
     EXPECT_EQ(RRClass::CH(), r->getClass());
     EXPECT_EQ(RRType::TXT(), r->getType());
     
-    r = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS())->getRRset();
+    hit = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_nsname, r->getName());
     EXPECT_EQ(RRClass::IN(), r->getClass());
     EXPECT_EQ(RRType::NS(), r->getType());
 
-    r = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT())->getRRset();
+    hit = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_ch, r->getName());
     EXPECT_EQ(RRClass::CH(), r->getClass());
     EXPECT_EQ(RRType::TXT(), r->getType());
 
-    r = cache.retrieve(test_name, RRClass::IN(), RRType::A())->getRRset();
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_name, r->getName());
     EXPECT_EQ(RRClass::IN(), r->getClass());
     EXPECT_EQ(RRType::A(), r->getType());
 
-    r = cache.retrieve(test_name, RRClass::IN(), RRType::A())->getRRset();
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(test_name, r->getName());
     EXPECT_EQ(RRClass::IN(), r->getClass());
     EXPECT_EQ(RRType::A(), r->getType());
 };
 
 TEST_F(CacheTest, flags) {
-    ConstCacheNodePtr c;
-    c = cache.retrieve(test_name, RRClass::IN(), RRType::A());
-    EXPECT_TRUE(c);
-    EXPECT_TRUE(c->getRRset());
-    EXPECT_EQ(1, c->getFlags());
-
-    c = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS());
-    EXPECT_TRUE(c);
-    EXPECT_TRUE(c->getRRset());
-    EXPECT_EQ(2, c->getFlags());
-
-    c = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT());
-    EXPECT_TRUE(c);
-    EXPECT_TRUE(c->getRRset());
-    EXPECT_EQ(4, c->getFlags());
+    bool hit;
+    RRsetPtr r;
+    uint32_t f;
+
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_TRUE(hit);
+    EXPECT_TRUE(r);
+    EXPECT_EQ(1, f);
+
+    hit = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS(), r, f);
+    EXPECT_TRUE(hit);
+    EXPECT_TRUE(r);
+    EXPECT_EQ(2, f);
+
+    hit = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT(), r, f);
+    EXPECT_TRUE(hit);
+    EXPECT_TRUE(r);
+    EXPECT_EQ(4, f);
 }
 
 TEST_F(CacheTest, retrieveFail) {
-    ConstCacheNodePtr c;
-
-    c = cache.retrieve(Name("fake"), RRClass::IN(), RRType::A());
-    EXPECT_FALSE(c);
-
-    c = cache.retrieve(test_name, RRClass::CH(), RRType::A());
-    EXPECT_FALSE(c);
-
-    c = cache.retrieve(test_name, RRClass::IN(), RRType::DNSKEY());
-    EXPECT_FALSE(c);
+    bool hit;
+    RRsetPtr r;
+    uint32_t f;
+
+    hit = cache.retrieve(Name("fake"), RRClass::IN(), RRType::A(), r, f);
+    EXPECT_FALSE(hit);
+
+    hit = cache.retrieve(test_name, RRClass::CH(), RRType::A(), r, f);
+    EXPECT_FALSE(hit);
+
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::DNSKEY(), r, f);
+    EXPECT_FALSE(hit);
 }
 
 TEST_F(CacheTest, expire) {
-
     // Insert "foo" with a duration of 2 seconds; sleep 3.  The
     // record should not be returned from the cache even though it's
     // at the top of the cache.
@@ -166,17 +182,23 @@
     cache.cache(aaaa, 0, 2);
 
     sleep(3);
-    ConstCacheNodePtr c = cache.retrieve(Name("foo"), RRClass::IN(),
-                                         RRType::AAAA());
-    EXPECT_FALSE(c);
+
+    RRsetPtr r;
+    uint32_t f;
+    bool hit = cache.retrieve(Name("foo"), RRClass::IN(), RRType::AAAA(), r, f);
+    EXPECT_FALSE(hit);
 };
 
 TEST_F(CacheTest, LRU) {
     // Retrieve a record, cache four new records; with five slots
     // in the LRU queue this should remove all the previous records
     // except the last one retreived.
-    RRsetPtr r = cache.retrieve(test_nsname, RRClass::IN(),
-                                RRType::NS())->getRRset();
+    bool hit;
+    RRsetPtr r;
+    uint32_t f;
+
+    hit = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS(), r, f);
+    EXPECT_TRUE(hit);
     EXPECT_EQ(3, cache.getCount());
 
     RRsetPtr one(new RRset(Name("one"), RRClass::IN(), RRType::TXT(),
@@ -203,25 +225,27 @@
     cache.cache(four, 0, 30);
     EXPECT_EQ(5, cache.getCount());
 
-    ConstCacheNodePtr c = cache.retrieve(test_name, RRClass::IN(),
-                                         RRType::A());
-    EXPECT_FALSE(c);
-
-    c = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS());
-    EXPECT_TRUE(c);
-
-    c = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT());
-    EXPECT_FALSE(c);
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_FALSE(hit);
+
+    hit = cache.retrieve(test_nsname, RRClass::IN(), RRType::NS(), r, f);
+    EXPECT_TRUE(hit);
+
+    hit = cache.retrieve(test_ch, RRClass::CH(), RRType::TXT(), r, f);
+    EXPECT_FALSE(hit);
 }
 
 TEST_F(CacheTest, ncache) {
     Name missing("missing.example.com");
     cache.ncache(missing, RRClass::IN(), RRType::DNSKEY(), 8, 30);
-    ConstCacheNodePtr c = cache.retrieve(missing, RRClass::IN(),
-                                         RRType::DNSKEY());
-    EXPECT_TRUE(c);
-    EXPECT_FALSE(c->getRRset());
-    EXPECT_EQ(8, c->getFlags());
+
+    RRsetPtr r;
+    uint32_t f;
+    bool hit = cache.retrieve(missing, RRClass::IN(), RRType::DNSKEY(), r, f);
+
+    EXPECT_TRUE(hit);
+    EXPECT_FALSE(r);
+    EXPECT_EQ(8, f);
 }
 
 TEST_F(CacheTest, overwrite) {
@@ -232,19 +256,21 @@
 
     EXPECT_NO_THROW(cache.cache(a, 16, 30));
     EXPECT_EQ(3, cache.getCount());
-    ConstCacheNodePtr c = cache.retrieve(test_name, RRClass::IN(),
-                                         RRType::A());
-    EXPECT_TRUE(c);
-    EXPECT_TRUE(c->getRRset());
-    EXPECT_EQ(16, c->getFlags());
-
-    EXPECT_NO_THROW(cache.ncache(test_name, RRClass::IN(), RRType::A(), 1,
-                                 30));
-    EXPECT_EQ(3, cache.getCount());
-    c = cache.retrieve(test_name, RRClass::IN(), RRType::A());
-    EXPECT_TRUE(c);
-    EXPECT_FALSE(c->getRRset());
-    EXPECT_EQ(1, c->getFlags());
+
+    RRsetPtr r;
+    uint32_t f;
+    bool hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_TRUE(hit);
+    EXPECT_TRUE(r);
+    EXPECT_EQ(16, f);
+
+    EXPECT_NO_THROW(cache.ncache(test_name, RRClass::IN(), RRType::A(), 1, 30));
+    EXPECT_EQ(3, cache.getCount());
+
+    hit = cache.retrieve(test_name, RRClass::IN(), RRType::A(), r, f);
+    EXPECT_TRUE(hit);
+    EXPECT_FALSE(r);
+    EXPECT_EQ(1, f);
 }
 
 TEST_F(CacheTest, reduceSlots) {




More information about the bind10-changes mailing list