handle Stephen's review comments

git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac327@3818 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/asiolink/asiolink.cc

@@ -291,69 +291,71 @@ namespace {
  * Used by RecursiveQuery::sendQuery.
  */
 class RunningQuery : public UDPQuery::Callback {
-        private:
-            // The io service to handle async calls
-            asio::io_service& io_;
-            // Info for (re)sending the query (the question and destination)
-            Question question_;
-            shared_ptr<AddressVector> upstream_;
-            // Buffer to store the result.
-            OutputBufferPtr buffer_;
-            /*
-             * FIXME This is said it does problems when it is shared pointer, as
-             *     it is destroyed too soon. But who deletes it now?
-             */
-            // Server to notify when we succeed or fail
-            shared_ptr<DNSServer> server_;
-            /*
-             * TODO Do something more clever with timeouts. In the long term, some
-             *     computation of average RTT, increase with each retry, etc.
-             */
-            // Timeout information
-            int timeout_;
-            unsigned retries_;
-            // (re)send the query to the server.
-            void send() {
-                const int uc = upstream_->size();
-                if (uc > 0) {
-                    int serverIndex(random() % uc);
-                    dlog("Sending upstream query (" + question_.toText() +
-                        ") to " + upstream_->at(serverIndex).first);
-                    UDPQuery query(io_, question_,
-                        upstream_->at(serverIndex).first,
-                        upstream_->at(serverIndex).second, buffer_, this,
-                        timeout_);
-                    io_.post(query);
-                } else {
-                    dlog("Error, no upstream servers to send to.");
-                }
-            }
-        public:
-            RunningQuery(asio::io_service& io, const Question &question,
-                shared_ptr<AddressVector> upstream,
-                OutputBufferPtr buffer, DNSServer* server, int timeout,
-                unsigned retries) :
-                io_(io),
-                question_(question),
-                upstream_(upstream),
-                buffer_(buffer),
-                server_(server->clone()),
-                timeout_(timeout),
-                retries_(retries)
-            {
-                send();
-            }
-            // This function is used as callback from DNSQuery.
-            virtual void operator()(UDPQuery::Result result) {
-                if (result == UDPQuery::TIME_OUT && retries_ --) {
-                    dlog("Resending query");
-                    // We timed out, but we have some retries, so send again
-                    send();
-                } else {
-                    server_->resume(result == UDPQuery::SUCCESS);
-                    delete this;
-                }
-            }
+private:
+    // The io service to handle async calls
+    asio::io_service& io_;
+
+    // Info for (re)sending the query (the question and destination)
+    Question question_;
+    shared_ptr<AddressVector> upstream_;
+
+    // Buffer to store the result.
+    OutputBufferPtr buffer_;
+
+    // Server to notify when we succeed or fail
+    shared_ptr<DNSServer> server_;
+
+    /*
+     * TODO Do something more clever with timeouts. In the long term, some
+     *     computation of average RTT, increase with each retry, etc.
+     */
+    // Timeout information
+    int timeout_;
+    unsigned retries_;
+
+    // (re)send the query to the server.
+    void send() {
+        const int uc = upstream_->size();
+        if (uc > 0) {
+            int serverIndex(random() % uc);
+            dlog("Sending upstream query (" + question_.toText() +
+                ") to " + upstream_->at(serverIndex).first);
+            UDPQuery query(io_, question_,
+                upstream_->at(serverIndex).first,
+                upstream_->at(serverIndex).second, buffer_, this,
+                timeout_);
+            io_.post(query);
+        } else {
+            dlog("Error, no upstream servers to send to.");
+        }
+    }
+public:
+    RunningQuery(asio::io_service& io, const Question &question,
+        shared_ptr<AddressVector> upstream,
+        OutputBufferPtr buffer, DNSServer* server, int timeout,
+        unsigned retries) :
+        io_(io),
+        question_(question),
+        upstream_(upstream),
+        buffer_(buffer),
+        server_(server->clone()),
+        timeout_(timeout),
+        retries_(retries)
+    {
+        send();
+    }
+
+    // This function is used as callback from DNSQuery.
+    virtual void operator()(UDPQuery::Result result) {
+        if (result == UDPQuery::TIME_OUT && retries_ --) {
+            dlog("Resending query");
+            // We timed out, but we have some retries, so send again
+            send();
+        } else {
+            server_->resume(result == UDPQuery::SUCCESS);
+            delete this;
+        }
+    }
 };
 
 }

src/lib/asiolink/asiolink.h

@@ -165,7 +165,10 @@ private:
 
 ///
 /// DNSService is the service that handles DNS queries and answers with
