[1747] addressed review comments

see
http://bind10.isc.org/ticket/1747?replyto=7#comment:9

diff --git a/src/bin/auth/query.cc b/src/bin/auth/query.cc
index e79bef7a46ba81023bfacaed120a884178f23a72..424d05b22a38a41bd2705daf5ec33db49a8138ca 100644 (file)
--- a/src/bin/auth/query.cc
+++ b/src/bin/auth/query.cc
@@ -133,14 +133,14 @@ Query::addNXDOMAINProofByNSEC(ZoneFinder& finder, ConstRRsetPtr nsec) {
     // and the best possible wildcard is *.b.example.com.  If the NXDOMAIN
     // NSEC is a.example.com. NSEC c.b.example.com., the longer suffix
     // is the next domain of the NSEC, and we get the same wildcard name.
-    const int qlabels = qname_.getLabelCount();
-    const int olabels = qname_.compare(nsec->getName()).getCommonLabels();
-    const int nlabels = qname_.compare(
+    const int qlabels = qname_->getLabelCount();
+    const int olabels = qname_->compare(nsec->getName()).getCommonLabels();
+    const int nlabels = qname_->compare(
         dynamic_cast<const generic::NSEC&>(nsec->getRdataIterator()->
                                            getCurrent()).
         getNextName()).getCommonLabels();
     const int common_labels = std::max(olabels, nlabels);
-    const Name wildname(Name("*").concatenate(qname_.split(qlabels -
+    const Name wildname(Name("*").concatenate(qname_->split(qlabels -
                                                            common_labels)));
 
     // Confirm the wildcard doesn't exist (this should result in NXDOMAIN;
@@ -206,12 +206,12 @@ Query::addNXDOMAINProofByNSEC3(ZoneFinder& finder) {
     // Firstly get the NSEC3 proves for Closest Encloser Proof
     // See Section 7.2.1 of RFC 5155.
     const uint8_t closest_labels =
-        addClosestEncloserProof(finder, qname_, false);
+        addClosestEncloserProof(finder, *qname_, false);
 
     // Next, construct the wildcard name at the closest encloser, i.e.,
     // '*' followed by the closest encloser, and add NSEC3 for it.
     const Name wildname(Name("*").concatenate(
-               qname_.split(qname_.getLabelCount() - closest_labels)));
+               qname_->split(qname_->getLabelCount() - closest_labels)));
     addNSEC3ForName(finder, wildname, false);
 }
 
@@ -226,7 +226,7 @@ Query::addWildcardProof(ZoneFinder& finder,
         // substitution.  Confirm that by specifying NO_WILDCARD.  It should
         // result in NXDOMAIN and an NSEC RR that proves it should be returned.
         ConstZoneFinderContextPtr fcontext =
-            finder.find(qname_, RRType::NSEC(),
+            finder.find(*qname_, RRType::NSEC(),
                         dnssec_opt_ | ZoneFinder::NO_WILDCARD);
         if (fcontext->code != ZoneFinder::NXDOMAIN || !fcontext->rrset ||
             fcontext->rrset->getRdataCount() == 0) {
@@ -241,7 +241,7 @@ Query::addWildcardProof(ZoneFinder& finder,
         // of the matching wildcard, so NSEC3 for its next closer (and only
         // that NSEC3) is what we are expected to provided per the RFC (if
         // this assumption isn't met the zone is broken anyway).
-        addClosestEncloserProof(finder, qname_, false, false);
+        addClosestEncloserProof(finder, *qname_, false, false);
     }
 }
 
@@ -254,7 +254,7 @@ Query::addWildcardNXRRSETProof(ZoneFinder& finder, ConstRRsetPtr nsec) {
     }
     
     ConstZoneFinderContextPtr fcontext =
-        finder.find(qname_, RRType::NSEC(),
+        finder.find(*qname_, RRType::NSEC(),
                     dnssec_opt_ | ZoneFinder::NO_WILDCARD);
     if (fcontext->code != ZoneFinder::NXDOMAIN || !fcontext->rrset ||
         fcontext->rrset->getRdataCount() == 0) {
@@ -296,21 +296,21 @@ Query::addNXRRsetProof(ZoneFinder& finder,
             addWildcardNXRRSETProof(finder, db_context.rrset);
         }
     } else if (db_context.isNSEC3Signed() && !db_context.isWildcard()) {
-        if (qtype_ == RRType::DS()) {
+        if (*qtype_ == RRType::DS()) {
             // RFC 5155, Section 7.2.4.  Add either NSEC3 for the qname or
             // closest (provable) encloser proof in case of optout.
-            addClosestEncloserProof(finder, qname_, true);
+            addClosestEncloserProof(finder, *qname_, true);
         } else {
             // RFC 5155, Section 7.2.3.  Just add NSEC3 for the qname.
-            addNSEC3ForName(finder, qname_, true);
+            addNSEC3ForName(finder, *qname_, true);
         }
     } else if (db_context.isNSEC3Signed() && db_context.isWildcard()) {
         // Case for RFC 5155 Section 7.2.5: add closest encloser proof for the
         // qname, construct the matched wildcard name and add NSEC3 for it.
         const uint8_t closest_labels =
-            addClosestEncloserProof(finder, qname_, false);
+            addClosestEncloserProof(finder, *qname_, false);
         const Name wname = Name("*").concatenate(
-            qname_.split(qname_.getLabelCount() - closest_labels));
+            qname_->split(qname_->getLabelCount() - closest_labels));
         addNSEC3ForName(finder, wname, true);
     }
 }
@@ -331,7 +331,7 @@ Query::addAuthAdditional(ZoneFinder& finder,
                   finder.getOrigin().toText());
     }
     authorities_.push_back(ns_context->rrset);
-    getAdditional(qname_, qtype_, *ns_context, additionals);
+    getAdditional(*qname_, *qtype_, *ns_context, additionals);
 }
 
 namespace {
@@ -352,7 +352,7 @@ findZone(const DataSourceClient& client, const Name& qname, RRType qtype) {
 
 void
 Query::process(datasrc::DataSourceClient& datasrc_client,
-               const isc::dns::Name qname, const isc::dns::RRType qtype,
+               const isc::dns::Name& qname, const isc::dns::RRType& qtype,
                isc::dns::Message& response, bool dnssec)
 {
     // First a bit of house cleaning
@@ -366,7 +366,7 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
 
     // Found a zone which is the nearest ancestor to QNAME
     const DataSourceClient::FindResult result = findZone(*datasrc_client_,
-                                                         qname_, qtype_);
+                                                         *qname_, *qtype_);
 
     // If we have no matching authoritative zone for the query name, return
     // REFUSED.  In short, this is to be compatible with BIND 9, but the
@@ -378,14 +378,12 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
         // If we tried to find a "parent zone" for a DS query and failed,
         // we may still have authority at the child side.  If we do, the query
         // has to be handled there.
-        if (qtype_ == RRType::DS() && qname_.getLabelCount() > 1 &&
+        if (*qtype_ == RRType::DS() && qname_->getLabelCount() > 1 &&
             processDSAtChild()) {
-            createResponse();
             return;
         }
         response_->setHeaderFlag(Message::HEADERFLAG_AA, false);
         response_->setRcode(Rcode::REFUSED());
-        createResponse();
         return;
     }
     ZoneFinder& zfinder = *result.zone_finder;
@@ -395,12 +393,12 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
     response_->setHeaderFlag(Message::HEADERFLAG_AA);
     response_->setRcode(Rcode::NOERROR());
     boost::function0<ZoneFinderContextPtr> find;
-    const bool qtype_is_any = (qtype_ == RRType::ANY());
+    const bool qtype_is_any = (*qtype_ == RRType::ANY());
     if (qtype_is_any) {
-        find = boost::bind(&ZoneFinder::findAll, &zfinder, qname_,
+        find = boost::bind(&ZoneFinder::findAll, &zfinder, *qname_,
                            boost::ref(answers_), dnssec_opt_);
     } else {
-        find = boost::bind(&ZoneFinder::find, &zfinder, qname_, qtype_,
+        find = boost::bind(&ZoneFinder::find, &zfinder, *qname_, *qtype_,
                            dnssec_opt_);
     }
     ZoneFinderContextPtr db_context(find());
@@ -420,7 +418,7 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
                 dynamic_cast<const rdata::generic::DNAME&>(
                 db_context->rrset->getRdataIterator()->getCurrent()));
             // The yet unmatched prefix dname
-            const Name prefix(qname_.split(0, qname_.getLabelCount() -
+            const Name prefix(qname_->split(0, qname_->getLabelCount() -
                 db_context->rrset->getName().getLabelCount()));
             // If we put it together, will it be too long?
             // (The prefix contains trailing ., which will be removed
@@ -436,11 +434,11 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
             // The new CNAME we are creating (it will be unsigned even
             // with DNSSEC, the DNAME is signed and it can be validated
             // by that)
-            RRsetPtr cname(new RRset(qname_, db_context->rrset->getClass(),
+            RRsetPtr cname(new RRset(*qname_, db_context->rrset->getClass(),
                 RRType::CNAME(), db_context->rrset->getTTL()));
             // Construct the new target by replacing the end
-            cname->addRdata(rdata::generic::CNAME(qname_.split(0,
-                qname_.getLabelCount() -
+            cname->addRdata(rdata::generic::CNAME(qname_->split(0,
+                qname_->getLabelCount() -
                 db_context->rrset->getName().getLabelCount()).
                 concatenate(dname.getDname())));
             answers_.push_back(cname);
@@ -471,7 +469,7 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
             }
 
             // Retrieve additional records for the answer
-            getAdditional(qname_, qtype_, *db_context, additionals_);
+            getAdditional(*qname_, *qtype_, *db_context, additionals_);
 
             // If apex NS records haven't been provided in the answer
             // section, insert apex NS records into the authority section
@@ -481,7 +479,7 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
             // qname is the zone origin.
             if (result.code != result::SUCCESS ||
                 db_context->code != ZoneFinder::SUCCESS ||
-                (qtype_ != RRType::NS() && !qtype_is_any))
+                (*qtype_ != RRType::NS() && !qtype_is_any))
             {
                 addAuthAdditional(*result.zone_finder, additionals_);
             }
@@ -497,8 +495,8 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
             // if we are an authority of the child, too.  If so, we need to
             // complete the process in the child as specified in Section
             // 2.2.1.2. of RFC3658.
-            if (qtype_ == RRType::DS() && processDSAtChild()) {
-                break;
+            if (*qtype_ == RRType::DS() && processDSAtChild()) {
+                return;
             }
 
             response_->setHeaderFlag(Message::HEADERFLAG_AA, false);
@@ -542,12 +540,12 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
 
 void
 Query::initialize(datasrc::DataSourceClient& datasrc_client,
-                  const isc::dns::Name qname, const isc::dns::RRType qtype,
+                  const isc::dns::Name& qname, const isc::dns::RRType& qtype,
                   isc::dns::Message& response, bool dnssec)
 {
     datasrc_client_ = &datasrc_client;
-    qname_ = qname;
-    qtype_ = qtype;
+    qname_ = &qname;
+    qtype_ = &qtype;
     response_ = &response;
     dnssec_ = dnssec;
     dnssec_opt_ = (dnssec ? isc::datasrc::ZoneFinder::FIND_DNSSEC :
@@ -570,7 +568,7 @@ Query::createResponse() {
 bool
 Query::processDSAtChild() {
     const DataSourceClient::FindResult zresult =
-        datasrc_client_->findZone(qname_);
+        datasrc_client_->findZone(*qname_);
 
     if (zresult.code != result::SUCCESS) {
         return (false);
@@ -589,13 +587,14 @@ Query::processDSAtChild() {
     response_->setRcode(Rcode::NOERROR());
     addSOA(*zresult.zone_finder);
     ConstZoneFinderContextPtr ds_context =
-        zresult.zone_finder->find(qname_, RRType::DS(), dnssec_opt_);
+        zresult.zone_finder->find(*qname_, RRType::DS(), dnssec_opt_);
     if (ds_context->code == ZoneFinder::NXRRSET) {
         if (dnssec_) {
             addNXRRsetProof(*zresult.zone_finder, *ds_context);
         }
     }
 
+    createResponse();
     return (true);
 }
 

diff --git a/src/bin/auth/query.h b/src/bin/auth/query.h
index 1adfd8bbadfc897928764319a6b5762bc8d160d4..062925cf5bd5ee9b1da0e5bc779657d386e40562 100644 (file)
--- a/src/bin/auth/query.h
+++ b/src/bin/auth/query.h
@@ -18,6 +18,8 @@
 #include <dns/rrset.h>
 #include <datasrc/zone.h>
 
+#include <boost/noncopyable.hpp>
+
 #include <vector>
 
 namespace isc {
@@ -34,6 +36,13 @@ class DataSourceClient;
 
 namespace auth {
 
+/// \brief Initial reserved size for the vectors in Query
+///
+/// The value is larger than we expect the vectors to even become, and
+/// has been chosen arbitrarily. The reason to set them quite high is
+/// to prevent reallocation on addition.
+const size_t RESERVE_RRSETS = 64;
+
 /// The \c Query class represents a standard DNS query that encapsulates
 /// processing logic to answer the query.
 ///
@@ -64,7 +73,7 @@ namespace auth {
 /// likely to misuse one of the classes instead of the other
 /// accidentally, and since it's considered a temporary development state,
 /// we keep this name at the moment.
-class Query {
+class Query : boost::noncopyable {
 private:
 
     /// \brief Adds a SOA.
@@ -239,7 +248,7 @@ private:
     /// \param dnssec If the answer should include signatures and NSEC/NSEC3 if
     ///     possible.
     void initialize(datasrc::DataSourceClient& datasrc_client,
-                    const isc::dns::Name qname, const isc::dns::RRType qtype,
+                    const isc::dns::Name& qname, const isc::dns::RRType& qtype,
                     isc::dns::Message& response, bool dnssec = false);
 
     /// \brief Fill in the response sections
@@ -250,7 +259,7 @@ private:
     ///
     /// This will take each RRset collected in answers_, authorities_, and
     /// additionals_, and add them to their corresponding sections in
-    /// the response packet.
+    /// the response message.
     ///
     /// After they are added, the vectors are cleared.
     void createResponse();
@@ -264,18 +273,19 @@ private:
     }
 
 public:
-    /// Empty Constructor.
+    /// Default constructor.
     ///
     /// Query parameters will be set by the call to process()
     ///
-    /// This constructor never throws an exception.
-    ///
     Query() :
-        datasrc_client_(NULL), qname_("."),
-        qtype_(isc::dns::RRType::A()),
+        datasrc_client_(NULL), qname_(NULL), qtype_(NULL),
         response_(NULL), dnssec_(false),
         dnssec_opt_(isc::datasrc::ZoneFinder::FIND_DEFAULT)
-    {}
+    {
+        answers_.reserve(RESERVE_RRSETS);
+        authorities_.reserve(RESERVE_RRSETS);
+        additionals_.reserve(RESERVE_RRSETS);
+    }
 
     /// Process the query.
     ///
@@ -312,7 +322,7 @@ public:
     /// \param dnssec If the answer should include signatures and NSEC/NSEC3 if
     ///     possible.
     void process(datasrc::DataSourceClient& datasrc_client,
-                 const isc::dns::Name qname, const isc::dns::RRType qtype,
+                 const isc::dns::Name& qname, const isc::dns::RRType& qtype,
                  isc::dns::Message& response, bool dnssec = false);
 
     /// \short Bad zone data encountered.
@@ -383,8 +393,8 @@ public:
 
 private:
     const isc::datasrc::DataSourceClient* datasrc_client_;
-    isc::dns::Name qname_;
-    isc::dns::RRType qtype_;
+    const isc::dns::Name* qname_;
+    const isc::dns::RRType* qtype_;
     isc::dns::Message* response_;
     bool dnssec_;
     isc::datasrc::ZoneFinder::FindOptions dnssec_opt_;