[trac449] Addressed most review comments

all of them, except the class issue in ResolverCache

src/lib/cache/cache_entry_key.cc

@@ -22,9 +22,7 @@ using namespace std;
 namespace isc {
 namespace cache {
 const std::string
-genCacheEntryName(const isc::dns::Name& name,
-                 const isc::dns::RRType& type)
-{
+genCacheEntryName(const isc::dns::Name& name, const isc::dns::RRType& type) {
     std::string keystr = name.toText();
     ostringstream stream;
     stream << type.getCode();
@@ -33,9 +31,7 @@ genCacheEntryName(const isc::dns::Name& name,
 }
 
 const std::string
-genCacheEntryName(const std::string& namestr,
-                 const uint16_t type)
-{
+genCacheEntryName(const std::string& namestr, const uint16_t type) {
     std::string keystr = namestr;
     ostringstream stream;
     stream << type;

src/lib/cache/cache_entry_key.h

@@ -39,8 +39,7 @@ namespace cache {
 /// \param type The RRType to create a text entry for
 /// \return return the entry name.
 const std::string
-genCacheEntryName(const isc::dns::Name& name,
-                 const isc::dns::RRType& type);
+genCacheEntryName(const isc::dns::Name& name, const isc::dns::RRType& type);
 
 ///
 /// \overload

src/lib/cache/local_zone_data.cc

@@ -50,13 +50,7 @@ LocalZoneData::update(const isc::dns::RRset& rrset) {
 
     rrsetCopy(rrset, *rrset_copy);
     RRsetPtr rrset_ptr(rrset_copy);
-    pair<RRsetMapIterator, bool> result = rrsets_map_.insert(RRsetMapPair(key, rrset_ptr));
-    if (!result.second) {
-        // Insert failed, we should remove the existed rrset first,
-        // then insert again.
-        rrsets_map_.erase(result.first);
-        rrsets_map_.insert(RRsetMapPair(key, rrset_ptr));
-    }
+    rrsets_map_[key] = rrset_ptr;
 }
 
 } // namespace cache

src/lib/cache/message_cache.cc

@@ -66,7 +66,6 @@ MessageCache::update(const Message& msg) {
     // add the message entry, maybe there is one way to touch it once.
     MessageEntryPtr old_msg_entry = message_table_.get(entry_key);
     if (old_msg_entry) {
-        message_table_.remove(entry_key);
         message_lru_.remove(old_msg_entry);
     }
 

src/lib/cache/message_cache.h

@@ -17,10 +17,8 @@
 #ifndef __MESSAGE_CACHE_H
 #define __MESSAGE_CACHE_H
 
-#include <map>
 #include <string>
 #include <boost/shared_ptr.hpp>
-#include <boost/noncopyable.hpp>
 #include <dns/message.h>
 #include "message_entry.h"
 #include <nsas/hash_table.h>
@@ -35,7 +33,11 @@ class RRsetCache;
 /// The object of MessageCache represents the cache for class-specific
 /// messages.
 ///
-class MessageCache: public boost::noncopyable {
+class MessageCache {
+// Noncopyable
+private:
+    MessageCache(const MessageCache& source);
+    MessageCache& operator=(const MessageCache& source);
 public:
     /// \param cache_size The size of message cache.
     MessageCache(boost::shared_ptr<RRsetCache> rrset_cache_,

src/lib/cache/message_entry.cc

@@ -44,6 +44,7 @@ MessageEntry::getRRsetEntries(vector<RRsetEntryPtr>& rrset_entry_vec,
                               const time_t time_now)
 {
     uint16_t entry_count = answer_count_ + authority_count_ + additional_count_;
+    rrset_entry_vec.reserve(rrset_entry_vec.size() + entry_count);
     for (int index = 0; index < entry_count; ++index) {
         RRsetEntryPtr rrset_entry = rrset_cache_->lookup(rrsets_[index].name_,
                                                         rrsets_[index].type_);
@@ -59,11 +60,13 @@ MessageEntry::getRRsetEntries(vector<RRsetEntryPtr>& rrset_entry_vec,
 
 void
 MessageEntry::addRRset(isc::dns::Message& message,
-                       const vector<RRsetEntryPtr> rrset_entry_vec,
-                       isc::dns::Message::Section section,
-                       bool dnssec_need) {
+                       const vector<RRsetEntryPtr>& rrset_entry_vec,
+                       const isc::dns::Message::Section& section,
+                       bool dnssec_need)
+{
     uint16_t start_index = 0;
     uint16_t end_index = answer_count_;
+    assert(section != Message::SECTION_QUESTION);
 
     if (section == Message::SECTION_AUTHORITY) {
         start_index = answer_count_;
@@ -82,7 +85,7 @@ bool
 MessageEntry::genMessage(const time_t& time_now,
                          isc::dns::Message& msg)
 {
-    if (time_now > expire_time_) {
+    if (time_now >= expire_time_) {
         // The message entry has expired.
         return (false);
     } else {
@@ -110,19 +113,28 @@ MessageEntry::genMessage(const time_t& time_now,
 
 RRsetTrustLevel
 MessageEntry::getRRsetTrustLevel(const Message& message,
-                   const isc::dns::RRsetPtr rrset,
-                   const isc::dns::Message::Section& section)
+    const isc::dns::RRsetPtr& rrset,
+    const isc::dns::Message::Section& section)
 {
     bool aa = message.getHeaderFlag(Message::HEADERFLAG_AA);
     switch(section) {
         case Message::SECTION_ANSWER: {
             if (aa) {
+                RRsetIterator rrset_iter = message.beginSection(section);
+
+                // Make sure we are inspecting the right RRset
+                while((*rrset_iter)->getName() != rrset->getName() &&
+                      (*rrset_iter)->getType() != rrset->getType() &&
+                      rrset_iter != message.endSection(section)) {
+                    ++rrset_iter;
+                }
+                
                 // According RFC2181 section 5.4.1, only the record
                 // describing that ailas is necessarily authoritative.
                 // If there is one or more CNAME records in answer section.
                 // CNAME records is assumed as the first rrset.
-                RRsetIterator rrset_iter = message.beginSection(section);
-                if ((*rrset_iter)->getType() == RRType("CNAME")) {
+                if ((*rrset_iter)->getType() == RRType::CNAME()) {
+                    // TODO: real equals for RRsets?
                     if ((*rrset_iter).get() == rrset.get()) {
                         return (RRSET_TRUST_ANSWER_AA);
                     } else {
@@ -136,11 +148,11 @@ MessageEntry::getRRsetTrustLevel(const Message& message,
                 // should be treated as non-authoritative.
                 // TODO, this part logic should be revisited later,
                 // since it's not mentioned by RFC2181.
-                if ((*rrset_iter)->getType() == RRType("DNAME")) {
+                if ((*rrset_iter)->getType() == RRType::DNAME()) {
+                    // TODO: real equals for RRsets?
                     if ((*rrset_iter).get() == rrset.get() ||
                         ((++rrset_iter) != message.endSection(section) &&
                                      (*rrset_iter).get() == rrset.get())) {
-
                         return (RRSET_TRUST_ANSWER_AA);
                     } else {
                         return (RRSET_TRUST_ANSWER_NONAA);

src/lib/cache/message_entry.h

@@ -20,7 +20,6 @@
 #include <vector>
 #include <dns/message.h>
 #include <dns/rrset.h>
-#include <boost/noncopyable.hpp>
 #include <nsas/nsas_entry.h>
 #include "rrset_entry.h"
 
@@ -54,9 +53,11 @@ struct RRsetRef{
 ///
 /// The object of MessageEntry represents one response message
 /// answered to the resolver client.
-class MessageEntry : public NsasEntry<MessageEntry>,
-                     public boost::noncopyable
-{
+class MessageEntry : public NsasEntry<MessageEntry> {
+// Noncopyable
+private:
+    MessageEntry(const MessageEntry& source);
+    MessageEntry& operator=(const MessageEntry& source);
 public:
 
     /// \brief Initialize the message entry object with one dns
@@ -73,14 +74,13 @@ public:
     /// \brief generate one dns message according
     ///        the rrsets information of the message.
     ///
-    /// \param response generated dns message.
     /// \param time_now set the ttl of each rrset in the message
     ///        as "expire_time - time_now" (expire_time is the
     ///        expiration time of the rrset).
+    /// \param response generated dns message.
     /// \return return true if the response message can be generated
     ///         from the cached information, or else, return false.
-    bool genMessage(const time_t& time_now,
-                    isc::dns::Message& response);
+    bool genMessage(const time_t& time_now, isc::dns::Message& response);
 
     /// \brief Get the hash key of the message entry.
     ///
@@ -124,8 +124,8 @@ protected:
     /// \param section Section of the rrset
     /// \return return rrset trust level.
     RRsetTrustLevel getRRsetTrustLevel(const isc::dns::Message& message,
-                               const isc::dns::RRsetPtr rrset,
-                               const isc::dns::Message::Section& section);
+        const isc::dns::RRsetPtr& rrset,
+        const isc::dns::Message::Section& section);
 
     /// \brief Add rrset to one section of message.
     ///
@@ -135,8 +135,8 @@ protected:
     /// \param section The section to add to
     /// \param dnssec_need need dnssec records or not.
     void addRRset(isc::dns::Message& message,
-                  const std::vector<RRsetEntryPtr> rrset_entry_vec,
-                  isc::dns::Message::Section section,
+                  const std::vector<RRsetEntryPtr>& rrset_entry_vec,
+                  const isc::dns::Message::Section& section,
                   bool dnssec_need);
 
     /// \brief Get the all the rrset entries for the message entry.

src/lib/cache/resolver_cache.cc

@@ -62,11 +62,12 @@ ResolverCache::classIsSupported(uint16_t klass) const {
                          class_supported_.end(), klass);
 }
 
+
 bool
 ResolverCache::lookup(const isc::dns::Name& qname,
-               const isc::dns::RRType& qtype,
-               const isc::dns::RRClass& qclass,
-               isc::dns::Message& response) const
+                      const isc::dns::RRType& qtype,
+                      const isc::dns::RRClass& qclass,
+                      isc::dns::Message& response) const
 {
     uint16_t class_code = qclass.getCode();
     if (!classIsSupported(class_code)) {

src/lib/cache/resolver_cache.h

@@ -75,7 +75,7 @@ public:
 
 /// \brief Resolver Cache.
 ///
-/// The object of ResolverCache represents the cache of the resolver. It may holds
+/// The object of ResolverCache represents the cache of the resolver. It may hold
 /// a list of message/rrset cache which are in different class.
 class ResolverCache {
 public:
@@ -98,9 +98,9 @@ public:
     /// \param qname The query name to look up
     /// \param qtype The query type to look up
     /// \param qclass The query class to look up
-    /// \param response the query message (must in RENDER mode)
-    ///        which has question section already(exception
-    ///        MessageNoQeustionSection) will be thrown if it has
+    /// \param response the query message (must be in RENDER mode)
+    ///        which has question section already (exception
+    ///        MessageNoQeustionSection will be thrown if it has
     ///        no question section). If the message can be found
     ///        in cache, rrsets for the message will be added to
     ///        different sections(answer, authority, additional).
@@ -230,6 +230,15 @@ protected:
     /// \brief cache the rrsets parsed from the received message.
     mutable RRsetCacheMap rrsets_cache_;
     //@}
+
+private:
+    /// Internal method that performs the actual method
+    /// (this is separated from the lookup() method itself
+    /// so that we can bypass unnecessary calls to classIsSupported()
+    bool lookupInternal(const isc::dns::Name& qname,
+                        const isc::dns::RRType& qtype,
+                        const isc::dns::RRClass& qclass,
+                        isc::dns::Message& response);
 };
 
 } // namespace cache

src/lib/cache/rrset_entry.cc

@@ -49,7 +49,9 @@ RRsetEntry::getExpireTime() const {
 void
 RRsetEntry::updateTTL(){
     uint32_t oldTTL = rrset_->getTTL().getValue();
-    if(oldTTL == 0) return;
+    if(oldTTL == 0) {
+        return;
+    }
 
     uint32_t now = time(NULL);
     uint32_t newTTL = now < expire_time_ ? (expire_time_ - now) : 0;

src/lib/cache/tests/cache_test_util.h

@@ -23,6 +23,10 @@ using namespace isc::dns;
 
 namespace {
 
+/// \brief Reads a Message from a data file
+///
+/// \param message Message to put the read data in
+/// \param datafile The file to read from
 void
 messageFromFile(Message& message, const char* datafile) {
     std::vector<unsigned char> data;
@@ -32,8 +36,14 @@ messageFromFile(Message& message, const char* datafile) {
     message.fromWire(buffer);
 }
 
+/// \brief Counts the number of rrsets in the given section
+///
+/// \param msg The message to count in
+/// \param section The section to count
+///
+/// \return The number of RRsets in the given section
 int
-section_rrset_count(Message& msg, Message::Section section) {
+sectionRRsetCount(Message& msg, Message::Section section) {
     int count = 0;
     for (RRsetIterator rrset_iter = msg.beginSection(section);
          rrset_iter != msg.endSection(section); 

src/lib/cache/tests/local_zone_data_unittest.cc

@@ -49,6 +49,9 @@ TEST_F(LocalZoneDataTest, updateAndLookup) {
 
     // Test whether the old one is replaced
     uint32_t ttl = (*rrset_iter)->getTTL().getValue();
+    // Make sure it is not zero
+    ASSERT_NE(ttl / 2, ttl);
+    
     RRsetPtr rrset_ptr = local_zone_data.lookup(name, type);
     EXPECT_EQ(ttl, rrset_ptr->getTTL().getValue());
 

src/lib/cache/tests/message_cache_unittest.cc

@@ -49,7 +49,7 @@ public:
     MessageCacheTest(): message_parse(Message::PARSE),
                         message_render(Message::RENDER)
     {
-        uint16_t class_ = 1; // class IN
+        uint16_t class_ = RRClass::IN().getCode();
         rrset_cache_.reset(new RRsetCache(RRSET_CACHE_DEFAULT_SIZE, class_));
         message_cache_.reset(new DerivedMessageCache(rrset_cache_, 
                                           MESSAGE_CACHE_DEFAULT_SIZE, class_ ));
@@ -66,8 +66,7 @@ TEST_F(MessageCacheTest, testLookup) {
     messageFromFile(message_parse, "message_fromWire1");
     EXPECT_TRUE(message_cache_->update(message_parse));
     Name qname("test.example.com.");
-    RRType qtype(1);
-    EXPECT_TRUE(message_cache_->lookup(qname, qtype, message_render));
+    EXPECT_TRUE(message_cache_->lookup(qname, RRType::A(), message_render));
     EXPECT_EQ(message_cache_->messages_count(), 1);
 
     Message message_net(Message::PARSE);
@@ -76,8 +75,7 @@ TEST_F(MessageCacheTest, testLookup) {
     EXPECT_EQ(message_cache_->messages_count(), 2);
 
     Name qname1("test.example.net.");
-    RRType qtype1(1);
-    EXPECT_TRUE(message_cache_->lookup(qname1, qtype1, message_render));
+    EXPECT_TRUE(message_cache_->lookup(qname1, RRType::A(), message_render));
 }
 
 TEST_F(MessageCacheTest, testUpdate) {
@@ -85,15 +83,14 @@ TEST_F(MessageCacheTest, testUpdate) {
     EXPECT_TRUE(message_cache_->update(message_parse));
 
     Name qname("example.com.");
-    RRType qtype(6);
-    EXPECT_TRUE(message_cache_->lookup(qname, qtype, message_render));
+    EXPECT_TRUE(message_cache_->lookup(qname, RRType::SOA(), message_render));
     EXPECT_FALSE(message_render.getHeaderFlag(Message::HEADERFLAG_AA));
 
     Message new_msg(Message::PARSE);
     messageFromFile(new_msg, "message_fromWire3");
     EXPECT_TRUE(message_cache_->update(new_msg));
     Message new_msg_render(Message::RENDER);
-    EXPECT_TRUE(message_cache_->lookup(qname, qtype, new_msg_render));
+    EXPECT_TRUE(message_cache_->lookup(qname, RRType::SOA(), new_msg_render));
     EXPECT_TRUE(new_msg_render.getHeaderFlag(Message::HEADERFLAG_AA));
 }
 

src/lib/cache/tests/message_entry_unittest.cc

@@ -164,7 +164,7 @@ TEST_F(MessageEntryTest, testGetRRsetTrustLevel_CNAME) {
     level = message_entry.getRRsetTrustLevelForTest(message_parse,
                                                     *rrset_iter,
                                                     Message::SECTION_ANSWER);
-    EXPECT_EQ(level, RRSET_TRUST_ANSWER_NONAA);
+    EXPECT_EQ(level, RRSET_TRUST_ANSWER_AA);
 }
 
 TEST_F(MessageEntryTest, testGetRRsetTrustLevel_DNAME) {
@@ -186,7 +186,7 @@ TEST_F(MessageEntryTest, testGetRRsetTrustLevel_DNAME) {
     level = message_entry.getRRsetTrustLevelForTest(message_parse,
                                                     *rrset_iter,
                                                     Message::SECTION_ANSWER);
-    EXPECT_EQ(level, RRSET_TRUST_ANSWER_NONAA);
+    EXPECT_EQ(level, RRSET_TRUST_ANSWER_AA);
 }
 
 // We only test the expire_time of the message entry.
@@ -223,9 +223,9 @@ TEST_F(MessageEntryTest, testGenMessage) {
     
     EXPECT_TRUE(msg.getHeaderFlag(Message::HEADERFLAG_AA));
     EXPECT_FALSE(msg.getHeaderFlag(Message::HEADERFLAG_TC));
-    EXPECT_EQ(1, section_rrset_count(msg, Message::SECTION_ANSWER)); 
-    EXPECT_EQ(1, section_rrset_count(msg, Message::SECTION_AUTHORITY)); 
-    EXPECT_EQ(5, section_rrset_count(msg, Message::SECTION_ADDITIONAL)); 
+    EXPECT_EQ(1, sectionRRsetCount(msg, Message::SECTION_ANSWER)); 
+    EXPECT_EQ(1, sectionRRsetCount(msg, Message::SECTION_AUTHORITY)); 
+    EXPECT_EQ(5, sectionRRsetCount(msg, Message::SECTION_ADDITIONAL)); 
 
     // Check the rrset in answer section.
     EXPECT_EQ(1, msg.getRRCount(Message::SECTION_ANSWER));

src/lib/cache/tests/resolver_cache_unittest.cc

@@ -87,11 +87,11 @@ TEST_F(ResolverCacheTest, testUpdateRRset) {
     cache.update(rrset_ptr);
 
     Message new_msg(Message::RENDER);
-    Question question(qname, klass, RRType(2));
+    Question question(qname, klass, RRType::NS());
     new_msg.addQuestion(question);
-    EXPECT_TRUE(cache.lookup(qname, RRType(2), klass, new_msg));
-    EXPECT_EQ(0, section_rrset_count(new_msg, Message::SECTION_AUTHORITY));
-    EXPECT_EQ(0, section_rrset_count(new_msg, Message::SECTION_ADDITIONAL));
+    EXPECT_TRUE(cache.lookup(qname, RRType::NS(), klass, new_msg));
+    EXPECT_EQ(0, sectionRRsetCount(new_msg, Message::SECTION_AUTHORITY));
+    EXPECT_EQ(0, sectionRRsetCount(new_msg, Message::SECTION_ADDITIONAL));
 }
 
 TEST_F(ResolverCacheTest, testLookupUnsupportedClass) {
@@ -100,11 +100,10 @@ TEST_F(ResolverCacheTest, testLookupUnsupportedClass) {
     cache.update(msg);
 
     Name qname("example.com.");
-    RRType qtype(6);
 
     msg.makeResponse();
-    EXPECT_FALSE(cache.lookup(qname, qtype, RRClass(2), msg));
-    EXPECT_FALSE(cache.lookup(qname, qtype, RRClass(2)));
+    EXPECT_FALSE(cache.lookup(qname, RRType::SOA(), RRClass::CH(), msg));
+    EXPECT_FALSE(cache.lookup(qname, RRType::SOA(), RRClass::CH()));
 }
 
 TEST_F(ResolverCacheTest, testLookupClosestRRset) {
@@ -113,18 +112,19 @@ TEST_F(ResolverCacheTest, testLookupClosestRRset) {
     cache.update(msg);
 
     Name qname("www.test.example.com.");
-    RRType qtype(6);
-    RRClass klass(1);
 
-    RRsetPtr rrset_ptr = cache.lookupClosestRRset(qname, qtype, klass);
+    RRsetPtr rrset_ptr = cache.lookupClosestRRset(qname, RRType::NS(),
+                                                  RRClass::IN());
     EXPECT_TRUE(rrset_ptr);
     EXPECT_EQ(rrset_ptr->getName(), Name("example.com."));
 
-    rrset_ptr = cache.lookupClosestRRset(Name("example.com."), RRType(2), klass);
+    rrset_ptr = cache.lookupClosestRRset(Name("example.com."),
+                                         RRType::NS(), RRClass::IN());
     EXPECT_TRUE(rrset_ptr);
     EXPECT_EQ(rrset_ptr->getName(), Name("example.com."));
 
-    rrset_ptr = cache.lookupClosestRRset(Name("com."), RRType(2), klass);
+    rrset_ptr = cache.lookupClosestRRset(Name("com."),
+                                         RRType::NS(), RRClass::IN());
     EXPECT_FALSE(rrset_ptr);
 }