[trac495] address review comments

src/bin/resolver/main.cc

@@ -130,7 +130,7 @@ main(int argc, char* argv[]) {
 
     Session* cc_session = NULL;
     ModuleCCSession* config_session = NULL;
-    //try {
+    try {
         string specfile;
         if (getenv("B10_FROM_BUILD")) {
             specfile = string(getenv("B10_FROM_BUILD")) +
@@ -212,10 +212,10 @@ main(int argc, char* argv[]) {
 
         dlog("Server started.");
         io_service.run();
-    //} catch (const std::exception& ex) {
-    //    dlog(string("Server failed: ") + ex.what(),true);
-    //    ret = 1;
-    //}
+    } catch (const std::exception& ex) {
+        dlog(string("Server failed: ") + ex.what(),true);
+        ret = 1;
+    }
 
     delete config_session;
     delete cc_session;

src/lib/asiolink/io_address.h

@@ -26,8 +26,6 @@
 
 #include <exceptions/exceptions.h>
 
-#include <iostream>
-
 namespace asiolink {
 
 /// \brief The \c IOAddress class represents an IP addresses (version

src/lib/nsas/nameserver_address_store.cc

@@ -95,9 +95,9 @@ NameserverAddressStore::lookup(const string& zone, const RRClass& class_code,
 
 void
 NameserverAddressStore::cancel(const string& zone,
-                               const RRClass& class_code,
-                               boost::shared_ptr<AddressRequestCallback> callback,
-                               AddressFamily family)
+    const RRClass& class_code,
+    const boost::shared_ptr<AddressRequestCallback>& callback,
+    AddressFamily family)
 {
     boost::shared_ptr<ZoneEntry> entry(zone_hash_->get(HashKey(zone,
                                                                class_code)));

src/lib/nsas/nameserver_address_store.h

@@ -91,7 +91,7 @@ public:
     ///
     /// \param callback Callback object that would be called
     void cancel(const std::string& zone, const dns::RRClass& class_code,
-                boost::shared_ptr<AddressRequestCallback> callback,
+                const boost::shared_ptr<AddressRequestCallback>& callback,
                 AddressFamily family = ANY_OK);
 
     /// \brief Protected Members

src/lib/nsas/zone_entry.cc

@@ -262,14 +262,13 @@ ZoneEntry::addCallback(CallbackPtr callback, AddressFamily family) {
 }
 
 void
-ZoneEntry::removeCallback(CallbackPtr callback, AddressFamily family) {
+ZoneEntry::removeCallback(const CallbackPtr& callback, AddressFamily family) {
     Lock lock(mutex_);
     std::vector<boost::shared_ptr<AddressRequestCallback> >::iterator i = 
         callbacks_[family].begin();
     for (; i != callbacks_[family].end(); ++i) {
         if (*i == callback) {
             callbacks_[family].erase(i);
-            return;
         }
     }
 }

src/lib/nsas/zone_entry.h

@@ -107,7 +107,7 @@ public:
      * \param callback The callback itself.
      * \param family Which address family is acceptable as an answer?
      */
-    void removeCallback(boost::shared_ptr<AddressRequestCallback>
+    void removeCallback(const boost::shared_ptr<AddressRequestCallback>&
                         callback, AddressFamily family);
 
     /// \short Protected members, so they can be accessed by tests.

src/lib/resolve/recursive_query.cc

@@ -165,7 +165,10 @@ private:
     
     // the 'current' zone we are in (i.e.) we start out at the root,
     // and for each delegation this gets updated with the zone the
-    // delegation points to
+    // delegation points to.
+    // TODO: make this a Name (it is a string right now because most
+    // of the call we use it in take a string, we need update those
+    // too).
     std::string cur_zone_;
     
     // This is the handler we pass on to the NSAS; it is called when
@@ -244,6 +247,7 @@ private:
         // forwarder. If not, ask the NSAS for an address
         const int uc = upstream_->size();
         if (uc > 0) {
+            // TODO: use boost, or rand()-utility function we provide
             int serverIndex = rand() % uc;
             dlog("Sending upstream query (" + question_.toText() +
                 ") to " + upstream_->at(serverIndex).first);
@@ -352,6 +356,9 @@ private:
                  ++rrsi) {
                 ConstRRsetPtr rrs = *rrsi;
                 if (rrs->getType() == RRType::NS()) {
+                    // TODO: make cur_zone_ a Name instead of a string
+                    // (this requires a few API changes in related
+                    // libraries, so as not to need many conversions)
                     cur_zone_ = rrs->getName().toText();
                     dlog("Referred to zone " + cur_zone_);
                     found_ns_address = true;
@@ -426,8 +433,7 @@ public:
         callback_called_(false),
         nsas_(nsas),
         cache_(cache),
-        nsas_callback_(boost::shared_ptr<ResolverNSASCallback>(
-                                     new ResolverNSASCallback(this))),
+        nsas_callback_(new ResolverNSASCallback(this)),
         nsas_callback_out_(false),
         outstanding_events_(0)
     {
@@ -538,7 +544,7 @@ public:
             struct timeval cur_time;
             gettimeofday(&cur_time, NULL);
             uint32_t rtt;
-            if (cur_time.tv_sec >= current_ns_qsent_time.tv_sec &&
+            if (cur_time.tv_sec >= current_ns_qsent_time.tv_sec ||
                 cur_time.tv_usec > current_ns_qsent_time.tv_usec) {
                 rtt = 1000 * (cur_time.tv_sec - current_ns_qsent_time.tv_sec);
                 rtt += (cur_time.tv_usec - current_ns_qsent_time.tv_usec) / 1000;

src/lib/resolve/recursive_query.h

@@ -12,8 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#ifndef __ASIOLINK_RECURSIVE_QUERY_H
-#define __ASIOLINK_RECURSIVE_QUERY_H 1
+#ifndef __RECURSIVE_QUERY_H
+#define __RECURSIVE_QUERY_H 1
 
 #include <asiolink/dns_service.h>
 #include <asiolink/dns_server.h>
@@ -116,4 +116,4 @@ private:
 };
 
 }      // namespace asiolink
-#endif // __ASIOLINK_RECURSIVE_QUERY_H
+#endif // __RECURSIVE_QUERY_H

src/lib/resolve/tests/recursive_query_unittest.cc

@@ -853,4 +853,7 @@ TEST_F(RecursiveQueryTest, DISABLED_recursiveSendNXDOMAIN) {
     EXPECT_EQ(0, answer->getRRCount(Message::SECTION_ANSWER));
 }
 
+// TODO: add tests that check whether the cache is updated on succesfull
+// responses, and not updated on failures.
+
 }