[trac497] addressed review comments

and updated some doxygens as per Jeremy's mail

src/bin/resolver/resolver.h

@@ -65,7 +65,10 @@ public:
     /// send the reply.
     ///
     /// \param io_message The raw message received
-    /// \param message Pointer to the \c Message object
+    /// \param query_message Pointer to the query Message object we
+    /// received from the client
+    /// \param answer_message Pointer to the anwer Message object we
+    /// shall return to the client
     /// \param buffer Pointer to an \c OutputBuffer for the resposne
     /// \param server Pointer to the \c DNSServer
     void processMessage(const asiolink::IOMessage& io_message,
@@ -146,7 +149,11 @@ public:
      * \short Set options related to timeouts.
      *
      * This sets the time of timeout and number of retries.
-     * \param timeout The time in milliseconds. The value -1 disables timeouts.
+     * \param query_timeout The timeout we use for queries we send
+     * \param client_timeout The timeout at which point we send back a
+     * SERVFAIL (while continuing to resolve the query)
+     * \param lookup_timeout The timeout at which point we give up and
+     * stop.
      * \param retries The number of retries (0 means try the first time only,
      *     do not retry).
      */

src/lib/asiolink/asiolink.cc

@@ -329,6 +329,10 @@ private:
     //shared_ptr<DNSServer> server_;
     isc::resolve::ResolverInterface::CallbackPtr resolvercallback_;
 
+    // To prevent both unreasonably long cname chains and cname loops,
+    // we simply keep a counter of the number of CNAMEs we have
+    // followed so far (and error if it exceeds RESOLVER_MAX_CNAME_CHAIN
+    // from lib/resolve/response_classifier.h)
     unsigned cname_count_;
 
     /*
@@ -420,7 +424,7 @@ private:
         case isc::resolve::ResponseClassifier::ANSWER:
         case isc::resolve::ResponseClassifier::ANSWERCNAME:
             // Done. copy and return.
-            isc::resolve::copyAnswerMessage(incoming, answer_message_);
+            isc::resolve::copyResponseMessage(incoming, answer_message_);
             return true;
             break;
         case isc::resolve::ResponseClassifier::CNAME:
@@ -436,8 +440,8 @@ private:
                 return true;
             }
 
-            isc::resolve::copySection(incoming, answer_message_,
+            incoming.copySection(*answer_message_,
-                                      Message::SECTION_ANSWER);
+                Message::SECTION_ANSWER);
             setZoneServersToRoot();
 
             question_ = Question(cname_target, question_.getClass(),
@@ -449,7 +453,7 @@ private:
             break;
         case isc::resolve::ResponseClassifier::NXDOMAIN:
             // NXDOMAIN, just copy and return.
-            isc::resolve::copyAnswerMessage(incoming, answer_message_);
+            isc::resolve::copyResponseMessage(incoming, answer_message_);
             return true;
             break;
         case isc::resolve::ResponseClassifier::REFERRAL:
@@ -473,7 +477,7 @@ private:
                         // to that address and yield, when it
                         // returns, loop again.
                         
-                        // should use NSAS
+                        // TODO should use NSAS
                         zone_servers_.push_back(addr_t(addr_str, 53));
                         found_ns_address = true;
                     }
@@ -485,8 +489,8 @@ private:
                 return false;
             } else {
                 dlog("[XX] no ready-made addresses in additional. need nsas.");
-                // this will result in answering with the delegation. oh well
+                // TODO this will result in answering with the delegation. oh well
-                isc::resolve::copyAnswerMessage(incoming, answer_message_);
+                isc::resolve::copyResponseMessage(incoming, answer_message_);
                 return true;
             }
             break;
@@ -565,11 +569,10 @@ public:
         if (upstream_root_->empty()) { //if no root ips given, use this
             zone_servers_.push_back(addr_t("192.5.5.241", 53));
         } else {
-            //copy the list
+            // copy the list
             dlog("Size is " + 
                 boost::lexical_cast<string>(upstream_root_->size()) + 
                 "\n");
-            //Use BOOST_FOREACH here? Is it faster?
             for(AddressVector::iterator it = upstream_root_->begin();
                 it < upstream_root_->end(); it++) {
             zone_servers_.push_back(addr_t(it->first,it->second));
@@ -626,7 +629,7 @@ public:
                 incoming.getRcode() == Rcode::NOERROR()) {
                 done_ = handleRecursiveAnswer(incoming);
             } else {
-                isc::resolve::copyAnswerMessage(incoming, answer_message_);
+                isc::resolve::copyResponseMessage(incoming, answer_message_);
                 done_ = true;
             }
             

src/lib/asiolink/asiolink.h

@@ -466,10 +466,12 @@ public:
     /// class.
     ///
     /// \param io_message The event message to handle
-    /// \param message The DNS MessagePtr that needs handling
+    /// \param query_message The DNS MessagePtr of the original query
-    /// \param buffer The result is put here
+    /// \param answer_message The DNS MessagePtr of the answer we are
+    /// building
+    /// \param buffer Intermediate data results are put here
     virtual void operator()(const IOMessage& io_message,
-                            isc::dns::MessagePtr message,
+                            isc::dns::MessagePtr query_message,
                             isc::dns::MessagePtr answer_message,
                             isc::dns::OutputBufferPtr buffer) const = 0;
 };
@@ -546,9 +548,10 @@ public:
     ///        to forward queries to.
     /// \param upstream_root Addresses and ports of the root servers
     ///        to use when resolving.
-    /// \param timeout How long to timeout the query, in ms
+    /// \param query_timeout Timeout value for queries we sent, in ms
-    ///     -1 means never timeout (but do not use that).
+    /// \param client_timeout Timeout value for when we send back an
-    ///     TODO: This should be computed somehow dynamically in future
+    ///        error, in ms
+    /// \param lookup_timeout Timeout value for when we give up, in ms
     /// \param retries how many times we try again (0 means just send and
     ///     and return if it returs).
     RecursiveQuery(DNSService& dns_service,

src/lib/asiolink/tests/asiolink_unittest.cc

@@ -852,7 +852,7 @@ TEST_F(ASIOLinkTest, forwardClientTimeout) {
     const uint16_t port = boost::lexical_cast<uint16_t>(TEST_CLIENT_PORT);
     // Set it up to retry twice before client timeout fires
     // Since the lookup timer has not fired, it should retry
-    // a third time
+    // four times
     RecursiveQuery query(*dns_service_,
                          singleAddress(TEST_IPV4_ADDR, port),
                          singleAddress(TEST_IPV4_ADDR, port),

src/lib/dns/message.cc

@@ -777,6 +777,26 @@ Message::clear(Mode mode) {
 }
 
 void
+Message::copySection(Message& target, const Section section) const {
+    if (section >= MessageImpl::NUM_SECTIONS) {
+        isc_throw(OutOfRange, "Invalid message section: " << section);
+    }
+
+    if (section == SECTION_QUESTION) {
+        BOOST_FOREACH(QuestionPtr q, impl_->questions_) {
+            std::cout << "[XX] copy question " << q << std::endl;
+            target.addQuestion(q);
+        }
+    } else {
+        std::cout << "[XX] copy section " << section << std::endl;
+        BOOST_FOREACH(RRsetPtr r, impl_->rrsets_[section]) {
+            std::cout << "[XX] copy rrset " << r << std::endl;
+            target.addRRset(section, r, false);
+        }
+    }
+}
+
+void
 Message::makeResponse() {
     if (impl_->mode_ != Message::PARSE) {
         isc_throw(InvalidMessageOperation,

src/lib/dns/message.h

@@ -498,6 +498,13 @@ public:
     /// specified mode.
     void clear(Mode mode);
 
+    /// \brief Adds all rrsets in the given section to the same section
+    /// in target
+    ///
+    /// \param target The target Message
+    /// \param section the section to copy
+    void copySection(Message& target, const Section section) const;
+
     /// \brief Prepare for making a response from a request.
     ///
     /// This will clear the DNS header except those fields that should be kept

src/lib/dns/tests/message_unittest.cc

@@ -380,6 +380,63 @@ TEST_F(MessageTest, badEndSection) {
     EXPECT_THROW(message_render.endSection(bogus_section), OutOfRange);
 }
 
+TEST_F(MessageTest, copySection) {
+    Message target(Message::RENDER);
+
+    // Section check
+    EXPECT_THROW(message_render.copySection(target, bogus_section),
+                 OutOfRange);
+
+    // Make sure nothing is copied if there is nothing to copy
+    message_render.copySection(target, Message::SECTION_QUESTION);
+    EXPECT_EQ(0, target.getRRCount(Message::SECTION_QUESTION));
+    message_render.copySection(target, Message::SECTION_ANSWER);
+    EXPECT_EQ(0, target.getRRCount(Message::SECTION_ANSWER));
+    message_render.copySection(target, Message::SECTION_AUTHORITY);
+    EXPECT_EQ(0, target.getRRCount(Message::SECTION_AUTHORITY));
+    message_render.copySection(target, Message::SECTION_ADDITIONAL);
+    EXPECT_EQ(0, target.getRRCount(Message::SECTION_ADDITIONAL));
+
+    // Now add some data, copy again, and see if it got added
+    message_render.addQuestion(Question(Name("test.example.com"),
+                                        RRClass::IN(), RRType::A()));
+    message_render.addRRset(Message::SECTION_ANSWER, rrset_a);
+    message_render.addRRset(Message::SECTION_AUTHORITY, rrset_a);
+    message_render.addRRset(Message::SECTION_ADDITIONAL, rrset_a);
+    message_render.addRRset(Message::SECTION_ADDITIONAL, rrset_aaaa);
+
+    message_render.copySection(target, Message::SECTION_QUESTION);
+    EXPECT_EQ(1, target.getRRCount(Message::SECTION_QUESTION));
+
+    message_render.copySection(target, Message::SECTION_ANSWER);
+    EXPECT_EQ(2, target.getRRCount(Message::SECTION_ANSWER));
+    EXPECT_TRUE(target.hasRRset(Message::SECTION_ANSWER, test_name,
+        RRClass::IN(), RRType::A()));
+
+    message_render.copySection(target, Message::SECTION_AUTHORITY);
+    EXPECT_EQ(2, target.getRRCount(Message::SECTION_AUTHORITY));
+    EXPECT_TRUE(target.hasRRset(Message::SECTION_AUTHORITY, test_name,
+        RRClass::IN(), RRType::A()));
+
+    message_render.copySection(target, Message::SECTION_ADDITIONAL);
+    EXPECT_EQ(3, target.getRRCount(Message::SECTION_ADDITIONAL));
+    EXPECT_TRUE(target.hasRRset(Message::SECTION_ADDITIONAL, test_name,
+        RRClass::IN(), RRType::A()));
+    EXPECT_TRUE(target.hasRRset(Message::SECTION_ADDITIONAL, test_name,
+        RRClass::IN(), RRType::AAAA()));
+
+    // One more test, test to see if the section gets added, not replaced
+    Message source2(Message::RENDER);
+    source2.addRRset(Message::SECTION_ANSWER, rrset_aaaa);
+    source2.copySection(target, Message::SECTION_ANSWER);
+    EXPECT_EQ(3, target.getRRCount(Message::SECTION_ANSWER));
+    EXPECT_TRUE(target.hasRRset(Message::SECTION_ANSWER, test_name,
+        RRClass::IN(), RRType::A()));
+    EXPECT_TRUE(target.hasRRset(Message::SECTION_ANSWER, test_name,
+        RRClass::IN(), RRType::AAAA()));
+    
+}
+
 TEST_F(MessageTest, fromWire) {
     factoryFromFile(message_parse, "message_fromWire1");
     EXPECT_EQ(0x1035, message_parse.getQid());

src/lib/nsas/zone_entry.h

@@ -74,6 +74,9 @@ public:
      * \param name Name of the zone
      * \param class_code Class of this zone (zones of different classes have
      *     different objects.
+     * \param nameserver_table Hashtable of NameServerEntry objects for
+     *     this zone
+     * \param namesever_lru LRU for the nameserver entries
      * \todo Move to cc file, include the lookup (if NSAS uses resolver for
      *     everything)
      */

src/lib/resolve/resolve.cc

@@ -44,22 +44,12 @@ makeErrorMessage(MessagePtr answer_message,
     answer_message->setRcode(error_code);
 }
 
-void
+void copyResponseMessage(const Message& source, MessagePtr target) {
-copySection(const Message& source, MessagePtr target,
-            Message::Section section)
-{
-    for_each(source.beginSection(section),
-             source.endSection(section),
-             SectionInserter(target, section));
-}
-
-void copyAnswerMessage(const Message& source, MessagePtr target) {
-
     target->setRcode(source.getRcode());
 
-    copySection(source, target, Message::SECTION_ANSWER);
+    source.copySection(*target, Message::SECTION_ANSWER);
-    copySection(source, target, Message::SECTION_AUTHORITY);
+    source.copySection(*target, Message::SECTION_AUTHORITY);
-    copySection(source, target, Message::SECTION_ADDITIONAL);
+    source.copySection(*target, Message::SECTION_ADDITIONAL);
 }
 
 

src/lib/resolve/resolve.h

@@ -40,24 +40,17 @@ namespace resolve {
 void makeErrorMessage(isc::dns::MessagePtr answer_message,
                       const isc::dns::Rcode::Rcode& error_code);
 
-/// \brief Copies all rrsets in the given section from source
-/// to target
-///
-/// \param source The source Message
-/// \param target The target MessagePtr
-/// \param section the section to copy
-void copySection(const isc::dns::Message& source,
-                 isc::dns::MessagePtr target,
-                 isc::dns::Message::Section section);
 
-/// \brief Copies the parts relevant for a DNS answer to the
+/// \brief Copies the parts relevant for a DNS reponse to the
 /// target message
 ///
 /// This adds all the RRsets in the answer, authority and
 /// additional sections to the target, as well as the response
 /// code
-void copyAnswerMessage(const isc::dns::Message& source,
+/// \param source The Message to copy the data from
-                       isc::dns::MessagePtr target);
+/// \param target The Message to copy the data to
+void copyResponseMessage(const isc::dns::Message& source,
+                         isc::dns::MessagePtr target);
 
 
 } // namespace resolve

src/lib/resolve/response_classifier.h

@@ -33,9 +33,6 @@ namespace resolve {
 /// This class is used in the recursive server.  It is passed an answer received
 /// from an upstream server and categorises it.
 ///
-/// TODO: It is unlikely that the code can be used in this form.  Some adaption
-/// of it will be required to put it in the server.
-///
 /// TODO: The code here does not take into account any EDNS0 fields.
 
 class ResponseClassifier {
@@ -95,10 +92,10 @@ public:
     /// \param question Question that was sent to the server
     /// \param message Pointer to the associated response from the server.
     /// \param cname_target If the message contains an (unfinished) CNAME
-    ///                     chain, this Name will be replaced by the
+    /// chain, this Name will be replaced by the target of the last CNAME
-    ///                     target of the last CNAME in the chain
+    /// in the chain
     /// \param cname_count This unsigned int will be incremented with
-    ///                    the number of CNAMEs followed
+    /// the number of CNAMEs followed
     /// \param tcignore If set, the TC bit in a response packet is
     /// ignored.  Otherwise the error code TRUNCATED will be returned.  The
     /// only time this is likely to be used is in development where we are not
@@ -140,10 +137,10 @@ private:
     /// cycles, but that should be minimal compared with the overhead of the
     /// memory management.
     /// \param cname_target If the message contains an (unfinished) CNAME
-    ///                     chain, this Name will be replaced by the
+    /// chain, this Name will be replaced by the target of the last CNAME
-    ///                     target of the last CNAME in the chain
+    /// in the chain
     /// \param cname_count This unsigned int will be incremented with
-    ///                    the number of CNAMEs followed
+    /// the number of CNAMEs followed
     /// \param size Number of elements to check.  See description of \c present
     /// for details.
     static Category cnameChase(const isc::dns::Name& qname,

src/lib/resolve/tests/resolve_unittest.cc

@@ -153,7 +153,7 @@ TEST_F(ResolveHelperFunctionsTest, copyAnswerMessage) {
     ASSERT_EQ(0, message_a_->getRRCount(Message::SECTION_AUTHORITY));
     ASSERT_EQ(0, message_a_->getRRCount(Message::SECTION_ADDITIONAL));
 
-    isc::resolve::copyAnswerMessage(*message_b_, message_a_);
+    isc::resolve::copyResponseMessage(*message_b_, message_a_);
 
     EXPECT_EQ(message_b_->getRcode(), message_a_->getRcode());
     ASSERT_EQ(message_b_->getRRCount(Message::SECTION_ANSWER),