-/// a given IOService.
+/// a given IOService. This class is mainly intended to hold all the
+/// logic that is shared between the authoritative and the recursive
+/// server implementations. As such, it handles asio, including config
+/// updates (through the 'Checkinprovider'), and listening sockets.
 /// 
 class DNSService {
     ///
@@ -247,7 +250,9 @@ private:
 ///
 /// Notes to developers:
 /// When constructed, this class (and its derived classes) will have its
-/// "self_" member set to point to "this".  Calls to methods in the base
+/// "self_" member set to point to "this".  Objects of this class (as
+/// instantiated through a base class) are sometimes passed by
+/// reference (as this superclass); calls to methods in the base
 /// class are then rerouted via this pointer to methods in the derived
 /// class.  This allows code from outside asiolink, with no specific
 /// knowledge of \c TCPServer or \c UDPServer, to access their methods.
@@ -343,7 +348,7 @@ protected:
     template <typename T>
     class AsyncLookup {
     public:
-        AsyncLookup(T caller) : caller_(caller) {}
+        AsyncLookup(T& caller) : caller_(caller) {}
         inline void operator()() { caller_.asyncLookup(); }
     private:
         T caller_;
@@ -466,6 +471,8 @@ public:
 /// \brief The \c SimpleCallback class is an abstract base class for a
 /// simple callback function with the signature:
 ///
+/// void simpleCallback(const IOMessage& io_message) const;
+///
 /// Specific derived class implementations are hidden within the
 /// implementation.  Instances of the derived classes can be called
 /// as functions via the operator() interface.  Pointers to these

src/lib/asiolink/internal/udpdns.h

@@ -121,22 +121,52 @@ private:
 //
 // Asynchronous UDP server coroutine
 //
+///
+/// \brief This class implements the coroutine to handle UDP
+///        DNS query event. As such, it is both a \c DNSServer and
+///        a \c coroutine
+///
 class UDPServer : public virtual DNSServer, public virtual coroutine {
 public:
+    /// \brief Constructor
+    /// \param io_service the asio::io_service to work with
+    /// \param addr the IP address to listen for queries on
+    /// \param port the port to listen for queries on
+    /// \param checkin the callbackprovider for non-DNS events
+    /// \param lookup the callbackprovider for DNS lookup events
+    /// \param answer the callbackprovider for DNS answer events
     explicit UDPServer(asio::io_service& io_service,
                        const asio::ip::address& addr, const uint16_t port,
                        SimpleCallback* checkin = NULL,
                        DNSLookup* lookup = NULL,
                        DNSAnswer* answer = NULL);
 
+    /// \brief The function operator
     void operator()(asio::error_code ec = asio::error_code(),
                     size_t length = 0);
 
+    /// \brief Calls the lookup callback
     void asyncLookup();
+
+    /// \brief Resume operation
+    ///
+    /// \param done Set this to true if the lookup action is done and
+    ///        we have an answer
     void resume(const bool done);
+
+    /// \brief Check if we have an answer
+    ///
+    /// \return true if we have an answer
     bool hasAnswer() { return (done_); }
+
+    /// \brief Returns the coroutine state value
+    ///
+    /// \return the coroutine state value
     int value() { return (get_value()); }
 
+    /// \brief Clones the object
+    ///
+    /// \return a newly allocated copy of this object
     DNSServer* clone() {
         UDPServer* s = new UDPServer(*this);
         return (s);
@@ -208,12 +238,12 @@ private:
 class UDPQuery : public coroutine {
 public:
     // TODO Maybe this should be more generic than just for UDPQuery?
-    /**
-     * \short Result of the query
-     *
-     * This is related only to contacting the remote server. If the answer
-     * indicates error, it is still counted as SUCCESS here, if it comes back.
-     */
+    ///
+    /// \brief Result of the query
+    ///
+    /// This is related only to contacting the remote server. If the answer
+    ///indicates error, it is still counted as SUCCESS here, if it comes back.
+    ///
     enum Result {
         SUCCESS,
         TIME_OUT,
@@ -225,14 +255,14 @@ public:
             /// This will be called when the UDPQuery is completed
             virtual void operator()(Result result) = 0;
     };
-    /**
-     * \short Constructor.
-     *
-     * It creates the query.
-     * @param callback will be called when we terminate. It is your task to
-     *     delete it if allocated on heap.
-     * @param timeout in ms.
-     */
+    ///
+    /// \brief Constructor.
+    ///
+    /// It creates the query.
+    /// @param callback will be called when we terminate. It is your task to
+    ///        delete it if allocated on heap.
+    ///@param timeout in ms.
+    ///
     explicit UDPQuery(asio::io_service& io_service,
                       const isc::dns::Question& q,
                       const IOAddress& addr, uint16_t port,
@@ -245,15 +275,15 @@ public:
 private:
     enum { MAX_LENGTH = 4096 };
 
-    /**
-     * \short Private data
-     *
-     * They are not private because of stability of the
-     * interface (this is private class anyway), but because this class
-     * will be copyed often (it is used as a coroutine and passed as callback
-     * to many async_*() functions) and we want keep the same data. Some of
-     * the data is not copyable too.
-     */
+    ///
+    /// \short Private data
+    ///
+    /// They are not private because of stability of the
+    /// interface (this is private class anyway), but because this class
+    /// will be copyed often (it is used as a coroutine and passed as callback
+    /// to many async_*() functions) and we want keep the same data. Some of
+    /// the data is not copyable too.
+    ///
     struct PrivateData;
     boost::shared_ptr<PrivateData> data_;
 };