BIND 10 trac497, updated. 81c35ef7e5e01da333654443045de3dd2663482c [trac497] addressed review comments
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Feb 9 11:24:02 UTC 2011
The branch, trac497 has been updated
via 81c35ef7e5e01da333654443045de3dd2663482c (commit)
from 4d29fda48f7304d2c5bfbc246326850ca5970b0f (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 81c35ef7e5e01da333654443045de3dd2663482c
Author: Jelte Jansen <jelte at isc.org>
Date: Wed Feb 9 12:23:38 2011 +0100
[trac497] addressed review comments
and updated some doxygens as per Jeremy's mail
-----------------------------------------------------------------------
Summary of changes:
src/bin/resolver/resolver.h | 11 ++++-
src/lib/asiolink/asiolink.cc | 23 ++++++-----
src/lib/asiolink/asiolink.h | 15 ++++---
src/lib/asiolink/tests/asiolink_unittest.cc | 2 +-
src/lib/dns/message.cc | 20 +++++++++
src/lib/dns/message.h | 7 +++
src/lib/dns/tests/message_unittest.cc | 57 +++++++++++++++++++++++++++
src/lib/nsas/zone_entry.h | 3 +
src/lib/resolve/resolve.cc | 18 ++-------
src/lib/resolve/resolve.h | 17 ++------
src/lib/resolve/response_classifier.h | 15 +++----
src/lib/resolve/tests/resolve_unittest.cc | 2 +-
12 files changed, 135 insertions(+), 55 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/resolver/resolver.h b/src/bin/resolver/resolver.h
index 3a5219f..2ae8079 100644
--- a/src/bin/resolver/resolver.h
+++ b/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).
*/
diff --git a/src/lib/asiolink/asiolink.cc b/src/lib/asiolink/asiolink.cc
index aa0d33e..a84af4c 100644
--- a/src/lib/asiolink/asiolink.cc
+++ b/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_,
- Message::SECTION_ANSWER);
+ incoming.copySection(*answer_message_,
+ 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
- isc::resolve::copyAnswerMessage(incoming, answer_message_);
+ // TODO this will result in answering with the delegation. oh well
+ 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;
}
diff --git a/src/lib/asiolink/asiolink.h b/src/lib/asiolink/asiolink.h
index 44ff383..ed9abf5 100644
--- a/src/lib/asiolink/asiolink.h
+++ b/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 buffer The result is put here
+ /// \param query_message The DNS MessagePtr of the original query
+ /// \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
- /// -1 means never timeout (but do not use that).
- /// TODO: This should be computed somehow dynamically in future
+ /// \param query_timeout Timeout value for queries we sent, in ms
+ /// \param client_timeout Timeout value for when we send back an
+ /// 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,
diff --git a/src/lib/asiolink/tests/asiolink_unittest.cc b/src/lib/asiolink/tests/asiolink_unittest.cc
index a546779..7862f6b 100644
--- a/src/lib/asiolink/tests/asiolink_unittest.cc
+++ b/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),
diff --git a/src/lib/dns/message.cc b/src/lib/dns/message.cc
index 698508c..752df5a 100644
--- a/src/lib/dns/message.cc
+++ b/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,
diff --git a/src/lib/dns/message.h b/src/lib/dns/message.h
index ad12855..fb7a8e6 100644
--- a/src/lib/dns/message.h
+++ b/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
diff --git a/src/lib/dns/tests/message_unittest.cc b/src/lib/dns/tests/message_unittest.cc
index 996ea3a..5b08290 100644
--- a/src/lib/dns/tests/message_unittest.cc
+++ b/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());
diff --git a/src/lib/nsas/zone_entry.h b/src/lib/nsas/zone_entry.h
index c105f70..c819692 100644
--- a/src/lib/nsas/zone_entry.h
+++ b/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)
*/
diff --git a/src/lib/resolve/resolve.cc b/src/lib/resolve/resolve.cc
index 39139ee..d581ca2 100644
--- a/src/lib/resolve/resolve.cc
+++ b/src/lib/resolve/resolve.cc
@@ -44,22 +44,12 @@ makeErrorMessage(MessagePtr answer_message,
answer_message->setRcode(error_code);
}
-void
-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) {
-
+void copyResponseMessage(const Message& source, MessagePtr target) {
target->setRcode(source.getRcode());
- copySection(source, target, Message::SECTION_ANSWER);
- copySection(source, target, Message::SECTION_AUTHORITY);
- copySection(source, target, Message::SECTION_ADDITIONAL);
+ source.copySection(*target, Message::SECTION_ANSWER);
+ source.copySection(*target, Message::SECTION_AUTHORITY);
+ source.copySection(*target, Message::SECTION_ADDITIONAL);
}
diff --git a/src/lib/resolve/resolve.h b/src/lib/resolve/resolve.h
index 7249247..421261c 100644
--- a/src/lib/resolve/resolve.h
+++ b/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,
- isc::dns::MessagePtr target);
+/// \param source The Message to copy the data from
+/// \param target The Message to copy the data to
+void copyResponseMessage(const isc::dns::Message& source,
+ isc::dns::MessagePtr target);
} // namespace resolve
diff --git a/src/lib/resolve/response_classifier.h b/src/lib/resolve/response_classifier.h
index aed9ee7..bee0628 100644
--- a/src/lib/resolve/response_classifier.h
+++ b/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
- /// target of the last CNAME in the chain
+ /// chain, this Name will be replaced by the target of the last CNAME
+ /// 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
- /// target of the last CNAME in the chain
+ /// chain, this Name will be replaced by the target of the last CNAME
+ /// 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,
diff --git a/src/lib/resolve/tests/resolve_unittest.cc b/src/lib/resolve/tests/resolve_unittest.cc
index 37753d2..fd68f16 100644
--- a/src/lib/resolve/tests/resolve_unittest.cc
+++ b/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),
More information about the bind10-changes
mailing list